Fix terminationhandler sync #648
Conversation
openshift-ci-robot
added
the
do-not-merge/hold
|
/hold |
…tion handler This openshift#535 introduced support to manage a damonSet which runs termination handler for spot intances. As an event handler is not passed to the damonSet informer changes to the resource won't trigger a reconcile. This PR fix that by passing the event handler to the daemonSet namespaced informer. This will be e2e tested by openshift/cluster-api-actuator-pkg#177
enxebre
force-pushed
the
fix-terminationhandler-sync
branch
from
383ae08
to
f9a7478
Compare
This openshift/machine-api-operator#535 introduced support to manage a damonSet which runs termination handler for spot intances. As an event handler is not passed to the damonSet informer changes to the resource won't trigger a reconcile. This PR openshift/machine-api-operator#648 fixes that by passing the event handler to the daemonSet namespaced informer. This PR cover this e2e.
This openshift/machine-api-operator#535 introduced support to manage a damonSet which runs termination handler for spot intances. As an event handler is not passed to the damonSet informer changes to the resource won't trigger a reconcile. This PR openshift/machine-api-operator#648 fixes that by passing the event handler to the daemonSet namespaced informer. This PR cover this e2e.
| @@ -112,6 +112,7 @@ func New( | |||
| } | |||
|
|
|||
| deployInformer.Informer().AddEventHandler(optr.eventHandlerDeployments()) | |||
| daemonsetInformer.Informer().AddEventHandler(optr.eventHandler()) | |||
There was a problem hiding this comment.
Should we use eventHandlerDeployments() instead so that we only reconcile when the DaemonSet is owned by the operator?
There was a problem hiding this comment.
The initial rationale behind that was to mitigate mao extra reconciling loops we were seeing and make sure the mao didn't watch itself and so racing with the CVO.
I think It's fine to keep this simple and let it watch any daemonSet on the namespace rather than perpetuate the eventHandlerDeployments pattern.
Then in the medium term we can consider to polish the mao logic, drop that custom eventHandler and if we need more granularity we can pass any SharedInformerOption with WithTweakListOptions to the NewSharedInformerFactoryWithOptions
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
1 similar comment
|
/lgtm |
This openshift/machine-api-operator#535 introduced support to manage a damonSet which runs termination handler for spot intances. As an event handler is not passed to the damonSet informer changes to the resource won't trigger a reconcile. This PR openshift/machine-api-operator#648 fixes that by passing the event handler to the daemonSet namespaced informer. This PR cover this e2e.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Danil-Grigorev 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 |
openshift-ci-robot
added
the
approved
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@enxebre: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/retest |
This #535 introduced support to manage a damonSet which runs termination handler for spot intances.
As an event handler is not passed to the damonSet informer changes to the resource won't trigger a reconcile.
This PR fixes that by passing the event handler to the daemonSet namespaced informer.
This will be e2e tested by openshift/cluster-api-actuator-pkg#177