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.
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
Introduce AEP with Provisioning Request CRD #5848
Introduce AEP with Provisioning Request CRD #5848
Changes from all commits
09fb959
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would the cluster-autoscaler implementation ignore things that didn't use the kubernetes.io names?
what would a user do if they wanted atomicScaleUp behavior AND some cloud-provider specific provisioner?
if we name this field provisioningClass, and have a ProvisioningClass API type in the future, I'd expect the value of this field to be the name of the API object, but making it domain-qualified (e.g.
kubernetes.io/atomicScaleUp
) is an invalid API object name (can't contain a slash). How will the ProvisioningClass instances be matched to the values specified here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In such a scenario, should we drop the '/' and use a '-' or '.' instead?
will the following be ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need validation to make sure this is a qualified name (
example.com/...
, etc) and has a max length, etcThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the format, so the '/' is not needed.
As for the max length I think we should mimic the validation of the name.
Is there an example which does mimics the name validation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API documentation should explicitly state that only unprivileged parameters (as defined by the ProvisioningClass provider) are allowed here, how parameters specified here interact with the same parameters specified in a ProvisioningClass object (which take precedence), and what happens if params unrecognized by the ProvisioningClass provider are specified.
This design should also describe how a ProvisioningClass provider would split parameters into privileged and unprivileged sets and document their use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to not include the details of how the privileged and unprivileged parameters work in this design as the ProvisioningClass is not strictly proposed here.
We discuss it and make the API ready for it but those details can be covered in a dedicated AEP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should there be validation on the allowed keys (qualified names, length), allowed values (length, at least), and max number of items
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about requiring the qualified names, but the length of the names, entries and number of entries seem reasonable.
Although I am not sure how to limit the length of the keys and values in kubebuilder.
Is there a macro that does that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the rules for scale down when nodes are provisioned but pods didn't start (or didn't start yet)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if this was somehow configurable in the longer run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current proposal would be to use the same logic for scale-down so it would be configurable via
--scale-down-unneeded-time
flag.For now there is no way for CA to know which VMs come from the atomic scale-up so we cannot differ.