-
Notifications
You must be signed in to change notification settings - Fork 5.6k
VMware cluster states + dependencies #43645
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
Conversation
… cluster using the new endpoint
…nter using the new endpoint
…s in a configuration dictionary to a cluster spec
…rding to spec, including configuring VSAN
…rding to spec, including VSAN configuration
…ompares dictionaries
…n datastore on a cluster
…t's storage system
…tastore objects - can retrieve the datastores visible to the root folder, a datacenter, a cluster and an ESXi host - can filter on datastore name and backing disk id (the latter, only if it is a VMFS datastore and the reference is an ESXi host)
…st of datastore representations - can filter on datastore name, backing disk id, backing_disk_scsi_addresses (the last two only apply to VMFS datastore and if the proxy ``esxi`` - pointing to an an ESXi host)
…and a vcenter description
…ses assigned to an entity
… added to the vCenter and assigned to the cluster
|
@alexbleotu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rallytime, @cro and @s0undt3ch to be potential reviewers. |
cachedout
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic! Let's start with the pylint fixes and then we'll start reviewing this in-depth. Thanks @alexbleotu!
https://jenkins.saltstack.com/job/PR/job/salt-pr-lint-n/14781/violations/
cachedout
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the best PRs I've seen in a long while. Great job, @alexbleotu. Just a few small comments in-line.
salt/config/schemas/esxcluster.py
Outdated
| admission_control_policy = AdmissionControlPolicyItem() | ||
| default_vm_settings = DefaultVmSettingsItem() | ||
| hb_ds_candidate_policy = StringItem( | ||
| title='Hartbeat Datastore Candidate Policy', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heartbeat?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
| Python 2.7.9, or newer must be present. This is due to an upstream dependency | ||
| in pyVmomi 6.0 that is not supported in Python versions 2.7 to 2.7.8. If the | ||
| version of Python is not in the supported range, you will need to install an | ||
| earlier version of pyVmomi. See `Issue #29537`_ for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough but are we detecting this condition and warning the user at all? It's a bit much to ask them to read the docs here to determine what could be a very hard-to-detect condition. I wonder if we could improve this at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how to do this though. The only thing I can think of is warnings in tests if there is an incompatibility, but there is no guarantee that everyone will run the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we detect this in __virtual__() and return False, 'State module did not load: Incompatible versions of Python and pyVmomi present', or something along those lines?
Also, this is minor, but I think the mention of Python 2.6 can be removed to reduce potential confusion. Support for Python 2.6 in salt has been removed as of the 2017.7.0 release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to. This needs to happen in all states though and the vsphere execution module. I will add it in this one to esxcluster and I will create a separate PR to fix the rest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I have a change - not as trivial as expected. Testing it out now. Question is if we should add it in the state files or just in the vsphere execution module where pyVmomi is actually used. @rallytime, @cachedout what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok did both. Let's see if the tests pass. From what I remember they don't actually test the __virtual__ function
salt/states/esxcluster.py
Outdated
| WARNING: vsan datastores will not exist until there is at least on host in | ||
| the cluster; the state assumes that the datastore exists and errors out if i | ||
| it doesn't; it's up to the user to accept the error or enable the state run | ||
| when de datastore does exist (grain: vsan_datastore_exists) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
de? the perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several typos; will fix ;)
|
#43674 depends on this |
7014b53 to
f46afa0
Compare
|
@rallytime Re-review please |
rallytime
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done! And thank you for adding tests, too. :)
What does this PR do?
Adds 3 states and all required dependencies (+ a missing external VSAN library from VMware) to be used with the
esxclusterproxy to configure a VMware cluster, rename the cluster's datastore, assign a VSAN license to the clusterFunctions added:
State functions:
Execution functions
Utils functions:
Tests written?
Yes (utils functions only)
Please review Salt's Contributing Guide for best practices.