KEP-4212: Declarative Node Maintenance #4213
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: atiratree 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 |
k8s-ci-robot
added
kind/kep
k8s-ci-robot
added
the
size/XXL
k8s-ci-robot
added
the
do-not-merge/work-in-progress
keps/sig-apps/4212-improve-node-maintenance/node-maintenance.svg
Outdated
Show resolved
Hide resolved
atiratree
force-pushed
the
improve-node-maintenance
branch
7 times, most recently
from
0ed835a
to
81b4589
Compare
atiratree
force-pushed
the
improve-node-maintenance
branch
2 times, most recently
from
6194058
to
00dc7c5
Compare
atiratree
changed the title
atiratree
force-pushed
the
improve-node-maintenance
branch
from
00dc7c5
to
686227c
Compare
Not sure yet, the API itself is SIG Node I guess, but it has a lot of implications for SIG- Apps. Let's delay this decision after I redo the KEP and we have additional rounds of discussions. |
| `nodemaintenance` admission plugin will be introduced. | ||
|
|
||
| It will validate all incoming requests for CREATE, UPDATE, and DELETE operations. All nodes | ||
| matching the `.spec.nodeSelector` must pass an authorization check for the DELETE operation. |
There was a problem hiding this comment.
are the nodes selected able to delete the NodeMaintainance object?
There was a problem hiding this comment.
For alpha, I wouldn't have the kubelet update .status of NodeMaintenance only; we can add fields into Node status if we want the kubelet to report something per-node that clients need to see.
After alpha we could revisit to decide what write to NodeMaintenance spec should be done by in-tree kubelet code.
There was a problem hiding this comment.
still don't clear what that means this authorization check, is kind of a nodeauthorizer check?
After alpha we could revisit to decide what write to NodeMaintenance spec should be done by in-tree kubelet code.
what is the rationale of this decision? seems an important thing to decide, at least and unresolved block of test should be explaining this and why is unresolved
There was a problem hiding this comment.
For now we expect the kubelet to only create the NodeMaintenance. This can be re-evaluated in the followup kubelet KEPs.
There was a problem hiding this comment.
still don't clear what that means this authorization check, is kind of a nodeauthorizer check?
@aojea This is for security. So it is not enough for a person to have permissions for NodeMaintenances to drain nodes and disrupt workloads. We were asked by sig-node to include this in the KEP.
After alpha we could revisit to decide what write to NodeMaintenance spec should be done by in-tree kubelet code.
I do not see this as a needed feature at this time. The only field that is reasonable to mutate is stage or maybe a reason. IMO it would be better to explore this in the kubelet KEPs.
| StageStatuses []StageStatus | ||
| // List of a maintenance statuses for all nodes targeted by this maintenance. | ||
| NodeStatuses []NodeMaintenanceNodeStatus |
There was a problem hiding this comment.
@thockin I do not like the pattern of using status as a database
how do we offer consistency , is there only one actor managing this?
If the status is wiped out by mistake are we able to rebuild the current state?
There was a problem hiding this comment.
is this going to support 5000 nodes per example?
There was a problem hiding this comment.
If the status is wiped out by mistake are we able to rebuild the current state?
- Kubernetes no longer expects
.statusto be rebuildable - I think it is reconstructable anyway; start from scratch (as if the kubelet had not previously been running)
There was a problem hiding this comment.
is this going to support 5000 nodes per example?
Good question.
We could add fields into Node status if we want the kubelet to report something per-node (and just report it, not using .status to record cluster administrator intent.
We could also add a SomethingSlice API like we did with Endpoints, but that does not feel like a good fit.
There was a problem hiding this comment.
+1, this is problematic:
I am trying to bend the status somehow, but it is not feasible.
We cannot even limit how many nodes the selectors should select on admission, since new nodes can appear during the NodeMaintenance lifetime (especially Idle).
We could introduce a MultiNodeMaintenance object, whose only point would be to create x number of NodeMaintenance objects, one for each node. And it would synchronize the current stage between itself and the children.
But yeah, storing this in the Node status probably sounds like the best solution.
There was a problem hiding this comment.
This should be solved in the current API design.
| When a `Cordon` or `Drain` stage is detected on the NodeMaintenance object, the controller | ||
| will set (and reconcile) `.spec.Unschedulable` to `true` on all nodes that satisfy | ||
| `.spec.nodeSelector`. It should alert via events if too many occur appear and a race to change | ||
| this field is detected. |
There was a problem hiding this comment.
what if there are other NodeMaintainance objects in other stage matching the same Pods and Nodes?
There was a problem hiding this comment.
Not sure about this but one way to handle cordon / uncordon is to label the NodeMaintenances that result from a cordon. kubectl uncordon could then remove any NodeMaintenance labelled as a cordon and matching the one specific node.
We either reject or warn on setting that label on a NodeMaintenance that selects or could select multiple nodes.
There was a problem hiding this comment.
these dependencies and combinations need to be clear or we can provide a big footgun to users
There was a problem hiding this comment.
Drain of a single node by multiple maintenances is supported, please see https://github.com/atiratree/kube-enhancements/blob/improve-node-maintenance/keps/sig-apps/4212-declarative-node-maintenance/README.md#pod-selection-and-draintargets-example
There was a problem hiding this comment.
If there is at least one maintenance with Cordon or Drain stage, the node remains unschedulable.
atiratree
force-pushed
the
improve-node-maintenance
branch
from
55b2526
to
d537832
Compare
|
As it stands, I support https://github.com/kubernetes/enhancements/blob/d537832d421a1ac9bee27a4a086fe6e49e82bb18/keps/sig-apps/4212-declarative-node-maintenance/README.md#out-of-tree-implementation over the proposal made in this KEP. However, we would / might need to make kubectl aware of the custom API, and so would still need a KEP. I can open a PR with that alternative selected if folks are interested. |
|
@atiratree |
|
@tuibeovince it depends on how your control plane is deployed. Ideally it should have the highest priority and pod type. So that all your workloads drain first and control plane last. You can also pull the plug earlier if you do not need the control plane to shut down gracefully. Do you have a special case in mind that you would like to see solved for a single node cluster? |
atiratree
force-pushed
the
improve-node-maintenance
branch
from
d537832
to
4db6894
Compare
|
@atiratree Thank you! and sorry if I might still be getting lost in the concepts (such as evacuation) being introduced within the scope of DNM and EvacuationAPI in general. In my understanding given a multiple worker node cluster, pretty much like drain, DNM will instigate an evacuation of pods in a node to be maintained, and have them evacuated to an available node. I guess for now what I wish to know about is the intended behavior of when the cluster is a single worker node and that node gets maintained. Like how are the pods handled before, during, and after the maintenance is completed in this case? Or are they simply just shutdown? are they gracefully killing the pods? and such Thank you in advance. |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
What's the statuys on this KEP? Is it coming back for 32? |
It is in scope definitions stage at this moment. There are many open questions that needs to be answered before moving forward on this. |
| // PodSelector selects pods according to their labels. | ||
| // This can help to select which pods of the same priority should be evacuated first. | ||
| // +optional | ||
| PodSelector *metav1.LabelSelector |
There was a problem hiding this comment.
Should it also be possible to select by namespace?
There was a problem hiding this comment.
IMO: not for alpha!
When you maintain a node, you expect to take all the Pods offline or put them at risk. Let's ship something useful and then see if we want to iterate.
There was a problem hiding this comment.
Begin this a global CRD, the lack of a namespace selector seems a relevant GAP for adoption
There was a problem hiding this comment.
What is the intended use case and benefit for the namespace selector?
The main differentiator should be the pod priority. I have added the label selector to mainly differentiate between different components running for example the system-cluster-critical or the system-node-critical priority. For example, if you are running some special etcd/apiserver configuration, you could control how the instances downscale during an upgrade. So, basically to have an additional ordering to just a priority. But in most cases the priority should be enough, and downscaling by namespaces could slow down the process considerably.
There was a problem hiding this comment.
To me it seems unrealistic that ordering for Pod drain will be configured by setting Pod priorities on all Pods in a cluster, especially if not a single team has control over an entire cluster and can set Pod priorities on all relevant Pods.
We had this case here in Cluster API where folks where using Portworx which uses an operator to manage Pods: kubernetes-sigs/cluster-api#11024 (not a DaemonSet)
I assume that if there is a use-case for a PodSelector the same probably also applies for a namespace selector (I think outside of very limited use cases it's not safe to assume Pod labels are unique across namespaces)
|
|
||
| When a `Cordon` or `Drain` stage is detected on the NodeMaintenance object, the controller | ||
| will set (and reconcile) `.spec.Unschedulable` to `true` on all nodes that satisfy | ||
| `.spec.nodeSelector`. It should alert via events if too many occur appear and a race to change |
There was a problem hiding this comment.
nit: Is there a word missing after many?
| #### Pod Selection | ||
|
|
||
| The pods for evacuation would first be selected by node (`.spec.nodeSelector`). NodeMaintenance | ||
| should eventually remove all the pods from each node. To do this in a graceful manner, the |
There was a problem hiding this comment.
eventually remove all the pods from each node
A few questions about this as this has a few implications
Compared to kubectl drain this proposal also proposes to terminate DaemonSet and static Pods.
I'm not entirely sure if the order is configurable (see questions above), but my current assumption is that we always drain Default, then DaemonSets then static Pods.
This means that e.g. monitoring agents (logging, metrics, ...) that are usually deployed via DaemonSets will be terminated before static pods. Which in turn means that we won't have visibility during the last stages of the drain. I think this would make it hard to detect / troubleshoot the last parts of the drain.
I'm also wondering in general if it would make sense to allow to skip drain for certain Pods:
- As already mentioned, it might be desirable to keep monitoring agents running until the Node shuts down entirely (this is possible today with
kubectl drainby deploying monitoring agents via DaemonSets) - What about Pods that are tolerating all taints. I think this is a valid use case as sometimes folks want to enforce that Pods are running on all Nodes independent of whatever taints might be on that Node (for vendors it's impossible to tell which custom taints folks might use).
- I think in that scenario a Node drain will be just stuck indefinitely. We are able to evict/evacuate these Pods, but the kube-scheduler will just reschedule them on the same Node.
- Because there can be different personas involved it's also not always possible for the cluster-admin to simply modify tolerations of "third-party" Pods
There was a problem hiding this comment.
+💯 to add support for rules allowing to intentionally skip drain for a set of Pods, it is a request that popped up several time in Cluster API, mostly due to folks deploying stuff tolerating all taints or just the ones applied by drain.
Notably, it is also worth to notice that users don't always have the chance to change Pods, e.g. because they are managed by some operator, because the app is installed by something that keep the spec in sync etc etc.
In those cases, skip drain could provide a nice exit path for operators trying to unblock node deletion.
There was a problem hiding this comment.
I'm not entirely sure if the order is configurable (see questions above),
it is not
This means that e.g. monitoring agents (logging, metrics, ...) that are usually deployed via DaemonSets will be terminated before static pods. Which in turn means that we won't have visibility during the last stages of the drain. I think this would make it hard to detect / troubleshoot the last parts of the drain.
This is similar to today: DaemonSets are also terminated before static pods.
I'm also wondering in general if it would make sense to allow to skip drain for certain Pods:
+💯 to add support for rules allowing to intentionally skip drain for a set of Pods, it is a request that popped up several time in Cluster API, mostly due to folks deploying stuff tolerating all taints or just the ones applied by drain.
I think the presumption is to better tune the types of workloads and their priorities. The goal of the NodeMaintenance is to remove all pods from a node and most likely to take the node offline.
Notably, it is also worth to notice that users don't always have the chance to change Pods, e.g. because they are managed by some operator, because the app is installed by something that keep the spec in sync etc etc.
In those cases, skip drain could provide a nice exit path for operators trying to unblock node deletion.
What about Pods that are tolerating all taints. I think this is a valid use case as sometimes folks want to enforce that Pods are running on all Nodes independent of whatever taints might be on that Node (for vendors it's impossible to tell which custom taints folks might use).
- I think in that scenario a Node drain will be just stuck indefinitely. We are able to evict/evacuate these Pods, but the kube-scheduler will just reschedule them on the same Node.
- Because there can be different personas involved it's also not always possible for the cluster-admin to simply modify tolerations of "third-party" Pods
These are the problems that the NodeMaintenance+Evacuation API tries to solve. Operators and any other actor should be aware of NodeMaintenance and Evacuation objects and they should offer the exit path automatically. And not block the drain and wait for manual intervention.
There was a problem hiding this comment.
This is similar to today: DaemonSets are also terminated before static pods.
Is this a part of the kubelet shutdown feature?
| A misconfigured .spec.nodeSelector could select all the nodes (or just all master nodes) in the | ||
| cluster. This can cause the cluster to get into a degraded and unrecoverable state. | ||
|
|
||
| An admission plugin ([NodeMaintenance Admission](#nodemaintenance-admission)) is introduced to |
There was a problem hiding this comment.
Static Pods don't stop during a node drain, right? So even if you do mark every node as under maintenance, you may still have a control plane. Perhaps, no working cluster network and no DNS, but you still have a control plane
kubectl drain today, yes. But you're proposing to also shutdown static Pods. I assume this includes API server, etcd, ... (at least for clusters setup via kubeadm).
I think because of the Kubernetes skew policy for kubelet <=> kube-apiserver, kubelets on control plane nodes have to use the local apiserver (more context: kubeadm: kubernetes/kubeadm#2271). Thus, as soon as we start terminating apiserver and etcd the kubelet won't be able to communicate with the apiserver anymore. Which basically means it's only partially functional at this point.
| command. However, we will deprecate it along with all the library functions. We can print a | ||
| deprecation warning when this command is used, and promote the NodeMaintenance. Additionally, pods | ||
| that support evacuation and have `evacuation.coordination.k8s.io/priority_${EVACUATOR_CLASS}` | ||
| annotations will block eviction requests. |
There was a problem hiding this comment.
Additionally, pods that support evacuation and have
evacuation.coordination.k8s.io/priority_${EVACUATOR_CLASS}annotations will block eviction requests.
Isn't this a breaking change for the eviction API?
I think all tools built on top of the eviction API won't be able to drain Nodes anymore as soon as a single Pod opts into evacuation.
There was a problem hiding this comment.
TBH, I think that this overlap between eviction and evacuation could be very confusing for users.
If someone defines a PDB for an application, IMO they are entitled to expect that the application is protected during drain.
There was a problem hiding this comment.
I think all tools built on top of the eviction API won't be able to drain Nodes anymore as soon as a single Pod opts into evacuation.
Not true, as the current design of the Evacuation API stands, the eviction API will still fully work as before and will be fully supported.
But you can opt into using NodeMaintenace+Evacuation API for more graceful pod terminations + node drain.
We could easily enable this for example in the cluster autoscaler to observe the benefit of this feature. And it would also use eviction for pods who do not have evacuators assigned.
There was a problem hiding this comment.
My understanding is that as soon as a Pod opts into evacuation, eviction won't work anymore for this Pod ("Additionally, pods
that support evacuation and have evacuation.coordination.k8s.io/priority_${EVACUATOR_CLASS}
annotations will block eviction requests").
This means as soon as a single Pod in a cluster opts into evacuation, all tools that have been build on top of eviction today (Cluster API, kubectl drain, many others) won't be able to drain the Node with this Pod anymore.
The consequence is that all of these tools will have to implement the evacuation API as the eviction API is not sufficient anymore to be able to drain Nodes. It's not in the hands of these tools if end users will opt into the Evacuation API when they eventually deploy workload on clusters managed by these tools.
| // DrainPlanEntry priority fields should be in ascending order for each podType. | ||
| // If the priority and podType are the same, concrete selectors are executed first. | ||
| // | ||
| // The following entries are injected into the drainPlan on the NodeMaintenance admission: |
There was a problem hiding this comment.
If a user sets drainPlan. How is the user value merged with this?
(related to the question right above, of course merging would be straightforward if we always enforce a certain order, i.e. Default -> DaemonSet -> Static, podPriority low -> high, and entires with poldSelector first)
There was a problem hiding this comment.
Yes, by podType, then by podPriority and then by podSelector. As also seen in the type documentation.
| // podType: "Static" | ||
| // - podPriority: 2000000000 // system-cluster-critical priority class | ||
| // podType: "Static" | ||
| // - podPriority: 2000001000 // system-node-critical priority class |
There was a problem hiding this comment.
If I'm interpreting this correctly, this is the stage where kube-apiserver and etcd will be terminated.
This means that at this point kubelets that use the local kube-apiserver (more context: kubeadm: kubernetes/kubeadm#2271) won't be able to communicate with the apiserver anymore. Are we still able to complete the NodeMaintenance if the communication between kubelet and kube-apiserver is broken at this stage?
There was a problem hiding this comment.
I would expect the apiserver to terminate last. Then the kubelet will lose the connection. Nevertheless this is a kubeadm issue, because kubelet is not connected to the load balancer. I do not see how the NodeMaintenace can help here?
There was a problem hiding this comment.
Nevertheless this is a kubeadm issue, because kubelet is not connected to the load balancer.
I think kubelet cannot comunicate via the loadbalancer with kube-apiservers without violating the Kubernetes skew policy (example: kubelet 1.31 is not allowed to communicate with 1.30 apiservers). The only option that kubeadm has to not violate the Kubernetes skew policy during upgrades is to communicate with the local apiserver (which has the same version as the kubelet).
| podType: Default | ||
| ... | ||
| --- | ||
| ``` |
There was a problem hiding this comment.
Reading through these examples I wonder how easy it will be for users to understand why Node maintenances / drains are not making progress.
I think a significant amount of complexity comes from the m:n mapping between NodeMaintenances and Nodes.
There was a problem hiding this comment.
+1
It could really help having an example that shows how a user can figure out why a node drain is blocked when multiple node maintenance apply.
(also considering all the moving parts 😅, or at least Evacuation, Eviction, DaemonSet controller, kubelet)
There was a problem hiding this comment.
Each Node will have its status and the NodeMaintenace should be the aggregate. Please take into account that these are not final designs and may change considerably from the final API. There is still no consensus if there should be m:n mapping, for example.
We can add the debugging parts once we move into the PRR phase.
| drainTargets: | ||
| - podPriority: 1000 | ||
| podType: Default | ||
| - podPriority: 1000 |
There was a problem hiding this comment.
This example is a bit confusing:
- Could it be that this does not match the types above (anymore)? (I couldn't find nodeStatuses)
- nit: this target doesn't exist in the drainPlan above
There was a problem hiding this comment.
We have moved nodeStatuses to each node #4213 (comment). These places require an update of that situation.
| ... | ||
| ``` | ||
|
|
||
| To fulfil the Evacuation API, the DaemonSet controller should register itself as a controller |
There was a problem hiding this comment.
| To fulfil the Evacuation API, the DaemonSet controller should register itself as a controller | |
| To fulfil the Evacuation API, the kubelet controller should register itself as a controller |
nit
| // The default value is Idle. | ||
| Stage NodeMaintenanceStage | ||
|
|
||
| // DrainPlan is executed from the first entry to the last entry during the Drain stage. |
There was a problem hiding this comment.
A few questions about the DrainPlan ordering and admission
executed from the first entry to the last entry during the Drain stage
This implies that the drainPlan is executed in order of the array
DrainPlanEntry podType fields should be in the following order: nil -> DaemonSet -> Static
This implies the DrainPlanEntires should be in this order. Is this actually a must?
(There are a few places further down in the doc that seem to implly that the order is always nil/Default -> DaemonSet -> Static)
If this is a must, what if there are dependencies between Pods that don't follow this order? (e.g. a static Pod depending on a DaemonSet, or a DaemonSet depending on a Default Pod)
DrainPlanEntry priority fields should be in ascending order for each podType.
Similar question. Is this just a should or will the ordering based on priority per podType be enforced?
If the priority and podType are the same, concrete selectors are executed first.
This implies that the stages are executed based on this order and not the order of the entries. (But this would make sense if the order of the entries are enforced to have this ordering)
There was a problem hiding this comment.
If this is a must, what if there are dependencies between Pods that don't follow this order? (e.g. a static Pod depending on a DaemonSet, or a DaemonSet depending on a Default Pod)
It is must. And I think basically all clusters follow this ordering today. Do you have an example where this is not the case?
Similar question. Is this just a should or will the ordering based on priority per podType be enforced?
enforced
This implies that the stages are executed based on this order and not the order of the entries. (But this would make sense if the order of the entries are enforced to have this ordering)
the order of the entries should conform to the type and priority ordering. A lower index entry with a podSelector and the same priority will be drained first.
There was a problem hiding this comment.
It is must. And I think basically all clusters follow this ordering today. Do you have an example where this is not the case?
Sorry no concrete example. I was only wondering if it's safe to assume that the order in which Pods should be drained can simply be determined by the way that they are deployed (Deployments/operator/... vs DaemonSet). The way that Pods are deployed and drain order seem orthogonal to me.
| For the above node maintenance, the controller should not react to Evacuations of DaemonSet pods | ||
| with a priority greater than 5000. This state should not normally occur, as Evacuation requests | ||
| should be coordinated with NodeMaintenance. If it does occur, we should not encourage this flow by |
There was a problem hiding this comment.
Just to confirm. Does this mean that for DaemonSet Pods the evacuation API cannot be used without also using the NodeMaintenance API? (Just wondering because I think it's different for the eviction API today)
There was a problem hiding this comment.
It can be used if you implement a custom Evacuator. I think it does not make sense for the DaemonSet controller to implement it separately without the NodeMaintenance ATM.
There was a problem hiding this comment.
It would be nice if we would be able to evict DaemonSet Pods independent of the NodeMaintenance API if the NodeMaintenance API cannot be used.
Example, I could imagine that NodeMaintenance API will not be compatible with what we want to be able to do in Cluster API, so it would have been just nice to use it without the NodeMaintenance API.
Just to clarify. We're aiming to provide folks the means to configure drain order in a very flexible way (and also a way to skip draining Pods). The problem that we would run into with the current state of the NodeMaintenance API is that it also takes care of draining Nodes, but in a way that is less flexibel then what we want to achieve.
If NodeMaintenance would be only about declaring the maintenance while still allowing other components to take over the drain / drain order / the decision which Pods to drain and which ones not to drain, we would be good :)
| Static PodType = "Static" | ||
| ) | ||
|
|
||
| type DrainPlanEntry struct { |
There was a problem hiding this comment.
I have question/doubt about this API.
- If I got it right, in order to define a rule that targets a specific set of Pods, it is required that those Pods have an identifying PodPriority. Is this correct?
If true, what if a user needs to define a drain rule for an application where they cannot define a pod priority? (the users not always are allowed to change pods in applications, e.g. if the Pods are managed by some operator)
Also, is it correct to introduce a dependency between PodPriority and drain? what if users wants to define a drain rule for an application, but they don't want that this application triggers preemption for other apps when scheduled? Are we mixing up two orthogonal problems?
- It seems also that each rule Must be specifically defined for a single PodType, Is this correct?
if true, I'm worried that we are surfacing in the API some internal detail of the of the system, the fact that different K8s components takes care of different type of pods.
As a user, I could find really useful to define a specific drain rule for my application, but when I try to image such rule I think at something like "I want to evict Portworx after evicting all the other pods (match labels app=portworx, drain priority=x)".
I would also expect it to be a responsibility of the system to figure out what kind of Pods my application has (in most cases a mix of Default pods and DaemonSets), and do the right thing.
But if instead we expect users to dig into applications, usually designed by other teams, might be not even in the same company, figure out what kind of pods the application has, and then map this to all the intricacies of Pod types and how K8s components handle node drain, this could really impact the usability of this feature.
Going one step further, ideally it would be really nice if drain rules are something that developers can craft and ship with the application (like CM, Deployments, PDB etc), instead of being something in the realm of the cluster operator.
There was a problem hiding this comment.
Good feedback. I'd be very happy to see an initial alpha API (either in-tree or a CRD) where we can practice triggering declarative draining, and encourage early adopters to give us their thoughts whilst the new API is still plastic (mouldable, fixable).
There was a problem hiding this comment.
To me, declarative node maintenance is / should be much more about:
- I intend to make disruptive changes to this node / this rack / everything with this storage atttached to it
(“disruptive change” can include taking it out of the datacenter and sending it for recycling) - here's when it will happen, and maybe here's a set of taints that are likely to get applied to the Node
- might be good to know in case your app intends to tolerate these
- node autoscalers can note this information without doing anything at Pod level
- controllers are welcome to handle this gracefully before k-c-m (I assume) starts telling the kubelet to delete Pods
and out of all of that, the most key detail to capture is that it is an intent to maintain, rather than a verb masquerading as a noun (in other words, we aren't making a NodeDrainRequest or a PodRemovalSchedule).
There was a problem hiding this comment.
Makes sense.
Might be the best way to bridge the two worlds is to think to a two step process/two CRDs:
- Allow application developers to declare Drain rules for their applications, like they already do for PDBs
- Allow cluster operators to declare node maintenance intent, which will then collect and use Drain rules if necessary
Also, FYI, in Cluster API we are thinking about something to make our machine drain process more flexible, and due to a few constraints, we need it in the short term; however, I can easily image how Cluster API users might benefit from something similar to what we are discussing in this thread (if and when it becomes available).
There was a problem hiding this comment.
I think the point of the DrainPlan is not to define rules on how to gracefully terminate an application. This is an admin level tool that is more coarse and basically just looks at different classes of applications (e.g. user vs system). And can differentiate between different control-plane components.
The user does not have to tinker with the priorities to achieve the desired result. They also have no guarantee that the NodeMaintenance will be the same in every cluster (e.g. all of the user app pods can fall into the same termination bucket).
To solve this, users can simply run all of their pods at e.g. priority 0 an utilize the Evacuation API logic. Users can add an evacuator to their app pods and downscale the app according to their needs. For example, the evacuator can keep pod that provides a service alive until a pod that depends on that service terminates.
NodeMaintenance and Evacuation are intended to be extensible and for users to be able to depend on the presence of these APIs in the cluster (similar to eviction + PDB). This is the main reason why it is hard to introduce this as a CRD.
There was a problem hiding this comment.
I agree declaring node maintenance is an admin operation.
But if node maintenance will only be about corse configuration (user vs system or deamon set vs static pods vs everything else), IMO it will fail to address the friction that exists between cluster admin and application teams when a node must be drained.
WRT to the evacuation API, my personal take is that this is more a solution for advanced use cases like operators. The majority of folks need some more generic, and DrainRules have the potential to fill this gap but in order to do so, they must be decoupled by NodeMaintenance.
There was a problem hiding this comment.
Yes there is a friction between the cluster admin and applicaiton teams. But it is quite a difficult task to offer a strictly declarative API to support all possible graceful draining scenarios. There could be a lot of dependencies and edge cases. I am not convinced we want to support that.
The Evacuation API allows you to do anything, but I agree that it is a quite advanced/handful for a normal user. That is why we will try to offer clever draining behaviors for some workloads (e.g. Deployment, HPA, configurable StatefulSet, etc.). And also, the Evacuation API can be used as a building block. So other components in the ecosystem can implement DrainRules and offer it as an Operator/CRD that normal users can use for a subset of applications.
There was a problem hiding this comment.
Thank you for the explanation, I appreciate your patience in following up to my questions.
If what we need to solve this problem for both cluster admin and application teams is to eventually build yet another layer on top of the evacuation API, frankly speaking IMO the complexity is just too much (it is hard to figure out how m:n rules works in this proposal, let's image another layer of rules on top).
If I combine this with proposed breaking changes like draining daemonst sets and static pods, and a few other thread of discussion, it is hard for me to figure out how would look like the final state with a cohesive solution that helps both cluster admin and application teams, and also when this will be available.
Considering this, I'm starting to lean towards the opinion already expressed by other contributors providing feedback on this proposal that NodeMaintenance in the current form should be incubated as an external CRD, and then move to k/k when it is mature. But this depends also by how other thread are addressed.
In the meantime, I kindly ask you to add the idea discussed in this thread as alternatives in the proposal:
- decoupling drain rules definition from the node maintenance initiation (the split is what enables multiple personas to play part in the solution of this problem)
- make drain rule more application friendly (also avoiding to surface K8s internals in the user facing API)
cc @vincepri @neolit123 @sbueringer @chrischdi given that this popped up in a few SIG Cluster lifecycle related discussions
There was a problem hiding this comment.
If what we need to solve this problem for both cluster admin and application teams is to eventually build yet another layer on top of the evacuation API, frankly speaking IMO the complexity is just too much (it is hard to figure out how m:n rules works in this proposal, let's image another layer of rules on top).
Another layer (whatever it is) above the Evacuation API is necessary as the Evacuation API servers as a base and implements graceful drain which is mostly not possible today.
If I combine this with proposed breaking changes like draining daemonst sets and static pods,
Can you please let me know which part do you consider a breaking change?
decoupling drain rules definition from the node maintenance initiation (the split is what enables multiple personas to play part in the solution of this problem)
I think it makes sense to decouple the rules. We probably still want to keep the admission logic. We could also introduce a Default DrainPlan that would be selected if none was supplied to the NodeMaintenance as I would expect it might be hard to select the correct one for some drainers.
make drain rule more application friendly (also avoiding to surface K8s internals in the user facing API)
Application user facing API could be a nice followup if the community finds it useful.
Agree, we need to build a stronger consensus first. |
|
Somewhat relevant: https://github.com/jukie/karpenter-deprovision-controller |
|
kured (https://kured.dev/ or https://github.com/kubereboot/kured) would also benefit from this, especially for the statefulset pdb case. Following this. If there is any way I can help, don't hesitate to ask, I just subscribed to be notified on this topic. |
|
@evrardjp would you be willing to prototype a schema (as in CustomResourceDefinition) for an out-of-tree prototype, of the basic alpha API? I think that would help, even with the KEP still in its early form. What you learn from starting that endeavor could give us good feedback on how to progress this KEP. |
| participating-sigs: | ||
| - sig-apps | ||
| - sig-autoscaling | ||
| - sig-cli |
There was a problem hiding this comment.
We should also add sig-cluster-lifecycle? Seems @fabriziopandini and @sbueringer have been collaborating a lot and providing feedback, and they should be added to the reviewer list a well
| - "@wangzhen127" | ||
| approvers: | ||
| - "@dchen1107" | ||
| - "@soltysh" |
There was a problem hiding this comment.
Please add @fabriziopandini as approver for SIG Cluster Lifecycle
TODO: