-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Refactor "multi" handlers to use listen #10542
Refactor "multi" handlers to use listen #10542
Conversation
Hi @VannTen. 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 |
d0788d0
to
a6220c5
Compare
Rebased to fix conflicts |
Looks like the CI runner had a a system failure |
Indeed, retrying |
/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.
@VannTen Nice and cleaner 👍
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.
Thanks! looks much cleaner! :D
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: floryut, MrFreezeex, VannTen 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 |
Hello, does this change of code can ensure the handler be executed in an expected order? |
Handlers are executed in the order they are defined, regardless of the order in which they are notified: see https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_handlers.html#notifying-handlers
|
* containerd: refactor handlers to use 'listen' * cri-dockerd: refactor handlers to use 'listen' * cri-o: refactor handlers to use 'listen' * docker: refactor handlers to use 'listen' * etcd: refactor handlers to use 'listen' * control-plane: refactor handlers to use 'listen' * kubeadm: refactor handlers to use 'listen' * node: refactor handlers to use 'listen' * preinstall: refactor handlers to use 'listen' * calico: refactor handlers to use 'listen' * kube-router: refactor handlers to use 'listen' * macvlan: refactor handlers to use 'listen'
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
We have some handlers which do nothing but notify other handlers (using
command: /bin/true
+ notify)This replaces all of them to use
listen
, which allow handlers to be triggered by one or more "listen topics"(see https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_handlers.html#naming-handlers)
The benefits are mostly for developing or debugging, when looking which handlers are triggered by a particular
notify
:instead of grepping twice, we only need to do it once.
I did some quick testing to see if their was any performance difference (from less
/bin/true
commands executed across ssh),but it does not seems to make any noticeable differences, at least on a small cluster.
Special notes for your reviewer:
I made one commit per role to make it more digestable, but I can squash that if you prefer.
Regarding the pre-install role: I'm not sure the
when
are completely coherent here regarding the Flatcar stuff:While the first "multi handler" was not run for Flatcar, most of the actual handlers were still triggered, since "Preinstall | reload kubelet" is run for Flatcar as well.
The change have made should not alter the execution flow, but maybe a follow-up PR (and someone more knowledgeable about Flatcar) should examine that.
Does this PR introduce a user-facing change?: