feat: add vsan fault domain support

[zxinyu08]

Description

Feat: add support for vSAN Fault Domain Service.

This feature is to support configuring vSAN Fault Domain. We added a new field vsan_fault_domains withfault_domain, including name and host_ids in compute_cluster resource. To remove all vSAN fault domains info, we need remove vsan_fault_domains whole block directly.
Testing Details:
We did end to end test from 2 standalone hosts based on the vSphere 7.0 and 8.0.
Following is an example for .tf file:

data "vsphere_host" "faultdomain1_hosts" {
  count         = length(var.faultdomain1_hosts)
  name          = var.faultdomain1_hosts[count.index]
  datacenter_id = data.vsphere_datacenter.datacenter.id
}

data "vsphere_host" "faultdomain2_hosts" {
  count         = length(var.faultdomain2_hosts)
  name          = var.faultdomain2_hosts[count.index]
  datacenter_id = data.vsphere_datacenter.datacenter.id
}

data "vsphere_datacenter" "datacenter" {
  name = "DataCenter"
}

data "vsphere_host" "hosts" {
  count         = length(var.hosts)
  name          = var.hosts[count.index]
  datacenter_id = data.vsphere_datacenter.datacenter.id
}
resource "vsphere_compute_cluster" "cluster" {
  name          = "test"
  datacenter_id = data.vsphere_datacenter.datacenter.id
  host_system_ids = "${data.vsphere_host.hosts.*.id}"

  vsan_enabled       = true
  vsan_fault_domains {
    fault_domain {
      name = "fd1"
      host_ids = "${data.vsphere_host.faultdomain1_hosts.*.id}"
    }
    fault_domain {
      name = "fd2"
      host_ids = "${data.vsphere_host.faultdomain2_hosts.*.id}"
    }
  }
}

Acceptance tests

• Have you added an acceptance test for the functionality being added?
• Have you run the acceptance tests on this branch?

Output from acceptance testing:

make testacc TESTARGS="-run=TestAccResourceVSphereComputeCluster_faultDomain"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccResourceVSphereComputeCluster_faultDomain -timeout 360m
...
?   	github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/vmworkflow	[no test files]
=== RUN   TestAccResourceVSphereComputeCluster_faultDomain
--- PASS: TestAccResourceVSphereComputeCluster_faultDomain (253.47s)
PASS
ok  	github.com/hashicorp/terraform-provider-vsphere/vsphere	255.387s
testing: warning: no tests to run
...
PASS
ok  	github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/virtualdevice	1.603s [no tests to run]

Release Note

Release note for CHANGELOG:

...

References

#1967

[appilon]

I have some general questions, but this PR seems overall pretty simple and shouldn't take much to get merged.

[tenthirtyam]

I'll be reviewing this one today.

[tenthirtyam]

@zxinyu08 - a couple questions:

• Should this be singular fault_domain versus the plural?

vsan_fault_domain {
    name = "rack-1"
    host_ids = [data.vsphere_host.rack1.*.id]
  }
vsan_ fault_domain {
    name = "rack-2"
    host_ids = [data.vsphere_host.rack2.*.id]
  }

If plural, I would see this more like:

vsan_fault_domains {
    fault_domain = {
        name = "rack-1"
        host_ids = [data.vsphere_host.rack1.*.id]
    }
    fault_domain = {
        name = "rack-2"
        host_ids = [data.vsphere_host.rack2.*.id]
    }
    fault_domain = {
        name = "rack-3"
        host_ids = [data.vsphere_host.rack3.*.id]
    }
}

• This configuration is specific to vSAN. Should this be vsan_fault_domain?
• Can you confirm that renaming of the fault domains was tested successfully.?
• Can you confirm that add, remove, reassign of hosts to fault domains was tested successfully?
• Perhaps I don't see if, but is there a dependency set that vsan_enabled must be true to allow fault_domains to be used?
• If vsan_enabled is set to false and the fault domains still exist, what would occur?
• If the [data.vsphere_host_foo.*.id] and [data.vsphere_host_foo.*.id] returns an ID for the same host I assume the API will throw and error. Or should that be caught by the provider first?
• If the number of hosts in [data.vsphere_host_foo.*.id] and [data.vsphere_host_foo.*.id] are not equal sized, should it at least be logged? This would not be a recommended configuration.
• The more I think about it, I like the mural example above, because then you could consider expecting a minimum of 3 fault domains as best practice and then logging if less than? Just an idea.

[tenthirtyam]

Marking as draft whilst the prior open comments are answered. 😃

[tenthirtyam]

Hi @zxinyu08 - could you take a look at my questions from the review? Let me know if you have any questions!

Reference: #1969 (review)

[zxinyu08]

Hi @tenthirtyam, we've addressed the main comment by modifying the field structure to plural way. And for other questions:

1. This configuration is specific to vSAN. Should this be vsan_fault_domain?
   A: Done

2. Can you confirm that renaming of the fault domains was tested successfully?
   A: tested in e2e test, passed

3. Can you confirm that add, remove, reassign of hosts to fault domains was tested successfully?
   A: tested in e2e test, passed

4. Perhaps I don't see if, but is there a dependency set that vsan_enabled must be true to allow fault_domains to be used?

5. If vsan_enabled is set to false and the fault domains still exist, what would occur?
   A: yes, should be, it's a good point, since other configurations are also lack of such check, we plan to fix it in another PR due to it's a common issue for vSAN confs. If users set vsan ks to false, vsan will be disabled directly, other sub-configurations will be disabled automatically, but tfstate file won't be changed if other sub-configurations are not changed in .tf file, it still tracks the former status. We'll fix by adding a vsan check in a new PR.

6. If the [data.vsphere_host_foo..id] and [data.vsphere_host_foo..id] returns an ID for the same host I assume the API will throw and error. Or should that be caught by the provider first?
   A: It's caught by provider now and FD configuration will be blocked.

7. If the number of hosts in [data.vsphere_host_foo..id] and [data.vsphere_host_foo..id] are not equal sized, should it at least be logged? This would not be a recommended configuration.

8. The more I think about it, I like the mural example above, because then you could consider expecting a minimum of 3 fault domains as best practice and then logging if less than? Just an idea.
   A: Since these two are recommended not must have, we added check for them and WARNING msg will be logged.

[tenthirtyam]

The functionality looks OK for me, just need to add code to skip the e2e test until we have a infra for it on the Acc environment. Several more environment variables would need be introduced.

Agree @vasilsatanasov?

[tenthirtyam]

LGTM! 🚀

Post merge, follow-up is required regarding setting it to skip the e2e test until the CI can support it and the vars that will need to be introduced, if added to e2e.

[tenthirtyam]

@zxinyu08 - could you rebase from origin/main and resolve the conflicts?

[zxinyu08]

LGTM! 🚀

Post merge, follow-up is required regarding setting it to skip the e2e test until the CI can support it and the vars that will need to be introduced, if added to e2e.

Hi @tenthirtyam @vasilsatanasov, I think acc test in this PR for fault domain could be supported based on current CI setup. No extra host outside of cluster needed, and fault domain is supported in 7.0, it should be okay as expected.

[tenthirtyam]

Thanks! I'll get this merged today and we can prepare for a release next week with the current payload.

[github-actions]

This functionality has been released in v2.6.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

[github-actions]

This functionality has been released in v2.6.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.