Skip to content
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

Enh/stable worker hash #4237

Merged
merged 2 commits into from
Jun 25, 2021
Merged

Conversation

voelzmo
Copy link
Member

@voelzmo voelzmo commented Jun 21, 2021

How to categorize this PR?
/kind enhancement

What this PR does / why we need it:
Exclude cri.name docker from worker hash calculation

Worker pools with CRI==nil use docker as their container runtime. Changing to an explicit configuration of docker should not trigger a node rollout. Therefore, we need to keep the Worker hash stable by excluding the cri.name property from hash calculation in this case.

Part of #4110

Special notes for your reviewer:
Marked as draft as it depends on #4218 .

Release note:

Do not trigger a node rollout when switching from `CRI.Name==nil` to `CRI.Name==docker`.

cc @BeckerMax

Worker pools with `CRI==nil` use `docker` as their container runtime. Changing to an explicit configuration of `docker` should not trigger a node rollout. Therefore, we need to keep the Worker hash stable by excluding the `cri.name` property from hash calculation in this case.

Co-authored-by: Max Becker <max.becker@sap.com>
Copy link
Member

@vpnachev vpnachev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small improvement to avoid one nested if block.

extensions/pkg/controller/worker/machines.go Outdated Show resolved Hide resolved
Copy link
Member

@vpnachev vpnachev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/squash

@vpnachev vpnachev merged commit 278e9b4 into gardener:master Jun 25, 2021
krgostev pushed a commit to krgostev/gardener that referenced this pull request Apr 21, 2022
* Exclude cri.name `docker` from worker hash calculation

Worker pools with `CRI==nil` use `docker` as their container runtime. Changing to an explicit configuration of `docker` should not trigger a node rollout. Therefore, we need to keep the Worker hash stable by excluding the `cri.name` property from hash calculation in this case.

Co-authored-by: Max Becker <max.becker@sap.com>

* Merge two if-blocks into single 'and' condition

Co-authored-by: Max Becker <max.becker@sap.com>
krgostev pushed a commit to krgostev/gardener that referenced this pull request Jul 5, 2022
* Exclude cri.name `docker` from worker hash calculation

Worker pools with `CRI==nil` use `docker` as their container runtime. Changing to an explicit configuration of `docker` should not trigger a node rollout. Therefore, we need to keep the Worker hash stable by excluding the `cri.name` property from hash calculation in this case.

Co-authored-by: Max Becker <max.becker@sap.com>

* Merge two if-blocks into single 'and' condition

Co-authored-by: Max Becker <max.becker@sap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/api-change API change with impact on API users size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants