Delete PDBs for hub/proxy deployments that doesn't support HA #1937
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some of the motivations for this choice is captured below: 1. Most other Helm chart defaults to having PDBs disabled by default making our chart's behaviour unexpected. 2. The idea of using a PDB is tightly coupled with applications that support high availability, in other words, to run in parallell to another. But, the hub/proxy/autohttps pod doesn't support it. As an indication of this, see the k8s docs on "configure-pdb" which has a "Before you begin" section that sais "You are the owner of an application runniong on a Kubernetes cluster that requires high availability. ref: https://kubernetes.io/docs/tasks/run-application/configure-pdb/ 3. When evictions take place, such as when a node is drained, the PDBs will be respected, and since we have `minAvailable: 1` and cannot create another one, a manual pod deletion is required. It is my perception that the act of draining a node should come with intent and consideration typically. If it comes as part of an automated k8s upgrade of nodes, it should at least be scheduled at a acceptable time window etc. So, it wouldn't be helpful to add the need to run `kubect delete pod` alongside this. 4. If the wish is to avoid making the cluster autoscaler relocate a pod part of a deployment, then it is more direct and sensible to use the `"cluster-autoscaler.kubernetes.io/safe-to-evict": "false"` annotation. ref: https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/FAQ.md#what-types-of-pods-can-prevent-ca-from-removing-a-node
consideRatio
force-pushed
the
pr/pdb-defaults
branch
from
8a93573
to
0581cdb
Compare
consideRatio
changed the title
consideRatio
changed the title
consideRatio
force-pushed
the
pr/pdb-defaults
branch
from
0581cdb
to
7ccfd50
Compare
3 tasks
|
Closed in favor of #1938 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For a discussion if this PR is the right way to go, let's continue the discussion in #1934.
Theory
The idea of using a PDB is tightly coupled with applications that support high availability (HA), in other words, to run in parallel to another. But, the hub/proxy/autohttps pod doesn't support it. As an indication of this, see the k8s docs on "configure-pdb" which has a "Before you begin" section that sais "You are the owner of an application runniong on a Kubernetes cluster that requires high availability.
ref: https://kubernetes.io/docs/tasks/run-application/configure-pdb/
PR proposal and outcome
The proposal is to remove the PDB resources for our deployment resources that doesn't support HA. it would close #1934.
I perceive the outcome to be that we...
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false") that influence the Cluster Autoscaler specifically, but wouldn't influence for example an scheduled and automated k8s node upgrade or manualkubectl draincommand. For more information see this documentation.scheduling.corePods.nodeAffinity.matchNodePurposeconfiguration.Why the extra commits?
I was on track to disable these resources by default initially instead of deleting them, but I then concluded it was even better to remove them after having written the commit message of 7ccfd50. By removing them, intent communication is clear and maintenance of something not part of the chart intent is removed.