-
Notifications
You must be signed in to change notification settings - Fork 519
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
Add two kubelet settings - topologyManagerPolicy and topologyManagerScope #1659
Conversation
With some previous settings I have advocated for excluding them from the kube config file when not set rather than 'hardcoding' a default. Should we do that again here? @tjkirch @bcressey My thinking is that it would be better to defer default behavior to kubelet rather than be the ones that determine what the default is. |
Agreed. |
81c8a93
to
95f2858
Compare
95f2858
to
f0629ee
Compare
Push above remove 'hardcode' defaults and update correct release version. |
f0629ee
to
2759b93
Compare
Push above removed those two new settings in |
Release.toml
Outdated
@@ -59,3 +59,6 @@ version = "1.1.4" | |||
"migrate_v1.1.3_kubelet-cpu-manager.lz4", | |||
] | |||
"(1.1.3, 1.1.4)" = [] | |||
"(1.1.4, 1.1.5)" = [ |
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.
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.
We should have all unreleased migrations under the same version pair so that, when we test upgrading and downgrading, all the migrations will run. In the event of a 'patch' level release, it may be necessary to edit Release.toml in the event that some migrations need to go into the patch.
As for what the unreleased version represented in Release.toml should be, I'm not sure. I don't think it matters as long as we agree (i.e. there are a few PRs open that should all agree).
pass topology-manager-scope argument to kubelet
pass topology-manager-policy argument to kubelet
…gerScope adds migration for two new settings `settings.kubernetes.topology-manager-scope` and `settings.kubernetes.topology-manager-policy`
2759b93
to
8af98dc
Compare
Push above fix release version (change 1.1.5 to 1.2.0) and pass migration 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.
🥾
Issue number:
#1597
Description of changes:
Pass two new arguments to kubelet
topologyManagerPolicy
andtopologyManagerScope
kubernetes.topology-manager-policy
as a settingkubernetes.topology-manager-scope
as a settingkubernetes.topology-manager-policy
andkubernetes.topology-manager-scope
Testing done:
topology-manager-policy
topology-manager-scope
cpu-manager-policy
cpu-manager-reconcile-policy
topology-manager-policy
launching instance with the AMI which contains new setting and check if
topology-manager-policy
has been configured expectantly.Step1 check if
topology-manager-policy
has been configured expectantly:Test with
topology-manager-policy= none
Test with default ("none")
Test with
topology-manager-policy= best-effort
Test with
topology-manager-policy= restricted
Test with
topology-manager-policy= single-numa-node
Step2: Launched nginx pods/containers
topology-manager-scope
launching instance with the AMI which contains new setting and check if
topology-manager-scope
has been configured expectantly.Step1 check if
topology-manager-scope
has been configured expectantly:Test with
topology-manager-scope = container
Test with
topology-manager-scope = pod
Test with default
Step2: Launched nginx pods/containers
Migration test:
upgrade
Step1: Upgrade to v1.2.0
Step2: Specify new setting
topology-manager-policy
andtopology-manager-scope
through control containerdowngrade
Step1: Check migration binary
Step2: Downgrade to previous verison
Step3: Check if
topology-manager-policy
andtopology-manager-scope
have been removedTerms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.