KEP-2170: Initial Implementations for v2 Manager #2236

tenzen-y · 2024-08-27T22:54:48Z

What this PR does / why we need it:
I implemented the skeleton v2 manager. So, this PR introduces only the following things:

Main manager setupper.
The skeleton controllers for TrainJob, TrainingRuntime, and ClusterTrainingRuntime.
The minimum integration tests using envtest so that we can be confident that this manager can be launched successfully.

These skeleton implementations allow us to unblock webhooks or other implementations for other contributors.

This depends on #2237, and #2237 blocks this PR.

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

Checklist:

Docs included if any changes are user facing

tenzen-y · 2024-08-27T23:38:53Z

/hold

coveralls · 2024-08-27T23:42:06Z

Pull Request Test Coverage Report for Build 10705754285

Details

46 of 46 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+68.2%) to 100.0%

Totals
Change from base Build 10701428188:	68.2%
Covered Lines:	46
Relevant Lines:	46

💛 - Coveralls

tenzen-y · 2024-08-30T22:34:02Z

Ready for review.
/assign @kubeflow/wg-training-leads

andreyvelich

Thank you for this @tenzen-y!

andreyvelich · 2024-09-02T13:41:08Z

Makefile

@@ -75,6 +75,10 @@ testall: manifests generate fmt vet golangci-lint test ## Run tests.
 test: envtest
 	KUBEBUILDER_ASSETS="$(shell setup-envtest use $(ENVTEST_K8S_VERSION) -p path)" go test ./... -coverprofile cover.out

+.PHONY: test-integrationv2
+test-integrationv2:


Do we need to add envtest instructions, similar to v1 ?

Oh, that's a good point.
You're right.

andreyvelich · 2024-09-02T13:59:21Z

cmd/training-operator.v2alpha1/main.go

+	flag.StringVar(&webhookServiceName, "webhook-service-name", "training-operator", "Name of the Service used as part of the DNSName")
+	flag.StringVar(&webhookSecretName, "webhook-secret-name", "training-operator-webhook-cert", "Name of the Secret to store CA  and server certs")


Should we separate resources names between V1 and V2 ? In that case, we can give user an option to deploy two versions of Training Operator.

That makes sense.
How about "webhook-secret-name=training-operator-v2-webhook-cert" and "webhook-service-name=training-operator-v2"?

Yes, I think it looks good.

andreyvelich · 2024-09-02T14:01:24Z

pkg/controller.v2/clustertrainingruntime_controller.go

+	}
+}
+
+func (r *ClusterTrainingRuntimeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {


Do we need to have reconcilers for Training Runtimes since we don't do any orchestration for them initially ?

We need this reconciler since the TrainingRuntime reconcilers have only event handlers and predicators.

I will implement event handlers and predicators in the third phase.

1st phase: this PR.
2nd phase: JobSet builder interface.
3rd phase: actual reconciler and event handlers.

What kind of event handlers and predicators are you planning to implement for Training Runtime ?

Sure. I'm planning the following handlers, but I may add more event handling mechanisms in the actual implementation steps.

Create: Trigger the TrainJob reconcilers by triggering the generic event of the TrainJob reconcilers.
Update: Ignore the Update event since the webhook can be disabled by webhookConfiguration failurePolicy.
Delete: Ignore the Delete event when the Runtime is still used by any TrainJob since the webhook can be disabled by webhookConfiguration failurePolicy.
Generic: None

@tenzen-y For the Create event do you mean: Trigger the TrainingRuntime reconcilers by triggering the generic event of the TrainingRuntime reconcilers ?
For the Delete, do you mean that user can't delete TrainingRuntime if it used by any TrainJob ?

For the Create event do you mean: Trigger the TrainingRuntime reconcilers by triggering the generic event of the TrainingRuntime reconcilers ?

No, we do not trigger the TrainingRuntime reconciler; we just trigger the TrainJob reconciler by the TrainingRuntime event handler.

For the Delete, do you mean that user can't delete TrainingRuntime if it used by any TrainJob ?

Yes, that's right. We should prevent accidently deleting the TrainingRuntime when the TrainingRuntime is referred by any TranJob.

No, we do not trigger the TrainingRuntime reconciler; we just trigger the TrainJob reconciler by the TrainingRuntime event handler.

@tenzen-y What orchestration for the TrainJob are we going to do when we trigger such event ?

Let me show the actual step in the following:

During setting up controllers, we pass the go channel for TrainJob Generic EventHandler to the TrainigRuntime controller.

TrainingRuntime Controller: Get TrainingRuntime creation event

TrainingRuntime Controller: Send the object to the TrainJob controller using go channel

TrainJob Controller: Start reconciling

This is a generic technique to start reconciling by out-of-reconcilers. I will implement the mechanism in the 3rd step.

I synced with @andreyvelich offline.
I was supposed to provide functionality creation of TraininigRuntime after the TrainJob was created.
So, I added TrainingRuntime reconcilers. But, we decided not to allow user to create TrainJob before the TrainingRuntime is created in the first iteration. So, users need to create the TrainigRuntime before the TrainJob is created.
Additionally, we decided to handle the deletion of TrainingRuntim by finalizers, and then we never used to reconciler for deletion event for TrainingRuntime.

andreyvelich · 2024-09-02T14:10:24Z

pkg/controller.v2/controller.go

+	}
+	if err := NewTrainJobReconciler(
+		mgr.GetClient(),
+		mgr.GetEventRecorderFor("training-operator-trainjob-controller"),


Do we need to add the training-operator- name in the event recorder ?

Yes, we should add the prefix since this event recorder name is used across the entire cluster.

andreyvelich · 2024-09-02T14:11:39Z

pkg/controller.v2/trainjob_controller.go

+
+func NewTrainJobReconciler(client client.Client, recorder record.EventRecorder) *TrainJobReconciler {
+	return &TrainJobReconciler{
+		log:      ctrl.Log.WithName("trainjob-reconciler"),


Should we call this logger: trainjob-controller ?

I think the "train job-reconciler" would be better since the controller contains reconcilers, event handlers, and predicators.

Yeah, but from the logging perspective we are not going to separate it, isn't ?
It might be easier from user perspective to see those logs under trainjob-controller name.

Uhm, it seems that you have a point.
So, I will replace trainjob-reconciler with trainjob-controller. Thanks

andreyvelich · 2024-09-02T14:16:13Z

test/controller.v2/trainjob_controller_test.go

@@ -0,0 +1,74 @@
+/*


Why do you want to separate those integration tests into separate folder (not into the /pkg/controller.v2/trainjob_controller_test.go)?

Maybe similar to Kueue and JobSet, we should create subfolder for e2e and integration tests under /test directory.

Why do you want to separate those integration tests into separate folder (not into the /pkg/controller.v2/trainjob_controller_test.go)?

This separate test directory allows us to avoid fragmentation of the place for testing.
Because, indeed, the integration testing depends on the multiple directories. So, I think locating integration testing in each directory would not be appropriate.

Maybe similar to Kueue and JobSet, we should create subfolder for e2e and integration tests under /test directory.

Yes, I assumed the directory structure. In the first implementation, we want to add unit and integration (envtest) tests. After that we want to implement E2E testing. How do you think?

Yes, but can we keep the folder structure correctly initially ?
E.g. add the integration tests under:

/test/integration/controller.v2/trainjob_controller_test.go /test/integration/controller.v2/trainingruntime_controller_test.go /test/integration/controller.v2/suite_test.go

Proposed directory structure looks good.
Thank you for proposing that!

andreyvelich · 2024-09-03T14:37:07Z

/assign @kubeflow/wg-training-leads for review

andreyvelich · 2024-09-03T21:18:33Z

test/integration/controller.v2/clustertrainingruntime_controller_test.go

@@ -0,0 +1,72 @@
+/*


Do we need to remove these test as well?

Yes, we can remove this.

andreyvelich · 2024-09-03T21:21:12Z

test/integration/controller.v2/suite_test.go

+	"k8s.io/client-go/rest"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/kubeflow/training-operator/test/integration/framework"


Do you see a value to separate framework logic into separate file ?
E.g. we can add this logic in the suite_test.go , similar to JobSet: https://github.com/kubernetes-sigs/jobset/blob/main/test/integration/controller/suite_test.go

This is needed since we will add webhook server integration testing and we will use this framework to set up testing environment.

- /test/integration/controller.v2/ - /test/integration/webhook.v2/trainjob_test.go - /test/integration/webhook.v2/trainingruntime_test.go - /test/integration/webhook.v2/clustertrainingruntime_test.go

Using integration testing framework would be better between testing for trainjob_controller, trainjob_wwbhook, trainingruntime_webhook, and clustertrainingruntime_webhook.

andreyvelich · 2024-09-03T21:21:26Z

test/integration/controller.v2/trainingruntime_controller_test.go

@@ -0,0 +1,74 @@
+/*


We can remove it for now.

Yes, we can.

andreyvelich · 2024-09-04T12:29:39Z

pkg/controller.v2/controller.go

@@ -0,0 +1,29 @@
+/*


Should we name this file setup.go to make it more descriptive ?

andreyvelich · 2024-09-04T12:31:12Z

pkg/controller.v2/trainjob_controller.go

+import (
+	"context"
+
+	"github.com/go-logr/logr"


I think, we agree before to use controller-runtime logger everywhere to be consistent: #2048
Should we make that change @tenzen-y ?

Actually, this is the controller-runtime logger. The controller-runtime logger implement this logr interface, so we need to import this library to use the controller-runtime logger. You can see the actual controller-runtime logger in the Reconcile function.

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

andreyvelich · 2024-09-04T16:47:52Z

pkg/controller.v2/trainjob_controller.go

+	if err := r.client.Get(ctx, req.NamespacedName, &trainJob); err != nil {
+		return ctrl.Result{}, client.IgnoreNotFound(err)
+	}
+	log := ctrl.LoggerFrom(ctx).WithValues("trainJob", klog.KObj(&trainJob))


What is the value to configure controller runtime logger with klog.KObj(&trainJob) ?
Also, since we add logger to the TrainJob reconciler struct, why do you create a new logger here ?

What is the value to configure controller runtime logger with klog.KObj(&trainJob) ?
Also, since we add logger to the TrainJob reconciler struct, why do you create a new logger here ?

This is not new logger, actually this logger is propagated from Reconciler and we reuse it.
Here, we add information for which objects are reconciling.

Indeed, we do not generate new logger, and not add a logger name (WithName) here. That is WithValue as you can see here.

I see, that makes sense!

andreyvelich

Thanks for doing this @tenzen-y!
/lgtm
/assign @kubeflow/wg-training-leads @shravan-achar @kannon92

andreyvelich

I think, we are ready to merge it!
/approve

google-oss-prow · 2024-09-06T14:01:57Z

[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-09-06T14:09:21Z

/hold cancel

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

google-oss-prow bot requested review from jinchihe and kuizhiqing August 27, 2024 22:54

google-oss-prow bot added the size/XXL label Aug 27, 2024

tenzen-y force-pushed the first-implementaton-for-traininig-v2 branch from 9265a1c to 25e6763 Compare August 27, 2024 23:23

tenzen-y mentioned this pull request Aug 27, 2024

KEP-2170: Generate CRD manifests for v2 CustomResources #2237

Merged

1 task

tenzen-y force-pushed the first-implementaton-for-traininig-v2 branch from 25e6763 to 5828054 Compare August 27, 2024 23:30

google-oss-prow bot added the do-not-merge/hold label Aug 27, 2024

tenzen-y marked this pull request as ready for review August 27, 2024 23:38

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

tenzen-y force-pushed the first-implementaton-for-traininig-v2 branch 4 times, most recently from 4780722 to d52692c Compare August 30, 2024 21:58

google-oss-prow bot added size/XL and removed size/XXL labels Aug 30, 2024

tenzen-y force-pushed the first-implementaton-for-traininig-v2 branch from d52692c to dafd032 Compare August 30, 2024 22:24

andreyvelich reviewed Sep 2, 2024

View reviewed changes

tenzen-y force-pushed the first-implementaton-for-traininig-v2 branch from dafd032 to 470512e Compare September 3, 2024 08:52

tenzen-y force-pushed the first-implementaton-for-traininig-v2 branch from 470512e to 0120280 Compare September 3, 2024 20:47

andreyvelich reviewed Sep 3, 2024

View reviewed changes

tenzen-y force-pushed the first-implementaton-for-traininig-v2 branch from 0120280 to c9b5a9c Compare September 3, 2024 21:34

google-oss-prow bot added size/L and removed size/XL labels Sep 3, 2024

andreyvelich reviewed Sep 4, 2024

View reviewed changes

KEP-2170: Initial Implementations for v2 Manager

9f8e37f

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

tenzen-y force-pushed the first-implementaton-for-traininig-v2 branch from c9b5a9c to 9f8e37f Compare September 4, 2024 16:29

andreyvelich reviewed Sep 4, 2024

View reviewed changes

google-oss-prow bot assigned andreyvelich Sep 4, 2024

google-oss-prow bot added the lgtm label Sep 4, 2024

andreyvelich reviewed Sep 6, 2024

View reviewed changes

google-oss-prow bot added the approved label Sep 6, 2024

google-oss-prow bot removed the do-not-merge/hold label Sep 6, 2024

google-oss-prow bot merged commit 6ddeb2b into kubeflow:master Sep 6, 2024
39 checks passed

tenzen-y deleted the first-implementaton-for-traininig-v2 branch September 6, 2024 14:53

		flag.StringVar(&webhookServiceName, "webhook-service-name", "training-operator", "Name of the Service used as part of the DNSName")
		flag.StringVar(&webhookSecretName, "webhook-secret-name", "training-operator-webhook-cert", "Name of the Secret to store CA and server certs")

KEP-2170: Initial Implementations for v2 Manager #2236

KEP-2170: Initial Implementations for v2 Manager #2236

Conversation

tenzen-y commented Aug 27, 2024 • edited Loading

tenzen-y commented Aug 27, 2024

coveralls commented Aug 27, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10705754285

Details

💛 - Coveralls

tenzen-y commented Aug 30, 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

Choose a reason for hiding this comment

tenzen-y Sep 2, 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

andreyvelich commented Sep 3, 2024

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 Sep 4, 2024 • edited Loading

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

andreyvelich left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Sep 6, 2024

tenzen-y commented Sep 6, 2024

tenzen-y commented Aug 27, 2024 •

edited

Loading

coveralls commented Aug 27, 2024 •

edited

Loading

tenzen-y Sep 2, 2024 •

edited

Loading

tenzen-y Sep 4, 2024 •

edited

Loading