-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🌱 Follow-up fixes: Add MachinePools to Runtime SDK and Rollout tests #9719
Conversation
/test pull-cluster-api-e2e-full-main |
1 similar comment
/test pull-cluster-api-e2e-full-main |
ce8bf21
to
957c082
Compare
/test pull-cluster-api-e2e-full-main |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx for the quick follow-up PR!
/test pull-cluster-api-e2e-full-main |
/retest |
4b7a33e
to
faac687
Compare
Let's also run e2e-full |
/test pull-cluster-api-e2e-full-main |
/retest |
@@ -186,7 +186,7 @@ func ClusterClassRolloutSpec(ctx context.Context, inputGetter func() ClusterClas | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on e2e-full we're checking for MD instead of MP somewhere in the new MP validations:
key "machinedeployment.clusters.x-k8s.io/revision" does not exist in map map[Cluster.topology.machinePool.annotation:Cluster.topology.machinePool.annotationValue ClusterClass.machinePool.annotation:ClusterClass.machinePool.annotationValue]
(I assume it's only surfaced on this PR, because now we're actually validating :))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the error happens here, right after applying the cluster template. I think MachineDeployment should exist in this case so I'm wondering what happened here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I think it's due to the lack of a revision annotation for MachinePools like it does for MachineDeployments. Removing this without()
should fix it. https://github.com/willie-yao/cluster-api/blob/fix-mp-cc-e2e/test/e2e/clusterclass_rollout.go#L647
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Looks like we now hit another similar case
kubectl.kubernetes.io/last-applied-configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think both InfraMachinePool and BootstrapConfig of a MachinePool don't have the last-applied-configuration annotation. So we can drop l.695 & l.719
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbueringer Willie is out sick today so I pushed this change to the branch to speed up getting test signal in case we want to get this in before cutting the RC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx!! I squashed & added co-authored-by
@willie-yao @CecileRobertMichon +/- getting e2e-full green lgtm from my side, if you want to merge before the weekend. |
8e08815
to
40680e8
Compare
/test pull-cluster-api-e2e-full-main |
2 similar comments
/test pull-cluster-api-e2e-full-main |
/test pull-cluster-api-e2e-full-main |
/lgtm |
LGTM label has been added. Git tree hash: 8891fe8ddc2ad9275aa93139edbdd017482795ae
|
37f7fb7
to
c7eb3c3
Compare
Co-authored-by: Cecile Robert-Michon <cecilerobertm@gmail.com>
c7eb3c3
to
dbdf199
Compare
/lgtm |
LGTM label has been added. Git tree hash: 8b6a4fc42e2de899da1f20546c974c20710fde51
|
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
This PR follows up #9703 with fixes listed here: #9703 (review)
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
/area testing