use getvmss api for spot instances in azure #6470

gandhipr · 2024-01-25T18:45:38Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR adds support for using getVMSS api for instances in order to collect most recent count in the spot nodepool

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

getVMSS api will be called when using spot instances for having better updated information

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

cluster-autoscaler/cloudprovider/azure/azure_scale_set.go

tallaxes

Added some comments. Please also update the PR name and description, and add a short summary of user-facing change

tallaxes · 2024-03-22T00:42:22Z

cluster-autoscaler/cloudprovider/azure/azure_config.go

+		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
+		}


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?)

Hmm. Looks like we're doing the same for others as well like - vmssVmsCacheTTL, vmssVmsCacheTTL, etc

No, those lack the "else" part, will preserve config file setting in the absence of env override

I assume the cloud config file settings will already have been primed into the cfg object? If so this should work

... } else if cfg.GetVmssSizeRefreshPeriod != 0 { cfg.GetVmssSizeRefreshPeriod = VmssSizeRefreshPeriodDefault }

Is that what you're thinking @tallaxes?

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 getVmssSizeRefreshPeriod, and we set AZURE_GET_VMSS_SIZE_REFRESH_PERIOD, then this AZURE_GET_VMSS_SIZE_REFRESH_PERIOD will never be used and this configuration will goes to default.

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.

To your second point:

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.

Do you mean:

read from config file fully and populate cfg

then for each variable, check to see if it is already set in cfg

if it's not, check to see if the corresponding env var is set + set in cfg if it is

finally, if no cfg or env var is set, set it to default

the assumption here is that people may set values with a mix of cloud config file and env vars?

That's not true, right?

The logic is:

if env var is set AND not empty string, then convert string to int and set cfg field
else: use default

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 else statement. It will reach this part only if configReader == nil, which, if we track it down, indicates that the user never provided the path to the config file.

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 }

Do you mean:

read from config file fully and populate cfg

then for each variable, check to see if it is already set in cfg

if it's not, check to see if the corresponding env var is set + set in cfg if it is

finally, if no cfg or env var is set, set it to default

That's one way to go with it---with config file > env > default.
Another way is env > config file > 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.

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:

If config is not nil, but doesn't set a field

the env var for that field will never be picked up

the default for that field (if present) will never be picked up

@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.

cluster-autoscaler/cloudprovider/azure/azure_scale_set.go

cluster-autoscaler/cloudprovider/azure/README.md

jackfrancis · 2024-03-22T03:58:32Z

/retest

x13n · 2024-05-07T18:58:15Z

Assigning since looks like this is being reviewed by you already.

/assign @tallaxes @rakechill

k8s-ci-robot · 2024-05-07T18:58:18Z

@x13n: GitHub didn't allow me to assign the following users: rakechill.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

Assigning since looks like this is being reviewed by you already.

/assign @tallaxes @rakechill

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

jackfrancis · 2024-05-07T20:08:56Z

/test pull-cluster-autoscaler-e2e-azure

jackfrancis · 2024-05-07T21:51:14Z

/retest

k8s-ci-robot · 2024-06-14T09:27:35Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

rakechill · 2024-06-25T18:59:10Z

@x13n can you assign this to me?

x13n · 2024-06-27T13:13:48Z

Looks like I can, done!

…todiscovery

jackfrancis · 2024-07-19T19:59:49Z

cluster-autoscaler/cloudprovider/azure/azure_config.go

@@ -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"`


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?

I think not quite applicable for this case. GetVMSS is 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.

jackfrancis

/lgtm
/approve

k8s-ci-robot · 2024-07-19T21:25:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: comtalyst, gandhipr, jackfrancis, rakechill

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:

~~cluster-autoscaler/cloudprovider/azure/OWNERS~~ [gandhipr,jackfrancis]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

comtalyst · 2024-07-21T19:01:22Z

cluster-autoscaler/cloudprovider/azure/azure_config.go

@@ -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"`


Looks like I missed this, but GetVmssSizeRefreshPeriod being an int introduces an inconsistency with the fork, where it is time.Duration. This time it is not too bad, but let us all be careful next time. I could see cases like this getting worse if there are more dependencies.

rakechill · 2024-07-26T17:15:44Z

/cherry-pick cluster-autoscaler-release-1.27

k8s-infra-cherrypick-robot · 2024-07-26T17:15:52Z

@rakechill: only kubernetes org members may request cherry picks. If you are already part of the org, make sure to change your membership to public. Otherwise you can still do the cherry-pick manually.

In response to this:

/cherry-pick cluster-autoscaler-release-1.27

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

willie-yao · 2024-07-26T17:27:25Z

/cherry-pick cluster-autoscaler-release-1.27

k8s-infra-cherrypick-robot · 2024-07-26T17:28:53Z

@willie-yao: #6470 failed to apply on top of branch "cluster-autoscaler-release-1.27":

Applying: introduce getVmssSizeRefreshPeriod env var
Applying: logic for getvmss call for spot, uniform vmss
Using index info to reconstruct a base tree...
M	cluster-autoscaler/cloudprovider/azure/azure_config.go
M	cluster-autoscaler/cloudprovider/azure/azure_manager_test.go
M	cluster-autoscaler/cloudprovider/azure/azure_scale_set.go
M	cluster-autoscaler/cloudprovider/azure/azure_scale_set_test.go
Falling back to patching base and 3-way merge...
Auto-merging cluster-autoscaler/cloudprovider/azure/azure_scale_set_test.go
Auto-merging cluster-autoscaler/cloudprovider/azure/azure_scale_set.go
Auto-merging cluster-autoscaler/cloudprovider/azure/azure_manager_test.go
CONFLICT (content): Merge conflict in cluster-autoscaler/cloudprovider/azure/azure_manager_test.go
Auto-merging cluster-autoscaler/cloudprovider/azure/azure_config.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 logic for getvmss call for spot, uniform vmss
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick cluster-autoscaler-release-1.27

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 25, 2024

k8s-ci-robot requested review from feiskyer and jackfrancis January 25, 2024 18:45

k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider area/cluster-autoscaler approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 25, 2024

rakechill reviewed Mar 12, 2024

View reviewed changes

cluster-autoscaler/cloudprovider/azure/azure_scale_set.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 17, 2024

gandhipr force-pushed the prachigandhi/azure-getvmss-spot branch from e6c51a3 to 6ca161e Compare March 19, 2024 16:48

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 19, 2024

rakechill approved these changes Mar 19, 2024

View reviewed changes

tallaxes suggested changes Mar 22, 2024

View reviewed changes

gandhipr changed the title ~~Prachigandhi/azure getvmss spot~~ use getvmss api for spot instances in azure Apr 17, 2024

gandhipr added 6 commits April 17, 2024 12:05

introduce getVmssSizeRefreshPeriod env var

41676ae

logic for getvmss call for spot, uniform vmss

a97ffab

modify test for spot verification

fe9c5b0

golint fixes

017e88d

update comment

c45e45d

review comments

eb99a37

gandhipr force-pushed the prachigandhi/azure-getvmss-spot branch from 12fb2e9 to eb99a37 Compare April 17, 2024 19:06

update gofmt

124a273

k8s-ci-robot assigned tallaxes May 7, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2024

x13n assigned rakechill Jun 27, 2024

Merge branch 'master' into prachigandhi/azure-getvmss-spot

c33c4ab

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 3, 2024

rakechill and others added 5 commits July 3, 2024 16:42

Add VMs pool checks back into target size test

317fd78

Add getVmssSizeRefreshPeriod default val to azure_manager_test for au…

6829c33

…todiscovery

Merge branch 'kubernetes:master' into prachigandhi/azure-getvmss-spot

d11a9c8

Update getVmssSizeRefreshPeriod to use time conversions

2551d86

remove changes to README

b69fd45

jackfrancis reviewed Jul 19, 2024

View reviewed changes

comtalyst approved these changes Jul 19, 2024

View reviewed changes

jackfrancis approved these changes Jul 19, 2024

View reviewed changes

k8s-ci-robot assigned jackfrancis Jul 19, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 19, 2024

k8s-ci-robot merged commit c8e4721 into kubernetes:master Jul 19, 2024
7 checks passed

comtalyst reviewed Jul 21, 2024

View reviewed changes

rakechill deleted the prachigandhi/azure-getvmss-spot branch July 26, 2024 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use getvmss api for spot instances in azure #6470

use getvmss api for spot instances in azure #6470

gandhipr commented Jan 25, 2024 •

edited

Loading

tallaxes left a comment

tallaxes Mar 22, 2024

gandhipr Mar 28, 2024

tallaxes Mar 28, 2024

jackfrancis May 7, 2024

comtalyst Jul 2, 2024 •

edited

Loading

rakechill Jul 2, 2024 •

edited

Loading

rakechill Jul 2, 2024

comtalyst Jul 2, 2024

comtalyst Jul 2, 2024

rakechill Jul 2, 2024

jackfrancis commented Mar 22, 2024

x13n commented May 7, 2024

k8s-ci-robot commented May 7, 2024

jackfrancis commented May 7, 2024

jackfrancis commented May 7, 2024

k8s-ci-robot commented Jun 14, 2024

rakechill commented Jun 25, 2024

x13n commented Jun 27, 2024

jackfrancis Jul 19, 2024

rakechill Jul 19, 2024

jackfrancis left a comment

k8s-ci-robot commented Jul 19, 2024

comtalyst Jul 21, 2024

rakechill commented Jul 26, 2024

k8s-infra-cherrypick-robot commented Jul 26, 2024

willie-yao commented Jul 26, 2024

k8s-infra-cherrypick-robot commented Jul 26, 2024

use getvmss api for spot instances in azure #6470

use getvmss api for spot instances in azure #6470

Conversation

gandhipr commented Jan 25, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

tallaxes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comtalyst Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

rakechill Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis commented Mar 22, 2024

x13n commented May 7, 2024

k8s-ci-robot commented May 7, 2024

jackfrancis commented May 7, 2024

jackfrancis commented May 7, 2024

k8s-ci-robot commented Jun 14, 2024

rakechill commented Jun 25, 2024

x13n commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 19, 2024

Choose a reason for hiding this comment

rakechill commented Jul 26, 2024

k8s-infra-cherrypick-robot commented Jul 26, 2024

willie-yao commented Jul 26, 2024

k8s-infra-cherrypick-robot commented Jul 26, 2024

gandhipr commented Jan 25, 2024 •

edited

Loading

comtalyst Jul 2, 2024 •

edited

Loading

rakechill Jul 2, 2024 •

edited

Loading