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

feature(descheduler): add grace_period_seconds for DeschedulerPolicy #1538

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feature(descheduler): add grace_period_seconds for DeschedulerPolicy #1538

wants to merge 1 commit into from

Conversation

googs1025
Copy link
Member

/kind feature

  • add grace_period_seconds for DeschedulerPolicy

Fix #1537

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 30, 2024
@googs1025 googs1025 mentioned this pull request Oct 30, 2024
Open
@googs1025 googs1025 marked this pull request as ready for review November 4, 2024 14:03
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 4, 2024
@googs1025 googs1025 force-pushed the feature/grace_period_seconds branch from 7a7d780 to 84abd13 Compare November 4, 2024 14:15
@ingvagabund
Copy link
Contributor

@googs1025 looks good in overall. Thank you. This is a good candidate for e2e testing. Would you please add an e2e test as well? So we can keep validating the functionality.

@ingvagabund ingvagabund self-assigned this Nov 12, 2024
@googs1025
Copy link
Member Author

@googs1025 looks good in overall. Thank you. This is a good candidate for e2e testing. Would you please add an e2e test as well? So we can keep validating the functionality.

sound great. maybe I can work this in weekend. :)

@googs1025
Copy link
Member Author

I looked at the e2e test. Do you @ingvagabund have any good suggestions? I am currently planning to pick one of them for testing. For example: use the test/e2e/e2e_failedpods_test.go Policy for testing. Set GracePeriodSeconds. For example: set GracePeriodSeconds to 60 seconds, use wait.PollUntilContextTimeout to test before 60 seconds, the expected EvictedPodCount should be 0, and use wait.PollUntilContextTimeout again after 60 seconds. The expected EvictedPodCount should be actualEvictedPodCount. 🤔

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 19, 2024
@ingvagabund
Copy link
Contributor

The simplest test will do :) I would go with https://github.com/kubernetes-sigs/descheduler/blob/master/test/e2e/e2e_toomanyrestarts_test.go. Better to evict a pod that's still running.

@googs1025 googs1025 force-pushed the feature/grace_period_seconds branch from 84abd13 to ac99d2e Compare December 19, 2024 12:43
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 19, 2024
@googs1025 googs1025 force-pushed the feature/grace_period_seconds branch from ac99d2e to 6a93bd3 Compare December 21, 2024 12:55
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 21, 2024
@googs1025 googs1025 force-pushed the feature/grace_period_seconds branch 2 times, most recently from 721d33b to d7725a4 Compare December 21, 2024 14:09
googs1025
googs1025 commented Dec 21, 2024
View reviewed changes
test/e2e/e2e_toomanyrestarts_test.go
@@ -197,6 +205,30 @@ func TestTooManyRestarts(t *testing.T) {
deschedulerPodName = deschedulerPods[0].Name
}

// Check if grace period is enabled and wait accordingly
if tc.enableGracePeriod {
Copy link
Member Author

Choose a reason for hiding this comment

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

We add a wait here to ensure that no pod is evicted during the grace period.

@googs1025 googs1025 force-pushed the feature/grace_period_seconds branch 2 times, most recently from baaed1c to 1ff63a4 Compare December 21, 2024 15:05
@googs1025 googs1025 force-pushed the feature/grace_period_seconds branch 3 times, most recently from e35cc6b to e682927 Compare December 21, 2024 15:42
@googs1025
Copy link
Member Author

/test pull-descheduler-test-e2e-k8s-master-1-31

@googs1025
Copy link
Member Author

@googs1025 looks good in overall. Thank you. This is a good candidate for e2e testing. Would you please add an e2e test as well? So we can keep validating the functionality.

@ingvagabund added

a7i
a7i reviewed Dec 23, 2024
View reviewed changes
pkg/descheduler/evictions/evictions.go Outdated
@@ -517,7 +519,7 @@ func (pe *PodEvictor) EvictPod(ctx context.Context, pod *v1.Pod, opts EvictOptio
return err
}

ignore, err := pe.evictPod(ctx, pod)
ignore, err := pe.evictPod(ctx, pod, pe.policyGroupVersion, pe.gracePeriodSeconds)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it being passed in as an argument, since it's already available on the PodEvictor receiver?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

a7i
a7i reviewed Dec 23, 2024
View reviewed changes
pkg/descheduler/evictions/evictions.go Outdated
Comment on lines 567 to 568
func (pe *PodEvictor) evictPod(ctx context.Context, pod *v1.Pod, policyGroupVersion string, gracePeriodSeconds *int64) (bool, error) {
deleteOptions := &metav1.DeleteOptions{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (pe *PodEvictor) evictPod(ctx context.Context, pod *v1.Pod, policyGroupVersion string, gracePeriodSeconds *int64) (bool, error) {
deleteOptions := &metav1.DeleteOptions{}
func (pe *PodEvictor) evictPod(ctx context.Context, pod *v1.Pod) (bool, error) {
deleteOptions := &metav1.DeleteOptions{
GracePeriodSeconds: pe.gracePeriodSeconds,
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Awkward..... The changes here were undone during rebase. thanks for this

@googs1025 googs1025 force-pushed the feature/grace_period_seconds branch 2 times, most recently from 65ac965 to b2a8f66 Compare December 23, 2024 01:59
@a7i
Copy link
Contributor

a7i commented Dec 23, 2024

/lgtm

/assign @ingvagabund

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 23, 2024
@googs1025 googs1025 force-pushed the feature/grace_period_seconds branch from b2a8f66 to 77b17fb Compare December 23, 2024 02:34
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 23, 2024
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from a7i. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@googs1025
Copy link
Member Author

/test pull-descheduler-test-e2e-k8s-master-1-31

@googs1025 googs1025 force-pushed the feature/grace_period_seconds branch from 77b17fb to 9485934 Compare December 23, 2024 04:49
@googs1025
Copy link
Member Author

/test pull-descheduler-test-e2e-k8s-master-1-30

@googs1025
Copy link
Member Author

/retest

ingvagabund
ingvagabund reviewed Jan 1, 2025
View reviewed changes
pkg/api/v1alpha2/types.go
// The value zero indicates delete immediately. If this value is nil, the default grace period for the
// specified type will be used.
// Defaults to a per object value if not specified. zero means delete immediately.
GracePeriodSeconds *int64
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing json:"gracePeriodSeconds,omitempty"

Copy link
Contributor

Choose a reason for hiding this comment

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

Also worth extending https://github.com/kubernetes-sigs/descheduler?tab=readme-ov-file#example-policy with e.g. gracePeriodSeconds: 60 alongside already existing pluginConfig arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a7i a7i a7i left review comments

@ingvagabund ingvagabund ingvagabund left review comments

@jklaw90 jklaw90 Awaiting requested review from jklaw90

@seanmalloy seanmalloy Awaiting requested review from seanmalloy

Assignees

@ingvagabund ingvagabund

@a7i a7i

Labels
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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feature: support setting gracePeriodSeconds in DeschedulerPolicy
4 participants
@googs1025 @k8s-ci-robot @ingvagabund @a7i