Skip to content
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

allow users to avoid aws instance not found spam #6265

Merged
merged 1 commit into from
Nov 20, 2023
Merged

allow users to avoid aws instance not found spam #6265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions cluster-autoscaler/cloudprovider/aws/aws_cloud_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,14 @@ func (aws *awsCloudProvider) HasInstance(node *apiv1.Node) (bool, error) {
return true, cloudprovider.ErrNotImplemented
}

// avoid log spam for not autoscaled asgs:
// Nodes that belong to an asg that is not autoscaled will not be found in the asgCache below,
// so do not trigger warning spam by returning an error from being unable to find them.
// Annotation is not automated, but users that see the warning can add the annotation to avoid it.
if node.Annotations != nil && node.Annotations["k8s.io/cluster-autoscaler/enabled"] == "false" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test confirming this works correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I can add tests if anyone with merge permission can confirm this is a good approach

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me, yes.

Rather: imo the ideal approach would be to just have that error be logged at a lower verbosity, but I think that's going to be non-trivial to accomplish given how the code is currently organized, so this seems fine as an alternate solution.

return false, nil
}

awsRef, err := AwsRefFromProviderId(node.Spec.ProviderID)
if err != nil {
return false, err
Expand Down
15 changes: 15 additions & 0 deletions cluster-autoscaler/cloudprovider/aws/aws_cloud_provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -723,4 +723,19 @@ func TestHasInstance(t *testing.T) {
assert.ErrorContains(t, err, nodeNotPresentErr)
assert.False(t, present)

// Case 4: correct node - not autoscaled -> not present in AWS -> no warning
node4 := &apiv1.Node{
ObjectMeta: metav1.ObjectMeta{
Name: "node-2",
Annotations: map[string]string{
"k8s.io/cluster-autoscaler/enabled": "false",
},
},
Spec: apiv1.NodeSpec{
ProviderID: "aws:///us-east-1a/test-instance-id-2",
},
}
present, err = provider.HasInstance(node4)
assert.NoError(t, err)
assert.False(t, present)
}
Loading