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.

Sign up for GitHub

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

Jump to bottom

Cleanup PostBind in coscheduling #554

Merged
merged 1 commit into from
Mar 28, 2023
Merged

Cleanup PostBind in coscheduling #554

merged 1 commit into from
Mar 28, 2023

Conversation

nayihz
Copy link
Contributor

@nayihz nayihz commented Mar 24, 2023 •
edited
Loading

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

this logic has been moved to PodGroup's controller.

Which issue(s) this PR fixes:

Fixes ##531

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Remove the PostBind implementation from the coscheduling plugin.

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 24, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @czybjtu. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 24, 2023
PiotrProkop
PiotrProkop reviewed Mar 24, 2023
View reviewed changes
pkg/controllers/podgroup_controller.go Outdated
@@ -109,6 +111,11 @@ func (r *PodGroupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
if len(pods) >= int(pg.Spec.MinMember) {
pgCopy.Status.Phase = schedv1alpha1.PodGroupPreScheduling
fillOccupiedObj(pgCopy, &pods[0])
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

why add this else? Is this necessary to remove PostBind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this else is unnecessary.
PostBind logic has been implmented in PodGroup controller. We can delete it as stated in comments.

scheduler-plugins/pkg/coscheduling/core/core.go

Lines 207 to 209 in 2ea5a97

// PostBind updates a PodGroup's status.
// TODO: move this logic to PodGroup's controller.
func (pgMgr *PodGroupManager) PostBind(ctx context.Context, pod *corev1.Pod, nodeName string) {

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@PiotrProkop
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 24, 2023
@nayihz nayihz force-pushed the cleanup_postbind branch from 280ae89 to d640256 Compare March 25, 2023 02:46
@PiotrProkop
Copy link
Contributor

/lgtm
/assign @Huang-Wei @denkensk for final approval

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2023
@ffromani
Copy link
Contributor

/cc

@k8s-ci-robot k8s-ci-robot requested a review from ffromani March 27, 2023 10:58
denkensk
denkensk reviewed Mar 28, 2023
View reviewed changes
pkg/controllers/podgroup_test.go
@@ -109,14 +109,13 @@ func Test_Run(t *testing.T) {
podNextPhase: v1.PodSucceeded,
},
{
name: "Group status convert from scheduling to succeed",
name: "Group status convert from pending to scheduling",
Copy link
Member

Choose a reason for hiding this comment

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

@czybjtu
Can you explain why we need to change this test case here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a duplicate of line 102.

scheduler-plugins/pkg/controllers/podgroup_test.go

Line 102 in 2ea5a97

name: "Group status convert from scheduling to succeed",

@denkensk
Copy link
Member

/approve

Thanks for your contribution. @czybjtu

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: czybjtu, denkensk

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 28, 2023
@k8s-ci-robot k8s-ci-robot merged commit 462baf8 into kubernetes-sigs:master Mar 28, 2023
@nayihz nayihz deleted the cleanup_postbind branch March 28, 2023 06:45
@nayihz nayihz mentioned this pull request Mar 28, 2023
Closed
@Huang-Wei
Copy link
Contributor

@czybjtu could you please update the release-notes section of this issue? like:

Remove the PostBind implementation from the coscheduling plugin.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Mar 29, 2023
@nayihz
Copy link
Contributor Author

nayihz commented Mar 29, 2023

@czybjtu could you please update the release-notes section of this issue? like:

Remove the PostBind implementation from the coscheduling plugin.

Done

@Gekko0114 Gekko0114 mentioned this pull request Mar 29, 2023
Closed
@nayihz nayihz mentioned this pull request Apr 10, 2023
Closed
9 tasks
@Gekko0114 Gekko0114 mentioned this pull request Apr 10, 2023
Merged
@tenzen-y tenzen-y mentioned this pull request Nov 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PiotrProkop PiotrProkop PiotrProkop left review comments

@denkensk denkensk denkensk left review comments

@seanmalloy seanmalloy Awaiting requested review from seanmalloy

@ffromani ffromani Awaiting requested review from ffromani

Assignees

@PiotrProkop PiotrProkop

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nayihz @k8s-ci-robot @PiotrProkop @ffromani @denkensk @Huang-Wei