Skip to content
This repository has been archived by the owner on Sep 12, 2023. It is now read-only.

Refactor PodGroup control with more generic design #203

Merged
merged 5 commits into from
Jan 18, 2023

Conversation

tenzen-y
Copy link
Member

@tenzen-y tenzen-y commented Jan 13, 2023

To support scheduler-plugins/coscheduling, this pr refactors the podgroup-related code.

Follow up on #185.

Part-of: kubeflow/training-operator#1722

Co-authored-by: Wang Zhang <zw199006@gmail.com>
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@tenzen-y tenzen-y changed the title [WIP] Refactor PodGroup control with more generic design Refactor PodGroup control with more generic design Jan 14, 2023
@tenzen-y
Copy link
Member Author

tenzen-y commented Jan 14, 2023

This PR is ready for review. PTAL.

/assign @terrytangyuan @gaocegege @johnugeorge @zw0610

@tenzen-y
Copy link
Member Author

Adapting training-operator to this feature with kubeflow/training-operator#1724.

@tenzen-y tenzen-y force-pushed the coscheduling branch 2 times, most recently from a9b5f32 to 99f066d Compare January 15, 2023 17:20
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@johnugeorge
Copy link
Member

Thanks @tenzen-y for adding this quickly before the release. I will review this in 2 days.

@zw0610 Can you also review this?

Copy link
Member

@zw0610 zw0610 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
thank you @tenzen-y so much for this tremendous work

@@ -1,3 +1,4 @@
//go:build !windows
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this line conflict the the line below?

Copy link
Member Author

@tenzen-y tenzen-y Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This marker is automatically added by go fmt ./....

ref: https://go.googlesource.com/proposal/+/master/design/draft-gobuild.md#design

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

//go:build lines
Go 1.17 introduced //go:build lines as a more readable way to write build constraints, instead of // +build lines. As > of Go 1.17, gofmt adds //go:build lines to match existing +build lines and keeps them in sync, while go vet > diagnoses when they are out of sync.

Since the release of Go 1.18 marks the end of support for Go 1.16, all supported versions of Go now understand > //go:build lines. In Go 1.18, go fix now removes the now-obsolete // +build lines in modules declaring go 1.18 or later > in their go.mod files.

For more information, see https://go.dev/design/draft-gobuild.

https://tip.golang.org/doc/go1.18#go-command

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it.

@johnugeorge
Copy link
Member

Thanks @tenzen-y for his hard work

/lgtm
/approve

@google-oss-prow google-oss-prow bot added the lgtm label Jan 18, 2023
@johnugeorge
Copy link
Member

/assign @terrytangyuan

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just left one minor comment regarding copyright header.

@@ -1,3 +1,19 @@
/*
Copyright 2023 The Kubernetes Authors.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be Kubeflow authors?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this is my bad. Yes, you're right.

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@google-oss-prow google-oss-prow bot removed the lgtm label Jan 18, 2023
@tenzen-y
Copy link
Member Author

@terrytangyuan I have addressed your comments. PTAL.

@terrytangyuan
Copy link
Member

/lgtm
/approve

@google-oss-prow google-oss-prow bot added the lgtm label Jan 18, 2023
@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnugeorge, terrytangyuan

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:

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

@google-oss-prow google-oss-prow bot merged commit 3eb4cb3 into kubeflow:master Jan 18, 2023
@tenzen-y tenzen-y deleted the coscheduling branch January 18, 2023 18:11
@tenzen-y
Copy link
Member Author

@terrytangyuan @johnugeorge Can you create a new release? We need to use this feature in kubeflow/training-operator#1724.

@tenzen-y
Copy link
Member Author

Or will @johnugeorge work on kubeflow/training-operator#1725 before we create a new release?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants