Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(apis): add completed state to podgroup&queue #66

Merged
merged 1 commit into from
Apr 27, 2022

Conversation

shinytang6
Copy link
Member

@shinytang6 shinytang6 commented Mar 26, 2022

@shinytang6 shinytang6 force-pushed the feat/new-pg-state branch 2 times, most recently from 2f2e235 to c92b255 Compare March 26, 2022 08:00
@shinytang6
Copy link
Member Author

Pls help take a look :)
/cc @Thor-wl @william-wang @k82cn @Yikun @hwdef

@@ -289,6 +292,8 @@ type QueueStatus struct {
Running int32 `json:"running,omitempty" protobuf:"bytes,4,opt,name=running"`
// The number of `Inqueue` PodGroup in this queue.
Inqueue int32 `json:"inqueue,omitempty" protobuf:"bytes,5,opt,name=inqueue"`
// The number of `Completed` PodGroup in this queue.
Completed int32 `json:"completed,omitempty" protobuf:"bytes,5,opt,name=completed"`
Copy link
Member

Choose a reason for hiding this comment

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

seq num should be 6.

Copy link
Member Author

Choose a reason for hiding this comment

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

changed it to 7 due to 6 already exists

@@ -47,7 +47,7 @@ EXT_APIS_PKG="$4"
GROUPS_WITH_VERSIONS="$5"
shift 5

go get k8s.io/code-generator/cmd/{defaulter-gen,conversion-gen,client-gen,lister-gen,informer-gen,deepcopy-gen,openapi-gen}@v0.23.0
go install k8s.io/code-generator/cmd/{defaulter-gen,conversion-gen,client-gen,lister-gen,informer-gen,deepcopy-gen,openapi-gen}@v0.23.0
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be better to include the patch updating get to install to a spearated commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, remove it

@Thor-wl
Copy link
Contributor

Thor-wl commented Mar 26, 2022

BTW, why add completed status to queue?

Signed-off-by: shinytang6 <1074461480@qq.com>
@@ -51,6 +51,9 @@ const (
// PodGroupInqueue means controllers can start to create pods,
// is a new state between PodGroupPending and PodGroupRunning
PodGroupInqueue PodGroupPhase = "Inqueue"

// PodGroupCompleted means all the pods of PodGroup are completed
PodGroupCompleted PodGroupPhase = "Completed"
Copy link
Member

@k82cn k82cn Mar 26, 2022

Choose a reason for hiding this comment

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

How about some Pods are completred but others are failed? Are we going to distinguish succeed & failure?

Copy link
Member Author

Choose a reason for hiding this comment

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

My idea is not distinguish succeed & failure pods. In most common scenary, if successful + failed pods == total replicas, we set the PodGroup to Completed.

@Thor-wl
Copy link
Contributor

Thor-wl commented Apr 27, 2022

/lgtm
/approve

@volcano-sh-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Thor-wl

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

@volcano-sh-bot volcano-sh-bot merged commit 9c3fef8 into volcano-sh:master Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants