Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add advanced options to vmware_cluster_vsan #289

Merged
merged 6 commits into from
Aug 10, 2020

Conversation

mariolenz
Copy link
Collaborator

@mariolenz mariolenz commented Jul 10, 2020

Depends-On: #250

SUMMARY

While configuring vsan clusters, setting the advanced parameters for the vsan service (Object repair timer, Thin Swap, Large Cluster Support etc.) should be possible.

Fixes #260

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

vmware_cluster_vsan

ADDITIONAL INFORMATION

Unfortunately, this PR introduces a dependecy on the SAN Management vSDK for Python. On the other hand, using ReconfigureComputeResource_Task to configure VSAN is deprecated so we would need this SDK anyway sooner or later.

edit: vmware/pyvmomi#909

Copy link
Member

@Akasurde Akasurde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for putting this together.

plugins/modules/vmware_cluster_vsan.py Outdated Show resolved Hide resolved
plugins/modules/vmware_cluster_vsan.py Show resolved Hide resolved
plugins/modules/vmware_cluster_vsan.py Show resolved Hide resolved
@mariolenz
Copy link
Collaborator Author

@lupa95 Can you please test this PR if it works for you?

@mariolenz mariolenz requested a review from Akasurde July 10, 2020 16:22
@mariolenz
Copy link
Collaborator Author

@goneri

So, I would say, I'm ok to merge to patch BUT we must the extra dependency must be mentioned in the documentation, and if someone try to run the module without the dependency, the error message should be as clear as possible.

Are the documentation and the error message OK for you? I hope so, otherwise just tell me.

@mariolenz
Copy link
Collaborator Author

@goneri

vmware_vsan_health_info already depends on the vsan sdk (a.ka vSAN Management SDK for Python). [...] We cannot properly test the module in the CI, since we cannot easily pip install the depency.

It looks like the CI fails for exactly this reason, it fails to import the required Python library (vSAN Management SDK for Python). How do I exclude the module from CI tests? That is, integration tests... PEP8, pylint and so on should be OK I think.

@goneri
Copy link
Member

goneri commented Jul 13, 2020

You can disable the test by adding disabled in the tests/integration/targets/vmware_cluster_vsan/aliases file. It's still a good thing to have a functional test, even if we don't run it in the CI.

changelogs/fragments/260-vmware_cluster_vsan.yml Outdated Show resolved Hide resolved
plugins/modules/vmware_cluster_vsan.py Outdated Show resolved Hide resolved
@goneri
Copy link
Member

goneri commented Jul 14, 2020

recheck

1 similar comment
@goneri
Copy link
Member

goneri commented Jul 14, 2020

recheck

@mariolenz
Copy link
Collaborator Author

recheck

@mariolenz mariolenz force-pushed the issue260 branch 2 times, most recently from d2ab0fa to 8247ebc Compare July 15, 2020 15:19
@mariolenz
Copy link
Collaborator Author

recheck

1 similar comment
@mariolenz
Copy link
Collaborator Author

recheck

@goneri
Copy link
Member

goneri commented Jul 16, 2020

FYI @mariolenz, the following job of the ongoing CI run will fail: ansible-test-sanity-vmware, the problem will be resolved as soon as #250 is merged.

@mariolenz
Copy link
Collaborator Author

FYI @mariolenz, the following job of the ongoing CI run will fail: ansible-test-sanity-vmware, the problem will be resolved as soon as #250 is merged.

Thanks for the information. I've added Depends-On: #250 to this PR, maybe this will help.

@mariolenz
Copy link
Collaborator Author

Depends-On: #250 seems to have helped, but two jobs ran into a RETRY_LIMIT. I'll try again, maybe this was only a temporary problem.

@mariolenz
Copy link
Collaborator Author

recheck

@mariolenz
Copy link
Collaborator Author

@goneri The last CI run failed again, but not like the one before... I'll try with another run.

It looks like the CI pipeline is a bit inconsistent :-/

@mariolenz
Copy link
Collaborator Author

recheck

@goneri
Copy link
Member

goneri commented Jul 17, 2020

@goneri The last CI run failed again, but not like the one before... I'll try with another run.

It looks like the CI pipeline is a bit inconsistent :-/

Well, the last run is green :-). The long series of "Waiting on logger" is actually a bug in Zuul. It should go away as soon as we upgrade ( https://review.opendev.org/#/c/741257/ ).

@mariolenz mariolenz changed the title [WIP] Add advanced options to vmware_cluster_vsan Add advanced options to vmware_cluster_vsan Jul 18, 2020
@mariolenz
Copy link
Collaborator Author

@Akasurde @goneri

There were so many commits that I decided to squash them and do a force-push. But the changes you've requested should all be there.

Now that the last CI run is finally green, I've removed [WIP] from the PR title because I think it's fit for merging now.

@mariolenz mariolenz requested a review from goneri July 18, 2020 15:22
@goneri goneri requested a review from Tomorrow9 July 20, 2020 13:42
plugins/modules/vmware_cluster_vsan.py Show resolved Hide resolved
changelogs/fragments/260-vmware_cluster_vsan.yml Outdated Show resolved Hide resolved
changelogs/fragments/260-vmware_cluster_vsan.yml Outdated Show resolved Hide resolved
plugins/modules/vmware_cluster_vsan.py Outdated Show resolved Hide resolved
plugins/modules/vmware_cluster_vsan.py Outdated Show resolved Hide resolved
plugins/modules/vmware_cluster_vsan.py Outdated Show resolved Hide resolved
@Akasurde
Copy link
Member

Akasurde commented Aug 3, 2020

@mariolenz I suggested some minor changes. Please ping me once done for merge.

@mariolenz
Copy link
Collaborator Author

@Akasurde I've committed the changes you've requested, but your first one was just a question. Do you want me to make it clearer that the requirement is vSAN 6.2 / ESXi 6.2U2 and later, but I've tested it only on 6.7?

@mariolenz
Copy link
Collaborator Author

recheck

@mariolenz
Copy link
Collaborator Author

@Akasurde What do you think, is this PR fit for merging now?

@mariolenz mariolenz requested a review from Akasurde August 9, 2020 16:24
@Akasurde Akasurde added the gate label Aug 10, 2020
@ansible-zuul ansible-zuul bot merged commit 0b3b677 into ansible-collections:main Aug 10, 2020
@mariolenz mariolenz deleted the issue260 branch August 10, 2020 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vmware_cluster_vsan - add support for advanced parameters
3 participants