-
Notifications
You must be signed in to change notification settings - Fork 455
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
New resource: vsphere_storage_drs_vm_override #450
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9eb4e4c
to
c06a697
Compare
Pending test and docs. This is a new resource for specifying Storage DRS overrides for a virtual machine, such as in the case where you want to disable SDRS for a VM that is not using it, change the automation level, or the VM affinity for a VM's disks.
Refactor the resource to make it a bit more sane, it was probably overly-functional before and some parts that could be re-used were not. This makes it better ahead of adding the tests.
Adding what should be functional tests - functional in that they will run, not that they will pass. ;) Also, removed the default for sdrs_automation_level. This is a string and not a string pointer, as such it was assumed that this could not be ommitted, but it is tagged as omitempty. Omitting it from the config should ensure it uses the cluster setting.
This allows us to see the embedded bools from the pointers, and also determine if the datastore cluster is missing during the destroy check.
Needs review, but should be complete. Last thing pending for this resource is a test for import.
Saving datastore cluster and VM IDs in read now as well to add completeness to the import process. Update seems to be broken, some nil values do not seem to be saving and we need to figure out why that is. Depending on the outcome we might just ForceNew all attributes versus update.
This fixes the systemic issues with dealing with TF 3-state bools and working with diffs in Terraform core. There are currently a couple of issues here: * GetOkExists does not function correctly when there are values in the state. If a value enters state it "exists", even when a value is being removed in config. * Diffs from zero values to nils do not register. This is because the diff code in the SDK was not designed with consideration for nils being a valid state - nils are converted to zero values and then ignored when they are compared, as the value will then be equal to the zero value that exists in state. We fix this for bools by using string fields, and then normalizing and validating all possible values to 3 values that will always match - "true", "false", and an empty string. This exploits current HCL functionality as well where mapstructure converts bareword bools to string values when the field type is TypeString. Note that this is not an issue that just applies to this resource. It's probably prudent to go back through some of the other resources (namely standard and distributed port groups) and check to see if we need to fix these things there as well. This also may need to be fixed for integer values as well, as all 3 of these issues apply there as well (setting nil will save as 0, going from 0 => nil will not trigger a diff). The issue with GetOkExists applies to *all* Terraform things and is a larger issue, even with strings.
The old name is more relevant to the API, but in the vSphere client these are referred to as "overrides". We also use this language a lot in the documentation, so it just makes sense to call this resource "override" instead of "config".
c06a697
to
bb1200b
Compare
We changed the default automation level to manual, so fixing this test so that it has meaning still.
bb1200b
to
2738d84
Compare
bill-rich
approved these changes
Apr 5, 2018
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.
LGTM
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds the
vsphere_storage_drs_vm_override
resource, which can be used to add a Storage DRS override when working with a VM that has been placed in a datastore cluster, but is not making use of Storage DRS via thedatastore_cluster_id
attribute. It also can be used to change the automation levels and VMDK affinity for virtual machines when SDRS is in use.This resource makes use of a "stringified" workaround for 3-state bools, ie:
*bool
API structures where all states are meaningful. In this case, the empty (nil
) state is significant for two attributes that would otherwise be bools:sdrs_enabled
andsdrs_intra_vm_affinity
, the absence of which tells SDRS to use the datastore cluster settings (no override). The workaround does not change the UX: something likesdrs_enabled = true
still works as intended. This workaround may need to be applied other resources in the provider too. Related: hashicorp/terraform#17557, hashicorp/terraform#17780.