pkg/cli/admin/release: support extracting baremetal installer #57

stbenjam · 2019-08-14T01:06:14Z

The installer for baremetal is in a separate image as part of the
release, we need to provide awareness to the oc command so we can
actually extract the installer and replace the release image location.

A user would extract the baremetal installer like this:

oc adm release extract --registry-config <pull secret> --command=openshift-baremetal-install --to /tmp

stbenjam · 2019-08-14T01:09:58Z

@markmc I believe this addresses your concern in #52, but I'm not sure if renaming the binary causes an issue - should be able to test tomorrow.

I'm not sure how we would do it otherwise

markmc · 2019-08-14T11:28:55Z

Ah, ok - so the binary is extracted as openshift-baremetal-install

The only downside is that ultimately I guess we all hope this support will end up in the stock openshift-install binary, and openshift-baremetal-install going away would be an incompatible change - but again, the platform is experimental, so hopefully that's not a major concern and ok as a simple solution for now

stbenjam · 2019-08-14T11:38:37Z

@markmc If we would rather not put this code into oc and wait until we have a mechanism to include it in openshift-install's normal distribution method, we could just extract baremetal-installer directly in dev-scripts / KNI install-scripts:

#!/bin/bash
set -e

INSTALLER_IMAGE="registry.svc.ci.openshift.org/origin/4.2:baremetal-installer"
INSTALLER_CONTAINER=$(podman create $INSTALLER_IMAGE)
podman cp $INSTALLER_CONTAINER:/usr/bin/openshift-install .
podman rm $INSTALLER_CONTAINER

What do you think?

markmc · 2019-08-14T11:43:53Z

I'd prefer if it wasn't so hidden - should be able to get it easily with oc adm release extract IMO

Also oc image extract would be a better fallback than podman cp

stbenjam · 2019-08-14T11:50:42Z

I'd prefer if it wasn't so hidden - should be able to get it easily with oc adm release extract IMO

Also oc image extract would be a better fallback than podman cp

Ah I didn't realize that was part of oc. Thanks, that's a good fallback if this isn't accepted.

stbenjam · 2019-08-15T11:08:53Z

Also, even if we need the fallback, it wouldn't be as simple. oc is responsible for replacing the placeholder in the binary with the real release image location: https://github.com/openshift/oc/blob/master/pkg/cli/admin/release/extract_tools.go#L597

We would need to do something similar by hand.

stbenjam · 2019-08-15T11:09:00Z

/assign @smarterclayton

markmc · 2019-08-15T11:40:56Z

Also, even if we need the fallback, it wouldn't be as simple. oc is responsible for replacing the placeholder in the binary with the real release image location: https://github.com/openshift/oc/blob/master/pkg/cli/admin/release/extract_tools.go#L597

We would need to do something similar by hand.

Great point. Agree a fallback isn't really viable

smarterclayton · 2019-08-15T12:46:09Z

This command is used to extract most release tooling by default. If we don’t expect this to be part of GA in 4.2 I think it needs to be more hidden - Ie you need to ask for it explicitly with —command. Also, this command must work against older clusters, so we can’t add a new binary that would fail if used against 4.1.

I’d probably prefer image extract is used in 4.2 rather than adding to release extract. If you can make the case that users won’t be able to succeed with that command, then this PR will need to take the above changes into account.

stbenjam · 2019-08-15T14:22:36Z

This command is used to extract most release tooling by default. If we don’t expect this to be part of GA in 4.2 I think it needs to be more hidden - Ie you need to ask for it explicitly with —command. Also, this command must work against older clusters, so we can’t add a new binary that would fail if used against 4.1.

I’d probably prefer image extract is used in 4.2 rather than adding to release extract. If you can make the case that users won’t be able to succeed with that command, then this PR will need to take the above changes into account.

image extract doesn't really work for us, as the binary isn't patched with the release information. We wouldn't be able to directly consume the extracted image. I have updated this PR to exclude the baremetal installer unless explicitly asked for as a command.

I wouldn't expect this to change for 4.2, the earliest we could address the issues that would remove the need for a separate baremetal-installer is 4.3.

stbenjam · 2019-08-15T14:26:27Z

There is also a related bug I've fixed as a separate PR here: #59

oc conflates the Command with the basepath of the source binary in the imgae, so openshift-baremtal-installer is not extracted by that name.

pkg/cli/admin/release/extract_tools.go

smarterclayton · 2019-08-16T15:29:17Z

pkg/cli/admin/release/extract_tools.go

-			targets[i].AsArchive = true
-			targets[i].AsZip = targets[i].OS == "windows"
+		// Only non-Hidden targets will be included in an archive.
+		for _, target := range availableTargets {


The ideal outcome is:

oc adm release extract —tools

Doesn’t include it, but

oc adm release extract —command openshift-baremetal-installer

Extracts the binary

oc adm release extract —command openshift-baremetal-installer —command-os=*

Creates an archive.

Hence the suggestion for optional - the explicit request overrides optional, rather than the current behavior here with always excluding from archive.

stbenjam · 2019-08-16T17:37:02Z

@smarterclayton Ok, so I believe this should work as you expect - see below for some examples. Previously --command and --command-os='*' together did not extract into archive(s). I have fixed that. I also had to incorporate the changes from #59 to make this all work correctly.

$ ./oc adm release extract --registry-config /tmp/pull.json --command=openshift-baremetal-install --command-os='*' --to /tmp/foo registry.svc.ci.openshift.org/ocp/release:4.2
$ ls /tmp/foo; rm -rf /tmp/foo/*
openshift-baremetal-install-linux-4.2.0-0.nightly-2019-08-15-232721.tar.gz 
release.txt
sha256sum.txt

$ ./oc adm release extract --registry-config /tmp/pull.json --command=openshift-baremetal-install --to /tmp/foo registry.svc.ci.openshift.org/ocp/release:4.2
$ ls /tmp/foo; rm -rf /tmp/foo/*
openshift-baremetal-install

$ ./oc adm release extract --registry-config /tmp/pull.json --tools --to /tmp/foo registry.svc.ci.openshift.org/ocp/release:4.2
$ ls /tmp/foo; rm -rf /tmp/foo/*
openshift-client-linux-4.2.0-0.nightly-2019-08-15-232721.tar.gz
openshift-install-linux-4.2.0-0.nightly-2019-08-15-232721.tar.gz
release.txt
sha256sum.txt

$ ./oc adm release extract --registry-config /tmp/pull.json --tools --command-os='*' --to /tmp/foo registry.svc.ci.openshift.org/ocp/release:4.2
$ ls /tmp/foo; rm -rf /tmp/foo/*
openshift-client-linux-4.2.0-0.nightly-2019-08-15-232721.tar.gz
openshift-client-windows-4.2.0-0.nightly-2019-08-15-232721.zip
openshift-client-mac-4.2.0-0.nightly-2019-08-15-232721.tar.gz
openshift-install-linux-4.2.0-0.nightly-2019-08-15-232721.tar.gz
openshift-install-mac-4.2.0-0.nightly-2019-08-15-232721.tar.gz
release.txt
sha256sum.txt

$ ./oc adm release extract --registry-config /tmp/pull.json --command='oc' --command-os='*' --to /tmp/foo registry.svc.ci.openshift.org/ocp/release:4.2
$ ls /tmp/foo; rm -rf /tmp/foo/*
openshift-client-linux-4.2.0-0.nightly-2019-08-15-232721.tar.gz
openshift-client-windows-4.2.0-0.nightly-2019-08-15-232721.zip  
openshift-client-mac-4.2.0-0.nightly-2019-08-15-232721.tar.gz
release.txt
sha256sum.txt

stbenjam · 2019-08-16T18:05:02Z

And just for completeness, the behavior of master is broken:

$ ./oc adm release extract --registry-config /tmp/pull.json --command=openshift-install --command-os='*' --to /tmp/foo registry.svc.ci.openshift.org/ocp/release:4.2
$ ls /tmp/foo
openshift-install

smarterclayton · 2019-08-19T14:15:57Z

What are the contents of the archives from:

…

--command='oc' --command-os='*' ?

On Fri, Aug 16, 2019 at 8:05 PM Stephen Benjamin ***@***.***> wrote: And just for completeness, the behavior of master is: $ ./oc adm release extract --registry-config /tmp/pull.json --command=openshift-install --command-os='*' --to /tmp/foo registry.svc.ci.openshift.org/ocp/release:4.2 $ ls /tmp/foo openshift-install — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#57?email_source=notifications&email_token=AAI37J54DD3GZFVJSUZ6P53QE3T47A5CNFSM4ILQBYX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4PJ3KI#issuecomment-522100137>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAI37J4GJ4OM7KFECBLDVGDQE3T47ANCNFSM4ILQBYXQ> .

stbenjam · 2019-08-19T15:02:30Z

I think it looks ok, except for Windows, it's missing the exe suffix. Will fix that logic.

Archive:  openshift-client-windows-4.2.0-0.nightly-2019-08-19-113631.zip
  Length      Date    Time    Name
---------  ---------- -----   ----
     1024  08-07-2019 19:35   README.md
 74884096  08-07-2019 19:35   oc
---------                     -------

$ tar vtzf openshift-client-mac-4.2.0-0.nightly-2019-08-19-113631.tar.gz
-rw-r--r-- 0/0             954 2019-08-07 19:34 README.md
-rwxr-xr-x 0/0        85865432 2019-08-07 19:34 oc
hrwxr-xr-x 0/0               0 2019-08-07 19:34 kubectl link to oc

$ tar vtzf openshift-client-linux-4.2.0-0.nightly-2019-08-19-113631.tar.gz
-rw-r--r-- 0/0             954 2019-08-18 16:31 README.md
-rwxr-xr-x 0/0        74327960 2019-08-18 16:31 oc
hrwxr-xr-x 0/0               0 2019-08-18 16:31 kubectl link to oc

When extracting commands from a release, the command is typically the same as the source executable in the image, however if that is not the case - for example, in the case of the baremetal installer, oc does the wrong thing. If the command is `openshift-baremetal-installer` but it is extracted from `usr/bin/openshift-installer`, oc extracts the file as openshift-installer.

stbenjam · 2019-08-19T18:27:02Z

@smarterclayton Fixed

$ tar tfv openshift-client-linux-4.2.0-0.nightly-2019-08-19-161103.tar.gz 
-rw-r--r-- 0/0             954 2019-08-18 16:31 README.md
-rwxr-xr-x 0/0        74327960 2019-08-18 16:31 oc
hrwxr-xr-x 0/0               0 2019-08-18 16:31 kubectl link to oc

$ tar tfv openshift-client-mac-4.2.0-0.nightly-2019-08-19-161103.tar.gz 
-rw-r--r-- 0/0             954 2019-08-07 19:34 README.md
-rwxr-xr-x 0/0        85865432 2019-08-07 19:34 oc
hrwxr-xr-x 0/0               0 2019-08-07 19:34 kubectl link to oc

$ unzip -l openshift-client-windows-4.2.0-0.nightly-2019-08-19-161103.zip 
Archive:  openshift-client-windows-4.2.0-0.nightly-2019-08-19-161103.zip
  Length      Date    Time    Name
---------  ---------- -----   ----
     1024  08-07-2019 19:35   README.md
 74884096  08-07-2019 19:35   oc.exe
---------                     -------
 74885120                     2 files

smarterclayton · 2019-08-20T13:11:47Z

pkg/cli/admin/release/extract_tools.go

+		if len(command) > 0 && currentOS != "*" {
+			return fmt.Errorf("command %q does not support the operating system %q", o.Command, currentOS)
+		} else {
+			return fmt.Errorf("the supported commands are 'oc', and 'openshift-install'")


This error message is inaccurate now (there is no guarantee command has been specified). You should also exit early, and the message should retain its original form.

switch { case len(command) > 0 && currentOS != "*": case len(command) > 0: return fmt.Errorf("the supported commands are 'oc' and 'openshift-install'") default: return fmt.Errorf("no available commands") }

to replace this if body.

Ah good point. Updated.

Although, I assume the first case in your example above shouldn't fall through but return the command %q does not support... error?

yes, sorry, I meant to add a # add correct error here

When specifying --command-os='*' and --command, we should use an archive as there can be more than one result. This previously didn't work.

The installer for baremetal is in a separate image as part of the release, we need to provide awareness to the `oc` command so we can actually extract the installer. This also introduces the concept of an 'optional' command that is not extracted when using --tools.

stbenjam · 2019-08-21T11:21:33Z

/test e2e-aws

smarterclayton · 2019-08-21T12:33:02Z

/lgtm

We need to watch carefully after merge to see if this breaks any tooling (such as the archive downloading on openshift-release.svc.ci.openshift.org or ART release tooling generating). @sosiouxme @tbielawa @jwforres be aware

No plans to back port this to 4.1 so should have no impact on z stream, unless ART is accidentally using a 4.2 oc binary with 4.1 builds.

openshift-ci-robot · 2019-08-21T12:33:14Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [smarterclayton]

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

stbenjam · 2019-08-21T13:15:19Z

Looks like this already landed in 4.2.0-0.ci-2019-08-21-125453, and cautiously optimistic that everything looks OK.

https://openshift-release-artifacts.svc.ci.openshift.org/4.2.0-0.ci-2019-08-21-125453/ has the tools extracted correctly.

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 14, 2019

openshift-ci-robot requested review from mfojtik and smarterclayton August 14, 2019 01:06

stbenjam mentioned this pull request Aug 14, 2019

pkg/cli/admin/release: include baremetal-installer in release #52

Closed

stbenjam force-pushed the baremetal branch from ee2019d to a5edf30 Compare August 14, 2019 01:09

openshift-ci-robot assigned smarterclayton Aug 15, 2019

stbenjam force-pushed the baremetal branch from a5edf30 to bb602d7 Compare August 15, 2019 14:19

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 15, 2019

stbenjam mentioned this pull request Aug 15, 2019

pkg/cli/admin/release: extract to target command not source #59

Closed

smarterclayton suggested changes Aug 16, 2019

View reviewed changes

stbenjam force-pushed the baremetal branch from bb602d7 to 02f455a Compare August 16, 2019 17:34

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 16, 2019

stbenjam force-pushed the baremetal branch from 02f455a to c3a625f Compare August 16, 2019 17:36

stbenjam force-pushed the baremetal branch from c3a625f to 6a05e5e Compare August 19, 2019 18:26

smarterclayton reviewed Aug 20, 2019

View reviewed changes

stbenjam force-pushed the baremetal branch 2 times, most recently from 52e0659 to 581d645 Compare August 20, 2019 14:01

stbenjam added 2 commits August 20, 2019 10:07

pkg/cli/admin/release: use archive with --command-os='*'

60a4fc1

When specifying --command-os='*' and --command, we should use an archive as there can be more than one result. This previously didn't work.

stbenjam force-pushed the baremetal branch from 581d645 to 2449822 Compare August 20, 2019 14:07

markmc mentioned this pull request Aug 19, 2019

Start adding 03_create_cluster.sh. openshift-kni/install-scripts#35

Merged

stbenjam mentioned this pull request Aug 21, 2019

Update release-payload docs now that installers are included openshift-metal3/dev-scripts#757

Merged

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2019

stbenjam mentioned this pull request Aug 21, 2019

Make it easier to drop the release override. openshift-kni/install-scripts#36

Closed

openshift-merge-robot merged commit 38aad26 into openshift:master Aug 21, 2019

stbenjam deleted the baremetal branch August 21, 2019 12:48

stbenjam mentioned this pull request Aug 24, 2019

Include openshift-install binary with baremetal support in release image openshift/installer#2021

Closed

wking mentioned this pull request Nov 21, 2019

Bug 1774642: pkg/cli/admin/release/extract: Multi-arch extraction with --filter-by-os #172

Closed

pkg/cli/admin/release: support extracting baremetal installer #57

pkg/cli/admin/release: support extracting baremetal installer #57

Uh oh!

Conversation

stbenjam commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stbenjam commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markmc commented Aug 14, 2019

Uh oh!

stbenjam commented Aug 14, 2019

Uh oh!

markmc commented Aug 14, 2019

Uh oh!

stbenjam commented Aug 14, 2019

Uh oh!

stbenjam commented Aug 15, 2019

Uh oh!

stbenjam commented Aug 15, 2019

Uh oh!

markmc commented Aug 15, 2019

Uh oh!

smarterclayton commented Aug 15, 2019

Uh oh!

stbenjam commented Aug 15, 2019

Uh oh!

stbenjam commented Aug 15, 2019

Uh oh!

Uh oh!

smarterclayton Aug 16, 2019

Choose a reason for hiding this comment

Uh oh!

stbenjam commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stbenjam commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarterclayton commented Aug 19, 2019 via email

Uh oh!

stbenjam commented Aug 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stbenjam commented Aug 19, 2019

Uh oh!

smarterclayton Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

stbenjam Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

smarterclayton Aug 21, 2019

Choose a reason for hiding this comment

Uh oh!

stbenjam commented Aug 21, 2019

Uh oh!

smarterclayton commented Aug 21, 2019

Uh oh!

openshift-ci-robot commented Aug 21, 2019

Uh oh!

stbenjam commented Aug 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

stbenjam commented Aug 14, 2019 •

edited

Loading

stbenjam commented Aug 14, 2019 •

edited

Loading

stbenjam commented Aug 16, 2019 •

edited

Loading

stbenjam commented Aug 16, 2019 •

edited

Loading

stbenjam commented Aug 19, 2019 •

edited

Loading