Skip to content

Conversation

@ecordell
Copy link
Contributor

@ecordell ecordell commented Nov 5, 2018

Differences from cluster-launch-installer-e2e:

  • Uses LOCAL_IMAGE_SRC instead of IMAGE_TESTS
  • Removes gingko commands

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 5, 2018
@ecordell
Copy link
Contributor Author

ecordell commented Nov 5, 2018

/assign @bbguimaraes

Copy link
Contributor

@bbguimaraes bbguimaraes left a comment

Choose a reason for hiding this comment

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

If you diff the two openshift-ansible templates, there's this section:

--- ci-operator/templates/cluster-launch-e2e.yaml
+++ ci-operator/templates/cluster-launch-src.yaml@@ -57,11 +59,22 @@
       secret:
         secretName: ${JOB_NAME_SAFE}-cluster-profile

+    initContainers:
+    - name: cli
+      image: ${IMAGE_CLI}
+      volumeMounts:
+      - name: shared-tmp
+        mountPath: /tmp/shared
+      command:
+      - cp
+      - /usr/bin/oc
+      - /tmp/shared/oc
+
     containers:

     # Once admin.kubeconfig exists, executes shared tests

It is meant to copy the oc binary from the cli image to a shared directory to make it accessible to the test container, which uses the src image (i.e. the component image) and may not contain it.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing these changes: #2062

@ecordell ecordell force-pushed the installer-template-src branch from ec26e62 to 1668277 Compare November 5, 2018 18:20
@ecordell
Copy link
Contributor Author

ecordell commented Nov 5, 2018

@bbguimaraes fixed - must've based this off of an older version of -e2e, sorry about that.

Copy link
Contributor

@bbguimaraes bbguimaraes left a comment

Choose a reason for hiding this comment

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

A few other discrepancies that show up in the diffs.

Also, can you put the blank lines back? They generate unnecessary noise when comparing the two templates (or remove them from the other file too, I don't really care either way).

Copy link
Contributor

Choose a reason for hiding this comment

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

-- name: IMAGE_TESTS
+- name: LOCAL_IMAGE_SRC
+  required: true
+- name: IMAGE_CLI

IMAGE_CLI needs to be a parameter of the template or we won't get its value from ci-operator.

Copy link
Contributor

Choose a reason for hiding this comment

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

-        export PATH=/usr/libexec/origin:$PATH
+        export PATH=/tmp/shared:$PATH

The binary is placed in a different location in this template.

Copy link
Contributor

Choose a reason for hiding this comment

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

-        cp "$(which oc)" /tmp/shared/

That's already done by the init container.

Copy link
Contributor

Choose a reason for hiding this comment

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

-        mkdir -p /tmp/output
-        cd /tmp/output

We can't cd out of the repository's directory or ${TEST_COMMAND} will likely fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

-        trap 'rc=$?; if test "${rc}" -eq 0; then touch /tmp/setup-success; else touch /tmp/exit; fi; exit "${rc}"' EXIT
+        trap 'rc=$?; if test "${rc}" -ne 0; then touch /tmp/exit; fi; exit "${rc}"' EXIT

This one's still outstanding from #2062.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify - I should be checking /tmp/setup-success, right?

@ecordell ecordell force-pushed the installer-template-src branch from 1668277 to 3e4c08a Compare November 5, 2018 19:44
@ecordell
Copy link
Contributor Author

ecordell commented Nov 5, 2018

@bbguimaraes Addressed comments

Full diff of e2e to src:

15c15,17
< - name: IMAGE_TESTS
---
> - name: LOCAL_IMAGE_SRC 
>   required: true
> - name: IMAGE_CLI
60c62,71
< 
---
>     initContainers:
>     - name: cli
>       image: ${IMAGE_CLI}
>       volumeMounts:
>       - name: shared-tmp
>         mountPath: /tmp/shared
>       command:
>       - cp
>       - /usr/bin/oc
>       - /tmp/shared/oc
65c76
<       image: ${IMAGE_TESTS}
---
>       image: ${LOCAL_IMAGE_SRC}
92c103
<         export PATH=/usr/libexec/origin:$PATH
---
>         export PATH=/tmp/shared:$PATH
96,98c107
< 
<         cp "$(which oc)" /tmp/shared/
< 
---
>         
180,183c189
< 
<         mkdir -p /tmp/output
<         cd /tmp/output
< 
---
>         
193,211d198
<         # TODO: the test binary should really be a more structured command - most of these flags should be
<         #       autodetected from the running cluster.
<         # TODO: bump nodes up to 40 again
<         function run-tests() {
<           if [[ -n "${TEST_FOCUS:-}" ]]; then
<             ginkgo -v -noColor -nodes="${TEST_PARALLELISM:-30}" $( which extended.test ) -- \
<               -ginkgo.focus="${TEST_FOCUS}" -ginkgo.skip="${TEST_SKIP:-"\\[local\\]"}" \
<               -e2e-output-dir /tmp/artifacts -report-dir /tmp/artifacts/junit \
<               -test.timeout=2h ${PROVIDER_ARGS-} || rc=$?
<           fi
<           if [[ -n "${TEST_FOCUS_SERIAL:-}" ]]; then
<             ginkgo -v -noColor -nodes=1 $( which extended.test ) -- \
<               -ginkgo.focus="${TEST_FOCUS_SERIAL}" -ginkgo.skip="${TEST_SKIP_SERIAL:-"\\[local\\]"}" \
<               -e2e-output-dir /tmp/artifacts -report-dir /tmp/artifacts/junit/serial \
<               -test.timeout=2h ${PROVIDER_ARGS-} || rc=$?
<           fi
<           exit ${rc:-0}
<         }
< 

@ecordell ecordell force-pushed the installer-template-src branch from 3e4c08a to 2cc9f71 Compare November 5, 2018 19:47
@wking
Copy link
Member

wking commented Nov 5, 2018

With the small difference between e2e and src, is this something we could be controlling with an environment variable or some such to keep all the code in cluster-launch-installer-e2e.yaml?

@ecordell
Copy link
Contributor Author

ecordell commented Nov 5, 2018

@wking I don't use Templates enough to speak authoritatively, but I don't think there are good ways to conditionally swap which parameter gets used in one place. The main concern is swapping image from ${IMAGE_TESTS} to ${LOCAL_IMAGE_SRC}.

For this PR I'm just copying the pattern for pre-4.0 templates.

@bbguimaraes
Copy link
Contributor

@wking: as @ecordell said, the fundamental difference between the two templates is which image to use for the test container, which is not something that can be switched with an environment variable.

@ecordell: comparing that diff and the diff between the openshift-ansible templates, the only thing missing is:

export ARTIFACT_DIR=/tmp/artifacts

Add that and we should be done with this.

@ecordell ecordell force-pushed the installer-template-src branch from 2cc9f71 to 49ce38a Compare November 6, 2018 13:07
@ecordell
Copy link
Contributor Author

ecordell commented Nov 6, 2018

@bbguimaraes Added the ARTIFACT_DIR env var

@bbguimaraes
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bbguimaraes, ecordell

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

The pull request process is described here

Details 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 Nov 6, 2018
@openshift-merge-robot openshift-merge-robot merged commit 06db336 into openshift:master Nov 6, 2018
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.

5 participants