Migrate to controller-runtime logger in mpi job controller #2177

champon1020 · 2024-07-18T16:42:03Z

What this PR does / why we need it:
As mentioned at #2048, we should migrate to Controller Runtime Logger while there are some types of logger currently. First, I modified it only in the MPI Job Controller since I want to determine a course of action. (I'm going to implement the remaining modifications in the next PR)

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):
Fixes #2048

Checklist:

Docs included if any changes are user facing

google-oss-prow · 2024-07-18T16:42:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign johnugeorge for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

coveralls · 2024-07-18T16:46:42Z

Pull Request Test Coverage Report for Build 11405323053

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 11390874125:	0.0%
Covered Lines:	73
Relevant Lines:	73

💛 - Coveralls

Signed-off-by: champon1020 <[email protected]>

champon1020 · 2024-07-18T17:34:03Z

pkg/controller.v1/mpi/mpijob_controller.go

@@ -74,7 +72,7 @@ func NewReconciler(mgr manager.Manager, gangSchedulingSetupFunc common.GangSched
 		Scheme:    mgr.GetScheme(),
 		recorder:  mgr.GetEventRecorderFor(controllerName),
 		apiReader: mgr.GetAPIReader(),
-		Log:       log.Log,
+		Log:       log.Log.WithName(controllerName),


note: attach controllerName as the logger name

champon1020 · 2024-07-18T17:34:48Z

pkg/controller.v1/mpi/mpijob_controller.go

 	}()

 	mpiJob = mpiJob.DeepCopy()
 	mpiJob.Status = *jobStatus.DeepCopy()

 	result := jc.Status().Update(context.Background(), mpiJob)
-
-	if result != nil {


note: I think this if statement may be no longer needed

I think so. WDYT @kubeflow/wg-training-leads

Yes, I think we can remove it.

github-actions · 2024-10-17T15:01:44Z

This pull request 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.

andreyvelich · 2024-10-17T16:39:30Z

/remove-lifecycle stale
/assign @kubeflow/wg-training-leads @Electronic-Waste @helenxie-bit @varshaprasad96

Electronic-Waste

Basically LGTM :)

I left a few comments for your reference.

Electronic-Waste · 2024-10-18T03:07:56Z

pkg/controller.v1/mpi/mpijob_controller.go

 		return ctrl.Result{}, err
 	}

 	t, err := util.DurationUntilExpireTime(&mpijob.Spec.RunPolicy, mpijob.Status)
 	if err != nil {
-		logrus.Warnf("Reconcile MPIJob Job error %v", err)
+		logger.Info("Reconcile MPIJob Job error", "error", err)


I think we need to replace it with logger.Error.

@Electronic-Waste fixed (c6de073)

Electronic-Waste · 2024-10-18T03:13:03Z

pkg/controller.v1/mpi/mpijob_controller.go

 	}()

 	mpiJob = mpiJob.DeepCopy()
 	mpiJob.Status = *jobStatus.DeepCopy()

 	result := jc.Status().Update(context.Background(), mpiJob)
-
-	if result != nil {


I think so. WDYT @kubeflow/wg-training-leads

Signed-off-by: champon1020 <[email protected]>

andreyvelich

Thank you for this @champon1020!
I left a few comments.

andreyvelich · 2024-10-18T15:57:38Z

pkg/controller.v1/mpi/mpijob_controller.go

@@ -25,7 +25,6 @@ import (
 	"time"

 	"github.com/go-logr/logr"
-	"github.com/sirupsen/logrus"


Did you get a chance to remove logrus from other parts of Training Operator:

training-operator/pkg/controller.v1/jax/jaxjob_controller.go

Line 32 in 6965c1a

"github.com/sirupsen/logrus"

?

That will allow us to remove that dependency from go.mod.

I found this comment from you @champon1020: #2048 (comment).
Do you want to work on migration for other files in the separate PR ?

Yes, I'm going to create another PR to do migration in other controllers.

andreyvelich · 2024-10-18T15:59:50Z

pkg/controller.v1/mpi/mpijob_controller.go

@@ -125,8 +123,7 @@ type MPIJobReconciler struct {
 // Reconcile is part of the main kubernetes reconciliation loop which aims to
 // move the current state of the cluster closer to the desired state.
 func (jc *MPIJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-	_ = log.FromContext(ctx)
-	logger := jc.Log.WithValues(kubeflowv1.MPIJobSingular, req.NamespacedName)
+	logger := jc.Log.WithValues("namespace", req.NamespacedName)


Why did you change it ?

We can get the logger from the context and augment it with additional info from the reconciler's request with labels:

log := ctrl.LoggerFrom(ctx).WithValues(....)

Why did you change it ?

I think the MPIJobSingular, whose value is "mpijob", is not clear as the key of NamespacedName.
Moreover, I attached the controllerName as the logger name here (#2177 (comment)).

Are there any reasons that MPIJobSingular was used here?

We can get the logger from the context and augment it with additional info from the reconciler's request with labels:

To use LoggerFrom method, we need to modify some methods to receive context.Context. Since it requires much more modifications, I think it is better to work on as another issue.

andreyvelich · 2024-10-18T16:01:12Z

pkg/controller.v1/mpi/mpijob_controller.go

 	if err := jc.Delete(context.Background(), mpiJob); err != nil {
+		logger.Error(err, "failed to delete job")


Suggested change

logger.Error(err, "failed to delete job")

logger.Error(err, "failed to delete MPIJob")

andreyvelich · 2024-10-18T16:01:28Z

pkg/controller.v1/mpi/mpijob_controller.go

 		return err
 	}

 	jc.Recorder.Eventf(mpiJob, corev1.EventTypeNormal, SuccessfulDeleteJobReason, "Deleted job: %v", mpiJob.Name)
-	log.Infof("job %s/%s has been deleted", mpiJob.Namespace, mpiJob.Name)
+	logger.Info("job has been deleted")


Suggested change

logger.Info("job has been deleted")

logger.Info("MPIJob has been deleted")

andreyvelich · 2024-10-18T16:01:45Z

pkg/controller.v1/mpi/mpijob_controller.go

@@ -573,8 +578,12 @@ func (jc *MPIJobReconciler) UpdateJobStatus(job interface{}, replicas map[kubefl
 		running := status.Active
 		failed := status.Failed

-		logrus.Infof("MPIJob=%s, ReplicaType=%s expected=%d, running=%d, succeeded=%d , failed=%d",
-			mpiJob.Name, rtype, expected, running, succeeded, failed)
+		logger.Info("replica status",


Suggested change

logger.Info("replica status",

logger.Info("MPIJob replica status",

andreyvelich · 2024-10-18T16:03:18Z

pkg/controller.v1/mpi/mpijob_controller.go

 	}()

 	mpiJob = mpiJob.DeepCopy()
 	mpiJob.Status = *jobStatus.DeepCopy()

 	result := jc.Status().Update(context.Background(), mpiJob)
-
-	if result != nil {


Yes, I think we can remove it.

varshaprasad96 · 2024-10-20T20:28:36Z

pkg/controller.v1/mpi/mpijob_controller.go

@@ -125,8 +123,7 @@ type MPIJobReconciler struct {
 // Reconcile is part of the main kubernetes reconciliation loop which aims to
 // move the current state of the cluster closer to the desired state.
 func (jc *MPIJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-	_ = log.FromContext(ctx)
-	logger := jc.Log.WithValues(kubeflowv1.MPIJobSingular, req.NamespacedName)
+	logger := jc.Log.WithValues("namespace", req.NamespacedName)


We can get the logger from the context and augment it with additional info from the reconciler's request with labels:

log := ctrl.LoggerFrom(ctx).WithValues(....)

varshaprasad96 · 2024-10-20T20:29:14Z

pkg/controller.v1/mpi/mpijob_controller.go

@@ -164,13 +161,13 @@ func (jc *MPIJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 	// MPIJob needs not service
 	err = jc.ReconcileJobs(mpijob, mpijob.Spec.MPIReplicaSpecs, mpijob.Status, &mpijob.Spec.RunPolicy)
 	if err != nil {
-		logrus.Warnf("Reconcile MPIJob error %v", err)
+		logger.Info("Reconcile MPIJob error", "error", err)


Do we want to set verbosity?

I've replaced logger.Info with logger.Error as mentioned here (#2177 (comment)), but using verbosity might be a good approach.
Is there any rules to set verbosity? (I don't have any ideas how much verbosity is proper here)

varshaprasad96 · 2024-10-20T20:36:26Z

pkg/controller.v1/mpi/mpijob_controller.go

@@ -316,9 +313,11 @@ func (jc *MPIJobReconciler) onOwnerCreateFunc() func(event.CreateEvent) bool {
 			return true
 		}

+		logger := jc.Log.WithValues("namespace", mpiJob.Namespace, "name", mpiJob.Name)


Is this namespace same as reconciler request namespace or different? If it's the same, can we insert the labelled logger into the context which we set while initialising (in line 126) and use it as is? If not, do we want to change the key name to something specific?

google-oss-prow bot added the do-not-merge/work-in-progress label Jul 18, 2024

google-oss-prow bot requested review from jinchihe and kuizhiqing July 18, 2024 16:42

google-oss-prow bot added the size/M label Jul 18, 2024

champon1020 force-pushed the patch-issue-2048 branch 2 times, most recently from 89ae18a to 00b6c51 Compare July 18, 2024 17:55

use controller-runtime logging in mpi job controller

bbc9872

Signed-off-by: champon1020 <[email protected]>

champon1020 force-pushed the patch-issue-2048 branch from 00b6c51 to bbc9872 Compare July 19, 2024 10:28

champon1020 commented Jul 19, 2024

View reviewed changes

champon1020 marked this pull request as ready for review July 19, 2024 11:32

google-oss-prow bot removed the do-not-merge/work-in-progress label Jul 19, 2024

github-actions bot added the lifecycle/stale label Oct 17, 2024

google-oss-prow bot removed the lifecycle/stale label Oct 17, 2024

Electronic-Waste reviewed Oct 18, 2024

View reviewed changes

fix: replace logger.Info with logger.Error

c6de073

Signed-off-by: champon1020 <[email protected]>

champon1020 force-pushed the patch-issue-2048 branch from 4746ae5 to c6de073 Compare October 18, 2024 14:10

champon1020 mentioned this pull request Oct 18, 2024

Migrate to controller-runtime logger #2048

Open

andreyvelich reviewed Oct 18, 2024

View reviewed changes

varshaprasad96 reviewed Oct 20, 2024

View reviewed changes

champon1020 requested review from Electronic-Waste, varshaprasad96 and andreyvelich November 1, 2024 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to controller-runtime logger in mpi job controller #2177

Migrate to controller-runtime logger in mpi job controller #2177

champon1020 commented Jul 18, 2024

google-oss-prow bot commented Jul 18, 2024

coveralls commented Jul 18, 2024 •

edited

Loading

champon1020 Jul 18, 2024

champon1020 Jul 18, 2024

Electronic-Waste Oct 18, 2024

andreyvelich Oct 18, 2024

github-actions bot commented Oct 17, 2024

andreyvelich commented Oct 17, 2024

Electronic-Waste left a comment

Electronic-Waste Oct 18, 2024

champon1020 Oct 18, 2024

Electronic-Waste Oct 18, 2024

andreyvelich left a comment

andreyvelich Oct 18, 2024

andreyvelich Oct 18, 2024

champon1020 Oct 29, 2024

andreyvelich Oct 18, 2024

varshaprasad96 Oct 20, 2024 •

edited

Loading

champon1020 Oct 29, 2024 •

edited

Loading

champon1020 Oct 29, 2024

andreyvelich Oct 18, 2024

andreyvelich Oct 18, 2024

andreyvelich Oct 18, 2024

andreyvelich Oct 18, 2024

varshaprasad96 Oct 20, 2024 •

edited

Loading

varshaprasad96 Oct 20, 2024

champon1020 Oct 29, 2024

varshaprasad96 Oct 20, 2024 •

edited

Loading

		if err := jc.Delete(context.Background(), mpiJob); err != nil {
		logger.Error(err, "failed to delete job")

	logger.Error(err, "failed to delete job")
	logger.Error(err, "failed to delete MPIJob")

	logger.Info("job has been deleted")
	logger.Info("MPIJob has been deleted")

	logger.Info("replica status",
	logger.Info("MPIJob replica status",

Migrate to controller-runtime logger in mpi job controller #2177

Are you sure you want to change the base?

Migrate to controller-runtime logger in mpi job controller #2177

Conversation

champon1020 commented Jul 18, 2024

google-oss-prow bot commented Jul 18, 2024

coveralls commented Jul 18, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11405323053

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 17, 2024

andreyvelich commented Oct 17, 2024

Electronic-Waste left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varshaprasad96 Oct 20, 2024 • edited Loading

Choose a reason for hiding this comment

champon1020 Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varshaprasad96 Oct 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varshaprasad96 Oct 20, 2024 • edited Loading

Choose a reason for hiding this comment

coveralls commented Jul 18, 2024 •

edited

Loading

varshaprasad96 Oct 20, 2024 •

edited

Loading

champon1020 Oct 29, 2024 •

edited

Loading

varshaprasad96 Oct 20, 2024 •

edited

Loading

varshaprasad96 Oct 20, 2024 •

edited

Loading