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

Node label/taint/annotation placing logic needs to be improved #740

Open
himanshu-kun opened this issue Aug 9, 2022 · 4 comments
Open
Labels
area/performance Performance (across all domains, such as control plane, networking, storage, etc.) related kind/discussion Discussion (enaging others in deciding about multiple options) kind/enhancement Enhancement, improvement, extension lifecycle/stale Nobody worked on this for 6 months (will further age) needs/planning Needs (more) planning with other MCM maintainers priority/3 Priority (lower number equals higher priority)

Comments

@himanshu-kun
Copy link
Contributor

himanshu-kun commented Aug 9, 2022

How to categorize this issue?

/area performance
/kind discussion
/kind enhancement
/priority 3

What would you like to be added:
MCM needs to place node annotations,labels,taints(ALT) configured more efficiently. Refer Canary issue # 2918

Why is this needed:
There could be reasons for delay in applying ALT to nodes by MCM. Some of the reasons could be:

  1. API server throttling
  2. MCM crashes and recovers and in between nodes were marked as Ready allowing pods to be scheduled onto them.
    This impacts correct scheduling of workloads onto nodes controlled either via node labels or taints.
@himanshu-kun himanshu-kun added the kind/enhancement Enhancement, improvement, extension label Aug 9, 2022
@gardener-robot gardener-robot added area/performance Performance (across all domains, such as control plane, networking, storage, etc.) related kind/discussion Discussion (enaging others in deciding about multiple options) priority/3 Priority (lower number equals higher priority) labels Aug 9, 2022
@himanshu-kun
Copy link
Contributor Author

himanshu-kun commented Aug 9, 2022

CCM initialization follows a mechanism where kubelet places the following taints

"taints": [
                    {
                        "effect": "NoSchedule",
                        "key": "node.cloudprovider.kubernetes.io/uninitialized",
                        "value": "true"
                    }

on the node , and then CCM places the annotations and then removes this taint.

We could follow a similar approach for MCM
Taints to be placed on node, could be configured on the kubelet via --register-with-taints flag.

Upside of approach

  • more deterministic
  • quicker to implement

Downside of approach

  • delay in node in becoming completely ready with all ALT applied even for nodes which do not have any taints or node labels which influence scheduling of workloads onto node(s).

@himanshu-kun
Copy link
Contributor Author

himanshu-kun commented Aug 9, 2022

The second approach is using mutating webhook to apply ALT

Upside of approach

  • deterministic
  • less delay as during admission itself , the node will get the ALT

Downside of approach

  • mutating webhook acts only on CRUD requests , so only at that time ALT update would happen and not in between, while currently it happens on machine reconciliation which gets triggered on machine object update on ALT update.
    • one solution could be to mutate node objects yaml on machine object CRUD request related to ALT update, but I don't think this is allowed.
  • will take more time to implement

@himanshu-kun
Copy link
Contributor Author

cc @unmarshall @vlerenc

@gardener-robot gardener-robot added the lifecycle/stale Nobody worked on this for 6 months (will further age) label Feb 5, 2023
@himanshu-kun
Copy link
Contributor Author

Post grooming discussion

We currently can't distinguish b/w important and un-important labels/annotations. Important labels/annotations are those which are required to be placed on the node immediately as they control scheduling of pods, while un-important are lets say for classification. So the taint logic for MCM to ensure all ALT are placed before pods are scheduled can't be used, as we can have problems if MCM is unavailable to remove the taint.

We identified another flaw in code which leads to late addition of ALT, and solving that would considerably improve the speed of ALT application. Once we swap these two calls it should be sufficient.

Regarding taints , all taints are important, so the plan should be:

  • pass the taint via kubelet --taint flag, for the taints we know during node creation
  • any change in the required taint will still be synced by MCM

But we need to read the documentation thoroughly before we implement the taint part.

@himanshu-kun himanshu-kun added priority/2 Priority (lower number equals higher priority) needs/planning Needs (more) planning with other MCM maintainers and removed lifecycle/stale Nobody worked on this for 6 months (will further age) priority/3 Priority (lower number equals higher priority) labels Feb 23, 2023
@gardener-robot gardener-robot added priority/3 Priority (lower number equals higher priority) and removed priority/2 Priority (lower number equals higher priority) labels Feb 24, 2023
@gardener-robot gardener-robot added the lifecycle/stale Nobody worked on this for 6 months (will further age) label Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/performance Performance (across all domains, such as control plane, networking, storage, etc.) related kind/discussion Discussion (enaging others in deciding about multiple options) kind/enhancement Enhancement, improvement, extension lifecycle/stale Nobody worked on this for 6 months (will further age) needs/planning Needs (more) planning with other MCM maintainers priority/3 Priority (lower number equals higher priority)
Projects
None yet
Development

No branches or pull requests

2 participants