-
Notifications
You must be signed in to change notification settings - Fork 370
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
[Windows] Remove prefix "rancher-wins" when collecting antrea-agent logs #6223
Conversation
/test-e2e |
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
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-vm-e2e |
/test-windows-all |
pkg/support/dump_windows_test.go
Outdated
@@ -41,15 +41,15 @@ func TestDumpLog(t *testing.T) { | |||
fs.MkdirAll(logDir, os.ModePerm) | |||
fs.MkdirAll(antreaWindowsOVSLogDir, os.ModePerm) | |||
fs.MkdirAll(antreaWindowsKubeletLogDir, os.ModePerm) | |||
fs.Create(filepath.Join(logDir, "rancher-wins-antrea-agent.log")) | |||
fs.Create(filepath.Join(logDir, "antrea-agent.exe.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.
the tests have passed so we can merge this, but is there a specific reason for adding the .exe
to the file name?
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.
No specific reasons, just because the agent bits is named as "antrea-agent.exe" on Windows. I use its Windows style name prefix here. The actual log file on Windows is also something like antrea-agent.exe.INFO
, but not antrea-agent.INFO, so I updated it in the test to avoid unexpected confusion.
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 was asking because it doesn't match what we do for kubelet for example (I see kubelet.log
below)
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.
Do you mean you want their names consistent?
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 realize now that this is just the unit test, and that in practice we are copying the files as they are on the host's filesystem.
I guess my only preference would be to rename antrea-agent.exe.log
to antrea-agent.exe.INFO
in the unit test, so that it better matches the actual log file name, based on what you mentioned above.
If this is the only change, we don't need to run e2e tests again before merging, we just need to make sure that unit tests are still passing.
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.
BTW, there is no kubelet logs persistent on Nodes in recent K8s versions (at least since K8s 1.28), we need to manually modify kubelet service's configurations to enforce it, so I created another PR to do it (#6221). If we plan to make the names consistent, I shall also modify the log file name as "kubelet.exe.log" and "kubelet.exe.err.log" in that change.
After switching to Containerd runtime, Antrea doesn't depend on rancher-wins when running agent. So the agent log files should start with "antrea-agent". Remove the prefix "rancher-wins" in the file name filters when collecting agent logs on Windows. Signed-off-by: Wenying Dong <[email protected]>
/skip-all |
@antoninbas Can we move this fix forward? |
After switching to Containerd runtime, Antrea doesn't depend on rancher-wins when running agent. So the agent log files should start with "antrea-agent".
This change has removed the prefix "rancher-wins" in the file name filters when collecting agent logs on Windows.
Fix: #6222