Allow to prefix provisioningClassName to filter provisioning requests #7676
Conversation
k8s-ci-robot
added
kind/feature
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: macsko 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
the
size/L
macsko
force-pushed
the
allow_to_prefix_provisioning_class_name_to_filter_prs
branch
from
001b922 to
4fa718f
Compare
macsko
force-pushed
the
allow_to_prefix_provisioning_class_name_to_filter_prs
branch
from
4fa718f to
43513b7
Compare
k8s-ci-robot
added
the
lgtm
|
Running multiple CA instances takes more than adding a prefix for provisioning requests. For instance, regular pending pods will trigger scale up in every CA and different provisioning requests may trigger scaling in the same node group. Is this a part of some broader feature you're trying to build? |
If we want to have one CA that will do scale up, (basic) provreq processing etc. and second CA that does only check capacity provreq processing (with prefixed class name), then this should be enough to work correctly. Check capacity If the above assumptions are not enough, we could add a yet another flag to CA that will disable scale up and scale down activities leaving only provreq processing. |
It's already possible to 'shard' the cluster and run multiple CA instances by passing a different set of node group (or node group prefixes) to each of them. In practice this will work only if each workload's requirements fit only node groups from the same shard, otherwise multiple instances can trigger scale-up. For regular pods that fulfill these requirements, the only consequence of sharding would be spamming fake NotTriggerScaleUp events from instances that can't request scale-up. Not perfect, but possible to ignore in absence of a solution. For ProvisioningRequest, the instances that can't do anything for them won't just spam events though - they'll actually modify the ProvisioningRequest object, updating the condition. This actually needs to be fixed for any multi-CA setup to work. |
|
Sharding can work only if you can split workloads and node groups at the same time. However, is this this specific scenario intended to work only with check-capacity Provisioning Requests? That sounds safe, as there's no responsibility overlap once prefixes are configured correctly on each instance. |
macsko
force-pushed
the
allow_to_prefix_provisioning_class_name_to_filter_prs
branch
from
43513b7 to
ad2725f
Compare
k8s-ci-robot
removed
the
lgtm
|
New changes are detected. LGTM label has been removed. |
|
I updated the code to handle prefix only for check capacity ProvisioningRequests. |
macsko
force-pushed
the
allow_to_prefix_provisioning_class_name_to_filter_prs
branch
from
ad2725f to
fec8627
Compare
| Since Cluster Autoscaler version 1.33, it is possible to configure the autoscaler | ||
| to process only those check capacity ProvisioningRequests, that have a prefix matching the `--check-capacity-provisioning-class-prefix=<prefix>` flag. | ||
| This allows to run two Cluster Autoscalers in the cluster, but instance with the configured prefix | ||
| **should only** handle check capacity ProvisioningRequests and not overlap node groups with the main instance. |
There was a problem hiding this comment.
Also, it is still responsibility of the user to actually ensure the capacity checks done are not overlapping, correct? This should be documented as well. Otherwise one CA instance will say the capacity exists for PR1, the other will say the capacity exists for PR2, but overall there may be capacity for just one of them.
| if !strings.HasPrefix(provisioningClassName, checkCapacityProvisioningClassPrefix) { | ||
| return false | ||
| } | ||
| return SupportedProvisioningClasses[provisioningClassName[len(checkCapacityProvisioningClassPrefix):]] |
There was a problem hiding this comment.
Shouldn't this only allow for check-capacity?
| var SupportedProvisioningClasses = map[string]bool{ | ||
| v1.ProvisioningClassCheckCapacity: true, | ||
| v1.ProvisioningClassBestEffortAtomicScaleUp: true, | ||
| } | ||
|
|
||
| // SupportedProvisioningClass verifies if the provisioningClassName with the given checkCapacityProvisioningClassPrefix is supported. | ||
| func SupportedProvisioningClass(provisioningClassName string, checkCapacityProvisioningClassPrefix string) bool { |
There was a problem hiding this comment.
After thinking about it a bit, I'm not a fan of changing semantics of ProvisioningClass. Why can't sharding be done via dedicated field on ProvisioningRequest? Or at least, via a label or annotation?
This will blow up as soon as there are conflicts - and since the set of of supported provisioning classes is managed independently from the set of prefixes, it may lead to weird behavior.
There was a problem hiding this comment.
One reason of using prefixed is compatibility with older versions of CA. Adding the field will not prevent older versions from processing ProvisioningRequests. Using prefixes, they will filter them out as unrecognized class names.
But, what if we implement two approaches at once? The recommended path using a separate field, label or anything, and backwards compatible, but not recommended prefixed alternative. Prefix approach could be eventually removed, leaving only the field option.
There was a problem hiding this comment.
Ok. Temporary solutions tend to be quite permanent, so let's make sure this one isn't. I suggest adding warnings in the logs that this approach is not recommended and will be removed in CA 1.35 or something (but let's pick a specific minor). Does that make sense?
There was a problem hiding this comment.
Yea, I think that's a good idea and v1.35 seems okay.
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds the ability to set a provisioningClassName prefix, and then the CA will only process provisioning requests that have a matching prefix. This can allow to simply run multiple CA instances and route specific provisioning requests to them, while being backward compatible.
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: