Sanitize taints before scheduling DSs on template node infos #5659
Conversation
k8s-ci-robot
added
the
kind/cleanup
|
@BigDarkClown: The label(s) In response to this:
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
added
cncf-cla: yes
| if err != nil { | ||
| return false, "", err | ||
| } | ||
| sanitizedNodeInfo, err := utils.SanitizeNodeInfo(nodeInfo, id, ignoredTaints, unwantedTaints) |
There was a problem hiding this comment.
This isn't really a part of your changes, but with the extra SanitizeNode call it's just more visible - why is the above logic different from the GetNodeInfoFromTemplate()? Doesn't it do the exact same thing just with different nodeInfo as a source?
I understand this may not be in the scope of this PR, so feel free not to do it, but maybe it would be worth a small refactor to unify those codepaths?
There was a problem hiding this comment.
That's a valid point, but this is not 100% true. We treat DS pods slightly differently depending on where we get template from. For existing nodes we take DS pods scheduled them and fill with missing DS pods if the option --force-ds is enabled. For instance template ones we always pack all the DS pods.
I think long term solution here is to set --force-ds to true always and unify both these logic pieces. But for now I'm not sure if it won't obfuscate the different underlying behaviour.
BigDarkClown
force-pushed
the
fix-pls
branch
3 times, most recently
from
5bce9f2 to
aac5184
Compare
|
/assign |
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
| MaxPodEvictionTime time.Duration | ||
| // IgnoredTaints is a list of taints to ignore when considering a node template for scheduling. | ||
| IgnoredTaints []string | ||
| // StatusTaints is a list of taints to remove when creating a node template for scheduling. |
There was a problem hiding this comment.
WDYT about: StatusTaints is a list of taints CA considers to reflect transient node status that should be removed when creating a node template for scheduling. Both IgnoredTaints and StatusTaints are expected to be temporary, the only difference is that IgnoredTaints are expected to appear during node startup.?
I'd like the comment to better explain the "why" we have this, rather than the "how" it is going to be used.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BigDarkClown, x13n The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
4 participants
What type of PR is this?
/kind bug
/kind cleanup
What this PR does / why we need it:
NodeInfoProvider returns node infos representing each node group for scale-up. If there is at least one node in the node group, then it is used as a template. If there are none, the node info is based on the node group instance template.
In both these cases we want to pack the node infos with all the expected DaemonSet pods. That way CA won't perform a scale-up which leaves some DSs pending. All DS pods should be added to the node info if it is generated from instance template or if it is generated from existing node and the
force-dsflag is enabled.The issue is that the node sanitisation, which includes cleaning unwanted taints, is performed before the DS are added. That means that it is possible to not include all of the DS pods in the node info if for example:
ignore taint.deletion taint.This change fixes it by performing additional sanitisation before trying to schedule DS pods. It also add additional
unwantedTaintsoption, which enables defining taints which are removed from templates but not treated as ignore taints.