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.

Sign up for GitHub

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

Jump to bottom

Copy tested AMI to more regions #330

Merged
merged 1 commit into from
Oct 2, 2018
Merged

Copy tested AMI to more regions #330

merged 1 commit into from
Oct 2, 2018

Conversation

cgwalters
Copy link
Member

Closes: #328

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 28, 2018
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 28, 2018
@cgwalters
Copy link
Member Author

Still playing with/testing this - it looks like we at least also need to replicate the launch permissions, and possibly the tags.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 28, 2018
@cgwalters
Copy link
Member Author

OK so I updated this to support copying tags and making the image public. Currently hitting:

$ env AWS_PROFILE=aws-ci ~/src/github/openshift/os/scripts/ami-copy-regions --source-region us-east-1 --source-image-id ami-01be9707253b2d46d --regions us-east-2,us-west-1 --copy-tags rhcos_tag,ostree_version --name rhcos_dev_WALTERS --out amis.json
Tags to copy: ['Key=rhcos_tag,Value=alpha', 'Key=ostree_version,Value=4.0.6225']
Uploading to: us-east-2
Complete, ImageId=ami-0a0d75593867c33a7
Copying tags...
Making public for launch

An error occurred (InvalidAMIID.Unavailable) when calling the ModifyImageAttribute operation: The AMI ID 'ami-0a0d75593867c33a7' is currently pending and may not be used for this operation

AFAICS, mantle has some code to retry/poll for GCE, but not for AWS? Googling for the error I see other people hitting it though, looks like at least...

Oh wait, hey there's https://docs.ansible.com/ansible/devel/modules/ec2_ami_copy_module.html#ec2-ami-copy-module

@cgwalters
Copy link
Member Author

cgwalters commented Sep 28, 2018 •
edited
Loading

Some more investigation turns up the handy aws ec2 wait image-available incantation which is obviously what we want. Updated ⬆️ (But still testing "locally", whee I'm leaking AMIs...)

@cgwalters cgwalters changed the title WIP: Copy tested AMI to more regions Copy tested AMI to more regions Sep 28, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 28, 2018
@cgwalters cgwalters force-pushed the upload-us-east-2 branch 2 times, most recently from ad1aa5e to 4cf21c8 Compare September 28, 2018 21:40
@cgwalters
Copy link
Member Author

OK, the script seems to work for me locally. I didn't test the Jenkinsfile changes.

Lifting WIP - but let's wait to merge till next Monday at the earliest.

@cgwalters
Copy link
Member Author

One thing I want to note about this is that the "AMI JSON" format intentionally matches the existing Container Linux schema.

yuqi-zhang
yuqi-zhang reviewed Oct 1, 2018
View reviewed changes
Jenkinsfile.aws-test Outdated
${WORKSPACE}/aws-${AWS_REGION}.json \
s3://${S3_PUBLIC_BUCKET}/aws-${AWS_REGION}-tested.json
# Upload the json files to a public location
for k in aws-${AWS_REGION}-tested.json aws-tested.json; do
Copy link
Contributor

Choose a reason for hiding this comment

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

We renamed aws-${AWS_REGION}-tested.json to aws-${AWS_REGION}.json in the workspace. It's only called aws-${AWS_REGION}-tested.json in the bucket and the artifact server.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK I rebased and fixed this ⬇️ , thanks for spotting it.

Jenkinsfile.aws-test Outdated
# Upload the json files to a public location
for k in aws-${AWS_REGION}-tested.json aws-tested.json; do
aws s3 cp --acl public-read ${WORKSPACE}/\${k} \
s3://${S3_PUBLIC_BUCKET}/\${k}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we're uploading them separately? I'm not against it, just thought it might be slightly confusing to have aws-us-test-1-tested.json for us-east-1 and aws-tested.json in a list format for us-east-2 and us-west-1, in case someone attempts to query, they would have to do different formatted queries for the 2 json files.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think my vote would be to delete the old file after ensuring that no one is using it.

Copy link
Contributor

Choose a reason for hiding this comment

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

As in remove aws-us-test-1-tested.json and have a list in aws-tested.json? I think that makes sense.

Jenkinsfile.aws-test Outdated
@@ -86,6 +99,7 @@ node(NODE) {
sshUserPrivateKey(credentialsId: params.ARTIFACT_SSH_CREDS_ID, keyFileVariable: 'KEY_FILE'),
]) {
utils.rsync_file_out_dest(ARTIFACT_SERVER, KEY_FILE, "${WORKSPACE}/aws-${AWS_REGION}.json", "${images}/aws-${AWS_REGION}-tested.json")
utils.rsync_file_out_dest(ARTIFACT_SERVER, KEY_FILE, "${WORKSPACE}/aws-tested.json", "${images}/aws-tested.json")
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thought as above

yuqi-zhang
yuqi-zhang approved these changes Oct 2, 2018
View reviewed changes
Copy link
Contributor

@yuqi-zhang yuqi-zhang left a comment

Choose a reason for hiding this comment

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

Let's give it a try, thanks for the fix!

@yuqi-zhang
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 2, 2018
@openshift-merge-robot openshift-merge-robot merged commit c610545 into openshift:master Oct 2, 2018
@cgwalters cgwalters mentioned this pull request Oct 10, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuqi-zhang yuqi-zhang yuqi-zhang approved these changes

@ashcrow ashcrow Awaiting requested review from ashcrow

@sdemos sdemos Awaiting requested review from sdemos

Assignees

@yuqi-zhang yuqi-zhang

Labels
lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cgwalters @yuqi-zhang @openshift-ci-robot @openshift-merge-robot