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

ci-operator/templates/openshift: Explicitly set AWS availability zones #3285

Merged

Conversation

wking
Copy link
Member

@wking wking commented Mar 28, 2019

This is very similar to the earlier e8921c3 (#3204). This time, however, I'm not changing the zones where the machines will run. By default, the installer will provisioning zone infrastructure in all available zones, but since openshift/installer@644f705286 (openshift/installer#1481) users who explicitly set zones in their install-config will no longer have unused zones provisioned with subnets, NAT gateways, EIPs, and other related infrastructure. This infrastructure reduction has two benefits in CI:

  1. We don't have to pay for resources that we won't use, and we will have more room under our EIP limits (although we haven't bumped into that one in a while, because we're VPC-constained).

  2. We should see reduced rates in clusters failing install because of AWS rate limiting, with results like:

    aws_route.to_nat_gw.3: Error creating route: timeout while waiting for state to become 'success' (timeout: 2m0s)
    

    The reduction is because:

    1. We'll be making fewer requests for these resources, because we won't need to create (and subsequently tear down) as many of them. This will reduce our overall AWS-API load somewhat, although the reduction will be incremental because we have so many other resources which are not associated with zones.

    2. Throttling for these per-zone resources are the ones that tend to break Terraform (1). So even if the rate of timeouts per-API request remains unchanged, a given cluster will only have half as many (three vs. the old six) per-zone chances of hitting one of the timeouts. This should give us something close to a 50% reduction in clusters hitting throttling timeouts.

The drawback is that we're diverging further from the stock "I just called openshift-install create cluster without providing an install-config.yaml" experience. openshift/installer#1487 is up for folks who wnt to weigh in on changing the installer default to only provision per-zone resources (and MachineSets) in zones that will receive install-time machines.

(1): With a cache of build-log.txt from the past ~48 hours:

$ grep -hr 'timeout while waiting for state' ~/.cache/openshift-deck-build-logs >timeouts
$ wc -l timeouts
362 timeouts
$ grep aws_route_table_association timeouts | wc -l
214
$ grep 'aws_route\.to_nat_gw' timeouts | wc -l
102

So (102+214)/362 is 87% of our timeouts, with the remainder being almost entirely related to the internet gateway (which is not per-zone).

CC @abhinavdahiya, @sdodson, @smarterclayton, @vrutkovs

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 28, 2019
@wking wking force-pushed the explicit-aws-availability-zones branch from 873dfef to 7ea560e Compare March 28, 2019 19:44
This is very similar to the earlier e8921c3
(ci-operator/templates/openshift: Get e2e-aws out of us-east-1b,
2019-03-22, openshift#3204).  This time, however, I'm not changing the zones
where the machines will run.  By default, the installer will
provisioning zone infrastructure in all available zones, but since
openshift/installer@644f705286 (data/aws/vpc: Only create subnet
infrastucture for zones with Machine(Set)s, 2019-03-27,
openshift/installer#1481) users who explicitly set zones in their
install-config will no longer have unused zones provisioned with
subnets, NAT gateways, EIPs, and other related infrastructure.  This
infrastructure reduction has two benefits in CI:

1. We don't have to pay for resources that we won't use, and we will
   have more room under our EIP limits (although we haven't bumped
   into that one in a while, because we're VPC-constained).

2. We should see reduced rates in clusters failing install because of
   AWS rate limiting, with results like [1]:

     aws_route.to_nat_gw.3: Error creating route: timeout while waiting for state to become 'success' (timeout: 2m0s)

   The reduction is because:

   i. We'll be making fewer requests for these resources, because we
      won't need to create (and subsequently tear down) as many of
      them.  This will reduce our overall AWS-API load somewhat,
      although the reduction will be incremental because we have so
      many other resources which are not associated with zones.

   ii. Throttling for these per-zone resources are the ones that tend
       to break Terraform [2].  So even if the rate of timeouts
       per-API request remains unchanged, a given cluster will only
       have half as many (three vs. the old six) per-zone chances of
       hitting one of the timeouts.  This should give us something
       close to a 50% reduction in clusters hitting throttling
       timeouts.

The drawback is that we're diverging further from the stock "I just
called 'openshift-install create cluster' without providing an
install-config.yaml" experience.

[1]: https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_console-operator/187/pull-ci-openshift-console-operator-master-e2e-aws-operator/575/artifacts/e2e-aws-operator/installer/.openshift_install.log
[2]: With a cache of build-log.txt from the past ~48 hours:

     $ grep -hr 'timeout while waiting for state' ~/.cache/openshift-deck-build-logs >timeouts
     $ wc -l timeouts
     362 timeouts
     $ grep aws_route_table_association timeouts | wc -l
     214
     $ grep 'aws_route\.to_nat_gw' timeouts | wc -l
     102

     So (102+214)/362 is 87% of our timeouts, with the remainder being
     almost entirely related to the internet gateway (which is not
     per-zone).
@wking wking force-pushed the explicit-aws-availability-zones branch from 7ea560e to 51c4a37 Compare March 28, 2019 19:45
@wking
Copy link
Member Author

wking commented Mar 28, 2019

If we wanted to slant even more strongly in favor of "less AWS throttling" at the expense of "approximating the default install", we could reduce to one or two zones. Personally, I'd rather see how the three-zone approach goes over first before deciding to do anything that drastic ;).

@sdodson
Copy link
Member

sdodson commented Mar 28, 2019

Yeah, I was going to propose that for the bulk of our CI jobs we only use two zones and for repos we think zones are more relevant we'd use three. I imagine some tests will likely fail if we went with only one zone.
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sdodson, 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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 28, 2019
@openshift-merge-robot openshift-merge-robot merged commit de5cab6 into openshift:master Mar 28, 2019
@openshift-ci-robot
Copy link
Contributor

@wking: Updated the following 8 configmaps:

  • prow-job-cluster-launch-e2e-40 configmap in namespace ci-stg using the following files:
    • key cluster-launch-e2e-40.yaml using file ci-operator/templates/openshift/openshift-ansible/cluster-launch-e2e-40.yaml
  • prow-job-cluster-scaleup-e2e-40 configmap in namespace ci using the following files:
    • key cluster-scaleup-e2e-40.yaml using file ci-operator/templates/openshift/openshift-ansible/cluster-scaleup-e2e-40.yaml
  • prow-job-cluster-scaleup-e2e-40 configmap in namespace ci-stg using the following files:
    • key cluster-scaleup-e2e-40.yaml using file ci-operator/templates/openshift/openshift-ansible/cluster-scaleup-e2e-40.yaml
  • prow-job-cluster-launch-installer-e2e configmap in namespace ci using the following files:
    • key cluster-launch-installer-e2e.yaml using file ci-operator/templates/openshift/installer/cluster-launch-installer-e2e.yaml
  • prow-job-cluster-launch-installer-e2e configmap in namespace ci-stg using the following files:
    • key cluster-launch-installer-e2e.yaml using file ci-operator/templates/openshift/installer/cluster-launch-installer-e2e.yaml
  • prow-job-cluster-launch-installer-src configmap in namespace ci using the following files:
    • key cluster-launch-installer-src.yaml using file ci-operator/templates/openshift/installer/cluster-launch-installer-src.yaml
  • prow-job-cluster-launch-installer-src configmap in namespace ci-stg using the following files:
    • key cluster-launch-installer-src.yaml using file ci-operator/templates/openshift/installer/cluster-launch-installer-src.yaml
  • prow-job-cluster-launch-e2e-40 configmap in namespace ci using the following files:
    • key cluster-launch-e2e-40.yaml using file ci-operator/templates/openshift/openshift-ansible/cluster-launch-e2e-40.yaml

In response to this:

This is very similar to the earlier e8921c3 (#3204). This time, however, I'm not changing the zones where the machines will run. By default, the installer will provisioning zone infrastructure in all available zones, but since openshift/installer@644f705286 (openshift/installer#1481) users who explicitly set zones in their install-config will no longer have unused zones provisioned with subnets, NAT gateways, EIPs, and other related infrastructure. This infrastructure reduction has two benefits in CI:

  1. We don't have to pay for resources that we won't use, and we will have more room under our EIP limits (although we haven't bumped into that one in a while, because we're VPC-constained).

  2. We should see reduced rates in clusters failing install because of AWS rate limiting, with results like:

    aws_route.to_nat_gw.3: Error creating route: timeout while waiting for state to become 'success' (timeout: 2m0s)
    

    The reduction is because:

    1. We'll be making fewer requests for these resources, because we won't need to create (and subsequently tear down) as many of them. This will reduce our overall AWS-API load somewhat, although the reduction will be incremental because we have so many other resources which are not associated with zones.

    2. Throttling for these per-zone resources are the ones that tend to break Terraform (1). So even if the rate of timeouts per-API request remains unchanged, a given cluster will only have half as many (three vs. the old six) per-zone chances of hitting one of the timeouts. This should give us something close to a 50% reduction in clusters hitting throttling timeouts.

The drawback is that we're diverging further from the stock "I just called openshift-install create cluster without providing an install-config.yaml" experience. openshift/installer#1487 is up for folks who wnt to weigh in on changing the installer default to only provision per-zone resources (and MachineSets) in zones that will receive install-time machines.

(1): With a cache of build-log.txt from the past ~48 hours:

$ grep -hr 'timeout while waiting for state' ~/.cache/openshift-deck-build-logs >timeouts
$ wc -l timeouts
362 timeouts
$ grep aws_route_table_association timeouts | wc -l
214
$ grep 'aws_route\.to_nat_gw' timeouts | wc -l
102

So (102+214)/362 is 87% of our timeouts, with the remainder being almost entirely related to the internet gateway (which is not per-zone).

CC @abhinavdahiya, @sdodson, @smarterclayton, @vrutkovs

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@wking wking deleted the explicit-aws-availability-zones branch March 28, 2019 21:17
wking added a commit to wking/openshift-release that referenced this pull request Mar 29, 2019
To help measure changes, like the smaller-than-expected drop in
CreateNatGateway after [1] landed on 2019-03-28T21:14 UTC.  You can
see the step down in the plot, but it's not a step down from six to
three, for reasons that are not yet clear to me.

[1]: openshift#3285
wking added a commit to wking/openshift-release that referenced this pull request Apr 8, 2021
This was originally part of avoiding broken zones, see e8921c3
(ci-operator/templates/openshift: Get e2e-aws out of us-east-1b,
2019-03-22, openshift#3204) and b717933
(ci-operator/templates/openshift/installer/cluster-launch-installer-*:
Random AWS regions for IPI, 2020-01-23, openshift#6833).  But the installer has
had broken-zone avoidence since way back in
openshift/installer@71aef620b6 (pkg/asset/machines/aws: Only return
available zones, 2019-02-07, openshift/installer#1210) I dunno how
reliably AWS sets 'state: impaired' and similar; it didn't seem to
protect us from e8921c3.  But we're getting ready to pivot to using
multiple AWS accounts, which creates two issues with hard-coding
region names in the step:

1. References by name are not stable between accounts.  From the AWS
   docs [1]:

     To ensure that resources are distributed across the Availability
     Zones for a Region, we independently map Availability Zones to
     names for each AWS account. For example, the Availability Zone
     us-east-1a for your AWS account might not be the same location as
     us-east-1a for another AWS account.

   So "aah, us-east-1a is broken, let's use b and c instead" might
   apply to one account but not the other.  And the installer does not
   currently accept zone IDs.

2. References by name may not exist in other accounts.  From the AWS
   docs [1]:

     As Availability Zones grow over time, our ability to expand them
     can become constrained. If this happens, we might restrict you
     from launching an instance in a constrained Availability Zone
     unless you already have an instance in that Availability
     Zone. Eventually, we might also remove the constrained
     Availability Zone from the list of Availability Zones for new
     accounts. Therefore, your account might have a different number
     of available Availability Zones in a Region than another account.

   And it turns out that for some reason they sometimes don't name
   sequentially, e.g. our new account lacks us-west-1a:

     $ AWS_PROFILE=ci aws --region us-west-1 ec2 describe-availability-zones | jq -r '.AvailabilityZones[] | .ZoneName + " " + .ZoneId + " " + .State' | sort
     us-west-1a usw1-az3 available
     us-west-1b usw1-az1 available
     $ AWS_PROFILE=ci-2 aws --region us-west-1 ec2 describe-availability-zones | jq -r '.AvailabilityZones[] | .ZoneName + " " + .ZoneId + " " + .State' | sort
     us-west-1b usw1-az3 available
     us-west-1c usw1-az1 available

   I have no idea why they decided to do that, but we have to work
   with the world as it is ;).

Removing the us-east-1 overrides helps reduce our exposure, although
we are still vulnerable to (2) with the a/b default line.  We'll do
something about that in follow-up work.

Leaving the "which zones?" decision up to the installer would cause it
to try to set up each available zone, and that causes more API
contention and resource consumption than we want.  Background on that
in 51c4a37 (ci-operator/templates/openshift: Explicitly set AWS
availability zones, 2019-03-28, openshift#3285) and d87fffb
(ci-operator/templates/openshift: Drop us-east-1c, 2019-04-26, openshift#3615),
as well as the rejected/rotted-out [2].

[1]: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/using-regions-availability-zones.html
[2]: openshift/installer#1487
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants