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

GRPA-2847: Copy AMI for ROSA during RHCOS sync #2478

Merged
merged 1 commit into from Jan 18, 2021

Conversation

LorbusChris
Copy link
Contributor

No description provided.

@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 Jan 4, 2021
@LorbusChris
Copy link
Contributor Author

@ashcrow @jharrington22 PTAL

Copy link
Contributor

@ashcrow ashcrow left a comment

Choose a reason for hiding this comment

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

Look fine to me. One super minor nit which isn't worth doing if nothing else comes up. 👍

build-scripts/rosa-sync/rosa_sync.sh Show resolved Hide resolved
@@ -22,5 +22,6 @@ RUN yum install -y \
rhpkg \
&& yum clean all

RUN pip3 install -U koji tox twine requests>=2.20 setuptools wheel codecov rh-doozer rh-elliott rh-ocp-build-data-validator
# TODO: Clarify: Is this the image used to run rhcos_sync in prod?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will lift the hold on this PR once this question is addressed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Specifically your asking if the image created here gets used as the jenkins worker, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor

Choose a reason for hiding this comment

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

I've ping'd ART folks asking if they can confirm.

Copy link
Contributor

Choose a reason for hiding this comment

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

@LorbusChris based on the response from ART are we OK with moving this forward?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc'ing @thegreyd who's assigned to it :) I guess installing awscli in this Dockerfile isn't strictly required for now - should I remove it or leave it here?

Copy link
Contributor

@thegreyd thegreyd Jan 12, 2021

Choose a reason for hiding this comment

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

Hey @LorbusChris 👋 I think you can leave this out and instead add the dependency in the initialize func in the build.groovy file for the job commonlib.shell(script: "pip install awscli"), each of our job runs in a venv. Also version lock it if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🎉 thank you @thegreyd! Updated, PTAL :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good ✅

@LorbusChris LorbusChris force-pushed the rosa-ami-sync branch 2 times, most recently from a9a252f to 5a8d8ae Compare January 13, 2021 14:04
@LorbusChris LorbusChris changed the title WIP: GRPA-2847: Copy AMI for ROSA during RHCOS sync GRPA-2847: Copy AMI for ROSA during RHCOS sync Jan 13, 2021
@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 Jan 13, 2021
Copy link

@jharrington22 jharrington22 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2021
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2021
@LorbusChris
Copy link
Contributor Author

LorbusChris commented Jan 13, 2021

thank you @sosiouxme :)

@ashcrow
Copy link
Contributor

ashcrow commented Jan 14, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 14, 2021
@ashcrow
Copy link
Contributor

ashcrow commented Jan 14, 2021

@sosiouxme Looks like this requires an ART member to merge.

DRY_RUN_FLAG="--dry-run"
fi

ami=$(jq -r "[.amis[]|select(.name==\"us-east-1\")][0].hvm" "${META_JSON_PATH}")
Copy link

Choose a reason for hiding this comment

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

this should be jq -r "[.amis[\"us-east-1\"].hvm][0]" ... based on my testing with the latest rhcos.json file here: https://github.com/openshift/installer/blob/master/data/data/rhcos.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When running manually, the input for this shouldn't be an installer rhcos.json!

In Jenkins, the URL to a meta.json file like this is passed to the job: https://releases-rhcos-art.cloud.privileged.psi.redhat.com/storage/releases/rhcos-4.6/46.82.202101131942-0/x86_64/meta.json (browser at https://releases-rhcos-art.cloud.privileged.psi.redhat.com/)
It's downloaded by the job and that path is later passed to rosa_sync.sh - so when running manually, a meta.json must be referenced.

The meta.json may contain multiple amis for one single region, which is why I used the select pipe here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tested this script only with a manual dry run with a meta.json -- could you confirm that works as intended for you?

Choose a reason for hiding this comment

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

We need @jharrington22 to weigh in here as I've been instructed to use the rhcos.json file instead to manually copy to the stage/prod rosa accounts.
@jharrington22 - meta.json and rhcos.json are in different formats, what file do we want to use for the sync?

Copy link

Choose a reason for hiding this comment

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

@LorbusChris - I've confirmed it works with a meta.json manually and the script copied the latest ami to the production rosa account.
The meta.json used: https://releases-rhcos-art.cloud.privileged.psi.redhat.com/storage/releases/rhcos-4.6/46.82.202101150340-0/x86_64/meta.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you @dustman9000 for the confirmation, and my apologies to all involved for not raising awareness of the differences of formats here before.

The intended goal for this PR is to copy the AMI for each bootimage release over to ROSA.
It was decided that this pipeline (which updates the download links on the mirrors) is the best place to put it, since not all z-stream releases get a boot imagebump, but this particular pipeline is run for each bootimage that does get released. It's triggered manually by someone from ART who has a reference to the exact build (it receives a particular build's meta.json as input).

The rhcos.json in the installer repo is also derivative of a build's meta.json and one should be able to backtrace it to its integral (https://github.com/openshift/installer/blob/master/hack/update-rhcos-bootimage.py#L2).

@ashcrow
Copy link
Contributor

ashcrow commented Jan 18, 2021

Bump :-)

@jupierce jupierce merged commit e80e34e into openshift-eng:master Jan 18, 2021
@sosiouxme
Copy link
Contributor

FYI to ARTists, awscli needs to be installed for this to work. We don't exactly have a place to declare dependencies like this for the jobs (we should...). I installed it for the jenkins user on buildvm 1&2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants