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

jenkins: Make RHCOS AMI Public #304

Merged
merged 2 commits into from
Sep 19, 2018
Merged

jenkins: Make RHCOS AMI Public #304

merged 2 commits into from
Sep 19, 2018

Conversation

kikisdeliveryservice
Copy link
Contributor

This is my first PR, so I'm a bit unsure about the format of the PR, etc... If you have any feedback let me know! :)

@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 18, 2018
@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 18, 2018
@kikisdeliveryservice kikisdeliveryservice changed the title WIP: jenkins/cloud: Make RHCOS AMI public WIP: jenkins/cloud: Make RHCOS AMI Public Sep 18, 2018
@miabbott
Copy link
Member

You are on the right track, but I think we should be making the AMI public after it passes our kola tests. That happens over here - https://github.com/openshift/os/blob/master/Jenkinsfile.aws-test#L61-L63

@kikisdeliveryservice
Copy link
Contributor Author

Thanks for the feedback @miabbott ! Will update & repush.

@ashcrow
Copy link
Member

ashcrow commented Sep 18, 2018

Good catch!

@kikisdeliveryservice kikisdeliveryservice changed the title WIP: jenkins/cloud: Make RHCOS AMI Public jenkins/cloud: Make RHCOS AMI Public Sep 18, 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 18, 2018
@kikisdeliveryservice kikisdeliveryservice changed the title jenkins/cloud: Make RHCOS AMI Public Make RHCOS AMI Public Sep 18, 2018
@kikisdeliveryservice kikisdeliveryservice changed the title Make RHCOS AMI Public jenkins: Make RHCOS AMI Public Sep 18, 2018
@@ -61,6 +61,9 @@ node(NODE) {
aws ec2 modify-image-attribute \
--image-id ${ami_intermediate} \
--launch-permission '{"Add":[{"UserId":"${AWS_CI_ACCOUNT}"}]}'
aws ec2 modify-image-attribute \
--image-id ${ami_intermediate} \
--launch-permission "Add=[{Group=all}]
Copy link
Member

Choose a reason for hiding this comment

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

We can drop the previous invocation of modify-image-attribute since now anyone can access right?

Copy link
Member

Choose a reason for hiding this comment

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

(Or alternatively think of this as just changing the argument to `--launch-permission)

Copy link
Member

Choose a reason for hiding this comment

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

Quoting looks a bit off -- I'm guessing it should be '{"Add":[{"Group":"all"}]}' ?

Copy link
Member

Choose a reason for hiding this comment

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

I agree. I think @jlebon has the right idea on the quoting.

--launch-permission "Add=[{Group=all}]

becomes

--launch-permission '{"Add":[{"Group":"all"}]}'

Copy link
Member

Choose a reason for hiding this comment

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

We can drop the previous invocation of modify-image-attribute since now anyone can access right?

Yup! We shouldn't need to do that any longer. @kikisdeliveryservice If you don't mind, add another commit in this PR to remove that section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @ashcrow @jlebon @cgwalters ! I'll make those changes and repush.

@ashcrow
Copy link
Member

ashcrow commented Sep 19, 2018

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 19, 2018
@kikisdeliveryservice
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 19, 2018
Copy link
Member

@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.

Looks good! Will defer to @jlebon, @miabbott, or @cgwalters on merge.

@miabbott
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2018
@openshift-merge-robot openshift-merge-robot merged commit cbf624c into openshift:master Sep 19, 2018
@kikisdeliveryservice kikisdeliveryservice deleted the public-rhcos-ami branch September 19, 2018 17:32
@kikisdeliveryservice kikisdeliveryservice restored the public-rhcos-ami branch September 19, 2018 17:32
@kikisdeliveryservice kikisdeliveryservice deleted the public-rhcos-ami branch September 19, 2018 17:35
wking added a commit to wking/openshift-installer that referenced this pull request Sep 21, 2018
We're pushing public AMIs since openshift/os@6dd20dc6 (jenkins: Make
RHCOS AMI Public, 2018-09-18, openshift/os#304).  There's still no
public analog to [1], so I'm just scraping this from metadata on
images available via the AWS API.  The analogous AWS command line
invocation is:

  $ AWS_DEFAULT_REGION=us-east-1 aws ec2 describe-images --filter 'Name=name,Values=rhcos*' --query 'sort_by(Images, &CreationDate)[-1].ImageId' --output text

with a few extra filters thrown in.  The full set of metadata on the
most recent current image is:

  $ AWS_DEFAULT_REGION=us-east-1 aws ec2 describe-images --filter 'Name=name,Values=rhcos*' --query 'sort_by(Images, &CreationDate)[-1]' --output json
  {
      "VirtualizationType": "hvm",
      "Description": "Red Hat CoreOS 4.0.5846 (c9a6bb48b837b5bcfeb9bd427be9a18b5bd75b6c57cb289245f211ff98b2a740)",
      "Hypervisor": "xen",
      "EnaSupport": true,
      "SriovNetSupport": "simple",
      "ImageId": "ami-08a5792a684330602",
      "State": "available",
      "BlockDeviceMappings": [
          {
              "DeviceName": "/dev/xvda",
              "Ebs": {
                  "Encrypted": false,
                  "DeleteOnTermination": true,
                  "VolumeType": "gp2",
                  "VolumeSize": 8,
                  "SnapshotId": "snap-00a45db4ad6173805"
              }
          },
          {
              "DeviceName": "/dev/xvdb",
              "VirtualName": "ephemeral0"
          }
      ],
      "Architecture": "x86_64",
      "ImageLocation": "531415883065/rhcos_dev_c9a6bb4-hvm",
      "RootDeviceType": "ebs",
      "OwnerId": "531415883065",
      "RootDeviceName": "/dev/xvda",
      "CreationDate": "2018-09-19T23:40:54.000Z",
      "Public": true,
      "ImageType": "machine",
      "Name": "rhcos_dev_c9a6bb4-hvm"
  }

That doesn't include the "tested" information, so there's still no
support for changing channels.  We'll need to wait for a public analog
of [1], which is blocked on getting stable, production hosting for the
release metadata.

I'd prefer to use JMESPath and server-side filtering in Go as well, to
only return the latest matching AMI.  But the AWS Go library doesn't
seem to support server-side filtering at the moment [2].  Docs for the
AWS Go APIs I'm using are in [3,4,5,6,7,8].

The filters I'm adding here are similar to those we used for Container
Linux before they were dropped in 702ee7b (*: Remove stale Container
Linux references, 2018-09-11, openshift#233).  I added a few more just to be
conservative (e.g. we don't want to match a pending or failed image,
so I require state to be available).

I haven't pushed the Context variables all the way up the stack yet,
so there are some context.TODO() entries.  The 30-second timeout keeps
us from hanging excessively when the caller lacks AWS credentials; the
error messages look like:

  failed to init cluster: failed to parse test config: failed to determine default AMI: NoCredentialProviders: no valid providers in chain. Deprecated.
    For verbose messaging see aws.Config.CredentialsChainVerboseErrors

You can test this error condition by removing the explicit AMI values
I've added to our fixtures in this commit and running:

  $ AWS_PROFILE=does-not-exist go test ./installer/pkg/...

[1]: http://aos-ostree.rhev-ci-vms.eng.rdu2.redhat.com/rhcos/images/aws-us-east-1-tested.json
[2]: aws/aws-sdk-go#2156
[3]: https://docs.aws.amazon.com/sdk-for-go/api/aws/session/#NewSessionWithOptions
[4]: https://docs.aws.amazon.com/sdk-for-go/api/aws/session/#Options
[5]: https://docs.aws.amazon.com/sdk-for-go/api/aws/session/#Must
[6]: https://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#New
[7]: https://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#EC2.DescribeImagesWithContext
[8]: https://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#DescribeImagesInput
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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants