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

topologySpreadConstraints: handle nodeTaintsPolicy and nodeAffinityPolicy constraints #1208

Closed
wants to merge 6 commits into from
Closed

topologySpreadConstraints: handle nodeTaintsPolicy and nodeAffinityPolicy constraints #1208

wants to merge 6 commits into from

Conversation

nitindagar0
Copy link
Contributor

@nitindagar0 nitindagar0 commented Jul 27, 2023 •
edited
Loading

In this PR Incorporated Marc's Changes for node-affinity as well as fixed failing test cases. Below is more information on existing PR

#1205

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 27, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 27, 2023
@k8s-ci-robot
Copy link
Contributor

Welcome @nitindagar0!

It looks like this is your first PR to kubernetes-sigs/descheduler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/descheduler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 27, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @nitindagar0. 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 Jul 27, 2023
@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 assign a7i for approval. 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

@k8s-ci-robot k8s-ci-robot requested review from a7i and damemi July 27, 2023 09:12
@nitindagar0 nitindagar0 changed the title Add handling for node eligibility Add handling for node eligibility with updated test cases Jul 27, 2023
@nitindagar0 nitindagar0 changed the title Add handling for node eligibility with updated test cases Add handling for node eligibility with updated test case Jul 27, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 27, 2023
@nitindagar0 nitindagar0 changed the title Add handling for node eligibility with updated test case Add handling for node eligibility with fixed test case Jul 27, 2023
@nitindagar0 nitindagar0 changed the title Add handling for node eligibility with fixed test case Add handling for node eligibility with test cases fixed Jul 27, 2023
@nitindagar0
Copy link
Contributor Author

Hi @a7i & @damemi, Please review this PR. The test cases have been fixed which were failing in existing PR #1205.

@nitindagar0
Copy link
Contributor Author

Also we are not able to access/fork this repo using our microsoft account and Hence we are using our personal account. Please allow our "nitindagar_microsoft" account to get the access.

@damemi
Copy link
Contributor

damemi commented Jul 31, 2023

@nitindagar0 thanks, should we close #1205 if this replaces it? Also please see the comment from that PR about if the new plans for NodeFit spec will fit with this: #1205 (comment)

I would also still like to see some test cases that clearly demonstrate the use case for this and shows that the new feature addresses that use case.

Regarding not being able to fork the repo, this is a public repo that anyone should be able to access. We don't grant individual accounts access and can't do that anyway as the repo's access is controlled by kubernetes-sig org policies. I tried to see your nitindagar_microsoft account and it looks like I need Microsoft SSO, so maybe you have some github policies from there preventing that account from forking publicly.

@damemi
Copy link
Contributor

damemi commented Jul 31, 2023

/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 Jul 31, 2023
@nitindagar0
Copy link
Contributor Author

/retest

@nitindagar0
Copy link
Contributor Author

nitindagar0 commented Aug 1, 2023 •
edited
Loading

@damemi we have gone through NodeFit spec this looks like a good approach for finding out eligble nodes but right now we wanted to quickly fix the issue and once nodefit spec PR is merged we will plan to use that.

for the other PR, Lets wait for Marc to close that as he is on leave and will be back in couple of days.

we are working on test cases and will add some more tests for this new behaviour.

@nitindagar0
Copy link
Contributor Author

@damemi , I have added few more test cases to test the new behaviour. Please review

a7i
a7i reviewed Aug 5, 2023
View reviewed changes
Copy link
Contributor

@a7i a7i left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@MarcPow and @nitindagar0 Thank you both for your contributions here. I've added a few questions and some nits to be addressed. Look forward to your feedback.

pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go Outdated Show resolved Hide resolved
pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go Outdated Show resolved Hide resolved
pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go Show resolved Hide resolved
pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/topologyspreadconstraint.go Show resolved Hide resolved
@a7i
Copy link
Contributor

a7i commented Aug 5, 2023

/retest-required

@a7i
Copy link
Contributor

a7i commented Aug 5, 2023

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 5, 2023
@a7i
Copy link
Contributor

a7i commented Aug 5, 2023

/retitle topologySpreadConstraints: handle nodeTaintsPolicy and nodeAffinityPolicy constraints

@k8s-ci-robot k8s-ci-robot changed the title Add handling for node eligibility with test cases fixed topologySpreadConstraints: handle nodeTaintsPolicy and nodeAffinityPolicy constraints Aug 5, 2023
@a7i
Copy link
Contributor

a7i commented Aug 5, 2023

/cc @knelasevero @ingvagabund

@a7i a7i requested a review from JaneLiuL August 5, 2023 02:16
@a7i
Copy link
Contributor

a7i commented Aug 7, 2023

@MarcPow @nitindagar0 Would you please add an e2e test for this here? Let me know if you run into any issues with that

@MarcPow MarcPow mentioned this pull request Aug 7, 2023
Closed
@nitindagar0
Copy link
Contributor Author

@MarcPow @nitindagar0 Would you please add an e2e test for this here? Let me know if you run into any issues with that

Wanted to understand more, What exact scenarios we wanted to add in e2e test cases, we already added scenario specific to nodeaffinity and node selector in tsc_test file

@a7i
Copy link
Contributor

a7i commented Aug 11, 2023

@MarcPow @nitindagar0 Would you please add an e2e test for this here? Let me know if you run into any issues with that

Wanted to understand more, What exact scenarios we wanted to add in e2e test cases, we already added scenario specific to nodeaffinity and node selector in tsc_test file

@nitindagar0 end-to-end tests run against a local Kubernetes cluster and test against real Kubernetes resources. It has several benefits such as:

  • ensure that scheduling is fine given input constraints, and descheduler evicts accordingly
  • parses policy/profile to ensure that things are validated end-to-end

If you can squash your commits, I can help with adding an extra commit with a simple end-to-end test.

@nitindagar0
squashed changes for test case addition
2b9f6d3 
corrected function name

refactored duplicate TopoContraint check logic

Added more test cases for testing node eligibility scenario

Added 5 test cases for testing scenarios related to node eligibility
@nitindagar0
Copy link
Contributor Author

@a7i I was able to squash last four commits only and pushed the changes

@nitindagar0
Copy link
Contributor Author

Hi @a7i, any update for E2E test case, We squashed the commits last week as you suggested

@a7i
Copy link
Contributor

a7i commented Aug 16, 2023

hey @nitindagar0 I do not have permission to push. Can you enable maintainers to edit?

@a7i a7i mentioned this pull request Aug 16, 2023
Merged
@nitindagar0 nitindagar0 requested a review from a7i August 17, 2023 07:31
@nitindagar0
Copy link
Contributor Author

nitindagar0 commented Aug 17, 2023 •
edited
Loading

@a7i Thankyou for adding e2e test case in the PR. Also I have checked "allow maintainers to edit" on this PR. I have also added you as collaborator in the forked repo "deschedulerUpdate"

@nitindagar0
Copy link
Contributor Author

Since you have created a separate PR. Should this PR be closed ?

@a7i
Copy link
Contributor

a7i commented Aug 20, 2023

You can either cherry-pick my commit or we can close this PR

@nitindagar0
Copy link
Contributor Author

Closing this PR as required changes are available in following PR
1218

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

@damemi damemi Awaiting requested review from damemi

@ingvagabund ingvagabund Awaiting requested review from ingvagabund

@knelasevero knelasevero Awaiting requested review from knelasevero

@JaneLiuL JaneLiuL Awaiting requested review from JaneLiuL

@a7i a7i Awaiting requested review from a7i

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nitindagar0 @k8s-ci-robot @damemi @a7i @MarcPow