Skip to content

WIP: *: Extract installer from the target update payload #210

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

Closed
wants to merge 1 commit into from

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
Env: env,
Command: []string{"/bin/sh", "-c"},
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.

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.

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.

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