🌱 Watch for Cluster resources in topology MachineSet & MachineDeployment controllers #10029
Conversation
k8s-ci-robot
added
area/testing
|
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. |
k8s-ci-robot
added
needs-ok-to-test
adityabhatia
force-pushed
the
clusterWatcherInMdCtrl
branch
from
4db410e
to
df50cd2
Compare
|
/test help |
|
@chrischdi: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
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 |
k8s-ci-robot
added
ok-to-test
|
/test pull-cluster-api-e2e-full-main |
adityabhatia
force-pushed
the
clusterWatcherInMdCtrl
branch
from
df50cd2
to
1f087aa
Compare
|
/test pull-cluster-api-e2e-full-main |
adityabhatia
changed the title
adityabhatia
force-pushed
the
clusterWatcherInMdCtrl
branch
from
1f087aa
to
ba855c9
Compare
|
/test pull-cluster-api-e2e-full-main |
|
/approve /hold /assign @chrischdi @fabriziopandini |
k8s-ci-robot
added
the
do-not-merge/hold
|
[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 |
k8s-ci-robot
added
the
approved
| @@ -96,7 +96,6 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt | |||
| // 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.
This is safe to remove because we have
WithEventFilter(predicates.ResourceNotPausedAndHasFilterLabel(ctrl.LoggerFrom(ctx), r.WatchFilterValue)).above right?
There was a problem hiding this comment.
right since WithEventFilter(...) adds global predicates
There was a problem hiding this comment.
Yeah the ones from WithEventFilter are added to the ones passed into For & Watches
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 31d154ffaf5450d16208eebc1ec8954f711e194a
|
|
/test pull-cluster-api-e2e-full-main |
|
/test pull-cluster-api-e2e-full-main /hold cancel (Should merge as soon as this run is green) |
k8s-ci-robot
removed
the
do-not-merge/hold
As a follow-up after the revert #9865
The test failure reported
no finalizers set for MachineSet/xyzWhat this PR does / why we need it:
The reason of the above test failure / flake appears to be due to the fact that previously (current main branch state) forced reconciliation of
MachineDeploymentcontroller (non-topology), which was in turn queuing theMachineSet(both non-topology and topology controllers) thereby reconciling the respective Finalizers. However after removing the forcedMachineDeploymentreconciliation, theMachineSetwas reconciling according to its usual queuing cycle, which resulted in flakes (timed out waiting for finalizer).This change introduces a
Clusterwatcher inMachineSetandMachineDeployment(topology controllers) to be queued for reconciliation, thereby ensuring Finalizers exist in both resources.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