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: add IgnorePodsWithResourceClaims parameter for configuration #1603

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

feature: add IgnorePodsWithResourceClaims parameter for configuration #1603

wants to merge 1 commit into from
+143 −76

Conversation

googs1025
Copy link
Member

@googs1025 googs1025 commented Jan 7, 2025 •
edited
Loading

  • add IgnorePodsWithResourceClaims parameter for configuration
    now we are gradually supporting DRA features
    like IgnorePvcPods param, we sometime also don't want Pods with ResourceClaims to be rescheduled frequently.

@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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 7, 2025
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 7, 2025
@googs1025 googs1025 force-pushed the feature/add_dra_arg branch from 23b925f to 4c25931 Compare January 7, 2025 12:08
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2025
@googs1025 googs1025 force-pushed the feature/add_dra_arg branch 2 times, most recently from 041c1c1 to c919367 Compare January 7, 2025 13:33
@googs1025
Copy link
Member Author

/retest

@googs1025 googs1025 changed the title [wip]: feature: add IgnorePodsWithResourceClaims parameter for configuration feature: add IgnorePodsWithResourceClaims parameter for configuration Jan 7, 2025
@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 Jan 7, 2025
@a7i
Copy link
Contributor

a7i commented Jan 7, 2025

@googs1025 can you expand on your use-case a bit more?

Also, note that README and chart values.yaml will need to be updated

@googs1025
Copy link
Member Author

/retest

@googs1025 googs1025 force-pushed the feature/add_dra_arg branch from c919367 to 6c717c5 Compare January 8, 2025 03:57
@a7i
Copy link
Contributor

a7i commented Jan 8, 2025 

!!! 'gofmt -s' needs to be run on the following files: 
./pkg/framework/plugins/defaultevictor/defaultevictor_test.go
make: *** [Makefile:118: verify-gofmt] Error 1

@googs1025 googs1025 force-pushed the feature/add_dra_arg branch from 6c717c5 to 6a8b875 Compare January 8, 2025 05:32
@a7i
Copy link
Contributor

a7i commented Jan 8, 2025

/retest

@a7i
Copy link
Contributor

a7i commented Jan 9, 2025

/retest-required

1 similar comment
@a7i
Copy link
Contributor

a7i commented Jan 9, 2025

/retest-required

@googs1025 googs1025 force-pushed the feature/add_dra_arg branch 2 times, most recently from a8b429a to 41ba74d Compare January 11, 2025 11:34
@googs1025
Copy link
Member Author

googs1025 commented Jan 11, 2025 •
edited
Loading

@googs1025 can you expand on your use-case a bit more?

Also, note that README and chart values.yaml will need to be updated

Yes, of course. We now have the DRA feature in kubernetes. We are also testing it in our internal environment. We hope that pods with resourceclaims will not be evicted frequently (because of the device). Therefore, we hope to have an ignorePodsWithResourceClaims param like the ignorePvcPods param to selectively ignore such pods.

@a7i
Copy link
Contributor

a7i commented Jan 13, 2025

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2025
a7i
a7i reviewed Jan 15, 2025
View reviewed changes
pkg/apis/componentconfig/v1alpha1/types.go Outdated
@@ -57,6 +57,9 @@ type DeschedulerConfiguration struct {
// IgnorePVCPods sets whether PVC pods should be allowed to be evicted
IgnorePVCPods bool `json:"ignorePvcPods,omitempty"`

// IgnorePodsWithResourceClaims sets whether pods using resource claims should be allowed to be evicted
IgnorePodsWithResourceClaims bool `json:"ignorePodsWithResourceClaims"`
Copy link
Contributor

Choose a reason for hiding this comment

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

other fields use omitempty is that needed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

whoops... I miss that.
fixed

@googs1025 googs1025 force-pushed the feature/add_dra_arg branch from 41ba74d to af85aea Compare January 16, 2025 07:07
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 16, 2025
@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 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

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 Awaiting requested review from ingvagabund

Assignees

@a7i a7i

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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.
3 participants
@googs1025 @a7i @k8s-ci-robot