descheduler
descheduler copied to clipboard
kubernetes-sigs
add warning when enabling both dryRun and leaderElection
Signed-off-by: Furkan [email protected] Co-authored-by: Emin [email protected]
We've tried to install Descheduler using Helm chart and enabled both dryRun and leaderElection mode in values.yaml. And noticed leader election does not work due to dryRun activated. So thought a warning might be useful in this case.
Hi @Dentrax. 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.
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by:
To complete the pull request process, please assign seanmalloy after the PR has been reviewed.
You can assign the PR to them by writing /assign @seanmalloy in a comment when ready.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
is there a reason leader election can't be enabled with dryRun?
That was a suggestion from @ingvagabund. I couldn't remember why we did this, also couldn't reach review history since it's gone.
To do leader election, it would have to create a lease. Which I would expect in dry-run mode that no resources are affected (only queried).
To do leader election, it would have to create a lease. Which I would expect in dry-run mode that no resources are affected (only queried).
I'm not entirely sure about that, but without any use cases for testing leader election with dry run I'm fine leaving it for now.
However, the warnings should be more descriptive, and this should be documented in the readme under the leader election section (it doesn't look like we even have a section for dry run mode, which should also be added but in another PR). We might even just want to fail or at least be very explicit that leader election will not be enabled in the logs.
is there a reason leader election can't be enabled with dryRun?
In cases when I want to run multiple instances of the descheduler all with --dry-run=true. E.g. in simulation when I am testing multiple configurations in parallel.
is there a reason leader election can't be enabled with dryRun?
In cases when I want to run multiple instances of the descheduler all with --dry-run=true. E.g. in simulation when I am testing multiple configurations in parallel.
I meant is there a reason that we never allow leader election with dry run? That use case makes sense, but in that case you could just disable it yourself
I meant is there a reason that we never allow leader election with dry run? That use case makes sense, but in that case you could just disable it yourself
From the user perspective I do not take the leader election into account while running two or more deschedulers in the dry mode. I would be a bit surprised to learn I need to disable it. Dry mode means a cluster is not changed by a descheduler running in a dry mode. Including keeping the lease after the previous descheduler run untouched.
I think we still had the open question of why leader election is never allowed with dryRun, so rather than adding this warning maybe we just allow LE+DR if we don't have any reason not to
Maybe it would be better merge this to give some awareness to end users. This is how it works in current implementation. On the other side, "why leader election is never allowed with dryRun" is an open-question and important to answer. IMHO is to move this discussion to an issue; so that if we were going to change the behavior in some day, it could be easy to remove these checks and warnings.
@Dentrax good point, rather than sitting on this as an issue we can unblock, merge the warning, and create a follow up issue to discuss the bigger question. /approve
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: damemi
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [damemi]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
@Dentrax can you rebase to get the latest?
corev1informers and schedulingv1informers are no longer used.
@Dentrax good point, rather than sitting on this as an issue we can unblock, merge the warning, and create a follow up issue to discuss the bigger question. /approve
Thanks! 🙏 There is follow-up discussion: https://github.com/kubernetes-sigs/descheduler/issues/997