KEP-2170: Implement TrainJob Reconciler to manage objects #2295

tenzen-y · 2024-10-20T16:44:12Z

What this PR does / why we need it:
I implemented the object creation and update in the TrainJob reconciler.
I will implement the TrainJob status operation in a follow-up PR.

Additionally, I restricted the directories in make test so that we can perform tests only for the v1 APIs and controllers.

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):
Part-of #2207
Relates to #2170

Checklist:

Docs included if any changes are user facing

coveralls · 2024-10-20T17:00:17Z

Pull Request Test Coverage Report for Build 11469279753

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 11426928089:	0.0%
Covered Lines:	77
Relevant Lines:	77

💛 - Coveralls

Signed-off-by: Yuki Iwai <[email protected]>

tenzen-y · 2024-10-20T17:34:44Z

/assign @kubeflow/wg-training-leads

tenzen-y · 2024-10-20T21:52:30Z

/hold

andreyvelich

Thank you for this @tenzen-y!
I left a few comments.
/assign @kubeflow/wg-training-leads @kuizhiqing @akshaychitneni @varshaprasad96

andreyvelich · 2024-10-21T16:48:21Z

Makefile

@@ -127,3 +128,18 @@ controller-gen: ## Download controller-gen locally if necessary.
 KUSTOMIZE = $(shell pwd)/bin/kustomize
 kustomize: ## Download kustomize locally if necessary.
 	GOBIN=$(PROJECT_DIR)/bin go install sigs.k8s.io/kustomize/kustomize/[email protected]
+
+## Download external CRDs for the integration testings.
+EXTERNAL_CRDS_DIR ?= $(PROJECT_DIR)/dep-crds


Maybe we should put them to /manifests/external-crds for consistency ?

This is only for integration testing, so these manifests is not stored git. Indeed, this directory is specified in the .gitignore.

Yeah, I understand this.
Just for the developers convenience, I suggested to put them under manifests/external-crds and add this folder to the .gitignore

That makes sense. I will move the dep-crds directory to the manifests/external-crds directory.
Thanks.

andreyvelich · 2024-10-21T16:51:25Z

pkg/controller.v2/trainjob_controller.go

 }

-func NewTrainJobReconciler(client client.Client, recorder record.EventRecorder) *TrainJobReconciler {
+func NewTrainJobReconciler(client client.Client, recorder record.EventRecorder, runs map[string]runtime.Runtime) *TrainJobReconciler {


Let's call it runtimes for consistency.

Suggested change

func NewTrainJobReconciler(client client.Client, recorder record.EventRecorder, runs map[string]runtime.Runtime) *TrainJobReconciler {

func NewTrainJobReconciler(client client.Client, recorder record.EventRecorder, runtimes map[string]runtime.Runtime) *TrainJobReconciler {

andreyvelich · 2024-10-21T16:53:14Z

pkg/controller.v2/trainjob_controller.go

+	log := ctrl.LoggerFrom(ctx)
+
+	// Controller assumes the runtime existence has already verified in the webhook on TrainJob creation.
+	run := r.runtimes[runtimeRefToGroupKind(trainJob.Spec.RuntimeRef).String()]


Suggested change

run := r.runtimes[runtimeRefToGroupKind(trainJob.Spec.RuntimeRef).String()]

runtime := r.runtimes[runtimeRefToGroupKind(trainJob.Spec.RuntimeRef).String()]

andreyvelich · 2024-10-21T16:58:34Z

pkg/controller.v2/trainjob_controller.go

+		Group: ptr.Deref(runtimeRef.APIGroup, ""),
+		Kind:  ptr.Deref(runtimeRef.Kind, ""),


Do we need to use Deref here if APIGroup and Kind can't be empty at this stage ?

I guess that we can not restrict these field like non empty. Especially, these should be accepted an empty value so that they can specify the core API type.

Especially, these should be accepted an empty value so that they can specify the core API type.

What is the use-case when users can reference the object from Kubernetes Core, like Service or Pods ?
I thought that users can only reference runtimes that being registered within our Training Operator manager.

I guess that they want to register Pod to the runtimes, and I sometimes have heard from some users the use cases since they often use the plain Pod with many special annotations in the production cluster to mitigate CRD migration costs. For sure, I understand that plain Pod with special annotations is not ideal approach. But, no restrictions would be better in the runtime registration level.

Alright, for the Kind, should it be always non-empty ?

Alright, for the Kind, should it be always non-empty ?

As runtime specifications, Kind should be always non-empty.
But, there are some ways to avoid the validations like failurePolicy. So, I would recommend leaving this deref.

andreyvelich · 2024-10-21T17:03:11Z

pkg/controller.v2/trainjob_controller.go

+			"namespace", obj.GetNamespace(),
+			"name", obj.GetName(),


Do we set name and namespace for the build objects ?
E.g. here I can see that we only set the TypeMeta and Spec:

training-operator/pkg/runtime.v2/core/trainingruntime.go

Lines 113 to 119 in 9e04bdd

info := runtime.NewInfo(&jobsetv1alpha2.JobSet{

TypeMeta: metav1.TypeMeta{

APIVersion: jobsetv1alpha2.SchemeGroupVersion.String(),

Kind: "JobSet",

},

Spec: *jobSetTemplateSpec.Spec.DeepCopy(),

}, opts...)

Yes, we do. Actual resource names are set in each plugins in the following:

JobSet:

training-operator/pkg/runtime.v2/framework/plugins/jobset/builder.go

Lines 42 to 43 in 9e04bdd

Namespace: objectKey.Namespace,

Name: objectKey.Name,

PodGroup:

training-operator/pkg/runtime.v2/framework/plugins/coscheduling/coscheduling.go

Lines 121 to 122 in 9e04bdd

Name: trainJob.Name,

Namespace: trainJob.Namespace,

Why for JobSet plugin you want to have separate Builder, but for co-scheduling you create a PodSpec object directly under Build() API ?

Why for JobSet plugin you want to have separate Builder, but for co-scheduling you create a PodSpec object directly under Build() API ?

That is simply reason. There are so many fields and nested levels in the JobSet, but the PodGroup has a few field and a few nested level. For sure, we can provide the builder in the PodGroup as well. But, I guess that the PodGroup builder seems to be slightly overkill.

Sounds good!

andreyvelich · 2024-10-21T17:08:20Z

pkg/runtime.v2/framework/plugins/jobset/jobset.go

+			},
+			Spec: raw.Spec,
+		})
+		oldJobSet = nil


Do you need this ?

Yes, we need this for the needsCreateOrUpdate function.

andreyvelich · 2024-10-21T17:13:58Z

pkg/controller.v2/trainjob_controller.go

+		switch {
+		case created:
+			log.V(5).Info("Succeeded to create object", logKeysAndValues)
+			continue
+		case client.IgnoreAlreadyExists(creationErr) != nil:
+			return creationErr
+		default:
+			// This indicates CREATE operation has not been performed or the object has already existed in the cluster.
+			if err = r.client.Update(ctx, obj); err != nil {
+				return err
+			}
+			log.V(5).Info("Succeeded to update object", logKeysAndValues)
+		}


Would it be possible to make this condition simpler ? E.g. if GetResourceVersion != "" we know that object exists and we should perform UPDATE operation, otherwise we should just perform CREATE operation.
If CREATE or UPDATE action fails, we return the error.

I believe that we need to check if the creationErr is AlreadyExists error. Because we do not know if the runtime interface returns objects with resourceVersion for UPDATE operation, and can not guarantee to have the resourceVersion as an implementation level.

After we migrate to the SSA patch (#2297), we can avoid this complexity and just issue the SSA patch for CREATE and UPDATE.

I see, that makes sense!

Signed-off-by: Yuki Iwai <[email protected]>

tenzen-y · 2024-10-22T22:08:53Z

@andreyvelich I addressed all your comments. PTAL, thanks.

andreyvelich

Thanks @tenzen-y!
Feel free to unblock it
/lgtm
/approve

google-oss-prow · 2024-10-23T01:58:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [andreyvelich]

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

tenzen-y · 2024-10-23T02:10:48Z

Thanks @tenzen-y!

Feel free to unblock it

/lgtm

/approve

Thank you for the review.
/hold cancel

@varshaprasad96 FYI, now you can start SSA patch implementations.

google-oss-prow bot requested review from jinchihe and kuizhiqing October 20, 2024 16:44

google-oss-prow bot added the size/L label Oct 20, 2024

tenzen-y force-pushed the implement-trainjob-reconciler branch from b31fc96 to e0fc3f9 Compare October 20, 2024 16:55

tenzen-y force-pushed the implement-trainjob-reconciler branch from e0fc3f9 to 55ac657 Compare October 20, 2024 17:20

KEP-2170: Implement TrainJob Reconciler to manage objects

9e04bdd

Signed-off-by: Yuki Iwai <[email protected]>

tenzen-y force-pushed the implement-trainjob-reconciler branch from 55ac657 to 9e04bdd Compare October 20, 2024 17:30

google-oss-prow bot added the do-not-merge/hold label Oct 20, 2024

andreyvelich reviewed Oct 21, 2024

View reviewed changes

tenzen-y mentioned this pull request Oct 21, 2024

KEP-2170: Decouple JobSet from TrainJob #2296

Merged

1 task

tenzen-y added 2 commits October 23, 2024 06:51

Mode dep-crds to manifests/external-crds

94507e6

Signed-off-by: Yuki Iwai <[email protected]>

Rename run with runtime

6016495

Signed-off-by: Yuki Iwai <[email protected]>

andreyvelich reviewed Oct 23, 2024

View reviewed changes

google-oss-prow bot assigned andreyvelich Oct 23, 2024

google-oss-prow bot added the lgtm label Oct 23, 2024

google-oss-prow bot added the approved label Oct 23, 2024

google-oss-prow bot removed the do-not-merge/hold label Oct 23, 2024

google-oss-prow bot merged commit 74b093e into kubeflow:master Oct 23, 2024
40 of 41 checks passed

tenzen-y deleted the implement-trainjob-reconciler branch October 23, 2024 02:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-2170: Implement TrainJob Reconciler to manage objects #2295

KEP-2170: Implement TrainJob Reconciler to manage objects #2295

tenzen-y commented Oct 20, 2024 •

edited

Loading

coveralls commented Oct 20, 2024 •

edited

Loading

tenzen-y commented Oct 20, 2024

tenzen-y commented Oct 20, 2024

andreyvelich left a comment

andreyvelich Oct 21, 2024

tenzen-y Oct 21, 2024

andreyvelich Oct 21, 2024

tenzen-y Oct 21, 2024

tenzen-y Oct 22, 2024

andreyvelich Oct 21, 2024

tenzen-y Oct 22, 2024

andreyvelich Oct 21, 2024

tenzen-y Oct 22, 2024

andreyvelich Oct 21, 2024

tenzen-y Oct 21, 2024

andreyvelich Oct 21, 2024

tenzen-y Oct 21, 2024

andreyvelich Oct 21, 2024

tenzen-y Oct 22, 2024

andreyvelich Oct 21, 2024

tenzen-y Oct 21, 2024

andreyvelich Oct 21, 2024

tenzen-y Oct 21, 2024

andreyvelich Oct 21, 2024

andreyvelich Oct 21, 2024

tenzen-y Oct 21, 2024

andreyvelich Oct 21, 2024

tenzen-y Oct 21, 2024

andreyvelich Oct 21, 2024

tenzen-y commented Oct 22, 2024

andreyvelich left a comment

google-oss-prow bot commented Oct 23, 2024

tenzen-y commented Oct 23, 2024

	func NewTrainJobReconciler(client client.Client, recorder record.EventRecorder, runs map[string]runtime.Runtime) *TrainJobReconciler {
	func NewTrainJobReconciler(client client.Client, recorder record.EventRecorder, runtimes map[string]runtime.Runtime) *TrainJobReconciler {

	run := r.runtimes[runtimeRefToGroupKind(trainJob.Spec.RuntimeRef).String()]
	runtime := r.runtimes[runtimeRefToGroupKind(trainJob.Spec.RuntimeRef).String()]

		Group: ptr.Deref(runtimeRef.APIGroup, ""),
		Kind: ptr.Deref(runtimeRef.Kind, ""),

	info := runtime.NewInfo(&jobsetv1alpha2.JobSet{
	TypeMeta: metav1.TypeMeta{
	APIVersion: jobsetv1alpha2.SchemeGroupVersion.String(),
	Kind: "JobSet",
	},
	Spec: *jobSetTemplateSpec.Spec.DeepCopy(),
	}, opts...)

KEP-2170: Implement TrainJob Reconciler to manage objects #2295

KEP-2170: Implement TrainJob Reconciler to manage objects #2295

Conversation

tenzen-y commented Oct 20, 2024 • edited Loading

coveralls commented Oct 20, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11469279753

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

tenzen-y commented Oct 20, 2024

tenzen-y commented Oct 20, 2024

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y commented Oct 22, 2024

andreyvelich left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Oct 23, 2024

tenzen-y commented Oct 23, 2024

tenzen-y commented Oct 20, 2024 •

edited

Loading

coveralls commented Oct 20, 2024 •

edited

Loading