Autoscaler: AWS cloud provider tests take 120 seconds

[losipiuk]

The problem started to occur after merging
https://github.com/kubernetes/autoscaler/commit/a36f8007afa65fcb2ff7ae291ee815be5512e01c
Each of 6 test method tries to get region from AWS api (see aws_manager.go getRegion method). This call times out after 20seconds.

@shatil Is there a chance that you could fix that?

[shatil]

@losipiuk, certainly!

Inside of AWS this returns instantly, and outside, it will always timeout, so I'm thinking to add a 1 second timeout around svc.Region().

[losipiuk]

Thanks!

I was thinking more that you could mock the http call in tests so it returns immediately.
Depending on build environment and sleeping (even 1s) in unit tests are both bad practices.

[aleksandra-malinowska]

I was thinking more that you could mock the http call in tests so it returns immediately.

This. Or set the env var in tests. Or read the env var earlier, e.g. when constructing AWS manager, and pass it as parameter. Etc.

[shatil]

@losipiuk @aleksandra-malinowska I like the environment variable idea. I had already mocked out another environment variable lookup when it occurred to me:

If the tests (indirectly) relying on getRegion supply an AWS_REGION env variable, there is no timeout. Is this approach acceptable? Or will this throw something off?

I think tests using createAWSManagerInternal would need it, and that's about it.

$ git grep createAWSManagerInternal
aws_manager.go:func createAWSManagerInternal(
aws_manager.go: return createAWSManagerInternal(configReader, discoveryOpts, nil, nil)
aws_manager_test.go:    m, err := createAWSManagerInternal(nil, do, &autoScalingWrapper{s}, nil)
aws_manager_test.go:    m, err := createAWSManagerInternal(nil, cloudprovider.NodeGroupDiscoveryOptions{}, nil, &ec2Wrapper{s})
aws_manager_test.go:                    m, err := createAWSManagerInternal(nil, cloudprovider.NodeGroupDiscoveryOptions{}, nil, &ec2Wrapper{s})
aws_manager_test.go:    m, err := createAWSManagerInternal(nil, do, &autoScalingWrapper{s})

I could set and reset it for that test alone, unless it's okay to set it for _all_ tests. Is there some central location where environment variables for tests are configured?

[aleksandra-malinowska]

please do it just for tests that require it (can be part of sth like createTestAWSManager ofc)

[jonathanbeber]

@losipiuk @shatil shouldn't it be closed once #1490 is merged?

[losipiuk]

Yeah, closing. Thanks.