Migrate to controller-runtime logger

[tenzen-y]

Currently, the training-operator uses some types of logger like this:

• sirupsen/logrus logger:

  training-operator/pkg/util/logger.go

  Line 85 in c204220

  func LoggerForUnstructured(obj *metav1unstructured.Unstructured, kind string) *log.Entry {

• controller-runtime logger:

  training-operator/cmd/training-operator.v1/main.go

  Line 111 in c204220

  ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts)))

So, based on this discussion, I'd like to suggest using the controller-runtime logger everywhere to increase maintainability and consistency.

[champon1020]

Could I work on this task?

[tenzen-y]

@champon1020 Sure, Feel free to assign yourself with a /assign comment.

[champon1020]

/assign

[champon1020]

Sorry for the late action but I'm going to tackle this issue.
Then, I have two questions about design of implementation.

The logger, which is initialized by calling ctrl.SetLogger, can be referred by "sigs.k8s.io/controller-runtime/pkg/log".Log and each k8s controller has the logger reference in their structure.

training-operator/pkg/controller.v1/mpi/mpijob_controller.go

Line 109 in f3792b0

Log logr.Logger

So, I think we should pass the logger and attach key-values instead of returning new logger in the LoggerForXXX functions of commonutil pacakge.

func LoggerForUnstructured(logger logr.Logger, *metav1unstructured.Unstructured, kind string) logr.Logger {
  return logger.WithValues(...)
}

Also, Warning method of logrus.Logger is used sometimes but logr.Logger only has Info and Error methods.
While it could be possible to distinguish the log-level by using V method of logr.Logger, I think it is better to set rules for log-level numbers. (Alternatively, it would be possible to unify Info and Warning logs into Info)

Please let me know your opinions. (cc: @tenzen-y)

[tenzen-y]

Sorry for the late action but I'm going to tackle this issue. Then, I have two questions about design of implementation.

The logger, which is initialized by calling ctrl.SetLogger, can be referred by "sigs.k8s.io/controller-runtime/pkg/log".Log and each k8s controller has the logger reference in their structure.

training-operator/pkg/controller.v1/mpi/mpijob_controller.go

Line 109 in f3792b0

Log logr.Logger

So, I think we should pass the logger and attach key-values instead of returning new logger in the LoggerForXXX functions of commonutil pacakge.

func LoggerForUnstructured(logger logr.Logger, *metav1unstructured.Unstructured, kind string) logr.Logger {
  return logger.WithValues(...)
}

Also, Warning method of logrus.Logger is used sometimes but logr.Logger only has Info and Error methods. While it could be possible to distinguish the log-level by using V method of logr.Logger, I think it is better to set rules for log-level numbers. (Alternatively, it would be possible to unify Info and Warning logs into Info)

Please let me know your opinions. (cc: @tenzen-y)

Actually, the original logger is no longer needed. In other words, we can remove the whole of this file.

Because we can define the controller runtime logger for each framework controller with the framework name, and then we can pass the generated logger into the common controllers like pod / job controller via context using these: https://github.com/kubernetes-sigs/controller-runtime/blob/1f5b39fa59d15fae78e521c9c9f2acabbbb3ea17/alias.go#L135-L147

[champon1020]

@tenzen-y Thank you for your description. I understood it 👍

[github-actions]

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.

[champon1020]

Migration ToDo List

• Common package
• Control package
• JAX job controller
• MPI job controller (Migrate to controller-runtime logger in mpi job controller #2177)
• Paddle job controller
• PyTorch job controller
• Tensorflow job controller
• XGBoost job controller