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.

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

Use TSCs in-place of Pod affinity/anti-affinity & always enforce them even if custom placement is specified #2720

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

Conversation

malayparida2000
Copy link
Contributor

Currently TSCs are enforced only if there is no other placement spec defined. Without the TSCs, the pods might not be distributed evenly. So we should always enforce the TSCs even if some custom placement is specified.

This also refactors the code to use TSCs in-place of Pod affinity/anti-affinity for ceph daemons like mgr, mon, mds, rgw, nfs.

Copy link
Contributor

openshift-ci bot commented Jul 30, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: malayparida2000
Once this PR has been reviewed and has the lgtm label, please assign nbalacha 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

Copy link
Contributor

@travisn travisn left a comment

Choose a reason for hiding this comment

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

This also refactors the code to use TSCs in-place of Pod affinity/anti-affinity for ceph daemons like mgr, mon, mds, rgw, nfs.

While I would expect the TSCs to work in place of the pod anti-affinity, it also feels like it has a bit of risk. Could you confirm some manual testing with this before merge? And we will want to point out to QE to watch for placement issues in case of regression.

Alternatively, it seems lower risk to always append our pod anti-affinity to the user's placement, instead of replacing it with a TSC.

controllers/defaults/placements.go Outdated Show resolved Hide resolved
controllers/defaults/placements.go Show resolved Hide resolved
@malayparida2000
Copy link
Contributor Author

This also refactors the code to use TSCs in-place of Pod affinity/anti-affinity for ceph daemons like mgr, mon, mds, rgw, nfs.

While I would expect the TSCs to work in place of the pod anti-affinity, it also feels like it has a bit of risk. Could you confirm some manual testing with this before merge? And we will want to point out to QE to watch for placement issues in case of regression.

Alternatively, it seems lower risk to always append our pod anti-affinity to the user's placement, instead of replacing it with a TSC.

Considering the high risk nature of this I am thinking of not taking this to 4.17 but keeping it in 4.18 only but nothing concrete yet. I think TSCs satisfy our requirement of even distribution perfectly & they are a cleaner approach then pod anti-affinity. So weighing on using TSCs always.

@malayparida2000
Copy link
Contributor Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 31, 2024
@malayparida2000 malayparida2000 deleted the all_tsc branch August 20, 2024 18:15
@malayparida2000 malayparida2000 restored the all_tsc branch August 20, 2024 18:15
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2024
Currently TSCs are enforced only if there is no other placement spec
defined. Without the TSCs, the pods might not be distributed evenly.
So we should always enforce the TSCs even if some custom placement is
specified.

This also refactors the code to use TSCs in-place of Pod
affinity/anti-affinity for ceph daemons like mgr, mon, mds, rgw, nfs.

Signed-off-by: Malay Kumar Parida <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants