use getvmss api for spot instances in azure #6470
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,6 +56,9 @@ const ( | |
| rateLimitWriteQPSEnvVar = "RATE_LIMIT_WRITE_QPS" | ||
| rateLimitWriteBucketsEnvVar = "RATE_LIMIT_WRITE_BUCKETS" | ||
|
|
||
| // VmssSizeRefreshPeriodDefault in seconds | ||
| VmssSizeRefreshPeriodDefault = 30 | ||
|
|
||
| // auth methods | ||
| authMethodPrincipal = "principal" | ||
| authMethodCLI = "cli" | ||
|
|
@@ -128,6 +131,9 @@ type Config struct { | |
| // Jitter in seconds subtracted from the VMSS cache TTL before the first refresh | ||
| VmssVmsCacheJitter int `json:"vmssVmsCacheJitter" yaml:"vmssVmsCacheJitter"` | ||
|
|
||
| // GetVmssSizeRefreshPeriod (seconds) defines how frequently to call GET VMSS API to fetch VMSS info per nodegroup instance | ||
| GetVmssSizeRefreshPeriod int `json:"getVmssSizeRefreshPeriod" yaml:"getVmssSizeRefreshPeriod"` | ||
|
There was a problem hiding this comment. Looks like I missed this, but |
||
|
|
||
| // number of latest deployments that will not be deleted | ||
| MaxDeploymentsCount int64 `json:"maxDeploymentsCount" yaml:"maxDeploymentsCount"` | ||
|
|
||
|
|
@@ -256,6 +262,15 @@ func BuildAzureConfig(configReader io.Reader) (*Config, error) { | |
| cfg.EnableDynamicInstanceList = dynamicInstanceListDefault | ||
| } | ||
|
|
||
| if getVmssSizeRefreshPeriod := os.Getenv("AZURE_GET_VMSS_SIZE_REFRESH_PERIOD"); getVmssSizeRefreshPeriod != "" { | ||
| cfg.GetVmssSizeRefreshPeriod, err = strconv.Atoi(getVmssSizeRefreshPeriod) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse AZURE_GET_VMSS_SIZE_REFRESH_PERIOD %q: %v", getVmssSizeRefreshPeriod, err) | ||
| } | ||
| } else { | ||
| cfg.GetVmssSizeRefreshPeriod = VmssSizeRefreshPeriodDefault | ||
| } | ||
|
Comment on lines
+265
to
+272
There was a problem hiding this comment. This logic uses either environment variable or default, never the value from cloud config file. If we don't support setting this from cloud config file - need to remove from docs. If we need to support it - should update the logic. (It looks like the same applies to the EnableDynamicInstanceList above?) There was a problem hiding this comment. Hmm. Looks like we're doing the same for others as well like - vmssVmsCacheTTL, vmssVmsCacheTTL, etc There was a problem hiding this comment. No, those lack the "else" part, will preserve config file setting in the absence of env override There was a problem hiding this comment. I assume the cloud config file settings will already have been primed into the Is that what you're thinking @tallaxes? There was a problem hiding this comment. From my understanding, line 127 will result in it being taken from cloud config file already. GetVmssSizeRefreshPeriod int `json:"getVmssSizeRefreshPeriod" yaml:"getVmssSizeRefreshPeriod"`Line 155-162 should take care of the parsing: if configReader != nil {
body, err := ioutil.ReadAll(configReader)
if err != nil {
return nil, fmt.Errorf("failed to read config: %v", err)
}
err = json.Unmarshal(body, cfg)
if err != nil {
return nil, fmt.Errorf("failed to unmarshal config body: %v", err)
}
}Then, on line 163 and below, the environment variables will be used only when cloud config file does not exist. else {
cfg.Cloud = os.Getenv("ARM_CLOUD")
...
}The problem I am seeing is when cloud config file exists, but does not contain My proposal: consider each variable individually rather than entire file at once when determining whether to use an environment variable. But not in this PR. There was a problem hiding this comment. To your second point:
Do you mean:
There was a problem hiding this comment. the assumption here is that people may set values with a mix of cloud config file and env vars? There was a problem hiding this comment.
It is true (right?, you may help me check again). All these environment variable parsing, from line 163 to 314, is encased in a giant if configReader != nil {
body, err := ioutil.ReadAll(configReader)
if err != nil {
return nil, fmt.Errorf("failed to read config: %v", err)
}
err = json.Unmarshal(body, cfg)
if err != nil {
return nil, fmt.Errorf("failed to unmarshal config body: %v", err)
}
}
Then, on line 163 and below, the environment variables will be used only when cloud config file does not exist.
else {
cfg.Cloud = os.Getenv("ARM_CLOUD")
...
// What we are looking at
}There was a problem hiding this comment.
That's one way to go with it---with config file > env > default. I actually prefer the latter more, given env being more delicate(?) than the config file, as well as having a use case I see in AKS. There was a problem hiding this comment. For now, I'm going to leave the logic as is. However, want to explicitly call out that we are not handling a key case for most of these variables:
@comtalyst is aiming to address this scenario in his PR to defork cloud-provider-azure It's a rather large PR so we may determine that this logic is better to be merged separately. Either way, I think it's fine to keep this field using the same structure as the others and address this in a follow-up PR. |
||
|
|
||
| if enableVmssFlex := os.Getenv("AZURE_ENABLE_VMSS_FLEX"); enableVmssFlex != "" { | ||
| cfg.EnableVmssFlex, err = strconv.ParseBool(enableVmssFlex) | ||
| if err != nil { | ||
|
|
||
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 qualifies as a nit, but maybe rename all of these properties that start w/ "get" so that they don't confuse readers that they are getter funcs. I think "vmssGet...|VmssGet..." instead would work without regressing the semantics for human readability?
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 think not quite applicable for this case.
GetVMSSis the actual function we're calling. switching the order around almost makes it sound (to me) like we're calling a function that gets the size rather than the VMSS.