CFE-1047: Add the field "CapacityReservationGroupID" to "AzureMachineProviderSpec" in openshift/api

[anirudhAgniRedhat]

This PR adds the support of capacity reservation group in AzureMachineProviderSpec. A new field, CapacityReservationGroupID, is added. This ID is used while creating the corresponding Azure VM and the VM is added to the capacity reservation group.

The capacity reservation group should already exist. Once the VM is created and associated to the capacity reservation group it cannot be removed from the group. Similarly, if a VM is created without any association to a capacity reservation group then it cannot be associated to one later on. Thus, CapacityReservationGroupID field is made immutable.

[openshift-ci-robot]

@anirudhAgniRedhat: This pull request references CFE-1046 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.16.0" version, but no target version was set.

In response to this:

This PR adds the support of capacity reservation group in AzureMachineSpec. A new field, CapacityReservationGroupID, is added. This ID is used while creating the corresponding Azure VM and the VM is added to the capacity reservation group.

The capacity reservation group should already exist. Once the VM is created and associated to the capacity reservation group it cannot be removed from the group. Similarly, if a VM is created without any association to a capacity reservation group then it cannot be associated to one later on. Thus, CapacityReservationGroupID field is made immutable.

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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci]

Hello @anirudhAgniRedhat! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

[openshift-ci-robot]

@anirudhAgniRedhat: This pull request references CFE-1047 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

This PR adds the support of capacity reservation group in AzureMachineSpec. A new field, CapacityReservationGroupID, is added. This ID is used while creating the corresponding Azure VM and the VM is added to the capacity reservation group.

The capacity reservation group should already exist. Once the VM is created and associated to the capacity reservation group it cannot be removed from the group. Similarly, if a VM is created without any association to a capacity reservation group then it cannot be associated to one later on. Thus, CapacityReservationGroupID field is made immutable.

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 openshift-eng/jira-lifecycle-plugin repository.

[arkadeepsen]

This PR adds the support of capacity reservation group in AzureMachineSpec.

AzureMachineSpec -> AzureMachineProviderSpec

[JoelSpeed]

What is the format of the reservation group ID? What characters are valid? What is the minimal and maximal length?

[anirudhAgniRedhat]

@JoelSpeed I found this is the format for Capacity reservation Group ID

I guess resourceGroupName and CapacityReservationGroupName may vary on differen Users.
subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Compute/capacityReservationGroups/<CapacityReservationGroupName>

Also didn't found maximum and minimum length for ID in AzureDocs.

Ref: https://learn.microsoft.com/en-us/azure/virtual-machines/capacity-reservation-create?tabs=cli1%2Capi1%2Capi2#create-a-capacity-reservation-1

[JoelSpeed]

In the upstream implementation, there is a validation implemented via a webhook, can you explain what that validation is doing?

[anirudhAgniRedhat]

if err := webhookutils.ValidateImmutable(
		field.NewPath("spec", "capacityReservationGroupID"),
		old.Spec.CapacityReservationGroupID,
		m.Spec.CapacityReservationGroupID); err != nil {
		allErrs = append(allErrs, err)
	}

this insures that the field is immutable and cannot be updated once set.

[JoelSpeed]

There's also a validation that it is a valid azure ID no?

[anirudhAgniRedhat]

Yes, there is a validation on pattern, that the input must be string having length more than 0,
Also the string should start with "/".

The keys which are used should be among "subscriptions", "providers" and "resourcegroups" following by valid ID or names respectively.

the ideal format for ID is /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Compute/capacityReservationGroups/<CapacityReservationGroupName>

I did not find check that the input provided is valid ID. It Just checks for the format of the input provided, does not insure the IDs or names provides are correct.

https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/4698/files#diff-222a5b91649bbd8b784ec4d5476460390cc2f7e5839e63a968d719b3484ace68R467

@JoelSpeed I have added the commit to call out validations in doc.

[JoelSpeed]

Yeah so I think we want to use the ParseResourceID as well, but, for that to pass, I need to understand, as an end user, what a valid input is. So, I'd like to have some text that explains what a valid input to that validation is

[JoelSpeed]

I see you've updated the description, looks good

[anirudhAgniRedhat]

/retest-required

[openshift-ci-robot]

@anirudhAgniRedhat: This pull request references CFE-1047 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

This PR adds the support of capacity reservation group in AzureMachineProviderSpec. A new field, CapacityReservationGroupID, is added. This ID is used while creating the corresponding Azure VM and the VM is added to the capacity reservation group.

The capacity reservation group should already exist. Once the VM is created and associated to the capacity reservation group it cannot be removed from the group. Similarly, if a VM is created without any association to a capacity reservation group then it cannot be associated to one later on. Thus, CapacityReservationGroupID field is made immutable.

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 openshift-eng/jira-lifecycle-plugin repository.

[JoelSpeed]

@anirudhAgniRedhat Do you have an implementation for this API that's ready to merge shortly after?

[anirudhAgniRedhat]

@JoelSpeed I will start working on its implementation from now onwards as we merge this PR.

[arkadeepsen]

@JoelSpeed do these nits look good to be added?

[JoelSpeed]

Yeah lets get those added

Also, since we are close to the feature freeze date, I'd like to see the feature tested end to end by QE before we merge the API, else we risk shipping an API in one release with no implementation. Please link a WIP PR here once you have it

[arkadeepsen]

Yeah lets get those added

Also, since we are close to the feature freeze date, I'd like to see the feature tested end to end by QE before we merge the API, else we risk shipping an API in one release with no implementation. Please link a WIP PR here once you have it

Sure @JoelSpeed

[anirudhAgniRedhat]

Yeah lets get those added

Also, since we are close to the feature freeze date, I'd like to see the feature tested end to end by QE before we merge the API, else we risk shipping an API in one release with no implementation. Please link a WIP PR here once you have it

@JoelSpeed This is WIP PR for its implementation openshift/machine-api-provider-azure#107

[JoelSpeed]

Drop these, they won't be able to be implemented within MAPI

[JoelSpeed]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anirudhAgniRedhat, JoelSpeed

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [JoelSpeed]

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

[anirudhAgniRedhat]

@JoelSpeed The PR requireacknowledge-critical-fixes-only label to merge it before 4.16 release If you feel we Should merge it Could you please add the label?

[JoelSpeed]

Considering proximity to the release branch cut off, and the fact that TRT have had to introduce the critical fixes requirement. I'm inclined to say we should slip this feature to 4.17, do you see that causing an issue?

[anirudhAgniRedhat]

Considering proximity to the release branch cut off, and the fact that TRT have had to introduce the critical fixes requirement. I'm inclined to say we should slip this feature to 4.17, do you see that causing an issue?

As this feature is anyway planed for 4.17, we should be good.