[GitHub] [yunikorn-k8shim] craigcondit commented on a diff in pull request #623: [YUNIKORN-1826] Convert shim to use scoped logging

["craigcondit (via GitHub)" <gi...@apache.org>]

craigcondit commented on code in PR #623:
URL: https://github.com/apache/yunikorn-k8shim/pull/623#discussion_r1244264667


##########
pkg/schedulerplugin/scheduler_plugin.go:
##########
@@ -216,17 +216,17 @@ func (sp *YuniKornSchedulerPlugin) PostBind(_ context.Context, _ *framework.Cycl
 
 // NewSchedulerPlugin initializes a new plugin and returns it
 func NewSchedulerPlugin(_ runtime.Object, handle framework.Handle) (framework.Plugin, error) {
-	log.SetDefaultLogger(log.K8Shim)
-	log.Logger().Info(fmt.Sprintf("Build info: version=%s date=%s isPluginVersion=%t goVersion=%s arch=%s coreSHA=%s siSHA=%s shimSHA=%s", conf.BuildVersion, conf.BuildDate, conf.IsPluginVersion, conf.GoVersion, conf.Arch, conf.CoreSHA, conf.SiSHA, conf.ShimSHA))
+	log.SetDefaultLogger(log.ShimSchedulerPlugin)

Review Comment:
   This should still be `log.SetDefaultLogger(log.Shim)` -- almost all the code still resolves to shim, only the plugin code is specific.



##########
pkg/admission/pki/certs.go:
##########


Review Comment:
   Let's use `log.AdmissionUtils` for the `pkg/admission/pki/*` files.



##########
pkg/log/logger.go:
##########
@@ -51,20 +51,46 @@ const (
 	levelSuffix = ".level"
 )
 
-// Predefined loggers: when adding new loggers, ids must be sequential, and all must be added to the loggers slice in the same order
+// Defined loggers: when adding new loggers, ids must be sequential, and all must be added to the loggers slice in the same order
 var (
-	K8Shim     = &LoggerHandle{id: 1, name: "k8shim"}
-	Kubernetes = &LoggerHandle{id: 2, name: "kubernetes"}
-	Admission  = &LoggerHandle{id: 3, name: "admission"}
-	Test       = &LoggerHandle{id: 4, name: "test"}
+	Kubernetes               = &LoggerHandle{id: 1, name: "kubernetes"}
+	Test                     = &LoggerHandle{id: 2, name: "test"}
+	Admission                = &LoggerHandle{id: 3, name: "admission"}
+	AdmissionClient          = &LoggerHandle{id: 4, name: "admission.client"}
+	AdmissionController      = &LoggerHandle{id: 5, name: "admission.controller"}

Review Comment:
   Introduce `log.AdmissionConf` => `admission.conf` for configuration-related admission controller logs.



##########
pkg/common/si_helper.go:
##########
@@ -42,7 +42,7 @@ func CreateTagsForTask(pod *v1.Pod) map[string]string {
 			if value.Kind == constants.DaemonSetType {
 				if pod.Spec.Affinity == nil || pod.Spec.Affinity.NodeAffinity == nil ||
 					pod.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution == nil {
-					log.Logger().Debug("DaemonSet pod's Affinity, NodeAffinity, RequiredDuringSchedulingIgnoredDuringExecution might empty")
+					log.Log(log.ShimContext).Debug("DaemonSet pod's Affinity, NodeAffinity, RequiredDuringSchedulingIgnoredDuringExecution might empty")

Review Comment:
   Let's use `log.ShimUtils` for this as that's logically where this goes (helper is a synonym for utility).



##########
pkg/log/logger.go:
##########
@@ -51,20 +51,46 @@ const (
 	levelSuffix = ".level"
 )
 
-// Predefined loggers: when adding new loggers, ids must be sequential, and all must be added to the loggers slice in the same order
+// Defined loggers: when adding new loggers, ids must be sequential, and all must be added to the loggers slice in the same order
 var (
-	K8Shim     = &LoggerHandle{id: 1, name: "k8shim"}
-	Kubernetes = &LoggerHandle{id: 2, name: "kubernetes"}
-	Admission  = &LoggerHandle{id: 3, name: "admission"}
-	Test       = &LoggerHandle{id: 4, name: "test"}
+	Kubernetes               = &LoggerHandle{id: 1, name: "kubernetes"}
+	Test                     = &LoggerHandle{id: 2, name: "test"}
+	Admission                = &LoggerHandle{id: 3, name: "admission"}
+	AdmissionClient          = &LoggerHandle{id: 4, name: "admission.client"}
+	AdmissionController      = &LoggerHandle{id: 5, name: "admission.controller"}
+	AdmissionWebhook         = &LoggerHandle{id: 6, name: "admission.webhook"}
+	AdmissionUtils           = &LoggerHandle{id: 7, name: "admission.utils"}
+	Shim                     = &LoggerHandle{id: 8, name: "shim"}

Review Comment:
   Put `Shim` as the first logger, with ID 1, as this is the default if none is provided.



##########
pkg/admission/conf/am_conf.go:
##########


Review Comment:
   Let's introduce `log.AdmissionConf` => `admission.conf` for these.



##########
pkg/log/logger.go:
##########
@@ -51,20 +51,46 @@ const (
 	levelSuffix = ".level"
 )
 
-// Predefined loggers: when adding new loggers, ids must be sequential, and all must be added to the loggers slice in the same order
+// Defined loggers: when adding new loggers, ids must be sequential, and all must be added to the loggers slice in the same order
 var (
-	K8Shim     = &LoggerHandle{id: 1, name: "k8shim"}
-	Kubernetes = &LoggerHandle{id: 2, name: "kubernetes"}
-	Admission  = &LoggerHandle{id: 3, name: "admission"}
-	Test       = &LoggerHandle{id: 4, name: "test"}
+	Kubernetes               = &LoggerHandle{id: 1, name: "kubernetes"}
+	Test                     = &LoggerHandle{id: 2, name: "test"}
+	Admission                = &LoggerHandle{id: 3, name: "admission"}
+	AdmissionClient          = &LoggerHandle{id: 4, name: "admission.client"}
+	AdmissionController      = &LoggerHandle{id: 5, name: "admission.controller"}

Review Comment:
   Remove `log.AdmissionController` and replace all usages with `log.Admission` as these are synonyms.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org