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
WINC-962: Pick up openshift/kubernetes 1.26 rebase updates #1395
WINC-962: Pick up openshift/kubernetes 1.26 rebase updates #1395
Conversation
mansikulkarni96
commented
Jan 30, 2023
•
edited by aravindhp
edited by aravindhp
- [services] Replace deprecated klog flags with kube-log-runner
- [submodule][kubelet] Update to 149fe52
- [vendor] Use 1.26 k8s and OpenShift libraries
- [manifests] Set v1.26 as minimum k8s version
@mansikulkarni96: This pull request references WINC-962 which is a valid jira issue. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Skipping CI for Draft Pull Request. |
@mansikulkarni96: This pull request references WINC-962 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
8c7ab6b
to
026397a
Compare
3617d92
to
8c276c3
Compare
8c276c3
to
408ca3c
Compare
d6afa21
to
0376618
Compare
pkg/services/services.go
Outdated
kubeLogCmd := fmt.Sprintf("%s -log-file=%s -redirect-stderr=false ", | ||
windows.KubeLogRunnerPath, windows.KubeletLog) | ||
|
||
kubeletServiceCmd := kubeLogCmd + windows.KubeletPath |
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.
Please add windows.KubeletPath as part of the Sprintf, no reason for it to be separate
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.
Does this need to be done for kube-proxy as well?
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.
@sebsoto yes it needs to be, I am contemplating on whether to add it in this PR or add it as part of kube-proxy bump when the new sdn branch is created. kube-log-runner only has the functionality to direct the logs to a file. For kube-proxy, we have been redirecting to a log-dir for a long time now. Is it acceptable to move them only to info and error files?
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.
Would doing this for kube-proxy have effect on how we collect log files (e.g. must-gather changes)?
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.
If kube-proxy
does not require this change we can leave it as is.
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.
@aravindhp It will once we update to the 1.26 SDN branch.
@saifshaikh48 yes we will need to update the file info here
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.
@aravindhp It will once we update to the 1.26 SDN branch.
We should definitely take care of this as part of that update
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 aside from a comment
@@ -177,6 +179,7 @@ func getFilesToTransfer() (map[*payload.FileInfo]string, error) { | |||
payload.KubeProxyPath: K8sDir, | |||
payload.KubeletPath: K8sDir, | |||
payload.AzureCloudNodeManagerPath: K8sDir, | |||
payload.KubeLogRunnerPath: K8sDir, |
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.
Should we also add this to requiredFiles?
Although I'm seeing that var is quite outdated so maybe it warrants a task of its own to update properly.
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 be updating the variable though with new additions. We can clean it up later if we need to delete any files from there.
pkg/services/services.go
Outdated
kubeLogCmd := fmt.Sprintf("%s -log-file=%s -redirect-stderr=false ", | ||
windows.KubeLogRunnerPath, windows.KubeletLog) | ||
|
||
kubeletServiceCmd := kubeLogCmd + windows.KubeletPath |
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.
Would doing this for kube-proxy have effect on how we collect log files (e.g. must-gather changes)?
This commit removes the usage of deprecated klog flags like --log-file and --logstostderr which are no lonegr compatible with the kube-1.26. The suggested alternative for continued logging to a log file is using the kube-log-runner exe Ref: https://github.com/kubernetes/kubernetes/tree/62d9f8ba80f4cc660a88dcc34f56c5c6f7df17ea/staging/src/k8s.io/component-base/logs/kube-log-runner KEP: https://github.com/kubernetes/enhancements/tree/3cb66bd0a1ef973ebcc974f935f0ac5cba9db4b2/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components#user-stories
Update to openshift/kubernetes@149fe52 This commit was generated using hack/update_submodules.sh
0376618
to
593bf7a
Compare
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. Just a few questions. I will leave official approval to @sebsoto.
@@ -177,6 +179,7 @@ func getFilesToTransfer() (map[*payload.FileInfo]string, error) { | |||
payload.KubeProxyPath: K8sDir, | |||
payload.KubeletPath: K8sDir, | |||
payload.AzureCloudNodeManagerPath: K8sDir, | |||
payload.KubeLogRunnerPath: K8sDir, |
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 be updating the variable though with new additions. We can clean it up later if we need to delete any files from there.
Manually update to 1.26 libraries Ran commands: go get -u github.com/openshift/api@61d971884921b77ca959fafbc026c8fe5666bdb4 go get -u github.com/openshift/client-go@72f107311084c1dabe65d31aa68d232e33950dc1 go mod tidy && go mod vendor
593bf7a
to
26fe803
Compare
@mansikulkarni96: This pull request references WINC-962 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mansikulkarni96, sebsoto 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 |
@mansikulkarni96: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |