aws-test: Drop need for parameters, just use latest/ #312

jlebon · 2018-09-25T15:22:36Z

Get rid of the need to pass parameters to the aws-test job in
preparations for turning on keepUndefinedParameters=true (edit: false, not true!) in the
Jenkins instance.

We only every run the aws-test job from the cloud job, so we know at
that point that the latest output is in latest/. We can just resolve
that back to a specific versioned dir before proceeding.

This normalizes the aws-test job to take the exact same parameters as
the others, and makes it easy to e.g. run the job manually like the
others.

See thread for context.

(Untested)

dm0-

Do we care about the ability to rerun tests on an image, like when it fails due to flakes?

yuqi-zhang · 2018-09-25T15:41:33Z

Jenkinsfile.cloud

@@ -216,7 +216,7 @@ node(NODE) {
                string(credentialsId: params.AWS_CI_ACCOUNT, variable: 'AWS_CI_ACCOUNT'),
            ]) {
                sh """
-                    amijson=${dirpath}/aws-${AWS_REGION}-smoketested.json
+                    amijson=${dirpath}/aws-${AWS_REGION}.json


Hmm, we decided to add the smoketested tag since we run rhcos.basic tests in the cloud job before we generate the ami, thus the generated image should at least boot, can ssh, etc. I'm ok with changing it, but that's the original reasoning we had behind it.

Since we don't upload it at all if it doesn't pass the smoketest, I think I'm OK with omitting it. Kinda bikeshed though, I'm fine keeping it if someone feels strongly about it.

yuqi-zhang · 2018-09-25T15:43:24Z

Jenkinsfile.aws-test

                currentBuild.description = "version=${version} ami=${ami_intermediate}"
                sh """
                    # Do testing with intermediate aws image passed in by cloud job
-                    if ! kola -b rhcos -p aws --aws-type ${aws_type} --tapfile rhcos-aws.tap --aws-ami ${ami_intermediate} --aws-region ${AWS_REGION} -j ${NUM_VMS} run; then
+                    if ! kola -b rhcos -p aws --aws-type t2.small --tapfile rhcos-aws.tap --aws-ami ${ami_intermediate} --aws-region ${AWS_REGION} -j ${NUM_VMS} run; then


I think we wanted to keep the configurable for potential future use, but again I'm ok with changing it to be hard defined to simplify things

Ahh OK, in that case we can make it a proper parameter. Though until then, I'd rather not make define_properties() more complex until we need it?

jlebon · 2018-09-25T18:21:13Z

Do we care about the ability to rerun tests on an image, like when it fails due to flakes?

Hmm, not sure. If the test fails, we can just rerun it, right? Sure, the latest/ might've changed again, but that's the one we'd want to promote anyway.

cgwalters · 2018-09-25T19:21:46Z

Jenkinsfile.aws-test

    try {
    utils.inside_assembler_container("-v /srv:/srv") {
    stage("Sync In") {
        withCredentials([
            string(credentialsId: params.ARTIFACT_SERVER, variable: 'ARTIFACT_SERVER'),
            sshUserPrivateKey(credentialsId: params.ARTIFACT_SSH_CREDS_ID, keyFileVariable: 'KEY_FILE'),
        ]) {
+            dirpath = "${images}/cloud/latest"
+            sh "mkdir -p ${dirpath}"
+            utils.rsync_file_in(ARTIFACT_SERVER, KEY_FILE, "${dirpath}/meta.json")


Doing things like this is going to trigger the problems I was trying to solve with #211 right?

I think we could adapt it when we adapt the rest of this script, no? E.g. we could just rsync to the workspace instead of /srv. Or I might be misunderstanding what you mean.

This job wouldn't be too hard to rework without /srv given that we're only syncing tiny JSON files. Let's do that in a follow-up patch?

As is today it adds a risk that we'll clobber a meta.json built by the cloud job right?

We're only rsync'ing out to one location though, right? (${images}/aws-${AWS_REGION}-tested.json). And that file is owned by this job.

We're rsyncing in to the same path though.

Ohh sigh, yes I see it now. OK, will take a look at it tomorrow.

OK, whipped something up in ⬇️. The delta is now larger though... so I'll have to test this after all.

jlebon · 2018-09-26T17:56:23Z

Rebased to resolve conflicts! ⬆️

miabbott · 2018-09-26T20:32:19Z

I like these changes 😄 LGTM

jlebon · 2018-09-26T21:26:50Z

/hold

I found it misleading that the `cloud` job uploaded the AMI JSON metadata with `-smoketested.json` appended *before* it was even tested. Let's make this clearer by keeping that suffix off until it's actually true.

Get rid of the need to pass parameters to the `aws-test` job in preparations for turning on `keepUndefinedParameters=true` in the Jenkins instance. We only every run the `aws-test` job from the `cloud` job, so we know at that point that the latest output is in `latest/`. We can just resolve that back to a specific versioned dir before proceeding. This normalizes the `aws-test` job to take the exact same parameters as the others, and makes it easy to e.g. run the job manually like the others.

We shouldn't `rsync` to the shared `/srv` location. Just sync directly to our `WORKSPACE` to avoid races. See also openshift#211.

jlebon · 2018-09-27T13:47:23Z

OK, rebased and tested, modulo the bits that actually push out to AWS/artifact server!
/hold cancel

cgwalters · 2018-09-27T14:05:49Z

OK let's give it a try!

/lgtm

Minor regression from openshift#312. We no longer copy this file in `/srv` now, we upload it directly from our workspace.

cgwalters · 2018-10-10T12:55:49Z

This normalizes the aws-test job to take the exact same parameters as
the others, and makes it easy to e.g. run the job manually like the
others.

I had a question about this. We could instead add parameters to the test job, and if we wanted to retest, we'd use "rebuild last" instead of just "rebuild" which should pass the exact same parameters - right?

yuqi-zhang · 2018-10-10T13:29:35Z

I assume the key parameter is just the ami, then we would just revert back to passing in the ami as a parameter from another job?

jlebon · 2018-10-10T14:19:31Z

We could instead add parameters to the test job, and if we wanted to retest, we'd use "rebuild last" instead of just "rebuild" which should pass the exact same parameters - right?

Yeah, basically this whole PR tried to solve the need for parameters by completely nixing them. We could've also made them explicit parameters only for the aws job at the expense of slightly complicating define_properties().

openshift-ci-robot requested review from dm0- and yuqi-zhang September 25, 2018 15:22

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 25, 2018

dm0- reviewed Sep 25, 2018

View reviewed changes

yuqi-zhang reviewed Sep 25, 2018

View reviewed changes

dm0- approved these changes Sep 25, 2018

View reviewed changes

cgwalters reviewed Sep 25, 2018

View reviewed changes

jlebon force-pushed the pr/undefined-params branch from a4e3734 to 4901a51 Compare September 26, 2018 17:56

miabbott mentioned this pull request Sep 26, 2018

adopt buildmaster and alpha tags for oscontainer #308

Merged

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 26, 2018

jlebon added 3 commits September 27, 2018 09:44

cloud: Don't actually call it -tested.json until it is

3a0c39c

I found it misleading that the `cloud` job uploaded the AMI JSON metadata with `-smoketested.json` appended *before* it was even tested. Let's make this clearer by keeping that suffix off until it's actually true.

aws-test: Stop using /srv

a5a1f58

We shouldn't `rsync` to the shared `/srv` location. Just sync directly to our `WORKSPACE` to avoid races. See also openshift#211.

jlebon force-pushed the pr/undefined-params branch from 77a0490 to a5a1f58 Compare September 27, 2018 13:46

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 27, 2018

yuqi-zhang approved these changes Sep 27, 2018

View reviewed changes

openshift-ci-robot assigned cgwalters Sep 27, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2018

openshift-merge-robot merged commit 6588070 into openshift:master Sep 27, 2018

jlebon added a commit to jlebon/os that referenced this pull request Sep 27, 2018

aws-test: Fix JSON filename passed to aws

e458937

Minor regression from openshift#312. We no longer copy this file in `/srv` now, we upload it directly from our workspace.

jlebon mentioned this pull request Sep 27, 2018

aws-test: Fix JSON filename passed to aws #324

Merged

jlebon deleted the pr/undefined-params branch April 23, 2023 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aws-test: Drop need for parameters, just use latest/ #312

aws-test: Drop need for parameters, just use latest/ #312

jlebon commented Sep 25, 2018 •

edited

Loading

dm0- left a comment

yuqi-zhang Sep 25, 2018

jlebon Sep 25, 2018

yuqi-zhang Sep 25, 2018

jlebon Sep 25, 2018

jlebon commented Sep 25, 2018

cgwalters Sep 25, 2018

jlebon Sep 25, 2018

jlebon Sep 26, 2018

cgwalters Sep 26, 2018

jlebon Sep 26, 2018

cgwalters Sep 26, 2018

jlebon Sep 26, 2018

jlebon Sep 26, 2018

jlebon commented Sep 26, 2018

miabbott commented Sep 26, 2018

jlebon commented Sep 26, 2018

jlebon commented Sep 27, 2018

cgwalters commented Sep 27, 2018

cgwalters commented Oct 10, 2018

yuqi-zhang commented Oct 10, 2018

jlebon commented Oct 10, 2018

aws-test: Drop need for parameters, just use latest/ #312

aws-test: Drop need for parameters, just use latest/ #312

Conversation

jlebon commented Sep 25, 2018 • edited Loading

dm0- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon commented Sep 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon commented Sep 26, 2018

miabbott commented Sep 26, 2018

jlebon commented Sep 26, 2018

jlebon commented Sep 27, 2018

cgwalters commented Sep 27, 2018

cgwalters commented Oct 10, 2018

yuqi-zhang commented Oct 10, 2018

jlebon commented Oct 10, 2018

jlebon commented Sep 25, 2018 •

edited

Loading