Use install-configs when invoking installer #2353

crawford · 2018-12-11T01:00:07Z

The installer is moving away from environment variables. They were
originally added as a work-around for CI, but now that install-configs
can be read directly, these should be used instead.

~~I have not tested these changes!~~

cc @wking

ci-operator/jobs/openshift/installer/openshift-installer-master-presubmits.yaml

ci-operator/templates/openshift/installer/cluster-launch-installer-e2e.yaml

cluster/test-deploy/aws/install-config.yml

These variables were removed from the installer recently (in favor of dynamically generated console credentials) and have no effect.

crawford · 2018-12-11T22:26:40Z

Tested locally with AWS. This seems to work as expected.

michaelgugino

/lgtm

michaelgugino · 2018-12-11T22:39:15Z

/lgtm cancel

The installer is moving away from environment variables. They were originally added as a work-around for CI, but now that install-configs can be read directly, these should be used instead.

michaelgugino

/lgtm

openshift-ci-robot · 2018-12-11T22:55:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crawford, michaelgugino

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:

~~ci-operator/templates/openshift/installer/OWNERS~~ [crawford]
~~ci-operator/templates/openshift/openshift-ansible/OWNERS~~ [michaelgugino]

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 · 2018-12-11T22:57:46Z

@crawford: Updated the following 3 configmaps:

prow-job-cluster-launch-installer-e2e configmap using the following files:
- key cluster-launch-installer-e2e.yaml using file ci-operator/templates/openshift/installer/cluster-launch-installer-e2e.yaml
prow-job-cluster-launch-installer-src configmap using the following files:
- key cluster-launch-installer-src.yaml using file ci-operator/templates/openshift/installer/cluster-launch-installer-src.yaml
prow-job-cluster-launch-e2e-40 configmap using the following files:
- key cluster-launch-e2e-40.yaml using file ci-operator/templates/openshift/openshift-ansible/cluster-launch-e2e-40.yaml

In response to this:

The installer is moving away from environment variables. They were
originally added as a work-around for CI, but now that install-configs
can be read directly, these should be used instead.

~~I have not tested these changes!~~

cc @wking

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.

vrutkovs · 2018-12-12T08:55:28Z

ci-operator/templates/openshift/openshift-ansible/cluster-launch-e2e-40.yaml

        mkdir /tmp/artifacts/installer &&
        /bin/openshift-install version >/tmp/artifacts/installer/version

-        export _CI_ONLY_STAY_AWAY_OPENSHIFT_INSTALL_AWS_USER_TAGS="{\"expirationDate\": \"$(date -d '4 hours' --iso=minutes --utc)\"}"
-        /bin/openshift-install --dir=/tmp/artifacts/installer --log-level=debug create install-config
+        export CLUSTER_ID=$(uuidgen --random)


Why not use JOB_NAME_HASH? This would help us identify which resources are created by particular CI job

The cluster ID has to be a version 4 UUID. If JOB_NAME_HASH satisfies that constraint we can use it. Do you know if it does?

Ah, right, it doesn't. Seems this value would be logged in the container logs, so resource can be tracked down to the CI job

vrutkovs · 2018-12-12T10:40:18Z

ci-operator/templates/openshift/installer/cluster-launch-installer-e2e.yaml

-      - name: OPENSHIFT_INSTALL_PLATFORM
-        value: ${CLUSTER_TYPE}
-      - name: OPENSHIFT_INSTALL_AWS_REGION
-        value: us-east-1
      - name: OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE


This is now silently dropped, so CI would use pinned release instead of latest

No, that variable is still respected; even after my installer PR.

vrutkovs · 2018-12-12T10:44:00Z

ci-operator/templates/openshift/openshift-ansible/cluster-launch-e2e-40.yaml

+        else
+            echo "Unsupported cluster type '${CLUSTER_NAME}'"
+            exit 1
+        fi

        /bin/openshift-install --dir=/tmp/artifacts/installer --log-level=debug create ignition-configs &


ignition-configs would destroy install-config.yml - is this an installer bug? In any case in CI we should be backing install config since its now cannot be reconstructed using env vars

That is core to the design of the installer (so no, not a bug). Why is the install config needed after the Ignition Configs are generated?

I'd keep the backup to ensure that the all the vars got templated correctly, this is not required though

The installer's state file (.openshift_install_state.json) should be archived, so the install-config can be pulled out of there if we need to inspect it after the fact.

CI stores this file, but it doesn't seem to contain any config - https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_installer/861/pull-ci-openshift-installer-master-e2e-aws/2219/artifacts/e2e-aws/installer/.openshift_install_state.json - a bug in PR 861?

nevermind, it does - installconfig.InstallConfig

vrutkovs · 2018-12-12T10:46:02Z

ci-operator/templates/openshift/openshift-ansible/cluster-launch-e2e-40.yaml

+        pullSecret: |
+          ${PULL_SECRET}
+        sshKey: |
+          ${SSH_PUB_KEY}


This should be left blank for BYOR - public key is being injected via playbook

The change should first land in the installer, see openshift/installer#861

…SECRET and similar These are local variables; we don't need to leak them down into child processes. At least some of these were introduced in 4134347 (Use install-configs when invoking installer, 2018-12-10, openshift#2353). The motivation behind the old approach is not clear to me, but I suspect overly broad pattern-matching ;).

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 11, 2018

openshift-ci-robot requested review from mtnbikenc and sdodson December 11, 2018 01:00

crawford mentioned this pull request Dec 11, 2018

*: remove support for environment variables openshift/installer#861

Merged

wking reviewed Dec 11, 2018

View reviewed changes

ci-operator/jobs/openshift/installer/openshift-installer-master-presubmits.yaml Outdated Show resolved Hide resolved

wking reviewed Dec 11, 2018

View reviewed changes

ci-operator/templates/openshift/installer/cluster-launch-installer-e2e.yaml Outdated Show resolved Hide resolved

wking reviewed Dec 11, 2018

View reviewed changes

cluster/test-deploy/aws/install-config.yml Outdated Show resolved Hide resolved

Remove email and password environment variables

c84e9df

These variables were removed from the installer recently (in favor of dynamically generated console credentials) and have no effect.

michaelgugino approved these changes Dec 11, 2018

View reviewed changes

openshift-ci-robot assigned michaelgugino Dec 11, 2018

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 11, 2018

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 11, 2018

Use install-configs when invoking installer

4134347

The installer is moving away from environment variables. They were originally added as a work-around for CI, but now that install-configs can be read directly, these should be used instead.

michaelgugino approved these changes Dec 11, 2018

View reviewed changes

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

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 11, 2018

openshift-merge-robot merged commit a3a8187 into openshift:master Dec 11, 2018

crawford deleted the env-vars branch December 11, 2018 23:00

vrutkovs reviewed Dec 12, 2018

View reviewed changes

vrutkovs added a commit to vrutkovs/release that referenced this pull request Dec 12, 2018

Revert BYOR part of openshift#2353

5f396ad

The change should first land in the installer, see openshift/installer#861

crawford mentioned this pull request Dec 12, 2018

Revert BYOR part of #2353 #2364

Closed

crawford mentioned this pull request Jan 18, 2019

templates/openshift: grab bootstrap log on failure #2624

Closed

wking mentioned this pull request Apr 24, 2019

ci-operator/templates/openshift: Remove unnecessary exports for PULL_SECRET and similar #3586

Closed

Use install-configs when invoking installer #2353

Use install-configs when invoking installer #2353

Uh oh!

Conversation

crawford commented Dec 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

crawford commented Dec 11, 2018

Uh oh!

michaelgugino left a comment

Choose a reason for hiding this comment

Uh oh!

michaelgugino commented Dec 11, 2018

Uh oh!

michaelgugino left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Dec 11, 2018

Uh oh!

openshift-ci-robot commented Dec 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrutkovs Dec 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

crawford commented Dec 11, 2018 •

edited

Loading

vrutkovs Dec 12, 2018 •

edited

Loading