-
Notifications
You must be signed in to change notification settings - Fork 163
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
flex karpenter node-pool configurations #7617
base: dev
Are you sure you want to change the base?
Conversation
bd77149
to
f723db0
Compare
f723db0
to
09c7c04
Compare
Signed-off-by: Mahmoud Gaballah <[email protected]>
09c7c04
to
05e35f4
Compare
@@ -112,7 +112,23 @@ spec: | |||
# Operators { In, NotIn, Exists, DoesNotExist, Gt, and Lt } are supported. | |||
# https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/#operators | |||
requirements: | |||
#{{ if and (eq (len .NodePool.InstanceTypes) 1) (eq (index .NodePool.InstanceTypes 0) "default-for-karpenter") }} | |||
#{{ if eq .NodePool.KarpenterInstanceTypeStrategy "default-for-karpenter" "not-specified" }} |
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.
Do we want two values here? I mean, I see default-for-karpenter
is being handled below anyway, maybe this should be not-specified
only?
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.
no, it is meant for both cases
in this block we exclude certain sizes and instance types when the configurations are very broad
this happen in 2 cases, when we use the default set of instance-families (default for karpenter) , and when we do not restrict instance types at all (not specified)
then we exclude certain sizes
when the user provides their own set of instance-types. we use them as is with out further limitation
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 see, makes sense.
So are these conditions OR
ed together? Or AND
ed? I don't see an operator. Because it will only work like you suggest in case of an AND
.
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 trying to do "if KarpenterInstanceTypeStrategy in {"default-for-karpenter" "not-specified"}
but maybe I am doing it wrong. will check again
# user's custom configurations | ||
#{{ if (index .NodePool.ConfigItems "requirements") }} | ||
# {{ range $requirement := .NodePool.KarpenterRequirements }} | ||
- key: "{{ $requirement.Key }}" | ||
operator: "{{ $requirement.Operator }}" | ||
values: | ||
# {{ range $value := $requirement.Values }} | ||
- "{{ $value}}" | ||
# {{ end }} | ||
# {{ end }} | ||
#{{ end }} | ||
# other configuration |
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.
Are these commented lines here for documentation purpose? It looks a little confusing, maybe we can just put an example configuration in a large comment block above this? just to seperate out the documenting comments and the actual configuration making it easier to read?
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.
things like
# other configuration
# user's custom configurations
are definately comments for cotext
however, # {{ range $requirement := .NodePool.KarpenterRequirements }}
and so on, are no comments. they are functional go templates.
they are comments in the yaml, but go templates can render them without a problem. this way I can get the IDE to parse the yaml file correctly
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.
Just looks a bit confusing, maybe we can remove them later when we merge, so it doesn't look confusing like this?
…ol-configs Signed-off-by: Mahmoud Gaballah <[email protected]>
4b00744
to
90d36f9
Compare
related to zalando-incubator/cluster-lifecycle-manager#775