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

feat: add vsan stretched cluster support #1885

Merged
merged 6 commits into from
Nov 9, 2023

Conversation

zxinyu08
Copy link
Contributor

@zxinyu08 zxinyu08 commented Apr 17, 2023

Description

This feature is to support enabling and disabling vSAN stretched cluster. We added a new field vsan_stretched_cluster in compute cluster resource for its configurations.

  • preferred_fault_domain_host_ids - (Mandatory) The managed object IDs of the hosts to put in the first fault domain.
  • secondary_fault_domain_host_ids - (Mandatory) The managed object IDs of the hosts to put in the second fault domain.
  • witness_node - (Mandatory) The managed object IDs of the host selected as witness node when enable stretched cluster.
  • preferred_fault_domain_name - (Optional) The name of first fault domain. Default is Preferred.
  • secondary_fault_domain_name - (Optional) The name of second fault domain. Default is Secondary.

Unit test for vSAN Stretched cluster enable/disable is added.

Testing Details:

We did end to end test based on the vSphere 7.0 and 8.0. A vSAN cluster could be set up from 3 standalone hosts and be converted to vSAN stretched cluster later by applying .tf file which contains the following settings, witness node is under datacenter but not in cluster.

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_host" "witness_node" {
  name          = "10.78.86.87"
  datacenter_id = data.vsphere_datacenter.datacenter.id
}

resource "vsphere_compute_cluster" "cluster" {
  name          = "cluster-test"
  datacenter_id = data.vsphere_datacenter.datacenter.id
  drs_enabled              = false
  drs_automation_level     = "fullyAutomated"
  ha_enabled               = false
  host_system_ids = "${data.vsphere_host.hosts.*.id}"

  vsan_enabled       = true
  vsan_stretched_cluster {
    preferred_fault_domain_host_ids = [data.vsphere_host.roothost1.id]
    secondary_fault_domain_host_ids = [data.vsphere_host.roothost2.id]
    witness_node = data.vsphere_host.roothost3.id
   }
}

To disable vSAN stretched cluster, we need remove vsan_stretched_cluster block. Some Negative E2E cases are passed: lack of mandatory fields, duplicately enable/disable.

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_vsanStretchedCluster"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccResourceVSphereComputeCluster_vsanStretchedCluster -timeout 360m
?       github.com/hashicorp/terraform-provider-vsphere [no test files]
=== RUN   TestAccResourceVSphereComputeCluster_vsanStretchedCluster
--- PASS: TestAccResourceVSphereComputeCluster_vsanStretchedCluster (429.86s)
PASS
ok      github.com/hashicorp/terraform-provider-vsphere/vsphere 430.357s
...

Release Note

`r/vsphere_compute_cluster`: Added support for vSAN stretched clusters.

References

Closes #1900

@zxinyu08 zxinyu08 requested a review from a team as a code owner April 17, 2023 08:56
@github-actions github-actions bot added documentation Type: Documentation provider Type: Provider size/l Relative Sizing: Large labels Apr 17, 2023
@tenthirtyam tenthirtyam changed the title Add feat: support vSAN Stretched cluster feat: add support for vSAN Stretched cluster Apr 18, 2023
@tenthirtyam tenthirtyam changed the title feat: add support for vSAN Stretched cluster feat: add support for vSAN stretched cluster Apr 18, 2023
@tenthirtyam tenthirtyam changed the title feat: add support for vSAN stretched cluster feat: add vSAN stretched cluster support Apr 18, 2023
@tenthirtyam
Copy link
Collaborator

Hi @zxinyu08 👋 -

Could we ask that you open an enhancement issue? We can then link the pull request to the enhancement for tracking purposes.

Ryan Johnson
Senior Staff Solutions Architect | Product Engineering @ VMware, Inc.

@hashicorp-cla
Copy link

hashicorp-cla commented May 31, 2023

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the size/xl Relative Sizing: Extra-Large label May 31, 2023
@zxinyu08 zxinyu08 force-pushed the feat/stretched_cluster branch 3 times, most recently from 47a40a2 to 8162df4 Compare June 7, 2023 03:02
@tenthirtyam tenthirtyam added area/storage Area: Storage area/clustering Area: Clustering enhancement Type: Enhancement and removed size/l Relative Sizing: Large labels Aug 7, 2023
@tenthirtyam tenthirtyam added this to the v2.5.0 milestone Aug 7, 2023
Copy link
Contributor

@appilon appilon left a comment

Choose a reason for hiding this comment

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

Need some changes but I think this PR shouldn't take too long to get ready for merging.

vsphere/resource_vsphere_compute_cluster.go Show resolved Hide resolved
vsphere/resource_vsphere_compute_cluster.go Outdated Show resolved Hide resolved
vsphere/resource_vsphere_compute_cluster.go Outdated Show resolved Hide resolved
vsphere/resource_vsphere_compute_cluster.go Outdated Show resolved Hide resolved
vsphere/resource_vsphere_compute_cluster.go Outdated Show resolved Hide resolved
vsphere/resource_vsphere_compute_cluster.go Outdated Show resolved Hide resolved
vsphere/resource_vsphere_compute_cluster.go Outdated Show resolved Hide resolved
vsphere/resource_vsphere_compute_cluster.go Outdated Show resolved Hide resolved
resource "vsphere_compute_cluster" "compute_cluster" {
name = "testacc-compute-cluster"
datacenter_id = data.vsphere_datacenter.rootdc1.id
host_system_ids = [data.vsphere_host.roothost1.id, data.vsphere_host.roothost2.id]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unfamiliar with vSAN in general, but for the record the CI and expected testing environment is setup as follows:

roothost1 Physical/main ESXi host expected to be in a default cluster we created on CI.
roothost2 nested ESXi host expected to be in a default cluster we created on CI.
roothost3 nested ESXi host you can feel free to move into a test specific cluster such as this one.
roothost4 same as 3.

Probably you don't want to put roothost1 in this cluster for sure, we can DM offline to get a sense for what you need for these tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also host 2,3,4 are only on a private network, 1 is on both public and private. Doesn't seem to affect the other vSAN tests in the past but thought I should add that context.

Copy link
Contributor Author

@zxinyu08 zxinyu08 Aug 23, 2023

Choose a reason for hiding this comment

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

We also have the setup of mixed nested and physical ESXis, but the performance might be unstable, so I'll replace hosts with roothost2/3/4 in case, and let's have a try to see if anything else needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To test stretched cluster, we could use roothost3 and roothost4 in primary and secondary fault domains, but we still need one more host outside of any cluster to be witness node, roothost1 and 2 doesn't meet this condition, so maybe this acceptance test will be failed as expected in CI, do we need keep code here and skip it? Or just let it failed and ignore its result.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest the failure should be expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO this vSAN case requires specific testing infrastructure which is not supported by the Acceptance environment. Looks we should consider skipping the test instead of accepting that it fails. What about introducing some env variables like TF_VSPHERE_VSAN_HOST_1, TF_VSPHERE_VSAN_HOST_2 and TF_VSPHERE_VSAN_WITNESS_HOST which will be the minimal setup for the stretched cluster
and add some info in the skip message or in the function which checks if they are present and set to describe what are the requirements for those hosts ?

@tenthirtyam tenthirtyam modified the milestones: v2.5.0, v2.6.0 Oct 9, 2023
Copy link
Contributor

@vasilsatanasov vasilsatanasov left a comment

Choose a reason for hiding this comment

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

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 should be introduced

@tenthirtyam
Copy link
Collaborator

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 should be introduced

Agreed!

@tenthirtyam tenthirtyam changed the title feat: add vSAN stretched cluster support feat: add vsan stretched cluster support Nov 9, 2023
Copy link
Collaborator

@tenthirtyam tenthirtyam left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

Post merge, follow-up on Vasil's comment is required regarding setting it to skip the e2e test until the CI can support it and the vars that need to be introduced.

Signed-off-by: Ryan Johnson <[email protected]>
@tenthirtyam tenthirtyam merged commit c644fcd into hashicorp:main Nov 9, 2023
4 checks passed
Copy link

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!

1 similar comment
Copy link

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!

Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/clustering Area: Clustering area/storage Area: Storage documentation Type: Documentation enhancement Type: Enhancement provider Type: Provider size/xl Relative Sizing: Extra-Large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for vSAN stretched cluster enablement
6 participants