✨ enables storage policy in failure domain #3219

RenilRaj-BR · 2024-10-08T23:56:49Z

What this PR does:
Currently failure domain doesn't support storage policy and it supports specifying a datastore only. We want to add a feature to enable specifying a storage policy for the failure domain. This provides flexibility and we don't have to specify the exact datastore name. It can also target multiple datastores when we specify a storage policy instead of a specific datastore

linux-foundation-easycla · 2024-10-08T23:56:52Z

The committers listed above are authorized under a signed CLA.

✅ login: RenilRaj-BR / name: Renil Raj (ff15ea9)

k8s-ci-robot · 2024-10-08T23:56:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign killianmuldoon for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2024-10-08T23:56:57Z

Welcome @RenilRaj-BR!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-vsphere 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-vsphere has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-10-08T23:56:58Z

Hi @RenilRaj-BR. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

chrischdi · 2024-10-09T06:43:57Z

apis/v1alpha3/vspherefailuredomain_types.go

+
+	// StoragePolicy is the name of the policy that is used to target a datastore
+	// in which the virtual machine is created/located
+	// +optional
+	StoragePolicy string `json:"storagePolicy,omitempty"`


We should only add this to v1beta1.

@chrischdi ,
I tried adding the storage policy only in v1beta1, but the conversion-gen fails to generate the zz_generated.conversion.go without compilation error due to missing field.
Could you please suggest

@chrischdi ,
I have removed the storagePolicy from v1alpha3 and v1alpha4. Have introduced the manual conversion for the same

chrischdi · 2024-10-09T06:47:52Z

@RenilRaj-BR : you need to have signed the CLA, otherwise we will not be able to merge this.

controllers/vspherecluster_reconciler.go

chrischdi · 2024-10-09T06:50:11Z

/ok-to-test

chrischdi · 2024-10-09T07:40:18Z

/test help

k8s-ci-robot · 2024-10-09T07:40:21Z

@chrischdi: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main
/test pull-cluster-api-provider-vsphere-e2e-vcsim-supervisor-main
/test pull-cluster-api-provider-vsphere-test-main
/test pull-cluster-api-provider-vsphere-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-vsphere-apidiff-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-vsphere-apidiff-main
pull-cluster-api-provider-vsphere-test-main
pull-cluster-api-provider-vsphere-verify-main

In response to this:

/test help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

chrischdi · 2024-10-09T07:40:27Z

/test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main
/test pull-cluster-api-provider-vsphere-e2e-vcsim-supervisor-main

chrischdi · 2024-10-09T13:07:24Z

CC @lubronzhan , @neolit123 , as this falls into govmomi territory :-)

rikatz · 2024-10-10T13:45:35Z

internal/webhooks/vspherefailuredomain.go

@@ -61,6 +61,11 @@ func (webhook *VSphereFailureDomainWebhook) ValidateCreate(_ context.Context, ra
 		allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "Topology", "ComputeCluster"), "cannot be empty if Hosts is not empty"))
 	}

+	// We should either pass a datastore or a storage policy, not both at the same time
+	if obj.Spec.Topology.Datastore != "" && obj.Spec.Topology.StoragePolicy != "" {


hum, why?

I am thinking on a situation where you want a failure domain to use a datastore, but also be compliant with the storage policy rules. As an example, let's say:

Failure domain A - Have DS1 and Storage Policy X, which selects DS1 but adds something like "replicate just 1 time (see vsanDatastore)

Failure domain B - Have DS2 and Storage Policy Y, with the same as above, but for SP Y I want to establish some other rules like replicate 2 times

I think having both datastores and storagepolicies is a valid scenario (unless I forgot something!)

Yeah looks like the priority of these two are handled at the vm creation level

cluster-api-provider-vsphere/pkg/services/govmomi/vcenter/clone.go

Lines 206 to 274 in a2bac7b

var datastoreRef *types.ManagedObjectReference

if vmCtx.VSphereVM.Spec.Datastore != "" {

datastore, err := vmCtx.Session.Finder.Datastore(ctx, vmCtx.VSphereVM.Spec.Datastore)

if err != nil {

return errors.Wrapf(err, "unable to get datastore %s for %q", vmCtx.VSphereVM.Spec.Datastore, ctx)

}

datastoreRef = types.NewReference(datastore.Reference())

spec.Location.Datastore = datastoreRef

}

var storageProfileID string

if vmCtx.VSphereVM.Spec.StoragePolicyName != "" {

pbmClient, err := pbm.NewClient(ctx, vmCtx.Session.Client.Client)

if err != nil {

return errors.Wrapf(err, "unable to create pbm client for %q", ctx)

}

storageProfileID, err = pbmClient.ProfileIDByName(ctx, vmCtx.VSphereVM.Spec.StoragePolicyName)

if err != nil {

return errors.Wrapf(err, "unable to get storageProfileID from name %s for %q", vmCtx.VSphereVM.Spec.StoragePolicyName, ctx)

}

var hubs []pbmTypes.PbmPlacementHub

// If there's a Datastore configured, it should be the only one for which we check if it matches the requirements of the Storage Policy

if datastoreRef != nil {

hubs = append(hubs, pbmTypes.PbmPlacementHub{

HubType: datastoreRef.Type,

HubId: datastoreRef.Value,

})

} else {

// Otherwise we should get just the Datastores connected to our pool

cluster, err := pool.Owner(ctx)

if err != nil {

return errors.Wrapf(err, "failed to get owning cluster of resourcepool %q to calculate datastore based on storage policy", pool)

}

dsGetter := object.NewComputeResource(vmCtx.Session.Client.Client, cluster.Reference())

datastores, err := dsGetter.Datastores(ctx)

if err != nil {

return errors.Wrapf(err, "unable to list datastores from owning cluster of requested resourcepool")

}

for _, ds := range datastores {

hubs = append(hubs, pbmTypes.PbmPlacementHub{

HubType: ds.Reference().Type,

HubId: ds.Reference().Value,

})

}

}

var constraints []pbmTypes.BasePbmPlacementRequirement

constraints = append(constraints, &pbmTypes.PbmPlacementCapabilityProfileRequirement{ProfileId: pbmTypes.PbmProfileId{UniqueId: storageProfileID}})

result, err := pbmClient.CheckRequirements(ctx, hubs, nil, constraints)

if err != nil {

return errors.Wrapf(err, "unable to check requirements for storage policy")

}

if len(result.CompatibleDatastores()) == 0 {

return fmt.Errorf("no compatible datastores found for storage policy: %s", vmCtx.VSphereVM.Spec.StoragePolicyName)

}

// If datastoreRef is nil here it means that the user didn't specify a Datastore. So we should

// select one of the datastores of the owning cluster of the resource pool that matched the

// requirements of the storage policy.

if datastoreRef == nil {

r := rand.New(rand.NewSource(time.Now().UnixNano())) //nolint:gosec // We won't need cryptographically secure randomness here.

ds := result.CompatibleDatastores()[r.Intn(len(result.CompatibleDatastores()))]

datastoreRef = &types.ManagedObjectReference{Type: ds.HubType, Value: ds.HubId}

}

}

@rikatz
removed the validation as it is handled in the vm creation phase

rikatz · 2024-10-14T19:36:48Z

/lgtm

From a provisioning perspective, looks good :)

Will leave for CAPV approvers to do the final review.

Thanks!

k8s-ci-robot · 2024-10-14T19:36:54Z

LGTM label has been added.

Git tree hash: 87891601235f4276d92098b972a0b1061b38cbd5

lubronzhan · 2024-10-15T01:09:47Z

LGTM

apis/v1beta1/vspherefailuredomain_types.go

config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherefailuredomains.yaml

neolit123

this is a small change, so please squash the commits to 1.

neolit123

/lgtm

k8s-ci-robot · 2024-10-15T11:13:50Z

LGTM label has been added.

Git tree hash: d1d412901bfeec0911d17feac7450b4890c55136

neolit123 · 2024-10-15T11:13:56Z

/assign @chrischdi

rikatz · 2024-10-15T12:42:17Z

/lgtm
Thanks

chrischdi · 2024-10-24T10:02:40Z

apis/v1alpha4/zz_generated.conversion.go

@@ -896,14 +896,10 @@ func autoConvert_v1beta1_Topology_To_v1alpha4_Topology(in *v1beta1.Topology, out
 	out.Hosts = (*FailureDomainHosts)(unsafe.Pointer(in.Hosts))
 	out.Networks = *(*[]string)(unsafe.Pointer(&in.Networks))
 	out.Datastore = in.Datastore
+	// WARNING: in.StoragePolicy requires manual conversion: does not exist in peer-type


Don't we need to do the manual conversion for this? (in both api versions)

In both vspherefailuredomain_conversion.go functions we have to do things similar to:

For the ConvertTo function:

https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/blob/main/apis/v1alpha4/vspheremachine_conversion.go#L33-L48

For the ConvertFrom function:

https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/blob/main/apis/v1alpha4/vspheremachine_conversion.go#L61-L64

fabriziopandini · 2024-10-28T19:33:56Z

/hold

Currently failure domain doesn't support storage policy and it supports specifying a datastore only.

It seems that it is not required to have different storagePolicy on each failure domain, and the storagePolicy setting in the vsphere machine template already cover this use case

k8s-ci-robot requested review from fabriziopandini and laozc October 8, 2024 23:56

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Oct 8, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 8, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 8, 2024

RenilRaj-BR changed the title ~~❇️ enables storage policy in failure domain and disables cluster module creation when failure domain is used~~ ✨ enables storage policy in failure domain and disables cluster module creation when failure domain is used Oct 9, 2024

chrischdi reviewed Oct 9, 2024

View reviewed changes

controllers/vspherecluster_reconciler.go Outdated Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 9, 2024

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 9, 2024

RenilRaj-BR changed the title ~~✨ enables storage policy in failure domain and disables cluster module creation when failure domain is used~~ ✨ enables storage policy in failure domain Oct 10, 2024

rikatz reviewed Oct 10, 2024

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 14, 2024

RenilRaj-BR requested review from chrischdi, rikatz and lubronzhan October 14, 2024 13:19

k8s-ci-robot assigned rikatz Oct 14, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 14, 2024

neolit123 reviewed Oct 15, 2024

View reviewed changes

apis/v1beta1/vspherefailuredomain_types.go Outdated Show resolved Hide resolved

config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherefailuredomains.yaml Outdated Show resolved Hide resolved

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 15, 2024

RenilRaj-BR requested a review from neolit123 October 15, 2024 10:27

neolit123 reviewed Oct 15, 2024

View reviewed changes

enables storage policy in failure domain

ff15ea9

RenilRaj-BR force-pushed the main branch from aed902f to ff15ea9 Compare October 15, 2024 10:42

RenilRaj-BR requested a review from neolit123 October 15, 2024 10:44

neolit123 reviewed Oct 15, 2024

View reviewed changes

k8s-ci-robot assigned neolit123 Oct 15, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 15, 2024

k8s-ci-robot assigned chrischdi Oct 15, 2024

chrischdi reviewed Oct 24, 2024

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ enables storage policy in failure domain #3219

✨ enables storage policy in failure domain #3219

RenilRaj-BR commented Oct 8, 2024 •

edited

Loading

linux-foundation-easycla bot commented Oct 8, 2024 •

edited

Loading

k8s-ci-robot commented Oct 8, 2024

k8s-ci-robot commented Oct 8, 2024

k8s-ci-robot commented Oct 8, 2024

chrischdi Oct 9, 2024

RenilRaj-BR Oct 10, 2024

RenilRaj-BR Oct 14, 2024

chrischdi commented Oct 9, 2024

chrischdi commented Oct 9, 2024

chrischdi commented Oct 9, 2024

k8s-ci-robot commented Oct 9, 2024

chrischdi commented Oct 9, 2024

chrischdi commented Oct 9, 2024

rikatz Oct 10, 2024

lubronzhan Oct 11, 2024

RenilRaj-BR Oct 14, 2024 •

edited

Loading

rikatz commented Oct 14, 2024

k8s-ci-robot commented Oct 14, 2024

lubronzhan commented Oct 15, 2024

neolit123 left a comment

neolit123 left a comment

k8s-ci-robot commented Oct 15, 2024

neolit123 commented Oct 15, 2024

rikatz commented Oct 15, 2024

chrischdi Oct 24, 2024

fabriziopandini commented Oct 28, 2024

	var datastoreRef *types.ManagedObjectReference
	if vmCtx.VSphereVM.Spec.Datastore != "" {
	datastore, err := vmCtx.Session.Finder.Datastore(ctx, vmCtx.VSphereVM.Spec.Datastore)
	if err != nil {
	return errors.Wrapf(err, "unable to get datastore %s for %q", vmCtx.VSphereVM.Spec.Datastore, ctx)
	}
	datastoreRef = types.NewReference(datastore.Reference())
	spec.Location.Datastore = datastoreRef
	}

	var storageProfileID string
	if vmCtx.VSphereVM.Spec.StoragePolicyName != "" {
	pbmClient, err := pbm.NewClient(ctx, vmCtx.Session.Client.Client)
	if err != nil {
	return errors.Wrapf(err, "unable to create pbm client for %q", ctx)
	}

	storageProfileID, err = pbmClient.ProfileIDByName(ctx, vmCtx.VSphereVM.Spec.StoragePolicyName)
	if err != nil {
	return errors.Wrapf(err, "unable to get storageProfileID from name %s for %q", vmCtx.VSphereVM.Spec.StoragePolicyName, ctx)
	}

	var hubs []pbmTypes.PbmPlacementHub

	// If there's a Datastore configured, it should be the only one for which we check if it matches the requirements of the Storage Policy
	if datastoreRef != nil {
	hubs = append(hubs, pbmTypes.PbmPlacementHub{
	HubType: datastoreRef.Type,
	HubId: datastoreRef.Value,
	})
	} else {
	// Otherwise we should get just the Datastores connected to our pool
	cluster, err := pool.Owner(ctx)
	if err != nil {
	return errors.Wrapf(err, "failed to get owning cluster of resourcepool %q to calculate datastore based on storage policy", pool)
	}
	dsGetter := object.NewComputeResource(vmCtx.Session.Client.Client, cluster.Reference())
	datastores, err := dsGetter.Datastores(ctx)
	if err != nil {
	return errors.Wrapf(err, "unable to list datastores from owning cluster of requested resourcepool")
	}
	for _, ds := range datastores {
	hubs = append(hubs, pbmTypes.PbmPlacementHub{
	HubType: ds.Reference().Type,
	HubId: ds.Reference().Value,
	})
	}
	}

	var constraints []pbmTypes.BasePbmPlacementRequirement
	constraints = append(constraints, &pbmTypes.PbmPlacementCapabilityProfileRequirement{ProfileId: pbmTypes.PbmProfileId{UniqueId: storageProfileID}})
	result, err := pbmClient.CheckRequirements(ctx, hubs, nil, constraints)
	if err != nil {
	return errors.Wrapf(err, "unable to check requirements for storage policy")
	}

	if len(result.CompatibleDatastores()) == 0 {
	return fmt.Errorf("no compatible datastores found for storage policy: %s", vmCtx.VSphereVM.Spec.StoragePolicyName)
	}

	// If datastoreRef is nil here it means that the user didn't specify a Datastore. So we should
	// select one of the datastores of the owning cluster of the resource pool that matched the
	// requirements of the storage policy.
	if datastoreRef == nil {
	r := rand.New(rand.NewSource(time.Now().UnixNano())) //nolint:gosec // We won't need cryptographically secure randomness here.
	ds := result.CompatibleDatastores()[r.Intn(len(result.CompatibleDatastores()))]
	datastoreRef = &types.ManagedObjectReference{Type: ds.HubType, Value: ds.HubId}
	}
	}

✨ enables storage policy in failure domain #3219

Are you sure you want to change the base?

✨ enables storage policy in failure domain #3219

Conversation

RenilRaj-BR commented Oct 8, 2024 • edited Loading

linux-foundation-easycla bot commented Oct 8, 2024 • edited Loading

k8s-ci-robot commented Oct 8, 2024

k8s-ci-robot commented Oct 8, 2024

k8s-ci-robot commented Oct 8, 2024

chrischdi Oct 9, 2024

Choose a reason for hiding this comment

RenilRaj-BR Oct 10, 2024

Choose a reason for hiding this comment

RenilRaj-BR Oct 14, 2024

Choose a reason for hiding this comment

chrischdi commented Oct 9, 2024

chrischdi commented Oct 9, 2024

chrischdi commented Oct 9, 2024

k8s-ci-robot commented Oct 9, 2024

chrischdi commented Oct 9, 2024

chrischdi commented Oct 9, 2024

rikatz Oct 10, 2024

Choose a reason for hiding this comment

lubronzhan Oct 11, 2024

Choose a reason for hiding this comment

RenilRaj-BR Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

rikatz commented Oct 14, 2024

k8s-ci-robot commented Oct 14, 2024

lubronzhan commented Oct 15, 2024

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 15, 2024

neolit123 commented Oct 15, 2024

rikatz commented Oct 15, 2024

chrischdi Oct 24, 2024

Choose a reason for hiding this comment

fabriziopandini commented Oct 28, 2024

RenilRaj-BR commented Oct 8, 2024 •

edited

Loading

linux-foundation-easycla bot commented Oct 8, 2024 •

edited

Loading

RenilRaj-BR Oct 14, 2024 •

edited

Loading