-
Notifications
You must be signed in to change notification settings - Fork 71
Refactor PodGroup control with more generic design #203
Conversation
badfe53
to
f0e1019
Compare
6e0e109
to
c8c72c8
Compare
Co-authored-by: Wang Zhang <zw199006@gmail.com> Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
c8c72c8
to
a5dd0b1
Compare
This PR is ready for review. PTAL. /assign @terrytangyuan @gaocegege @johnugeorge @zw0610 |
Adapting training-operator to this feature with kubeflow/training-operator#1724. |
a9b5f32
to
99f066d
Compare
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
99f066d
to
63a9a34
Compare
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
136948c
to
b0caefa
Compare
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
b0caefa
to
865b544
Compare
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it.
Thanks @tenzen-y for his hard work /lgtm |
/assign @terrytangyuan |
There was a problem hiding this 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.
pkg/apis/common/v1/interface.go
Outdated
@@ -1,3 +1,19 @@ | |||
/* | |||
Copyright 2023 The Kubernetes Authors. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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>
@terrytangyuan I have addressed your comments. PTAL. |
/lgtm |
[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 |
@terrytangyuan @johnugeorge Can you create a new release? We need to use this feature in kubeflow/training-operator#1724. |
Or will @johnugeorge work on kubeflow/training-operator#1725 before we create a new release? |
To support scheduler-plugins/coscheduling, this pr refactors the podgroup-related code.
Follow up on #185.
Part-of: kubeflow/training-operator#1722