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

New resource: vsphere_storage_drs_vm_override #450

Merged
merged 9 commits into from
Apr 5, 2018

Conversation

vancluever
Copy link
Contributor

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 the datastore_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 and sdrs_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 like sdrs_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.

@vancluever vancluever added the new-resource Feature: New Resource label Apr 4, 2018
@vancluever vancluever requested a review from a team April 4, 2018 18:14
@vancluever vancluever force-pushed the f-storage-drs-vm-config branch 2 times, most recently from 9eb4e4c to c06a697 Compare April 4, 2018 21:53
@vancluever vancluever added the enhancement Type: Enhancement label Apr 4, 2018
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".
@vancluever vancluever force-pushed the f-storage-drs-vm-config branch from c06a697 to bb1200b Compare April 4, 2018 23:19
We changed the default automation level to manual, so fixing this test
so that it has meaning still.
@vancluever vancluever force-pushed the f-storage-drs-vm-config branch from bb1200b to 2738d84 Compare April 4, 2018 23:39
Copy link
Contributor

@bill-rich bill-rich left a comment

Choose a reason for hiding this comment

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

LGTM

@vancluever vancluever merged commit d98e5ab into master Apr 5, 2018
@vancluever vancluever deleted the f-storage-drs-vm-config branch April 6, 2018 21:02
@ghost ghost locked and limited conversation to collaborators Apr 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Type: Enhancement new-resource Feature: New Resource
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants