Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented Feb 5, 2019

Take advantage of openshift/origin@59dab63d (openshift/origin#21637) to avoid installer/update-payload mismatches. This does not address install-config.yaml/installer mismatches, but it's a step in the right direction.

Also:

  • Replace shell syntax highlighting with console, because these blocks have prompts. And switch from the traditionally-PS2 > to the traditionally-PS1 $ for those prompts.
  • Stop exporting variables that are not needed by subprocesses.
  • Drop OPENSHIFT_INSTALL_*. This information is provided via install-config.yaml, and the installer ignores the environment variables since openshift/installer@6be4c253 (*: remove support for environment variables installer#861).

There's an outstanding FIXME while I wait for guidance about who's job it is to pull the installer image out of the update payload. If we do that in generate.go, we need to vendor some not-really-designed-as-a-library origin code. If we do it in the launched container, we need both oc and podman (or some other way to actually run the discovered installer image).

Take advantage of openshift/origin@59dab63d (Temporarily force the
installer to be part of the payload, 2018-12-08,
openshift/origin#21637) to avoid installer/update-payload mismatches.
This does not address install-config.yaml/installer mismatches, but
it's a step in the right direction.

Also:

* Replace 'shell' syntax highlighting with 'console', because these
  blocks have prompts.  And switch from the traditionally-PS2 '>' to
  the traditionally-PS1 '$' for those prompts.
* Stop exporting variables that are not needed by subprocesses.
* Drop OPENSHIFT_INSTALL_*.  This information is provided via
  install-config.yaml, and the installer ignores the environment
  variables since openshift/installer@6be4c253 (*: remove support for
  environment variables, 2018-12-10, openshift/installer#861).

There's an outstanding FIXME while I wait for guidance about who's job
it is to pull the installer image out of the update payload.  If we do
that in generate.go, we need to vendor some
not-really-designed-as-a-library origin code.  If we do it in the
launched container, we need both oc and Podman (or some other way to
actually run the discovered installer image).
@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 5, 2019
Args: []string{"cp -v /bin/openshift-install /output && ls -la /output"},
// FIXME: should this code or the remote container find and execute the installer container?
// $ oc adm release info --image-for installer registry.svc.ci.openshift.org/openshift/origin-release:v4.0
// registry.svc.ci.openshift.org/openshift/origin-v4.0-2019-02-05-150550@sha256:33a38e5e93d29e283ca93da8dde370b792b128181deac4b7e0d3c78e473deccd
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 going to be a bit tricky, the extracting from a release image is a bit time consuming. That feels like it should not be in generate.go, which will soon be getting run more frequently on sync to make sure the install job looks like it should (if the cd changes it will be updated and restarted). We wouldn't want to repeatedly do this.

Can a container execute a container?

CC @csrwng

Copy link
Member Author

Choose a reason for hiding this comment

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

We wouldn't want to repeatedly do this

On this front, if wherever you're pulling it has a layer cache, this should just be "check the tag, and, good, I already have that manifest and an unpacked image filesystem" (like pull-always) unless the tag had shifted, and you'd want the re-pull then anyway. If we're willing to commit to not clobbering tags (e.g. not using the floating registry.svc.ci.openshift.org/openshift/origin-release:v4.0), then yeah, we could cache the resolved installer payload in Go.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can a container execute a container?

@dgoodwin not sure how it would be needed in this case

Copy link
Contributor

Choose a reason for hiding this comment

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

Referring to the comment above re a remote container finding and executing the installer container.

If we do it in controllers it could be slow, if we do it in a pod we're just doing a lookup for the install container to run another pod with.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a controller maintaining a configmap of release image to extracted installer image, for every release image that shows up in a clusterdeployment. Install job can't start until our item shows up there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would a release image be required now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Would a release image be required now?

No, I set the same OKD default master installer builds use. But those are garbage-collected after 72 hours, so you probably want to set an update payload (or change the default).

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2019
@openshift-ci-robot
Copy link

@wking: PR needs rebase.

Details

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

@wking: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/integration 6ccc51f link /test integration

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

@dgoodwin
Copy link
Contributor

Obsolete via #241

/close

@openshift-ci-robot
Copy link

@dgoodwin: Closed this PR.

Details

In response to this:

Obsolete via #241

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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.

4 participants