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

openshift/install e2e-aws add run smoke tests #1317

Merged

Conversation

sallyom
Copy link
Contributor

@sallyom sallyom commented Aug 29, 2018

This PR depends on openshift/installer#223 for Dockerfiles

@bbguimaraes @stevekuznetsov
EDITED:
Ran this from local system, from openshift/ci-operator dir:
and I've placed 3 files in a local installer/secret dir: license, pull-secret, and .aws/credentials
Running this cmd:

./ci-operator -template /path/to/modified-for-local-dev/cluster-launch-installer-e2e.yaml \
 -config /path/to/release/ci-operator/config/openshift/installer/master.json \
 -secret-dir=/path/to/local/openshfit.yaml \
 -secret-dir=/local/path/installer/secret  \
 -git-ref=sallyom/installer@mybranch \
 -namespace=myns

And then, creating like so (noting here so I have record/don't forget):
1.oc create secret generic dev-cluster-profile --from-file=/local/path/to/aws/openshift.yaml -n myns
2.oc create secret -n myns generic cluster-secrets-aws --from-file=/path/to/secret/pull-secret --from-file=/path/to/secret/license --from-file=/path/to/secret/credentials -o yaml --dry-run | oc apply -n myns -f -
3. oc create -f release/cluster/ci/config/installer-origin-release-bazel.yaml -n openshift (tested in my ns)

I have tested this several times locally, all looks good, hoping it works same way w/in the presubmit job

@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 Aug 29, 2018
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2018
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 29, 2018
@sallyom sallyom force-pushed the openshift-installer-add-smoke branch from 790c36f to aafe668 Compare August 29, 2018 14:06
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2018
@@ -250,6 +255,12 @@ objects:
cp $KUBECONFIG /tmp/admin.kubeconfig
echo "Installation successful"

echo "Running smoke tests..."
# is /tmp/cluster where the installer source code is here?
/tmp/bazel build smoke_tests
Copy link
Contributor

Choose a reason for hiding this comment

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

you should be able to just bazel run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh good

@@ -16,6 +16,8 @@ parameters:
required: true
- name: IMAGE_CLI
required: true
- name: BAZEL_IMAGE
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this vary per job?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, it should not

- /usr/bin/bazel
- /tmp/shared/bazel

containers:
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems wrong

Copy link
Contributor Author

@sallyom sallyom Aug 30, 2018

Choose a reason for hiding this comment

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

WIP lol It was wrong.. I've updated (since I was kind of able to test locally) I've made the change to the correct template now

@sallyom sallyom force-pushed the openshift-installer-add-smoke branch from aafe668 to 63e0e75 Compare August 30, 2018 11:51
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 30, 2018
@@ -62,6 +62,8 @@ presubmits:
configMapKeyRef:
key: master.json
name: ci-operator-openshift-installer
- name: BAZEL_IMAGE
value: quay.io/coreos/tectonic-builder:bazel-v0.3
image: ci-operator:latest
Copy link
Contributor Author

@sallyom sallyom Aug 30, 2018

Choose a reason for hiding this comment

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

@stevekuznetsov This is only to get the bazel binary available, to run bazel build smoke_tests . If we can add that to the base image, we won't need this.

# Is /tmp/cluster where the installer source code is here?
cd /tmp/artifacts/installer
/tmp/bazel build smoke_tests
bazel-bin/tests/smoke/linux_amd64_stripped/go_default_test -cluster -test.v
Copy link
Contributor Author

@sallyom sallyom Aug 30, 2018

Choose a reason for hiding this comment

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

@stevekuznetsov I need to run above 2 lines from directory where openshift/installer source code is.. is this where it is?? I haven't gotten that far in my testing to find out...

Copy link
Contributor

Choose a reason for hiding this comment

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

You want a cluster-launch-installer-src template, look at cluster-launch-src for examples

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

@sallyom sallyom force-pushed the openshift-installer-add-smoke branch from 63e0e75 to 64d5368 Compare August 30, 2018 13:41
@@ -153,7 +153,8 @@ aws:
# To use Azure-provided DNS, `BaseDomain` should be set to `""`
# If using DNS records, ensure that `BaseDomain` is set to a properly configured external DNS zone.
# Instructions for configuring delegated domains for Azure DNS can be found here: https://docs.microsoft.com/en-us/azure/dns/dns-delegate-domain-azure-dns
baseDomain: origin-ci-int-aws.dev.rhcloud.com
#baseDomain: origin-ci-int-aws.dev.rhcloud.com
baseDomain: openshiftdemo.org

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stevekuznetsov This is for local testing using rh-dev account.. will change it back or put in an if_local_dev block or something

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the local dev bits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup done

@sallyom sallyom force-pushed the openshift-installer-add-smoke branch from a7afe39 to 806c4c7 Compare August 30, 2018 19:55
@openshift openshift deleted a comment from stevekuznetsov Aug 30, 2018
@sallyom sallyom force-pushed the openshift-installer-add-smoke branch 4 times, most recently from afec203 to f0ebf51 Compare September 3, 2018 12:10
@sallyom sallyom changed the title [WIP] openshift/install e2e-aws add run smoke tests openshift/install e2e-aws add run smoke tests Sep 3, 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 3, 2018
@sallyom sallyom force-pushed the openshift-installer-add-smoke branch from f0ebf51 to 169fc45 Compare September 3, 2018 12:27
echo "Starting installer smoke tests..."
export SMOKE_KUBECONFIG=${KUBECONFIG}
export SMOKE_MANIFEST_PATHS=/tmp/cluster/generated/manifests
# 3 masters/3 workers/1 bootstrap(if post remove etcd)
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 remove "(if post remove etcd)", since both this repo and the installer repo are past that point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, there was talk that the bootstrap instance would eventually not be included in the overall node count though? if it's terminated after the bootstrap is complete? we'll have to keep an eye on that number, it definitely shouldn't be hardcoded the way it is

Copy link
Member

Choose a reason for hiding this comment

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

...there was talk that the bootstrap instance would eventually not be included in the overall node count though?

I'm hoping that when that happens, we'll have terminated the bootstrap node by tge time the installer exits. We may need parallel release/installer PRs for that, but the etcd comment is stale ;).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, got it

export SMOKE_KUBECONFIG=${KUBECONFIG}
export SMOKE_MANIFEST_PATHS=/tmp/cluster/generated/manifests
# 3 masters/3 workers/1 bootstrap(if post remove etcd)
# TODO: Make this a param
Copy link
Member

Choose a reason for hiding this comment

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

What's this about? Are you suggesting calculating this from inputs.yaml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, or keeping it in a variable rather than hardcoded there

@sallyom sallyom force-pushed the openshift-installer-add-smoke branch from 169fc45 to b675583 Compare September 4, 2018 15:39
- |
#!/bin/bash
set -euo pipefail
mkdir /tmp/installer-src
Copy link
Member

Choose a reason for hiding this comment

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

nit: we should be able to drop this. cp can create the target directory on its own:

$ mkdir -p a/b
$ cd a
$ cp -vR . /tmp/installer-src
‘.’ -> ‘/tmp/installer-src’
‘./b’ -> ‘/tmp/installer-src/b’

Once we drop the mkdir, the only thing left is the cp call. So we should be able to drop the shell entirely, and go straight to cp:

command:
- cp
- -R
- .
- /tmp/installer-src

or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will do

# wait for source code to show up
if [[ ! -d /tmp/installer-src ]]; then
echo "Waiting for installer source code to show up ..."
sleep 10 & wait
Copy link
Member

Choose a reason for hiding this comment

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

There's previous discussion of source access here. But using parallel containers for the copy and Bazel tests seems pretty complicated. I'm fine with this approach to get this PR landed, but it would be nice if there was an easier way...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it would be nice! took me quite awhile to figure that out lol

echo "Waiting for installer source code to show up ..."
sleep 10 & wait
fi
export USER=bazel && which bazel
Copy link
Member

Choose a reason for hiding this comment

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

What's this which doing? Can we drop it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, it was also debugging, fixing..

mountPath: /tmp
- name: artifacts
# bazel needs to write to .cache
mountPath: /.cache
Copy link
Member

Choose a reason for hiding this comment

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

Does the cache need to be mounted into the container? We don't mount it here, and the tarball build seems to work. After a successful build, you're already copying the smoke-test binary out of its symlinked location into /tmp/smoke. I expect all other Bazel files could die with the container.

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, I can def delete the other files, meant to do that.. I was stumped with the .cache stuff, I'm not sure what's going on there, yea, our other job does not require that, I know! I kept getting Permission Denied, can't create .cache/bazel-something so creating a writable path for that solved the issue.. anyways, I'll clean up the other files we don't need...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cleaned up bazel-bin

Copy link
Member

Choose a reason for hiding this comment

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

I kept getting Permission Denied, can't create .cache/bazel-something so creating a writable path for that solved the issue...

Ah, probably because of the random UID Prow uses and the lack of $HOME. More on this issue in #1178 and #1185. Try something like this (and if you also go that route for USER, you can drop your internal export from the script).

export USER=bazel && which bazel
cd /tmp/installer-src
mkdir /tmp/output
/usr/bin/bazel --output_base=/tmp/output build "$@" smoke_tests
Copy link
Member

Choose a reason for hiding this comment

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

Can we drop the "${@}"? This script isn't going to be called with arguments, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, fixing

#!/bin/bash
set -euo pipefail
echo "ls -al /tmp/src-installer"
ls -al /tmp/installer-src
Copy link
Member

Choose a reason for hiding this comment

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

These two lines feel like debugging information that we can drop, now that you have the wait loop set up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, right. fixing..

@sallyom sallyom force-pushed the openshift-installer-add-smoke branch from e515cac to 04fdedc Compare September 4, 2018 17:19
@sallyom sallyom force-pushed the openshift-installer-add-smoke branch 3 times, most recently from 3776f5c to a1e585d Compare September 10, 2018 15:30
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 10, 2018
@sallyom sallyom force-pushed the openshift-installer-add-smoke branch from a1e585d to 199f2e0 Compare September 10, 2018 15:53
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 10, 2018
@smarterclayton
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sallyom, smarterclayton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2018
@openshift-merge-robot openshift-merge-robot merged commit 710cff0 into openshift:master Sep 10, 2018
@openshift-ci-robot
Copy link
Contributor

@sallyom: Updated the following 2 configmaps:

  • job-config configmap using the following files:
    • key openshift-installer-presubmits.yaml using file ci-operator/jobs/openshift/installer/openshift-installer-presubmits.yaml
  • ci-operator-openshift-installer configmap using the following files:
    • key master.yaml using file ci-operator/config/openshift/installer/master.yaml

In response to this:

This PR depends on openshift/installer#223 for Dockerfiles

@bbguimaraes @stevekuznetsov
EDITED:
Ran this from local system, from openshift/ci-operator dir:
and I've placed 3 files in a local installer/secret dir: license, pull-secret, and .aws/credentials
Running this cmd:

./ci-operator -template /path/to/modified-for-local-dev/cluster-launch-installer-e2e.yaml \
-config /path/to/release/ci-operator/config/openshift/installer/master.json \
-secret-dir=/path/to/local/openshfit.yaml \
-secret-dir=/local/path/installer/secret  \
-git-ref=sallyom/installer@mybranch \
-namespace=myns

And then, creating like so (noting here so I have record/don't forget):
1.oc create secret generic dev-cluster-profile --from-file=/local/path/to/aws/openshift.yaml -n myns
2.oc create secret -n myns generic cluster-secrets-aws --from-file=/path/to/secret/pull-secret --from-file=/path/to/secret/license --from-file=/path/to/secret/credentials -o yaml --dry-run | oc apply -n myns -f -
3. oc create -f release/cluster/ci/config/installer-origin-release-bazel.yaml -n openshift (tested in my ns)

I have tested this several times locally, all looks good, hoping it works same way w/in the presubmit job

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link
Contributor

@sallyom: The following updates succeeded:

  • /usr/bin/make applyTemplate WHAT=projects/origin-release/pipeline.yaml
    
    $ /usr/bin/make applyTemplate WHAT=projects/origin-release/pipeline.yaml
    oc process -f projects/origin-release/pipeline.yaml | oc apply -f -
    imagestream "origin-release" configured
    buildconfig "origin-release-golang-1.8" configured
    buildconfig "origin-release-golang-1.9" configured
    buildconfig "origin-release-golang-1.10" configured
    buildconfig "origin-release-golang-1.11" configured
    buildconfig "origin-release-bazel" created
    buildconfig "origin-release-nodejs-8" configured
    buildconfig "origin-release-nodejs-8-browser-tests" configured
    

In response to this:

This PR depends on openshift/installer#223 for Dockerfiles

@bbguimaraes @stevekuznetsov
EDITED:
Ran this from local system, from openshift/ci-operator dir:
and I've placed 3 files in a local installer/secret dir: license, pull-secret, and .aws/credentials
Running this cmd:

./ci-operator -template /path/to/modified-for-local-dev/cluster-launch-installer-e2e.yaml \
-config /path/to/release/ci-operator/config/openshift/installer/master.json \
-secret-dir=/path/to/local/openshfit.yaml \
-secret-dir=/local/path/installer/secret  \
-git-ref=sallyom/installer@mybranch \
-namespace=myns

And then, creating like so (noting here so I have record/don't forget):
1.oc create secret generic dev-cluster-profile --from-file=/local/path/to/aws/openshift.yaml -n myns
2.oc create secret -n myns generic cluster-secrets-aws --from-file=/path/to/secret/pull-secret --from-file=/path/to/secret/license --from-file=/path/to/secret/credentials -o yaml --dry-run | oc apply -n myns -f -
3. oc create -f release/cluster/ci/config/installer-origin-release-bazel.yaml -n openshift (tested in my ns)

I have tested this several times locally, all looks good, hoping it works same way w/in the presubmit job

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@wking
Copy link
Member

wking commented Sep 10, 2018

This is giving us errors like:

error: could not resolve inputs: could not determine inputs for step [input:base-smoke]: could not resolve base image: imagestreamtags.image.openshift.io "origin-release" not found

This is possibly because the origin-release image is not pushed to Quay?

@wking
Copy link
Member

wking commented Sep 10, 2018

imagestreamtags.image.openshift.io "origin-release" not found

@sallyom has filed #1428 working on this.

@wking
Copy link
Member

wking commented Sep 10, 2018

And also #1431.

@wking
Copy link
Member

wking commented Sep 10, 2018

And also #1433.

@wking
Copy link
Member

wking commented Sep 10, 2018

And also #1434.

wking added a commit to wking/openshift-release that referenced this pull request Sep 10, 2018
Like 81a9e69 (openshift-installer-presubmits: Normalize job names,
2018-09-04, openshift#1354), but for the new smoke-test job from 199f2e0
(openshift/install e2e-aws add run smoke tests, 2018-08-28, openshift#1317).
wking added a commit to wking/openshift-installer that referenced this pull request Sep 10, 2018
Since 199f2e07 (openshift/install e2e-aws add run smoke tests,
2018-08-28, openshift/release#1317) we've been running the smoke tests
on AWS via Prow, so we can drop our Jenkins bindings.  This commit
also drops Dockerfiles for some images which have been supplanted in
Prow by 5e94276 (add Dockerfiles for ci, 2018-09-06, openshift#223).
wking added a commit to wking/openshift-release that referenced this pull request Sep 11, 2018
…onfig

Catching up with 56600df (add configmap for e2e-aws-smoke,
2018-09-10, openshift#1434) and ultimately with 199f2e0 (openshift/install
e2e-aws add run smoke tests, 2018-08-28, openshift#1317).
derekhiggins pushed a commit to derekhiggins/release that referenced this pull request Oct 24, 2023
This is only needed in rhcos.sh for old versions which lack the
openshift-install coreos-print-stream-json option, and the
file moved in openshift/installer#5252
so we should only copy if the "old" location is detected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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
Development

Successfully merging this pull request may close these issues.

9 participants