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.

Sign up for GitHub

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

Jump to bottom

feat: add vsan file services #1968

Closed
wants to merge 1 commit into from
Closed

feat: add vsan file services #1968

wants to merge 1 commit into from

Conversation

zxinyu08
Copy link
Contributor

@zxinyu08 zxinyu08 commented Aug 1, 2023 •
edited

Description

Feat: add support for vSAN File Service enablement and its domain configurations.

This feature is to support configuring vSAN File service and its domain settings. We added two new fields in compute cluster resource vsan_file_service_enabled and vsan_file_service_conf.
vsan_file_service_enabled - Switch to enable and disable.
vsan_file_service_conf - Configurations for enabling vSAN FS. It contains:
- network - (mandatory) The network selected for vSAN FS.
- vsan_file_service_domain_conf - FS domain configurations.
Testing Details:
We did end to end test from 3 standalone hosts based on the vSphere 7.0 and 8.0. vSAN FS could be enabled with domain configurations, or be disabled by setting switch to false.
Please note in this change, FS domain reconfiguration is not supported, this will be implemented in future.
Following is an example for .tf file.

data "vsphere_host" "hosts" {
  count         = length(var.hosts)
  name          = var.hosts[count.index]
  datacenter_id = data.vsphere_datacenter.datacenter.id
}
data "vsphere_network" "network" {
  name          = "VM Network"
  datacenter_id = data.vsphere_datacenter.datacenter.id
}
resource "vsphere_compute_cluster" "cluster" {
  name          = "Cluster-1"
  datacenter_id = data.vsphere_datacenter.datacenter.id
  vsan_enabled = true
  vsan_file_service_enabled = true
  vsan_file_service_conf {
    network = data.vsphere_network.network.id
    vsan_file_service_domain_conf {
      name = "<your_fs_domain_name>"
      dns_server_addresses = ["1.2.3.4"]
      dns_suffixes = ["example.com"]
      gateway = "192.168.111.1"
      subnet_mask = "255.255.255.0"
      file_server_ip_config {
        fqdn = "h192-168-111-2.example.com"
        ip_address = "192.168.111.2"
        is_primary = true
      }
      file_server_ip_config {
        fqdn = "h192-168-111-3.example.com"
        ip_address = "192.168.111.3"
        is_primary = false
      }
      file_server_ip_config {
        fqdn = "h192-168-111-4.example.com"
        ip_address = "192.168.111.4"
        is_primary = false
      }
    }
  }
  host_system_ids = "${data.vsphere_host.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:

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

Release Note

Release note for CHANGELOG:

...

References

#1965

@zxinyu08 zxinyu08 requested a review from a team as a code owner August 1, 2023 13:34
@github-actions github-actions bot added documentation Type: Documentation provider Type: Provider size/xl Relative Sizing: Extra-Large labels Aug 1, 2023
@tenthirtyam tenthirtyam added area/storage Area: Storage area/clustering Area: Clustering enhancement Type: Enhancement labels Aug 7, 2023
@tenthirtyam tenthirtyam added this to the v2.5.0 milestone Aug 7, 2023
vasilsatanasov
vasilsatanasov suggested changes Oct 9, 2023
View reviewed changes
vsphere/resource_vsphere_compute_cluster.go
Description: "The subnet mask of vsan file server ip config.",
},
"file_server_ip_config": {
Type: schema.TypeSet,
Copy link
Contributor

Choose a reason for hiding this comment

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

Like @appilon suggested in you may use schema.TypeList and MaxItems: 1 in order to ensure a single entry.

vsphere/resource_vsphere_compute_cluster.go
domainConf := fileServiceConf["vsan_file_service_domain_conf"].(*schema.Set).List()[0].(map[string]interface{})

var serverIpConf []vsantypes.VsanFileServiceIpConfig
for _, ip := range domainConf["file_server_ip_config"].(*schema.Set).List() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since thevsan_file_service_domain_conf is optional should not there be something like a null check ?
I also do not see where the d variable is used. If I am not missing something this should be removed from the function arguments.

vsphere/resource_vsphere_compute_cluster.go
if err != nil {
return fmt.Errorf("cannot find vsan file service OVF url, err: %s", err)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add log statement with fileServiceOvfUrl

vsphere/resource_vsphere_compute_cluster.go

if err := vsanclient.DownloadFileServiceOvf(meta.(*Client).vsanClient, meta.(*Client).vimClient, fileServiceOvfUrl); err != nil {
return fmt.Errorf("cannot download vsan file service OVF with url: %s", fileServiceOvfUrl)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need of the else statement sine there is a return in the if statement

vsphere/resource_vsphere_compute_cluster.go
}
}

if !d.Get("vsan_file_service_enabled").(bool) && d.HasChange("vsan_file_service_enabled") {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about if the file service is enabled and there is a change ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this comment and the next one, we're planning to combine two fields as former features (fd/sc) did, if the block start to appear it means we're going to enable file service, if the content of block changed, it means reconfigure file service, and if we remove whole block from .tf file, it means disable file service, however in this PR, for the first version of this feature, only basic enable/disable will be supported, since reconfigure fs need new vmodl API, we'll add inline comments accordingly.

vsphere/resource_vsphere_compute_cluster.go
"vsan_file_service_domain_conf": fsDomainConf,
})

if err := d.Set("vsan_file_service_conf", serviceConf); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

How is vsan_file_service_conf set if vsanConfig.FileServiceConfig.Enabled equals false ?

@tenthirtyam tenthirtyam modified the milestones: v2.5.0, v2.6.0 Oct 9, 2023
@tenthirtyam tenthirtyam linked an issue Nov 10, 2023 that may be closed by this pull request
Add support for vSAN File Service enablement #1965
Open
4 tasks
@tenthirtyam tenthirtyam changed the title feat: add vSAN File Service support feat: add van file services support Nov 10, 2023
@tenthirtyam tenthirtyam marked this pull request as draft November 10, 2023 15:27
@tenthirtyam
Copy link
Collaborator

Converting to draft whilst merged conflicts and open comments are resolved by @zxinyu08.

@tenthirtyam tenthirtyam modified the milestones: v2.6.0, Backlog, v2.7.0 Nov 10, 2023
@tenthirtyam tenthirtyam changed the title feat: add van file services support feat: add vsan file services support Nov 10, 2023
@tenthirtyam tenthirtyam modified the milestones: v2.7.0, On Deck Jan 23, 2024
tenthirtyam
tenthirtyam requested changes Mar 6, 2024
View reviewed changes
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 @SlimYang - could you rebase and review the existing comments so this can be pulled forward?

@tenthirtyam tenthirtyam changed the title feat: add vsan file services support feat: add vsan file services Mar 6, 2024
@tenthirtyam tenthirtyam modified the milestones: v2.8.0, On Deck Apr 29, 2024
@zxinyu08
Copy link
Contributor Author

This PR now only offers limited support in file service configuration, so close this draft PR as of now.

@zxinyu08 zxinyu08 closed this May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tenthirtyam tenthirtyam tenthirtyam requested changes

@vasilsatanasov vasilsatanasov vasilsatanasov requested changes

@SlimYang SlimYang Awaiting requested review from SlimYang

Assignees

@zxinyu08 zxinyu08

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
Milestone
On Deck
Development

Successfully merging this pull request may close these issues.

Add support for vSAN File Service enablement
3 participants
@zxinyu08 @tenthirtyam @vasilsatanasov