implement GetOptions for Azure #4237
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
Converted to draft until the corresponding GCE pull request gets in, so I can backport improvement suggested during GCE review there, and use global symbols defined in that other PR. |
bpineau
force-pushed
the
autoscaling-options-azure
branch
from
31b82cf to
7c46e5c
Compare
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
Updated with feedback and constants from the GCE equivalent PR, ready for review. |
| @@ -246,6 +247,29 @@ func (m *AzureManager) GetNodeGroupForInstance(instance *azureRef) (cloudprovide | |||
| return m.azureCache.FindForInstance(instance, m.config.VMType) | |||
| } | |||
|
|
|||
| // GetScaleSetOptions parse options extracted from VMSS tags and merges them with provided defaults | |||
| func (m *AzureManager) GetScaleSetOptions(scaleSetName string, defaults config.NodeGroupAutoscalingOptions) *config.NodeGroupAutoscalingOptions { | |||
There was a problem hiding this comment.
should we propagate an error back from here? How are you handling those errors for other cloudprovider implementations?
There was a problem hiding this comment.
Intent is not to impact operations on errors, but make them visible and auditable. Missing options tags is just fine (and likely). Likewise for incomplete/partial tags set. We log and ignore bogus (or similar-looking yet different, etc) tag values. That's similar for aws and gce cloudproviders.
| if !reflect.DeepEqual(m.getAutoscalingOptions(ref), options) { | ||
| klog.V(4).Infof("Extracted autoscaling options from %q ScaleSet tags: %v", *vmss.Name, options) | ||
| } | ||
| newAutoscalingOptions[ref] = options |
There was a problem hiding this comment.
nit: should this be done in the above if check?
There was a problem hiding this comment.
We're rebuilding that option cache from scratch (newAutoscalingOptions is empty before that loop) so we have to store both changed and previously known options there.
bpineau
force-pushed
the
autoscaling-options-azure
branch
from
7c46e5c to
4b0d5f7
Compare
Support per-VMSS (scaledown) settings as permited by the cloudprovider's interface `GetOptions()` method.
bpineau
force-pushed
the
autoscaling-options-azure
branch
from
4b0d5f7 to
28cd49c
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bpineau, marwanad 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
…zure implement GetOptions for Azure
…zure implement GetOptions for Azure
Support per-VMSS (scaledown) settings as exposed by the cloudprovider's interface
GetOptions()method.For ref, providing similar GetOptions support for other providers: