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 esa support #1874

Merged
merged 10 commits into from
Oct 18, 2023
Merged

Conversation

zxinyu08
Copy link
Contributor

@zxinyu08 zxinyu08 commented Mar 29, 2023

Description

Feat: Add vSAN ESA support

This feature is to support configuring vSAN ESA cluster. We added a new field vsan_esa_enabled in compute cluster resource. Unit tests for vSAN ESA service is also added.

Testing Details:
We did end to end test from 3 standalone hosts based on the vSphere 8.0. Test cases are as follows:

  1. create cluster with vsan and vsan esa enabled.
  2. update cluster to vsan enabled but vsan esa disabled -> failed as vsan esa could not be set separately.
  3. update cluster to both vsan and vsan esa disabled.
  4. update cluster to only vsan enabled.
  5. update cluster to vsan enabled and vsan esa enabled -> failed as vsan esa could not be set separately.

For example, a cluster could be created with vSAN ESA enabled by applying .tf file which contains the following settings.

resource "vsphere_compute_cluster" "compute_cluster" {
  name            = "terraform-compute-cluster-test"
  datacenter_id   = "${data.vsphere_datacenter.datacenter.id}"
  host_system_ids = "${data.vsphere_host.hosts.*.id}"
  vsan_enabled = true
  vsan_esa_enabled = true
}

Acceptance tests

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

Output from acceptance testing:

$make testacc TESTARGS="-run=TestAccResourceVSphereComputeCluster_vsanEsaEnabled"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccResourceVSphereComputeCluster_vsanEsaEnabled -timeout 240m
?       github.com/hashicorp/terraform-provider-vsphere [no test files]
=== RUN   TestAccResourceVSphereComputeCluster_vsanEsaEnabled
PASS: TestAccResourceVSphereComputeCluster_vsanEsaEnabled (320.63s)
PASS
ok      github.com/hashicorp/terraform-provider-vsphere/vsphere 320.654s
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/administrationroles    [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/clustercomputeresource  [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/computeresource [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/contentlibrary  [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/customattribute [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/datacenter      [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/datastore       [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/dvportgroup     [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/envbrowse       [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/folder  [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/hostsystem      [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/network [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/nsx     [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/ovfdeploy       [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/provider        [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/resourcepool    [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/spbm    [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/storagepod      [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/structure       [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/testhelper      [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/utils   [no test files]
?       github.com/hashicorp/terraform-provider-vsphere/vsphere/internal/helper/vappcontainer   [no test files]
testing: warning: no tests to run
PASS
...

Release Note

Release note for CHANGELOG:

...

References

#1899

@zxinyu08 zxinyu08 requested a review from a team as a code owner March 29, 2023 11:59
@github-actions github-actions bot added documentation Type: Documentation provider Type: Provider size/m Relative Sizing: Medium labels Mar 29, 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.

Initial thought is that there should be a version check for 8.0 or later.

@tenthirtyam tenthirtyam added area/clustering Area: Clustering enhancement Type: Enhancement labels Mar 29, 2023
@tenthirtyam tenthirtyam added this to the Backlog milestone Mar 29, 2023
@tenthirtyam tenthirtyam marked this pull request as draft March 29, 2023 16:01
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.

@zxinyu08 and @SlimYang - could you open an enhancement issue to link to this pull request?

Also, since vSAN ESA is only for 8.0 and later there a version check should be considered. Something like:

if !d.Get("vsan_esa_enabled").(bool) {
    if version.Older(viapi.VSphereVersion{Product: version.Product, Major: 8, Minor: 0}) {
    return fmt.Errorf("vsan_esa_enabled is only supported on vSphere 8.0 and higher")
    }
}

Marking as draft.

@appilon
Copy link
Contributor

appilon commented Mar 29, 2023

Hello,

Thank you for this contribution. We will be focussing on some technical debt in the provider. One of those focusses will be coming up with a better way to manage version checking/flagging of features. With that in mind, this PR will likely need to wait until we come up with that design.

@zxinyu08
Copy link
Contributor Author

@zxinyu08 and @SlimYang - could you open an enhancement issue to link to this pull request?

Also, since vSAN ESA is only for 8.0 and later there a version check should be considered. Something like:

if !d.Get("vsan_esa_enabled").(bool) {
    if version.Older(viapi.VSphereVersion{Product: version.Product, Major: 8, Minor: 0}) {
    return fmt.Errorf("vsan_esa_enabled is only supported on vSphere 8.0 and higher")
    }
}

Marking as draft.

Thank you @tenthirtyam @appilon for reviewing and it's good to know there will be an enhancement for version checking. We're considering if we could get this merged first with above version checking added as a temp workaround. When the new design is finished, we'll make change accordingly, if this is not a hard dependency. We'll also create an issue to keep this tracked. Please help review if there's any concern? Thanks.

@github-actions github-actions bot added the size/l Relative Sizing: Large label Apr 10, 2023
@tenthirtyam tenthirtyam marked this pull request as ready for review April 10, 2023 12:45
@tenthirtyam tenthirtyam self-requested a review April 10, 2023 12:45
@tenthirtyam tenthirtyam dismissed their stale review April 10, 2023 12:46

changes made

@tenthirtyam
Copy link
Collaborator

Marked as Ready for Review.

@zxinyu08
Copy link
Contributor Author

Hi folks, I just patch a fix in latest commit. The issue is -
terraform apply fails when vsan_esa_enabled is set as true and vsan_unmap_enabled is unset. It occurs when we enable vSAN ESA for the first time. The root casue is that vSAN unmap service is by default enabled when vSAN ESA is enabled.

However, the value of vsan_unmap_enabled will be computed after apply, which means that the value will be null, if this field is not set manually. And null value of bool type will be seen as false. So the pre-check in resourceVSphereComputeClusterApplyVsanConfig() will return error directly when vsan_esa_enabled is set as true and vsan_unmap_enabled is unset.

Resolution:
Add GetOkExists() to check whether vsan_unmap_enabled is set manually in .tf file or is computed after apply. If it's null for the first time apply, we'll set it to true by default.

@tenthirtyam tenthirtyam modified the milestones: Backlog, v2.5.0 May 4, 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.

@tenthirtyam tenthirtyam removed the size/m Relative Sizing: Medium label 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.

I have some comments around the nuance of how we want to handle the Boolean logic. However the "READ" side of the implemenation is missing. Please see the ReadFunc and ensure vSphere API request is copied back into state (this is how Terraform ensures what exists on the backend matches when the user specified). I apologize I missed the d.Set call.

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 Show resolved Hide resolved
vsphere/resource_vsphere_compute_cluster.go Outdated Show resolved Hide resolved
@tenthirtyam tenthirtyam changed the title feat: add vSAN ESA support feat: add vsan era support Oct 17, 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! 🚀

I'll add in the CHANGELOG.md update prior to approval from @iBrandyJackson.

@tenthirtyam tenthirtyam changed the title feat: add vsan era support feat: add vsan esa support Oct 17, 2023
@tenthirtyam tenthirtyam added vsphere/v7 vSphere 7.0 vsphere/v8 vSphere 8.0 area/storage Area: Storage and removed size/m Relative Sizing: Medium labels Oct 17, 2023
Updated `CHANGELOG.md`.

Signed-off-by: Ryan Johnson <[email protected]>
@tenthirtyam
Copy link
Collaborator

@iBrandyJackson - the CHANGELOG.md has been update and is ready for your approval.

@iBrandyJackson iBrandyJackson merged commit 7c5fae0 into hashicorp:main Oct 18, 2023
4 checks passed
@iBrandyJackson iBrandyJackson linked an issue Oct 18, 2023 that may be closed by this pull request
4 tasks
@tenthirtyam tenthirtyam linked an issue Nov 10, 2023 that may be closed by this pull request
4 tasks
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 Nov 18, 2023
@hashicorp hashicorp unlocked this conversation Nov 30, 2023
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/l Relative Sizing: Large vsphere/v7 vSphere 7.0 vsphere/v8 vSphere 8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for configuring vSAN ESA cluster. Add support for vSAN ESA enablement
5 participants