Setting the manifest directory when it is required by kubelet #5939
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
needs-ok-to-test
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
k8s-ci-robot
added
the
lgtm
|
/assign @justinsb |
|
This is desperately needed in kops 1.11 |
nodeup/pkg/model/kubelet.go
Outdated
| @@ -240,6 +258,8 @@ func (b *KubeletBuilder) buildSystemdService() *nodetasks.Service { | |||
| manifest.Set("Service", "StartLimitInterval", "0") | |||
| manifest.Set("Service", "KillMode", "process") | |||
| manifest.Set("Service", "User", "root") | |||
| manifest.Set("Service", "CPUAccounting", "true") | |||
| manifest.Set("Service", "MemoryAccounting", "true") | |||
There was a problem hiding this comment.
Is this related to the manifest directory change? If not, let's split it out so that we can review the ideas separately. This isn't as clear to me as the directory change, so it will delay the directory change.
There was a problem hiding this comment.
Not a problem. I just reverted that commit and re-synced my fork's feature branch with kops' master branch.
nodeup/pkg/model/kubelet_test.go
Outdated
| } | ||
| context.AddTask(task) | ||
| } | ||
|
|
There was a problem hiding this comment.
Hmmm... I'm trying to remember why we don't just call Build... this change is right, but I'm just hoping you might know why we don't call Build?
There was a problem hiding this comment.
(And I guess this is why we split out the buildManifestDirectory function ... which otherwise it would feel more natural to inline)
There was a problem hiding this comment.
@justinsb, for me, I followed the pattern of breaking it out into a separate function call b/c the test (kubelet_test.go) was imitating the internal calls inside the build method, and needed access to what build was actually calling. It felt weird when I saw that, but I limited my scope to just fix the issue, and not how kubelet's build method is tested.
|
Code looks good, but I'm not sure about the CPUAccounting / MemoryAccounting changes. Are they related? If not, can we split them out so we can get this into 1.11? |
|
@justinsb, I can separate the concerns and remove the CPU/Memory account changes. I can probably get to that today. |
k8s-ci-robot
removed
the
lgtm
mmerrill3
force-pushed
the
feature/kops-5463
branch
from
dbf67e2
to
7c4b2a6
Compare
|
Thanks @mmerrill3 - really appreciate the willingness to split it up! /approve (I'll try to figure out the test failures, I don't think it is your PR) |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrisz100, justinsb, mmerrill3 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
|
/retest |
This PR satisfies #5463. The manifest directory will be setup by nodeup when it is configuring kubelet. Kubelet itself needs the directory set, and nodeup is currently configuring kubelet.