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.

Sign up for GitHub

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

Jump to bottom

fix: implement function to identify if node is present in aws #5632

Merged

k8s-ci-robot merged 1 commit into kubernetes:master from vadasambar:fix/4456/identify-deleted-nodes-for-aws

Apr 10, 2023

Member

vadasambar commented Mar 29, 2023 •

edited

Loading

this is a follow-up to Identifying cloud provider deleted nodes #5054
this might fix CA fails to scale-up or cancel in-progress scale down when there are un-schedulable pods #4456

What type of PR is this?

/kind bug

What this PR does / why we need it:

I think description of #5054 (comment) explains it well:

...original intent of determining the deleted nodes was incorrect, which led to the issues reported by other users. The nodes tainted with ToBeDeleted were misidentified as Deleted instead of Ready/Unready, which caused a miscalculation of the node being included as Upcoming. This caused problems described in #3949 and #4456.

#5054 exposed interface function HasInstance for cloud providers to implement. This function is used to determine if a node is deleted by querying the cloud provider. It falls back to the ToBeDeleted taint if the function is not implemented.

Which issue(s) this PR fixes:

Fixes #4456

Special notes for your reviewer:

Nothing in particular.

Does this PR introduce a user-facing change?

NONE

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

Nothing in particular.

k8s-ci-robot added do-not-merge/work-in-progress kind/bug cncf-cla: yes size/S labels

k8s-ci-robot requested a review from drmorr0

March 29, 2023 05:55

k8s-ci-robot added the area/cluster-autoscaler label

k8s-ci-robot requested a review from gjtempleton

March 29, 2023 05:55

vadasambar commented

View reviewed changes

cluster-autoscaler/cloudprovider/aws/aws_cloud_provider.go Outdated

+              	}
+              	// we don't care about the instance health
+              	if _, ok := aws.awsManager.asgCache.instanceStatus[*awsRef]; !ok {

Member Author

vadasambar Mar 29, 2023

We already have instanceStatus field on asgCache to track health status by EC2 instance. I am re-using the instantStatus map to check if the node is present without caring for its health since we just want to check if the instance is present in the cloud provider.

instanceStatus is updated at

autoscaler/cluster-autoscaler/cloudprovider/aws/auto_scaling_groups.go

Lines 401 to 404 in 1d56b81

    
           ref := m.buildInstanceRefFromAWS(instance) 
        
           newInstanceToAsgCache[ref] = asg 
        
           newAsgToInstancesCache[asg.AwsRef][i] = ref 
        
           newInstanceStatusMap[ref] = instance.HealthStatus

vadasambar marked this pull request as ready for review

March 29, 2023 06:05

k8s-ci-robot removed the do-not-merge/work-in-progress label

k8s-ci-robot requested a review from jaypipes

March 29, 2023 06:05

jaypipes suggested changes

View reviewed changes

cluster-autoscaler/cloudprovider/aws/aws_cloud_provider.go Outdated

+              	}
+              	// we don't care about the instance health
+              	if _, ok := aws.awsManager.asgCache.instanceStatus[*awsRef]; !ok {

Contributor

jaypipes Mar 29, 2023

@vadasambar please use the asgCache.InstanceStatus function instead of directly accessing (in a non-thread-safe fashion) the underlying map :)

Member Author

vadasambar Mar 30, 2023

Thank you for noticing it! I have updated the code.

vadasambar mentioned this pull request

Mar 2023 vadafoss/daily-updates#7

Closed

vadasambar commented

View reviewed changes

cluster-autoscaler/cloudprovider/aws/aws_cloud_provider.go

+              	// we don't care about the status
+              	status, err := aws.awsManager.asgCache.InstanceStatus(*awsRef)
+              	if status != nil {

Member Author

vadasambar Mar 30, 2023

I am checking status for nil because I think this is a more reliable way (as opposed to checking there was no error) to check if the node is present since InstanceStatus wouldn't return nil for a status unless some error occurs.

Contributor

jaypipes Mar 30, 2023

Agreed. The fact that we return any error from HasInstance or asgCache.InstanceStatus is kind of annoying anyway. Really, these methods should just return bool IMHO.

vadasambar commented

View reviewed changes

cluster-autoscaler/cloudprovider/aws/aws_cloud_provider.go

+              		return true, nil
+              	}
+              	return false, fmt.Errorf("node is not present in aws: %v", err)

Member Author

vadasambar Mar 30, 2023

I believe this CA should print the line number of the error but I added an extra wrapper around the error just in case to identify the error came from HasInstance. Happy to change it based on feedback.

vadasambar requested review from jaypipes and removed request for drmorr0 and gjtempleton

March 30, 2023 04:47

jaypipes approved these changes

View reviewed changes

Contributor

jaypipes left a comment

👍 thank you @vadasambar!

cluster-autoscaler/cloudprovider/aws/aws_cloud_provider.go

+              	// we don't care about the status
+              	status, err := aws.awsManager.asgCache.InstanceStatus(*awsRef)
+              	if status != nil {

Contributor

jaypipes Mar 30, 2023

Agreed. The fact that we return any error from HasInstance or asgCache.InstanceStatus is kind of annoying anyway. Really, these methods should just return bool IMHO.

Contributor

k8s-ci-robot commented Mar 30, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jaypipes, vadasambar

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/aws/OWNERS~~ [jaypipes]

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

k8s-ci-robot added the approved label


          fix: implement function to identify if node is present in aws

1cb55fe

- this is a follow-up to kubernetes#5054
- this might fix kubernetes#4456

Signed-off-by: vadasambar <[email protected]>

fix: make `HasInstance` in aws provider thread-safe

Signed-off-by: vadasambar <[email protected]>

vadasambar force-pushed the fix/4456/identify-deleted-nodes-for-aws branch from 16695e9 to 1cb55fe Compare

March 31, 2023 04:16

Member Author

vadasambar commented Mar 31, 2023

+1 thank you @vadasambar!

Thank you for the review. I have squashed the commits into a single commit. I think I can't merge the PR by myself.

This is what I see:

Member

gjtempleton commented Apr 10, 2023

/lgtm

k8s-ci-robot assigned gjtempleton

k8s-ci-robot added the lgtm label

k8s-ci-robot merged commit dfaadfa into kubernetes:master

This was referenced Apr 14, 2023

Apr 2023 vadafoss/daily-updates#8

Closed

Nodes tainted with ToBeDeletedByClusterAutoscaler remain in the cluster blocking cluster-autoscaler normal operations #5657

Closed

vadasambar mentioned this pull request

REQUEST: New membership for @vadasambar kubernetes/org#4203

Closed

9 tasks

This was referenced May 22, 2023

[cherry-pick to 1.24]: implement function to identify if node is present in aws #5785

Merged

[cherry-pick to 1.25]: implement function to identify if node is present in aws #5786

Merged

[cherry-pick to 1.26]: implement function to identify if node is present in aws #5787

Merged

Cluster Autoscaler patch releases May 2023 #5774

Closed

vadasambar mentioned this pull request

AWS provider warning Failed to check cloud provider has instance for fargate is flooding the log #5842

Closed

zaafar commented Nov 7, 2023 •

edited

Loading

Can this be back ported to 1.24 and 1.25?

EDIT: nevermind, looks like it was.

MaxFedotov mentioned this pull request

fix: implement function to identify if node is present in cluster-api #6708

Merged

claassen mentioned this pull request

[BUG] Cluster autoscaler bug requires Azure specific implementation to resolve Azure/AKS#4286

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved area/cluster-autoscaler cncf-cla: yes kind/bug lgtm size/S