✨ Add v1beta2 conditions infrastructure to AWSCluster#5854
✨ Add v1beta2 conditions infrastructure to AWSCluster#5854Arpit529Srivastava wants to merge 7 commits intokubernetes-sigs:mainfrom
Conversation
Signed-off-by: arpit529srivastava <arpitsrivastava529@gmail.com>
|
Hi @Arpit529Srivastava. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
/ok-to-test |
Signed-off-by: arpit529srivastava <arpitsrivastava529@gmail.com>
Signed-off-by: arpit529srivastava <arpitsrivastava529@gmail.com>
|
@damdo a gentle ping, thanks :) |
damdo
left a comment
There was a problem hiding this comment.
Hey @Arpit529Srivastava thanks a lot for your PR!
This is a good start :)
Left some comments
| @@ -150,6 +150,9 @@ func (src *AWSCluster) ConvertTo(dstRaw conversion.Hub) error { | |||
| } | |||
There was a problem hiding this comment.
I don't think we should touch v1beta1 types at all.
Only v1beta2 ones
There was a problem hiding this comment.
@damdo just to make sure i’m understanding correctly about the v1beta1 conversion changes, should i remove the V1Beta3 restoration logic from awscluster_conversion.go? i followed the same pattern used for existing fields like ControlPlaneLoadBalancer, but i’m totally fine removing it if that’s not the direction we want to take.
wanted to confirm this part before i move on to the rest of the feedback. thanks
There was a problem hiding this comment.
As we are adding new fields in v1beta2 then we need to preserve roundtrip compatibility. So it will require that you update the conversions in v1beta1 like you have done here to restore the value.
There was a problem hiding this comment.
A lot of people make the mistake of adding the new field to old api versions as well to make the conversions work. But restoring the value from the saved state is the correct way (like you are doing).
There was a problem hiding this comment.
As we are adding new fields in v1beta2 then we need to preserve roundtrip compatibility. So it will require that you update the conversions in v1beta1 like you have done here to restore the value.
Good point! Hadn't thought about that!
api/v1beta2/awscluster_types.go
Outdated
| // AWSClusterV1Beta3Status groups all the fields that will be added or modified in AWSClusterStatus with the V1Beta3 version. | ||
| // See https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md for more context. | ||
| type AWSClusterV1Beta3Status struct { | ||
| // conditions represents the observations of an AWSCluster's current state. |
There was a problem hiding this comment.
| // conditions represents the observations of an AWSCluster's current state. | |
| // Conditions represents the observations of an AWSCluster's current state. |
api/v1beta2/conditions_consts.go
Outdated
| DedicatedHostReleaseFailedReason = "DedicatedHostReleaseFailed" | ||
| ) | ||
|
|
||
| // AWSCluster's v1beta2 conditions and corresponding reasons. |
There was a problem hiding this comment.
| // AWSCluster's v1beta2 conditions and corresponding reasons. | |
| // AWSCluster's v1beta3 conditions and corresponding reasons. |
api/v1beta2/conditions_consts.go
Outdated
| DedicatedHostReleaseFailedReason = "DedicatedHostReleaseFailed" | ||
| ) | ||
|
|
||
| // AWSCluster's v1beta2 conditions and corresponding reasons. |
There was a problem hiding this comment.
Also we should probably move these to a different place, how are other CAPI providers doing this during their transition?
There was a problem hiding this comment.
i took another look and separating them does seem like the cleaner approach. i’ll move the new v1beta3 constants into a separate file (api/v1beta2/conditions_consts_v1beta3.go).
api/v1beta2/conditions_consts.go
Outdated
| ) | ||
|
|
||
| // AWSCluster's v1beta2 conditions and corresponding reasons. | ||
| // These will be used with the V1Beta2 API version. |
api/v1beta2/conditions_consts.go
Outdated
| // AWSCluster's v1beta2 conditions and corresponding reasons. | ||
| // These will be used with the V1Beta2 API version. | ||
|
|
||
| // AWSCluster's Ready condition and corresponding reasons that will be used in v1Beta2 API version. |
| // +listType=map | ||
| // +listMapKey=type | ||
| // +kubebuilder:validation:MaxItems=32 | ||
| Conditions []metav1.Condition `json:"conditions,omitempty"` |
There was a problem hiding this comment.
Are we only dealing with AWSCluster and Conditions here?
If we do it this granular we'll potentially end up with (number of API types) x (number of fields to add) PRs
There was a problem hiding this comment.
my bad, i was initially planning to split these into separate prs to keep things smaller,. i agree it’s better to avoid creating too many prs for closely related changes.
There was a problem hiding this comment.
rethinking again, once we settle on the approach for v1beta1 conversions, #5854 (comment) i’ll add the same V1Beta3 status field to the other types as well (AWSMachine, AWSMachinePool, AWSManagedMachinePool, etc ...) in this same pr. just wanted to make sure we’re aligned on the pattern with AWSCluster first before rolling it out everywhere.
There was a problem hiding this comment.
I'm happy either way. Smaller PRs always helps with review though.
thanks for the review 😃, taking a look. |
|
/assign @nrb @damdo @richardcase @clebs |
Signed-off-by: arpit529srivastava <arpitsrivastava529@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@clebs, can you please take a look. |
| @@ -0,0 +1,114 @@ | |||
| /* | |||
There was a problem hiding this comment.
Why are these constants introduced? They are not being used anywhere and most values are duplicating existing constants.
There was a problem hiding this comment.
hmm, make sense.. i initially added these constants as groundwork for the next step, where the AWSClusterController will dual-write conditions into both status.conditions and status.v1beta3.conditions. however, rightg now since nothing in this pr actually consumes them yet, ill remove them.
There was a problem hiding this comment.
will introduce them together with the controller changes in the follow-up pr where they’ll actually be used.
There was a problem hiding this comment.
Great thanks! Makes sense to introduce them in the next step. Also only introducing what is necessary.
There was a problem hiding this comment.
Looking at the initial comment from @damdo, he is saying you should not update files under api/v1beta1. Only files under api/v1beta2.
The comment is not asking to change CAPI conversion, just to not include it in the older CAPA v1beta1 API.
Please bear in mind that CAPI v1beta1 and CAPA v1beta1 are 2 different things.
There was a problem hiding this comment.
thanks @clebs for the review will keep this in mind.
Signed-off-by: arpit529srivastava <arpitsrivastava529@gmail.com>
| @@ -150,6 +150,9 @@ func (src *AWSCluster) ConvertTo(dstRaw conversion.Hub) error { | |||
| } | |||
There was a problem hiding this comment.
As we are adding new fields in v1beta2 then we need to preserve roundtrip compatibility. So it will require that you update the conversions in v1beta1 like you have done here to restore the value.
| @@ -150,6 +150,9 @@ func (src *AWSCluster) ConvertTo(dstRaw conversion.Hub) error { | |||
| } | |||
There was a problem hiding this comment.
A lot of people make the mistake of adding the new field to old api versions as well to make the conversions work. But restoring the value from the saved state is the correct way (like you are doing).
| // +listType=map | ||
| // +listMapKey=type | ||
| // +kubebuilder:validation:MaxItems=32 | ||
| Conditions []metav1.Condition `json:"conditions,omitempty"` |
There was a problem hiding this comment.
I'm happy either way. Smaller PRs always helps with review though.
api/v1beta2/awscluster_types.go
Outdated
| } | ||
|
|
||
| // GetV1Beta3Conditions returns the set of conditions for this object. | ||
| func (r *AWSCluster) GetV1Beta3Conditions() []metav1.Condition { |
There was a problem hiding this comment.
This is where the naming of things is going to get weird 😉
We need to adopt paused.EnsurePausedCondition in our controllers. This means that AWSCluster (and our other resource types) need to implement the v1beta2conditions.Setter/Getter interfaces . So these methods need to be named:
// SetV1Beta2Conditions sets conditions for an API object.
// Note: SetV1Beta2Conditions will be renamed to SetConditions in a later stage of the transition to V1Beta2.
SetV1Beta2Conditions([]metav1.Condition)
// GetV1Beta2Conditions returns the list of conditions for a cluster API object.
// Note: GetV1Beta2Conditions will be renamed to GetConditions in a later stage of the transition to V1Beta2.
GetV1Beta2Conditions() []metav1.ConditionOr we need additional functions.
It will be very messy/weird until we drop the old fields.
There was a problem hiding this comment.
thanks for clarifying 😄
i’ve updated the implementation to use GetV1Beta2Conditions() and SetV1Beta2Conditions() along with the Note: comments you mentioned.
it does feel a bit awkward during the transition, but i agree it’s better to align exactly with capi’s interface expectations.
Signed-off-by: arpit529srivastava <arpitsrivastava529@gmail.com>
|
@richardcase @damdo made all corrections, PTAL . |
richardcase
left a comment
There was a problem hiding this comment.
Looking good @Arpit529Srivastava . I made some suggestions around the comments to make it obvious that we are talking about "CAPI contract" v1beta2 and not CAPA v1beta2.
Would you now plan to add the same changes for the other resource kinds?
api/v1beta2/awscluster_types.go
Outdated
| V1Beta2 *AWSClusterV1Beta2Status `json:"v1beta2,omitempty"` | ||
| } | ||
|
|
||
| // AWSClusterV1Beta2Status groups all the fields that will be added or modified in AWSCluster with the V1Beta2 version. |
There was a problem hiding this comment.
| // AWSClusterV1Beta2Status groups all the fields that will be added or modified in AWSCluster with the V1Beta2 version. | |
| // AWSClusterV1Beta2Status groups all the fields that will be added or modified in AWSCluster with the V1Beta2 CAPI contract version. |
api/v1beta2/awscluster_types.go
Outdated
| Bastion *Instance `json:"bastion,omitempty"` | ||
| Conditions clusterv1beta1.Conditions `json:"conditions,omitempty"` | ||
|
|
||
| // v1beta2 groups all the fields that will be added or modified in AWSCluster's status with the V1Beta2 version. |
There was a problem hiding this comment.
| // v1beta2 groups all the fields that will be added or modified in AWSCluster's status with the V1Beta2 version. | |
| // v1beta2 groups all the fields that will be added or modified in AWSCluster's status with the V1Beta2 CAPI contract version. |
There was a problem hiding this comment.
addressed
api/v1beta2/awscluster_types.go
Outdated
| } | ||
|
|
||
| // GetV1Beta2Conditions returns the set of conditions for this object. | ||
| // Note: GetV1Beta2Conditions will be renamed to GetConditions in a later stage of the transition to V1Beta2. |
There was a problem hiding this comment.
| // Note: GetV1Beta2Conditions will be renamed to GetConditions in a later stage of the transition to V1Beta2. | |
| // Note: GetV1Beta2Conditions will be renamed to GetConditions in a later stage of the transition to CAPI contract V1Beta2. |
api/v1beta2/awscluster_types.go
Outdated
| } | ||
|
|
||
| // SetV1Beta2Conditions sets conditions for an API object. | ||
| // Note: SetV1Beta2Conditions will be renamed to SetConditions in a later stage of the transition to V1Beta2. |
There was a problem hiding this comment.
| // Note: SetV1Beta2Conditions will be renamed to SetConditions in a later stage of the transition to V1Beta2. | |
| // Note: SetV1Beta2Conditions will be renamed to SetConditions in a later stage of the transition to CAP contract V1Beta2. |
Signed-off-by: arpit529srivastava <arpitsrivastava529@gmail.com>
for the other resource kinds - yes, i’m planning to add them as well. since you mentioned that smaller prs are easier to review, i started with is there anything specific i should watch out for when applying the same pattern to the other resources? any heads-up before i proceed? |
|
@Arpit529Srivastava - it might be nice to add a task list to #5835 so that progress can be tracked and people can help out. Once the conditions are added to the types, i'm guessing that the next stage is to then dual right conditions to the new fields? |
|
sure that is much better way, added a task list to #5835 for tracking. thanks |
What type of PR is this?
/kind api-change
/kind feature
What this PR does / why we need it:
This pr prepares
AWSClusterfor migrating conditions to the newmetav1.Conditionformat, following the same explicit dual-write pattern used in CAPI.it adds a new
V1Beta2field toAWSClusterStatusto hold the next-generation condition set alongside the existing one, and implementsGetV1Beta2ConditionsandSetV1Beta2ConditionsonAWSCluster. the conversion logic is updated so theV1Beta2field is preserved across version conversions (v1beta2 ↔ v1beta1).like capi, this keeps the dual-write logic explicit rather than introducing helper abstractions. that way, deprecated v1beta1 logic stays clearly separated from the newer v1beta2+ logic, making it easier to remove the legacy path later.
the actual v1beta2 condition constants will be introduced in a follow-up pr alongside the controller changes that will consume them.
Which issue(s) this PR fixes:
related : #5835
Special notes for your reviewer:
PR only includes API/Status changes and constants. No controller logic is changed in this PR. The controller implementation (dual-writing) will be handled in a follow-up PR.
Checklist:
Release note: