-
Notifications
You must be signed in to change notification settings - Fork 367
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
Explicitly set kubelet's log files in Prepare-Node.ps1 on Windows #6221
Conversation
/test-all |
@@ -157,6 +157,10 @@ Write-Host "Registering kubelet service" | |||
nssm install kubelet $global:Powershell $global:PowershellArgs $global:StartKubeletScript | |||
|
|||
nssm set kubelet DependOnService $ContainerRuntime | |||
nssm set kubelet AppStdout C:\var\log\kubelet\kubelet.log | |||
nssm set kubelet AppStdoutCreationDisposition 2 |
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.
could you add a comment there explaining what this does (setting AppStdoutCreationDisposition
to 2)?
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.
added
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.
@antoninbas could you share your thoughts on this ask about if we need to keep collecting kubelet logs on Windows in the support bundle?
nssm set kubelet AppStdout C:\var\log\kubelet\kubelet.log | ||
# Replace kubelet.log on every service restart | ||
nssm set kubelet AppStdoutCreationDisposition 2 | ||
nssm set kubelet AppStderr C:\var\log\kubelet\kubelet.err.log |
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.
I think the path in CI support bundle should also be updated?
Signed-off-by: Wenying Dong <wenyingd@vmware.com>
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.
LGTM
/test-all |
@XinShuYang is there a windows test we should run before merging this? |
/test-windows-containerd-e2e |
Yes, test.sh changes need to be verified by at least one windows job. |
Ci passed, we can merge this PR @antoninbas. |
@wenyingd should this PR be backported? |
For Antrea itself, Prepare-Node.ps1 is not used in the runtime but used in Windows Node preparation stage, so I don't think it is necessary to back port. Even for tkgm setup, they are using their own solutions (cloud-init) to install kubelet services, so they are not impacted. As for CI change (removing docker log collections), the CI tests in earlier releases are not blocked by this. So I don't feel necessary to back port it. |
Fix: #6219