implement GetOptions for GCE #4236
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Maciej, would you mind having a look? This is entirely standing on your amazing "per Nodegroups configs" work! |
| return nil, err | ||
| } | ||
|
|
||
| if opt, ok := getFloat64Option(options, template.Name, "scaledownutilizationthreshold"); ok { |
There was a problem hiding this comment.
These strings occur in other cloud provider PRs as well. Is it possible to make them global constants?
There was a problem hiding this comment.
Given the way to pass the config is different between providers do we really see value in shared string consts? I can easily see providers striving to make them consistent with the rest of config for given provider (ex. use the same prefix), so we can't guarantee they would be portable anyway.
That being said I would also prefer if the strings were consts instead (whether defined at provider level or as something like DefaultScaleDownUtilizationThresholdKey next to NodeGroupAutoscalingOptions definition).
There was a problem hiding this comment.
Currently we have in-code constants that the author defined (I assume) that happen to be exactly the same as on other cloud providers. We could try to push it more towards a standard.
But if you prefer local constants then I'm fine with this solution as well. It will still be better than magical strings hidden in code :).
There was a problem hiding this comment.
Made that shared constants (we don't often have opportunities to enforce some consistency ;), and changed AWS and Azure PRs to draft until this gets in (so I'll refresh them to pick those symbols + feedback from this PR).
| return make(map[string]string), nil | ||
| } | ||
|
|
||
| return parseKeyValueListToMap(optionsAsString) |
There was a problem hiding this comment.
I'd consider logging the values obtained from template (the actual parsed value). Knowing when the config changes and what the new value is seems useful for debugging.
However, we should only do this if we refresh rarely enough (see my other comment).
There was a problem hiding this comment.
Good point, added a log entry as part of the refactoring suggested in your other comment.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return m.templates.BuildAutoscalingOptions(template, defaults) |
There was a problem hiding this comment.
I don't think this should be called here directly:
- GetOptions is similar to MinSize() and MaxSize(): CA code calls it at any point in the loop and we generally assume that:
- The value returned should not change during CA loop.
- The call is "free" from performance point of view (ie. no API calls should happen as a result).
The latter is probably already true due to caching in migInstanceTemplatesProvider, but I think the way to update any provider level settings should be to fetch it in Refresh() call and only return cached value here. In this case I propose to move extractAutoscalingOptionsFromKubeEnv and associated logic into forceRefresh() call and only apply the options to defaults in GetMigOptions.
WDYT?
There was a problem hiding this comment.
Updated as suggested
bpineau
force-pushed
the
autoscaling-options-gce
branch
2 times, most recently
from
a5183ad to
84f5dcb
Compare
|
@mwielgus and @MaciekPytel Thanks for the reviews; updated following the suggestion + re-tested, please PTAL. |
| } | ||
| kubeEnvValue, err := getKubeEnvValueFromTemplateMetadata(template) | ||
| if err != nil { | ||
| klog.Warningf("Failed to extract KubeEnv from %q instance template's metadata: %v", template.Name, err) |
There was a problem hiding this comment.
nit: this is a very generic warning (we get a lot of stuff from kube env in template). I like the ones above more, as they make it clear where the error is coming from.
There was a problem hiding this comment.
Right, updated to make it clear this comes from extracting autoscaling options.
|
/lgtm |
k8s-ci-robot
added
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bpineau, MaciekPytel 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
Support per-MIG (scaledown) settings as permited by the cloudprovider's interface `GetOptions()` method.
bpineau
force-pushed
the
autoscaling-options-gce
branch
from
84f5dcb to
d905ec2
Compare
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
lgtm
implement GetOptions for GCE
Support per-MIG (scaledown) settings as permited by the
cloudprovider's interface
GetOptions()method.