Remove glog #527
Merged
Remove glog #527
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 6643614723
💛 - Coveralls |
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
1 similar comment
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
adrianchiris
force-pushed
the
align-config-daemon-loggin
branch
from
199956d
to
ffb562a
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
2 similar comments
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
adrianchiris
force-pushed
the
align-config-daemon-loggin
branch
from
2f989eb
to
196bae5
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
1 similar comment
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
1 similar comment
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
adrianchiris
force-pushed
the
align-config-daemon-loggin
branch
from
ad3c31d
to
f9b2894
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
- create new log pkg to handle log initialization and setup - update operator, daemon and webhook log setup and init logic Signed-off-by: adrianc <adrianc@nvidia.com>
Signed-off-by: adrianc <adrianc@nvidia.com>
update logs to use controller-runtime log Signed-off-by: adrianc <adrianc@nvidia.com>
update logs to use controller-runtime log Signed-off-by: adrianc <adrianc@nvidia.com>
update logs to use controller-runtime logs Signed-off-by: adrianc <adrianc@nvidia.com>
update logs to use controller-runtime logs Signed-off-by: adrianc <adrianc@nvidia.com>
update logs to use controller-runtime log Signed-off-by: adrianc <adrianc@nvidia.com>
update logs to use controller-runtime logs Signed-off-by: adrianc <adrianc@nvidia.com>
update logs to use controller-runtime logs Signed-off-by: adrianc <adrianc@nvidia.com>
update logs to use controller-runtime logs Signed-off-by: adrianc <adrianc@nvidia.com>
remove glog flags from manifest Signed-off-by: adrianc <adrianc@nvidia.com>
update logs to use controller-runtime logs Signed-off-by: adrianc <adrianc@nvidia.com>
Signed-off-by: adrianc <adrianc@nvidia.com>
adrianchiris
force-pushed
the
align-config-daemon-loggin
branch
from
f9b2894
to
37924ef
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
/test-all |
|
Sample log from sriov config daemon: |
SchSeba
approved these changes
Oct 31, 2023
|
LGTM Question, why is the old log package still an indirect dependency? |
maybe because of dependency on sriov device plugin ? |
| @@ -48,7 +48,7 @@ var ( | |||
| Use: "start", | |||
| Short: "Starts SR-IOV Network Config Daemon", | |||
| Long: "", | |||
| Run: runStartCmd, | |||
| RunE: runStartCmd, | |||
| @@ -35,10 +33,12 @@ import ( | |||
| "k8s.io/client-go/rest" | |||
| "k8s.io/client-go/tools/clientcmd" | |||
| "k8s.io/client-go/util/connrotation" | |||
| "sigs.k8s.io/controller-runtime/pkg/log" | |||
There was a problem hiding this comment.
I think it's better if we import "sigs.k8s.io/controller-runtime/pkg/log" only in the new log package of the sriov operator. For instance, here I see that you use it to get a new logger instance with a name element. Therefore, you could mask this function in the log package. Something like
func WithName(name string) logr.Logger {
return log.Log.WithName(name)
}
|
|
||
| // To help debugging, immediately log version | ||
| glog.V(2).Infof("Version: %+v", version.Version) | ||
| setupLog.V(2).Info("sriov-network-config-daemon", "version", version.Version) |
There was a problem hiding this comment.
could you please remind me why we call the V function to get a new logger with different level? Just wondering if we could simplify this now that you making some changes for logging.
| rootCmd.PersistentFlags().AddGoFlagSet(flag.CommandLine) | ||
| } | ||
|
|
||
| func main() { | ||
| if err := rootCmd.Execute(); err != nil { | ||
| glog.Exitf("Error executing mcd: %v", err) | ||
| log.Log.Error(err, "error executing sriov-network-config-daemon") |
There was a problem hiding this comment.
let's get the logger from the new log package. You can have there. I believe all files should only import the new log package and not other log related imports.
import "sigs.k8s.io/controller-runtime/pkg/log"
var Log = log.Log
|
|
||
| // handleLogLevelChange handles log level change | ||
| func (dn *Daemon) handleLogLevelChange(logLevel int) { | ||
| newLevel := int8(logLevel * -1) |
There was a problem hiding this comment.
perhaps you can use log.Log.V(logLevel).Enabled() to verify whether the new level is the same or not. You won't need to have a reference to the zap package.
|
Too late for comments. Already merged. |
This was referenced Nov 6, 2023
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove the use of glog package.
use controller-runtime logging & zap instead.
roughly 460 substitutions required.