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.

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

fix: CA on fargate causing log flood #5887

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 11 additions & 1 deletion cluster-autoscaler/cloudprovider/aws/aws_cloud_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ import (
const (
// GPULabel is the label added to nodes with GPU resource.
GPULabel = "k8s.amazonaws.com/accelerator"
// nodeNotPresentErr indicates no node with the given identifier present in AWS
nodeNotPresentErr = "node is not present in aws"
)

var (
Expand Down Expand Up @@ -129,6 +131,14 @@ func (aws *awsCloudProvider) NodeGroupForNode(node *apiv1.Node) (cloudprovider.N

// HasInstance returns whether a given node has a corresponding instance in this cloud provider
func (aws *awsCloudProvider) HasInstance(node *apiv1.Node) (bool, error) {
// we haven't implemented a way to check if a fargate instance
// exists in the cloud provider
// returning 'true' because we are assuming the node exists in AWS
// this is the default behavior if the check is unimplemented
if strings.HasPrefix(node.GetName(), "fargate") {
return true, cloudprovider.ErrNotImplemented
}

awsRef, err := AwsRefFromProviderId(node.Spec.ProviderID)
if err != nil {
return false, err
Expand All @@ -140,7 +150,7 @@ func (aws *awsCloudProvider) HasInstance(node *apiv1.Node) (bool, error) {
return true, nil
}

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

// Pricing returns pricing model for this cloud provider or error if not available.
Expand Down
77 changes: 72 additions & 5 deletions cluster-autoscaler/cloudprovider/aws/aws_cloud_provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
apiv1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
"k8s.io/autoscaler/cluster-autoscaler/cloudprovider/aws/aws-sdk-go/aws"
"k8s.io/autoscaler/cluster-autoscaler/cloudprovider/aws/aws-sdk-go/service/autoscaling"
Expand All @@ -39,9 +40,9 @@ var testAwsManager = &AwsManager{
awsService: testAwsService,
}

func newTestAwsManagerWithMockServices(mockAutoScaling autoScalingI, mockEC2 ec2I, mockEKS eksI, autoDiscoverySpecs []asgAutoDiscoveryConfig) *AwsManager {
func newTestAwsManagerWithMockServices(mockAutoScaling autoScalingI, mockEC2 ec2I, mockEKS eksI, autoDiscoverySpecs []asgAutoDiscoveryConfig, instanceStatus map[AwsInstanceRef]*string) *AwsManager {
awsService := awsWrapper{mockAutoScaling, mockEC2, mockEKS}
return &AwsManager{
mgr := &AwsManager{
awsService: awsService,
asgCache: &asgCache{
registeredAsgs: make(map[AwsRef]*asg, 0),
Expand All @@ -55,16 +56,21 @@ func newTestAwsManagerWithMockServices(mockAutoScaling autoScalingI, mockEC2 ec2
autoscalingOptions: make(map[AwsRef]map[string]string),
},
}

if instanceStatus != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

To pass custom instanceStatus so that it is easy to test HasInstance()

mgr.asgCache.instanceStatus = instanceStatus
}
return mgr
}

func newTestAwsManagerWithAsgs(t *testing.T, mockAutoScaling autoScalingI, mockEC2 ec2I, specs []string) *AwsManager {
m := newTestAwsManagerWithMockServices(mockAutoScaling, mockEC2, nil, nil)
m := newTestAwsManagerWithMockServices(mockAutoScaling, mockEC2, nil, nil, nil)
m.asgCache.parseExplicitAsgs(specs)
return m
}

func newTestAwsManagerWithAutoAsgs(t *testing.T, mockAutoScaling autoScalingI, mockEC2 ec2I, specs []string, autoDiscoverySpecs []asgAutoDiscoveryConfig) *AwsManager {
m := newTestAwsManagerWithMockServices(mockAutoScaling, mockEC2, nil, autoDiscoverySpecs)
m := newTestAwsManagerWithMockServices(mockAutoScaling, mockEC2, nil, autoDiscoverySpecs, nil)
m.asgCache.parseExplicitAsgs(specs)
return m
}
Expand Down Expand Up @@ -592,7 +598,7 @@ func TestDeleteNodesAfterMultipleRefreshes(t *testing.T) {
func TestGetResourceLimiter(t *testing.T) {
mockAutoScaling := &autoScalingMock{}
mockEC2 := &ec2Mock{}
m := newTestAwsManagerWithMockServices(mockAutoScaling, mockEC2, nil, nil)
m := newTestAwsManagerWithMockServices(mockAutoScaling, mockEC2, nil, nil, nil)

provider := testProvider(t, m)
_, err := provider.GetResourceLimiter()
Expand All @@ -604,3 +610,64 @@ func TestCleanup(t *testing.T) {
err := provider.Cleanup()
assert.NoError(t, err)
}

func TestHasInstance(t *testing.T) {
nodeStatus := "Healthy"
mgr := &AwsManager{
asgCache: &asgCache{
registeredAsgs: make(map[AwsRef]*asg, 0),
asgToInstances: make(map[AwsRef][]AwsInstanceRef),
instanceToAsg: make(map[AwsInstanceRef]*asg),
interrupt: make(chan struct{}),
awsService: &testAwsService,
instanceStatus: map[AwsInstanceRef]*string{
{
ProviderID: "aws:///us-east-1a/test-instance-id",
Name: "test-instance-id",
}: &nodeStatus,
},
},
awsService: testAwsService,
}
provider := testProvider(t, mgr)

// Case 1: correct node - present in AWS
node1 := &apiv1.Node{
ObjectMeta: metav1.ObjectMeta{
Name: "node-1",
},
Spec: apiv1.NodeSpec{
ProviderID: "aws:///us-east-1a/test-instance-id",
},
}
present, err := provider.HasInstance(node1)
assert.NoError(t, err)
assert.True(t, present)

// Case 2: incorrect node - fargate is unsupported
node2 := &apiv1.Node{
ObjectMeta: metav1.ObjectMeta{
Name: "fargate-1",
},
Spec: apiv1.NodeSpec{
ProviderID: "aws:///us-east-1a/test-instance-id",
},
}
present, err = provider.HasInstance(node2)
assert.Equal(t, cloudprovider.ErrNotImplemented, err)
assert.True(t, present)

// Case 3: correct node - not present in AWS
node3 := &apiv1.Node{
ObjectMeta: metav1.ObjectMeta{
Name: "node-2",
},
Spec: apiv1.NodeSpec{
ProviderID: "aws:///us-east-1a/test-instance-id-2",
Copy link
Member Author

Choose a reason for hiding this comment

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

This node has a provider ID which is not present in the asg cache.

},
}
present, err = provider.HasInstance(node3)
assert.ErrorContains(t, err, nodeNotPresentErr)
assert.False(t, present)

}