pkg/asset/machines/aws: Only return available zones

[wking]

Zones can also be impaired, unavailable, or "information" (docs here and here). I dunno what that last one is about. But we only want to put machines in available zones ;).

This is a bit racy, because we could get different responses at both call-sites. Moving forward, we'll want to shift this request into a single available-zones asset.

Fixes #1209.

[wking]

e2e-aws:

Flaky tests:

[Feature:DeploymentConfig] deploymentconfigs with test deployments [Conformance] should run a deployment to completion and then scale to zero [Suite:openshift/conformance/parallel/minimal]

Failing tests:

[Conformance][Area:Networking][Feature:Router] The default ClusterIngress should support default wildcard reencrypt routes through external DNS [Suite:openshift/conformance/parallel/minimal]

/retest

[wking]

All green, when you have a moment @abhinavdahiya, @staebler.

[abhinavdahiya]

Moving forward, we'll want to shift this request into a single available-zones asset.

Do we have an PR open or tracking issue...

[wking]

Moving forward, we'll want to shift this request into a single available-zones asset.

Do we have an PR open or tracking issue...

#1121 will get us closer. I can file a ticket for what will still be left after that later tonight.

[wking]

But "zone goes offline mid-asset-graph" is probably going to break automatic zone selection even once we're down to a single zone-listing call (we'll just be internally consistent when we die ;).

[wking]

Ok, I think #1045, #1121, and #1212 together will address the consolidation to a single available-zones request in Go. I still think we want this PR to get the filtering in Go (which we'll want forever) and Terraform (which we'll need until #1045 and #1121 together let us drop that Terraform zone request).

[staebler]

Ok, I think #1045, #1121, and #1212 together will address the consolidation to a single available-zones request in Go. I still think we want this PR to get the filtering in Go (which we'll want forever) and Terraform (which we'll need until #1045 and #1121 together let us drop that Terraform zone request).

I don't think we are quite there yet for dropping the Terraform zone request even after #1045 and #1121. I would like to see us send from Go to Terraform the union of the zones used in all of the machine pools. Then, we can get rid of the Terraform zone request.

[staebler]

Do we need to modify the Go fetch of availability zones as well to only return the zones that have a state of "available"?

[wking]

Do we need to modify the Go fetch of availability zones as well to only return the zones that have a state of "available"?

Like this?

[staebler]

Do we need to modify the Go fetch of availability zones as well to only return the zones that have a state of "available"?

Like this?

Yeah, just like that. It boggles my mind that I made that comment. I can't understand how I could have missed that, particularly given that it is the majority of the changes in the PR.

[abhinavdahiya]

Maybe I'm wrong, but with #1121 the available check in go will make sure that the masterAZs chosen were available and az_to_<public/private>subnet maps will make sure we only create machines in AZs chosen by installer

?

[wking]

Maybe I'm wrong, but with #1121 the available check in go...

Yup, the Terraform part of this is code that is hopefully going away soon. But we want the Go part of this long-term, and the Terraform code is still there now, so I think we might as well fix it too.

[abhinavdahiya]

Well technically terraform should use the AZs given by the installer ie if the AZ goes out of service between asset generation and terraform call... it would be better if we fail with error that AZ is out of service rather than the AZ not found in the az_to_<public/private>subnet maps.

[wking]

Well technically terraform should use the AZs given by the installer...

Agreed.

... ie if the AZ goes out of service between asset generation and terraform call...

I think using Go-provided zones means we can drop this data "aws_availability_zones" section altogether. Guarding against a zone going down is always going to be racy up through actually creating resources in the zone, so I'd rather save a few lines and the mental overhead and just have Terraform assume Go has already done any required validation and blindly consume what it was given.

[abhinavdahiya]

I think using Go-provided zones means we can drop this data "aws_availability_zones" section altogether. Guarding against a zone going down is always going to be racy up through actually creating resources in the zone, so I'd rather save a few lines and the mental overhead and just have Terraform assume Go has already done any required validation and blindly consume what it was given.

I can get behind that change. :)

[wking]

I can get behind that change. :)

Did you want me to pick that up in this PR? Or submit it as a follow-up once this lands?

[abhinavdahiya]

I think using Go-provided zones means we can drop this data "aws_availability_zones" section altogether. Guarding against a zone going down is always going to be racy up through actually creating resources in the zone, so I'd rather save a few lines and the mental overhead and just have Terraform assume Go has already done any required validation and blindly consume what it was given.

I can get behind that change. :)

But that would need some changes esp https://jira.coreos.com/browse/CORS-902, that will push this PR to 4.1

[abhinavdahiya]

I can get behind that change. :)

Did you want me to pick that up in this PR? Or submit it as a follow-up once this lands?

follow up is fine.

[abhinavdahiya]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, wking

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:

• OWNERS [abhinavdahiya,wking]

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