-
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
🌱 Watch for Cluster resources in topology MD controller #9865
🌱 Watch for Cluster resources in topology MD controller #9865
Conversation
Hi @adityabhatia. 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. 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/test-infra repository. |
/ok-to-test |
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.
Nice cleanup!
internal/controllers/topology/machinedeployment/machinedeployment_controller.go
Outdated
Show resolved
Hide resolved
24484c8
to
814ed35
Compare
internal/controllers/topology/machinedeployment/machinedeployment_controller.go
Outdated
Show resolved
Hide resolved
32ccef1
to
e668a38
Compare
/test pull-cluster-api-e2e-full-main |
Named("topology/machinedeployment"). | ||
WithEventFilter(predicates.ResourceHasFilterLabel(ctrl.LoggerFrom(ctx), r.WatchFilterValue)). |
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 separated the combined filter ResourceNotPausedAndHasFilterLabel
, since IMO it is easier to read and we do not have any other Watched resources in this controller
// TODO: should this wait for Cluster.Status.InfrastructureReady similar to Infra Machine resources? | ||
predicates.All(ctrl.LoggerFrom(ctx), | ||
predicates.ClusterUnpaused(ctrl.LoggerFrom(ctx)), | ||
predicates.ResourceHasFilterLabel(ctrl.LoggerFrom(ctx), r.WatchFilterValue), |
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.
this could be taken over by the global predicate
/area testing |
/retest |
e668a38
to
a8dafb0
Compare
/test pull-cluster-api-e2e-full-main |
@sbueringer could you PTAL again? |
Will take a look soon |
Thank you! /lgtm |
LGTM label has been added. Git tree hash: ada2b519327823d0a7d19436285bfbe7cf186eb1
|
[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 |
Could it be that this caused some current e2e flakes/issues? https://testgrid.k8s.io/sig-cluster-lifecycle-cluster-api#capi-e2e-main |
100% > https://kubernetes.slack.com/archives/C8TSNPY4T/p1704976187538149 |
What this PR does / why we need it:
This PR adds a Watch on
Cluster
Resources in the topology basedMachineDeployment
controller, thereby triggering reconciliation onCluster
resource / status update.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 # #9532
/area testing