Skip to content

Conversation

@dmc5179
Copy link
Contributor

@dmc5179 dmc5179 commented Jul 22, 2019

Add support for selecting the Availability Zone in AWS. Not all resources in AWS are available in each zone of a region. Current example being GPU instances. They are available in us-east1-a/c/f but not b. Without this setting it is generally random what Availability Zone the instances end up in.

…s that are not available in all zones within a region
Copy link
Owner

@jaredhocutt jaredhocutt left a comment

Choose a reason for hiding this comment

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

As you have it here, this forces people to choose a zone which isn't desirable. For most users, we want it to randomly choose a zone.

To fix this, anywhere you're using the aws_zone variable, use it like this:

zone: "{{ aws_zone | default(omit) }}"

- name: Delete EC2 instances
ec2_instance:
region: "{{ aws_region }}"
availability_zone: "{{ aws_zone }}"
Copy link
Owner

Choose a reason for hiding this comment

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

You shouldn't need to limit by availability zone here as it should still find the EC2 instances to delete. You can remove it.

filters:
tag:OpenShiftClusterName: "{{ cluster_name }}"
tag:OpenShiftClusterNameVerbose: "{{ cluster_name_verbose }}"
availability-zone: "{{ aws_zone }}"
Copy link
Owner

Choose a reason for hiding this comment

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

You shouldn't need to limit by availability zone here either.

@dmc5179
Copy link
Contributor Author

dmc5179 commented Jul 22, 2019

Make sense. I'm not a huge fan of the fact that git lets you manipulate history and I meant to add all of this to a branch in case I needed to make edits which is much easier to do in a PR with a branch. Do you mind if I close this PR, add the original changes, along with your modification requests, to that branch, and then create another PR from there? Sorry for the trouble. It has always bugged me that a revision control system like git lets you change history even when there is a mistake.

@jaredhocutt
Copy link
Owner

Just make the changes and add as a new commit on your master. It'll automatically update the PR for you and I can squash merge when I accept the PR.

… file. Make default AZ setting random. Remove AZ setting from tasks that do not require it
@dmc5179
Copy link
Contributor Author

dmc5179 commented Jul 22, 2019

OK. I made the changes:

  • Removed the default from the example var file so users will have to set it and hopefully not all choose the same
  • Added the variable and its description to the README file
  • Remove the AZ setting where it was not required as you mentioned
  • Updated the AZ setting to use either the user set value or omit the parameter which should result in a semi-random result for those tasks.

Copy link
Owner

@jaredhocutt jaredhocutt left a comment

Choose a reason for hiding this comment

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

LGTM

@jaredhocutt
Copy link
Owner

Can you run a test deployment by not setting aws_zone and ensure everything works as it always has? Once you confirm things are good, I'll merge the PR.

Thanks!

@dmc5179
Copy link
Contributor Author

dmc5179 commented Jul 22, 2019

Sure. I'm going to pull the repo fresh and just run it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants