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

[postfilter-impl-1] Refactor scheduler preempt interface #92009

Merged
merged 1 commit into from Jun 16, 2020
Merged

[postfilter-impl-1] Refactor scheduler preempt interface #92009

merged 1 commit into from Jun 16, 2020

Conversation

Huang-Wei
Copy link
Member

What type of PR is this?

/kind feature
/sig scheduling

What this PR does / why we need it:

This is the first PR of defaultpreemption plugin, which covers:

  • Replace error with NodeToStatusMap in Preempt() signature
  • Eliminate podPreemptor interface and expose its original functions statelessly
  • Move logic in scheduler.go#preempt() to generic_scheduler.go#Preempt()

It corresponds to the first part of @ahg-g's #91426 (comment)

Which issue(s) this PR fixes:

Part of #91038.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 10, 2020
@Huang-Wei
Copy link
Member Author

/assign @ahg-g

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Huang-Wei

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 Jun 10, 2020
This was referenced Jun 10, 2020
Closed
Closed
@Huang-Wei
Copy link
Member Author

/retest

@Huang-Wei
Copy link
Member Author

/assign @ahg-g

It's ready for review.

ahg-g
ahg-g reviewed Jun 13, 2020
View reviewed changes
Copy link
Member

@ahg-g ahg-g left a comment

Choose a reason for hiding this comment

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

Thanks!

pkg/scheduler/util/utils.go Outdated
Comment on lines 151 to 158
func RemoveNominatedNodeName(cs kubernetes.Interface, pod *v1.Pod) error {
if len(pod.Status.NominatedNodeName) == 0 {
return nil
}
podCopy := pod.DeepCopy()
podCopy.Status.NominatedNodeName = ""
return PatchPod(cs, pod, podCopy)
}
Copy link
Member

Choose a reason for hiding this comment

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

how about

func ClearNominatedNodeName(cs kubernetes.Interface, pods ...*v1.Pod) error {
  for _, p := range pods {
    if len(pod.Status.NominatedNodeName) == 0 {
      continue
    }
    podCopy := p.DeepCopy()
    podCopy.Status.NominatedNodeName = ""
    if err := PatchPod(cs, p, podCopy); err != nil {
      klog.Errorf("Cannot clear 'NominatedNodeName' field of pod %v/%v: %v", p.Namespace, p.Name, err)
      // We do not return as this error is not critical.
     }
   }
}

and remove CleanUpNominatedPods below

Copy link
Member Author

Choose a reason for hiding this comment

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

SG.

pkg/scheduler/util/utils.go

// GetUpdatedPod returns the latest version of <pod> from API server.
func GetUpdatedPod(cs kubernetes.Interface, pod *v1.Pod) (*v1.Pod, error) {
return cs.CoreV1().Pods(pod.Namespace).Get(context.TODO(), pod.Name, metav1.GetOptions{})
Copy link
Member

Choose a reason for hiding this comment

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

we should change this to get the pod from informer cache, not in this PR though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me add a todo.

pkg/scheduler/core/generic_scheduler_test.go Outdated
Comment on lines 2464 to 2465
test.pod.Status.NominatedNodeName = node
client.CoreV1().Pods(test.pod.Namespace).Update(context.TODO(), test.pod, metav1.UpdateOptions{})
Copy link
Member

Choose a reason for hiding this comment

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

why are we doing this inside the loop iterating over the victims?

Copy link
Member Author

Choose a reason for hiding this comment

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

As mentioned in your previous comment, we're now still fetching the pod from clientset, so the test needs to update the pod in fake clienset as well.

Copy link
Member

Choose a reason for hiding this comment

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

sure, but why are we doing the update inside the loop iterating over deletedPodNames, shouldn't the update happen once after or before the loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, the old logic which assigns "node" to "test.pod.Status.NominatedNodeName" misleads me. Updated.

PS: technically I can make expectedPods as a string set to shorten the code, but I have a PR on refactoring the whole TestPreempt testcase, which'd make this more neat. So for now, I'd prefer to just make minimum change.

pkg/scheduler/core/generic_scheduler_test.go
Comment on lines +2471 to +2473
now := metav1.Now()
pod.DeletionTimestamp = &now
deletedPodNames.Delete(pod.Name)
Copy link
Member

Choose a reason for hiding this comment

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

why not do this inside the loop above that iterates over deletedPodNames?

Copy link
Member Author

Choose a reason for hiding this comment

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

The test.expectedPods in the above loop is a string slice, so I cannot update the pod there.

Copy link
Member Author

Choose a reason for hiding this comment

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

As mentioned in #92009 (comment), there will be a PR refactoring whole TestPreempt test later.

@ahg-g ahg-g mentioned this pull request Jun 13, 2020
Merged
@ahg-g
Copy link
Member

ahg-g commented Jun 15, 2020

Thanks, please squash.

@Huang-Wei
Refactor scheduler preempt interface
36c8ecc 
- replace error with NodeToStatusMap in Preempt() signature
- eliminate podPreemptor interface and expose its functions statelessly
- move logic in scheduler.go#preempt to generic_scheduler.go#Preempt()
@ahg-g
Copy link
Member

ahg-g commented Jun 15, 2020

/lgtm
/retest

@Huang-Wei
Copy link
Member Author

Huang-Wei commented Jun 15, 2020 •
edited

@ahg-g Thanks for reviewing! Commits squashed. PTAL.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 15, 2020
@Huang-Wei
Copy link
Member Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit 84799c4 into kubernetes:master Jun 16, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jun 16, 2020
@Huang-Wei Huang-Wei deleted the postfilter-impl-1 branch June 16, 2020 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahg-g ahg-g ahg-g left review comments

@damemi damemi Awaiting requested review from damemi

@resouer resouer Awaiting requested review from resouer

Assignees

@ahg-g ahg-g

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/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Huang-Wei @k8s-ci-robot @ahg-g