-
Notifications
You must be signed in to change notification settings - Fork 438
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
Use klog for katib #506
Comments
I think we are using it now, with the help of kube-builder. |
#112 traced log solution |
@gaocegege The |
#388 also traces it, we can keep one about log solution |
@YujiOshima @richardsliu look forward to more comments from you as well. |
I think |
SGTM. klog is the standard logging in k8s ecosystem and developed actively. |
/assign |
@gyliu513 Katib v1alpha2 version uses log solution that is provided by controller-runtime and it is not the standard It can support Klog also. kubernetes-sigs/controller-runtime#291 Since we are using controller-runtime library for implementation, I feel that we should use the log package provided by controller-runtime and configure the logger(instead of using klog directly) |
@johnugeorge I actually want to use |
Sorry. I didn't understand. Why can't Another side effect is that logs in controller-runtime will get hidden(including error logs) if the log library is not initialized. https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/internal/controller/controller.go#L41 |
@johnugeorge If I want to upgrade |
No. You don't need to upgrade controller-runtime. Klog will be an external dependency. |
@johnugeorge I see, so we do not need to add |
There is an issue for klog at kubernetes/klog#22 , the klog cannot handle the case of some sort with a predefined set of variables which are used as suffix for each log line. Will hold on for some files in katib like https://github.com/kubeflow/katib/blob/master/pkg/controller/v1alpha2/experiment/experiment_util.go#L24 etc. |
I think it may affect us deeply. We rely on this feature in many files. |
@gaocegege I will check how we can workaround for this, but meantime, can we avoid try to using child logger for future new added files? |
I have no objection on it, LGTM |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This issue has been automatically closed because it has not had recent activity. Please comment "/reopen" to reopen it. |
/kind feature
Describe the solution you'd like
[A clear and concise description of what you want to happen.]
Use the permanent forked version of glog for kubernetes instead of log.
ref:
kubernetes/kubernetes#70264
https://github.com/kubernetes/klog/blob/master/README.md
Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]
/cc @hougangliu @richardsliu @gaocegege
The text was updated successfully, but these errors were encountered: