Bug 1715001: If extracted from payload, 'oc version' reports payload version #88

sallyom · 2019-09-10T12:42:44Z

This PR refactors oc adm release extract to report payload version for oc version.
In case of oc version -o yaml full output will print the version struct plus a new field releaseClientVersion: payloadversion

See output below.
make build w/ this PR checked out, then
with oc adm release extract --tools quay.io/sallyom/release:test -v=3 access a test release image that includes:

registry.svc.ci.openshift.org/ocp/release:4.3.0-0.ci-2019-10-07-053008
cli image built from this PR

/assign @soltysh

cc @wking @tnozicka @smarterclayton

openshift-ci-robot · 2019-09-10T12:42:46Z

@sallyom: This pull request references Bugzilla bug 1715001, which is invalid:

expected the bug to target the "4.2.0" release, but it targets "4.3.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1715001: If extracted from payload, 'oc|openshift-install version' reports payload version

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.

sallyom · 2019-09-10T12:47:52Z

output when extracting tools from release image w/ this PR's cli image and system:admin on a cluster
and using local oc built from this PR:

$ ./oc version
Client Version: 4.3.0-0.ci-2019-10-07-053008
Server Version: 4.3.0-0.ci-2019-10-07-053008
Kubernetes Version: v1.16.0-beta.2+3d9b587

full output:

$ ./oc version -o yaml
clientVersion:
  buildDate: "2019-10-07T12:16:08Z"
  compiler: gc
  gitCommit: 13215d7
  gitTreeState: clean
  gitVersion: v4.2.0-alpha.0-173-g13215d7
  goVersion: go1.12.9
  major: ""
  minor: ""
  platform: linux/amd64
openshiftVersion: 4.3.0-0.ci-2019-10-07-053008
releaseClientVersion: 4.3.0-0.ci-2019-10-07-053008
serverVersion:
  buildDate: "2019-10-06T01:13:02Z"
  compiler: gc
  gitCommit: 3d9b587
  gitTreeState: clean
  gitVersion: v1.16.0-beta.2+3d9b587
  goVersion: go1.12.5
  major: "1"
  minor: 16+
  platform: linux/amd64

oc built from this PR (not extracted):

$ oc version 
Client Version: v4.2.0-alpha.0-173-g13215d7
Server Version: 4.3.0-0.ci-2019-10-07-053008
Kubernetes Version: v1.16.0-beta.2+3d9b587

openshift-ci-robot · 2019-09-10T12:48:30Z

@sallyom: This pull request references Bugzilla bug 1715001, which is invalid:

expected the bug to target the "4.2.0" release, but it targets "4.3.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1715001: If extracted from payload, 'oc|openshift-install version' reports payload version

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 · 2019-09-10T12:48:58Z

@sallyom: This pull request references Bugzilla bug 1715001, which is invalid:

expected the bug to target the "4.2.0" release, but it targets "4.3.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1715001: If extracted from payload, 'oc|openshift-install version' reports payload version

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.

sallyom · 2019-09-10T13:45:39Z

/hold cancel

wking · 2019-09-10T14:03:09Z

gitVersion is misleading here, since it's not really gitCommit but it's payload string

This should be baked in at build time, because renaming a release doesn't change it. The only things that release renaming should change (which would require extract-time injection) are the release pullspec and release name.

wking · 2019-09-10T14:09:08Z

$ ./oc version
Client Version: 4.2.0-0.ci-2019-09-10-003223
...

Is this constructed on the fly from that version structure? Which property does the ci come from? I'd expected you'd need to inject both the pullspec and release name, but don't see the release name (which I'd expect to be used as an opaque string here) getting injected.

sallyom · 2019-09-10T16:53:12Z

Is this constructed on the fly from that version structure?

yes, it's from k8s.io/apimachinery/pkg/version.Info, reports code a binary was built from.

Which property does the ci come from?

I built the release using registry.svc.ci.openshift.org/ocp/release:4.2.0-0.ci-2019-09-10-003223 + the cli image from the ci-opxxx ns of ci cluster. oc adm release new --from-release ...

I'd expected you'd need to inject both the pullspec and release name, but don't see the release name (which I'd expect to be used as an opaque string here) getting injected.

not sure what you mean, oc version reports same version as
the Release Metadata: Version (bc I built from that)

openshift-ci-robot · 2019-09-10T20:01:35Z

@sallyom: This pull request references Bugzilla bug 1715001, which is invalid:

expected the bug to target the "4.2.0" release, but it targets "4.3.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1715001: If extracted from payload, 'oc|openshift-install version' reports payload version

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.

pkg/cli/admin/release/extract_tools.go

sallyom · 2019-09-10T20:40:54Z

gitVersion is misleading here, since it's not really gitCommit but it's payload string

This should be baked in at build time, because renaming a release doesn't change it. The only things that release renaming should change (which would require extract-time injection) are the release pullspec and release name.

I've updated this, no longer replaces the build-time field, adds a new field if/when extracting from payload.

soltysh

I agree with @wking comment, but oc is sufficient to have just the short version. Fix that and this is good to go.

pkg/cli/admin/release/extract_tools.go

openshift-ci-robot · 2019-09-11T20:44:54Z

@sallyom: This pull request references Bugzilla bug 1715001, which is invalid:

expected the bug to target the "4.2.0" release, but it targets "4.3.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1715001: If extracted from payload, 'oc|openshift-install version' reports payload version

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 · 2019-09-11T21:07:59Z

@sallyom: This pull request references Bugzilla bug 1715001, which is invalid:

expected the bug to target the "4.2.0" release, but it targets "4.3.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1715001: If extracted from payload, 'oc|openshift-install version' reports payload version

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 · 2019-09-12T13:14:56Z

@sallyom: This pull request references Bugzilla bug 1715001, which is invalid:

expected the bug to target the "4.2.0" release, but it targets "4.3.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1715001: If extracted from payload, 'oc version' reports payload version

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.

sallyom · 2019-09-23T19:51:06Z

/bugzilla refresh

openshift-ci-robot · 2019-09-23T19:51:12Z

@sallyom: This pull request references Bugzilla bug 1715001, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

/bugzilla refresh

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.

…report payload version

openshift-ci-robot · 2019-10-07T12:38:45Z

@sallyom: This pull request references Bugzilla bug 1715001, which is valid.

Details

In response to this:

Bug 1715001: If extracted from payload, 'oc version' reports payload version

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 · 2019-10-07T12:46:03Z

@sallyom: This pull request references Bugzilla bug 1715001, which is valid.

Details

In response to this:

Bug 1715001: If extracted from payload, 'oc version' reports payload version

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.

soltysh · 2019-10-07T19:30:20Z

pkg/cli/admin/release/extract_tools.go

 			var matched bool
-			matched, err = copyAndReplaceReleaseImage(w, r, 4*1024, exactReleaseImage)
+			errmsg := ""
+			switch target.Command {


Honestly, this switch could be reversed since only installer relies on full release info and everything else (default) should be using the other, but that can be fixed later on when we'll be extending this mechanism at some point.

soltysh · 2019-10-07T19:30:35Z

/lgtm
/approve

openshift-ci-robot · 2019-10-07T19:30:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sallyom, soltysh

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~~ [soltysh]

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 · 2019-10-07T21:30:51Z

@sallyom: All pull requests linked via external trackers have merged. Bugzilla bug 1715001 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1715001: If extracted from payload, 'oc version' reports payload version

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.

…aller We've been injecting the release version into oc since 13215d7 (Bug 1715001: when extracting tools from payload, 'oc version' should report payload version, 2019-09-10, openshift#88), but we want to inject it into the installer to to avoid potential confusion from: $ oc adm release extract --command=openshift-install quay.io/openshift-release-dev/ocp-release:4.2.7 $ ./openshift-install version ./openshift-install v4.2.5 built from commit 425e4ff0037487e32571258640b39f56d5ee5572 release image quay.io/openshift-release-dev/ocp-release@sha256:bac62983757570b9b8f8bc84c740782984a255c16372b3e30cfc8b52c0a187b9 This commit refactors the split helpers from f55426f (Bug 1763728: fix 'oc adm release extract' version injection, 2019-10-17, openshift#131) with a single helper that takes a replacement slice. I've also pushed the unmatched warning down into the replacement helper itself so we don't have to return a slice of replacements that did or did not match. Now that replacements are no longer baked in, I've made the unit test a bit more readable by dispensing with the fakeInput helpers and just hard-coding small input/output strings in the test-case definitions. There's also the "oc is overwriting its own ...RELEASE_VERSION_LOCATION..." constant issue, previously fixed by ded896d (Bug 1768516: fix extracted-oc adm extract oc, 2019-11-08, openshift#155). In this commit, I've avoidied that by using ! as the leading character in the marker constants and replacing it with a null byte when constructing replacements in extractCommand. With a single replacement per marker, we need our marker constant to never match; I dunno what the previous doubled constant was about (but see the len(value) point next for why the previous doubled marker worked at all). I've also fixed some additional bugs: * The previous implementation only used len(value) of the marker when searching the incoming bytes. Especially for short strings like version replacement, this meant we were only looking for five bytes for 4.2.7, which is \x00_REL. That's not very specific. It does not distinquish between \x00_RELEASE_IMAGE_LOCATION_\x00XX... and \x00_RELEASE_VERSION_LOCATION_\x00XX.... It doesn't even distinguish between the defaultVersionPadded and defaultVersionPrefix constants in pkg/version. With this commit, we search for the full marker, regardless of len(value). * The previous implementation had: nextOffset := end - len(marker) ... _, wErr := w.Write(buf[:end-nextOffset]) ... copy(buf[:nextOffset], buf[end-nextOffset:end]) offset = nextOffset That was problematic for a few reasons: * It didn't write much data. Substituting for nextOffset, we were writing to: end - nextOffset = end - (end - len(marker)) = len(marker) this makes the buffer size largely meaningless, and means lots of inefficient, small reads/writes. With this commit, we have a writeTo variable that is everything except the *last* len(marker). * It ignored the amound of data written. You can *want* to write 1k bytes but only actually write 50 bytes with a given Write call. That didn't happen often before; because of the previous list entry we were only attempting to write a hundred or so bytes at a time. But now that we are trying to write the bulk of the buffer size, it happens more often. With this commit, we keep attempting Write until it errors out on us or we finish pushing all the bytes we no longer need. Because 13215d7, f55426f, and ded896d all touched the version marker, this commit will mean that oc build with this commit and later will not inject version names into oc built before ded896d. But that's ok, because 4.2 binaries have no version markers (just a RELEASE_IMAGE_LOCATION constant for the installer's marker). And we haven't cut 4.3 yet. So with this commit and no future changes to the oc marker, we'll be able to inject the version name into all supported oc which are prepared to receive injected version names. In action extracting from [1]: $ go build -o oc-mine ./cmd/oc $ ./oc-mine adm release extract --command=oc quay.io/openshift-release-dev/ocp-release-nightly@sha256:502d184ac8742073cb3007a0ad9abe8672b0a2db37331cfbfb4cb1b564c893fd $ ./oc version --client Client Version: 4.3.0-0.nightly-2019-11-13-233341 [1]: https://mirror.openshift.com/pub/openshift-v4/clients/ocp-dev-preview/4.3.0-0.nightly-2019-11-13-233341/release.txt

…lace We've been injecting the release version into oc since 13215d7 (Bug 1715001: when extracting tools from payload, 'oc version' should report payload version, 2019-09-10, openshift#88). This commit refactors the split helpers from f55426f (Bug 1763728: fix 'oc adm release extract' version injection, 2019-10-17, openshift#131) with a single helper that takes a replacement slice. I've also pushed the unmatched warning down into the replacement helper itself so we don't have to return a slice of replacements that did or did not match. This sets the stage for injecting release version names into the installer as well, although I'm not doing that in this commit because Abhinav wants some time to look into the ecosystem around that [1]. Now that replacements are no longer baked in, I've made the unit test a bit more readable by dispensing with the fakeInput helpers and just hard-coding small input/output strings in the test-case definitions. There's also the "oc is overwriting its own ...RELEASE_VERSION_LOCATION..." constant issue, previously fixed by ded896d (Bug 1768516: fix extracted-oc adm extract oc, 2019-11-08, openshift#155). In this commit, I've avoidied that by using ! as the leading character in the marker constants and replacing it with a null byte when constructing replacements in extractCommand. With a single replacement per marker, we need our marker constant to never match; I dunno what the previous doubled constant was about (but see the len(value) point next for why the previous doubled marker worked at all). I've also fixed some additional bugs: * The previous implementation only used len(value) of the marker when searching the incoming bytes. Especially for short strings like version replacement, this meant we were only looking for five bytes for 4.2.7, which is \x00_REL. That's not very specific. It does not distinquish between \x00_RELEASE_IMAGE_LOCATION_\x00XX... and \x00_RELEASE_VERSION_LOCATION_\x00XX.... It doesn't even distinguish between the defaultVersionPadded and defaultVersionPrefix constants in pkg/version. With this commit, we search for the full marker, regardless of len(value). * The previous implementation had: nextOffset := end - len(marker) ... _, wErr := w.Write(buf[:end-nextOffset]) ... copy(buf[:nextOffset], buf[end-nextOffset:end]) offset = nextOffset That was problematic for a few reasons: * It didn't write much data. Substituting for nextOffset, we were writing to: end - nextOffset = end - (end - len(marker)) = len(marker) this makes the buffer size largely meaningless, and means lots of inefficient, small reads/writes. With this commit, we have a writeTo variable that is everything except the *last* len(marker). * It ignored the amound of data written. You can *want* to write 1k bytes but only actually write 50 bytes with a given Write call. That didn't happen often before; because of the previous list entry we were only attempting to write a hundred or so bytes at a time. But now that we are trying to write the bulk of the buffer size, it happens more often. With this commit, we keep attempting Write until it errors out on us or we finish pushing all the bytes we no longer need. Because 13215d7, f55426f, and ded896d all touched the version marker, this commit will mean that oc build with this commit and later will not inject version names into oc built before ded896d. But that's ok, because 4.2 binaries have no version markers (just a RELEASE_IMAGE_LOCATION constant for the installer's marker). And we haven't cut 4.3 yet. So with this commit and no future changes to the oc marker, we'll be able to inject the version name into all supported oc which are prepared to receive injected version names. In action extracting from [2]: $ go build -o oc-mine ./cmd/oc $ ./oc-mine adm release extract --command=oc quay.io/openshift-release-dev/ocp-release-nightly@sha256:502d184ac8742073cb3007a0ad9abe8672b0a2db37331cfbfb4cb1b564c893fd $ ./oc version --client Client Version: 4.3.0-0.nightly-2019-11-13-233341 [1]: openshift/installer#2682 (comment) [2]: https://mirror.openshift.com/pub/openshift-v4/clients/ocp-dev-preview/4.3.0-0.nightly-2019-11-13-233341/release.txt

…place We've been injecting the release version into oc since 13215d7 (Bug 1715001: when extracting tools from payload, 'oc version' should report payload version, 2019-09-10, openshift#88). This commit refactors the split helpers from f55426f (Bug 1763728: fix 'oc adm release extract' version injection, 2019-10-17, openshift#131) with a single helper that takes a replacement slice. I've also pushed the unmatched warning down into the replacement helper itself so we don't have to return a slice of replacements that did or did not match. This sets the stage for injecting release version names into the installer as well, although I'm not doing that in this commit because Abhinav wants some time to look into the ecosystem around that [1]. Now that replacements are no longer baked in, I've made the unit test a bit more readable by dispensing with the fakeInput helpers and just hard-coding small input/output strings in the test-case definitions. There's also the "oc is overwriting its own ...RELEASE_VERSION_LOCATION..." constant issue, previously fixed by ded896d (Bug 1768516: fix extracted-oc adm extract oc, 2019-11-08, openshift#155). In this commit, I've avoidied that by using ! as the leading character in the marker constants and replacing it with a null byte when constructing replacements in extractCommand. With a single replacement per marker, we need our marker constant to never match; I dunno what the previous doubled constant was about (but see the len(value) point next for why the previous doubled marker worked at all). I've also fixed some additional bugs: * The previous implementation only used len(value) of the marker when searching the incoming bytes. Especially for short strings like version replacement, this meant we were only looking for five bytes for 4.2.7, which is \x00_REL. That's not very specific. It does not distinquish between \x00_RELEASE_IMAGE_LOCATION_\x00XX... and \x00_RELEASE_VERSION_LOCATION_\x00XX.... It doesn't even distinguish between the defaultVersionPadded and defaultVersionPrefix constants in pkg/version. With this commit, we search for the full marker, regardless of len(value). * The previous implementation had: nextOffset := end - len(marker) ... _, wErr := w.Write(buf[:end-nextOffset]) ... copy(buf[:nextOffset], buf[end-nextOffset:end]) offset = nextOffset That was problematic for a few reasons: * It didn't write much data. Substituting for nextOffset, we were writing to: end - nextOffset = end - (end - len(marker)) = len(marker) this makes the buffer size largely meaningless, and means lots of inefficient, small reads/writes. With this commit, we have a writeTo variable that is everything except the *last* len(marker). * It ignored the amound of data written. You can *want* to write 1k bytes but only actually write 50 bytes with a given Write call. That didn't happen often before; because of the previous list entry we were only attempting to write a hundred or so bytes at a time. But now that we are trying to write the bulk of the buffer size, it happens more often. With this commit, we keep attempting Write until it errors out on us or we finish pushing all the bytes we no longer need. Because 13215d7, f55426f, and ded896d all touched the version marker, this commit will mean that oc build with this commit and later will not inject version names into oc built before ded896d. But that's ok, because 4.2 binaries have no version markers (just a RELEASE_IMAGE_LOCATION constant for the installer's marker). And we haven't cut 4.3 yet. So with this commit and no future changes to the oc marker, we'll be able to inject the version name into all supported oc which are prepared to receive injected version names. In action extracting from [2]: $ go build -o oc-mine ./cmd/oc $ ./oc-mine adm release extract --command=oc quay.io/openshift-release-dev/ocp-release-nightly@sha256:502d184ac8742073cb3007a0ad9abe8672b0a2db37331cfbfb4cb1b564c893fd $ ./oc version --client Client Version: 4.3.0-0.nightly-2019-11-13-233341 [1]: openshift/installer#2682 (comment) [2]: https://mirror.openshift.com/pub/openshift-v4/clients/ocp-dev-preview/4.3.0-0.nightly-2019-11-13-233341/release.txt

…place We've been injecting the release version into oc since 13215d7 (Bug 1715001: when extracting tools from payload, 'oc version' should report payload version, 2019-09-10, openshift#88). This commit refactors the split helpers from f55426f (Bug 1763728: fix 'oc adm release extract' version injection, 2019-10-17, openshift#131) with a single helper that takes a replacement slice. I've also pushed the unmatched warning down into the replacement helper itself so we don't have to return a slice of replacements that did or did not match. This sets the stage for injecting release version names into the installer as well, although I'm not doing that in this commit because Abhinav wants some time to look into the ecosystem around that [1]. Now that replacements are no longer baked in, I've made the unit test a bit more readable by dispensing with the fakeInput helpers and just hard-coding small input/output strings in the test-case definitions. There's also the "oc is overwriting its own ...RELEASE_VERSION_LOCATION..." constant issue, previously fixed by ded896d (Bug 1768516: fix extracted-oc adm extract oc, 2019-11-08, openshift#155). In this commit, I've avoidied that by using ! as the leading character in the marker constants and replacing it with a null byte when constructing replacements in extractCommand. With a single replacement per marker, we need our marker constant to never match; I dunno what the previous doubled constant was about (but see the len(value) point next for why the previous doubled marker worked at all). I've also fixed some additional bugs: * The previous implementation only used len(value) of the marker when searching the incoming bytes. Especially for short strings like version replacement, this meant we were only looking for five bytes for 4.2.7, which is \x00_REL. That's not very specific. It does not distinquish between \x00_RELEASE_IMAGE_LOCATION_\x00XX... and \x00_RELEASE_VERSION_LOCATION_\x00XX.... It doesn't even distinguish between the defaultVersionPadded and defaultVersionPrefix constants in pkg/version. With this commit, we search for the full marker, regardless of len(value). * The previous implementation had: nextOffset := end - len(marker) ... _, wErr := w.Write(buf[:end-nextOffset]) ... copy(buf[:nextOffset], buf[end-nextOffset:end]) offset = nextOffset That was problematic for a few reasons: * It didn't write much data. Substituting for nextOffset, we were writing to: end - nextOffset = end - (end - len(marker)) = len(marker) this makes the buffer size largely meaningless, and means lots of inefficient, small reads/writes. With this commit, we have a writeTo variable that is everything except the *last* len(marker). * It ignored the amound of data written. You can *want* to write 1k bytes but only actually write 50 bytes with a given Write call. That didn't happen often before; because of the previous list entry we were only attempting to write a hundred or so bytes at a time. But now that we are trying to write the bulk of the buffer size, it happens more often. With this commit, we keep attempting Write until it errors out on us or we finish pushing all the bytes we no longer need. Because 13215d7, f55426f, and ded896d all touched the version marker, this commit will mean that oc build with this commit and later will not inject version names into oc built before ded896d. But that's ok, because 4.2 binaries have no version markers (just a RELEASE_IMAGE_LOCATION constant for the installer's marker). And we haven't cut 4.3 yet. So with this commit and no future changes to the oc marker, we'll be able to inject the version name into all supported oc which are prepared to receive injected version names. The Fprintf warning follows package precedent. Clayton [2]: > In these commands warnings are printed with fprintf. > > Klog is for debugging and servers, not end users. In action extracting from [3]: $ go build -o oc-mine ./cmd/oc $ ./oc-mine adm release extract --command=oc quay.io/openshift-release-dev/ocp-release-nightly@sha256:502d184ac8742073cb3007a0ad9abe8672b0a2db37331cfbfb4cb1b564c893fd $ ./oc version --client Client Version: 4.3.0-0.nightly-2019-11-13-233341 [1]: openshift/installer#2682 (comment) [2]: openshift#167 (comment) [3]: https://mirror.openshift.com/pub/openshift-v4/clients/ocp-dev-preview/4.3.0-0.nightly-2019-11-13-233341/release.txt

…place We've been injecting the release version into oc since 13215d7 (Bug 1715001: when extracting tools from payload, 'oc version' should report payload version, 2019-09-10, openshift#88). This commit refactors the split helpers from f55426f (Bug 1763728: fix 'oc adm release extract' version injection, 2019-10-17, openshift#131) with a single helper that takes a replacement slice. I've also pushed the unmatched warning down into the replacement helper itself so we don't have to return a slice of replacements that did or did not match. This sets the stage for injecting release version names into the installer as well, although I'm not doing that in this commit because Abhinav wants some time to look into the ecosystem around that [1]. Now that replacements are no longer baked in, I've made the unit test a bit more readable by dispensing with the fakeInput helpers and just hard-coding small input/output strings in the test-case definitions. There's also the "oc is overwriting its own ...RELEASE_VERSION_LOCATION..." constant issue, previously fixed by ded896d (Bug 1768516: fix extracted-oc adm extract oc, 2019-11-08, openshift#155). In this commit, I've avoided that by using ! as the leading character in the marker constants and replacing it with a null byte when constructing replacements in extractCommand. With a single replacement per marker, we need our marker constant to never match; I dunno what the previous doubled constant was about (but see the len(value) point next for why the previous doubled marker worked at all). I've also fixed some additional bugs: * The previous implementation only used len(value) of the marker when searching the incoming bytes. Especially for short strings like version replacement, this meant we were only looking for five bytes for 4.2.7, which is \x00_REL. That's not very specific. It does not distinguish between \x00_RELEASE_IMAGE_LOCATION_\x00XX... and \x00_RELEASE_VERSION_LOCATION_\x00XX.... It doesn't even distinguish between the defaultVersionPadded and defaultVersionPrefix constants in pkg/version. With this commit, we search for the full marker, regardless of len(value). * The previous implementation had: nextOffset := end - len(marker) ... _, wErr := w.Write(buf[:end-nextOffset]) ... copy(buf[:nextOffset], buf[end-nextOffset:end]) offset = nextOffset That was problematic for a few reasons: * It didn't write much data. Substituting for nextOffset, we were writing to: end - nextOffset = end - (end - len(marker)) = len(marker) this makes the buffer size largely meaningless, and means lots of inefficient, small reads/writes. With this commit, we have a writeTo variable that is everything except the *last* len(marker). * It ignored the amount of data written. You can *want* to write 1k bytes but only actually write 50 bytes with a given Write call. That didn't happen often before; because of the previous list entry we were only attempting to write a hundred or so bytes at a time. But now that we are trying to write the bulk of the buffer size, it happens more often. With this commit, we keep attempting Write until it errors out on us or we finish pushing all the bytes we no longer need. Because 13215d7, f55426f, and ded896d all touched the version marker, this commit will mean that oc build with this commit and later will not inject version names into oc built before ded896d. But that's ok, because 4.2 binaries have no version markers (just a RELEASE_IMAGE_LOCATION constant for the installer's marker). And we haven't cut 4.3 yet. So with this commit and no future changes to the oc marker, we'll be able to inject the version name into all supported oc which are prepared to receive injected version names. The Fprintf warning follows package precedent. Clayton [2]: > In these commands warnings are printed with fprintf. > > Klog is for debugging and servers, not end users. In action extracting from [3]: $ go build -o oc-mine ./cmd/oc $ ./oc-mine adm release extract --command=oc quay.io/openshift-release-dev/ocp-release-nightly@sha256:502d184ac8742073cb3007a0ad9abe8672b0a2db37331cfbfb4cb1b564c893fd $ ./oc version --client Client Version: 4.3.0-0.nightly-2019-11-13-233341 [1]: openshift/installer#2682 (comment) [2]: openshift#167 (comment) [3]: https://mirror.openshift.com/pub/openshift-v4/clients/ocp-dev-preview/4.3.0-0.nightly-2019-11-13-233341/release.txt

…aller We've been injecting the release version into oc since 13215d7 (Bug 1715001: when extracting tools from payload, 'oc version' should report payload version, 2019-09-10, openshift#88), but we want to inject it into the installer to to avoid potential confusion from: $ oc adm release extract --command=openshift-install quay.io/openshift-release-dev/ocp-release:4.2.7 $ ./openshift-install version ./openshift-install v4.2.5 built from commit 425e4ff0037487e32571258640b39f56d5ee5572 release image quay.io/openshift-release-dev/ocp-release@sha256:bac62983757570b9b8f8bc84c740782984a255c16372b3e30cfc8b52c0a187b9

openshift-ci-robot assigned smarterclayton and soltysh Sep 10, 2019

openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Sep 10, 2019

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 10, 2019

openshift-ci-robot requested review from deads2k and smarterclayton September 10, 2019 12:43

sallyom force-pushed the oc-version-adm-extract branch from 9e4c596 to 0488318 Compare September 10, 2019 13:09

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 10, 2019

sallyom force-pushed the oc-version-adm-extract branch from 0488318 to 9e5e8b3 Compare September 10, 2019 13:15

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 10, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 10, 2019

sallyom commented Sep 10, 2019

View reviewed changes

pkg/cli/admin/release/extract_tools.go Outdated Show resolved Hide resolved

sallyom force-pushed the oc-version-adm-extract branch from 9e5e8b3 to d4253ff Compare September 10, 2019 20:38

soltysh suggested changes Sep 11, 2019

View reviewed changes

pkg/cli/admin/release/extract_tools.go Outdated Show resolved Hide resolved

pkg/cli/admin/release/extract_tools.go Outdated Show resolved Hide resolved

sallyom force-pushed the oc-version-adm-extract branch 2 times, most recently from 506645c to 05d5c6d Compare September 11, 2019 20:42

sallyom mentioned this pull request Sep 11, 2019

Update 'openshift-install version' to print shorter releaseName as well as the full image-digest openshift/installer#2345

Closed

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Sep 23, 2019

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Sep 23, 2019

Bug 1715001: when extracting tools from payload, 'oc version' should …

13215d7

…report payload version

sallyom force-pushed the oc-version-adm-extract branch from bb0a0d6 to 13215d7 Compare October 7, 2019 12:44

soltysh approved these changes Oct 7, 2019

View reviewed changes

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

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

openshift-merge-robot merged commit f990efc into openshift:master Oct 7, 2019

sallyom mentioned this pull request Oct 22, 2019

Bug 1763728: fix 'oc adm release extract' version injection #131

Merged

wking mentioned this pull request Nov 16, 2019

Bug 1773257: pkg/cli/admin/release/extract_tools: Inject release version into installer #165

Merged

fckbo mentioned this pull request Sep 11, 2020

oc version format inconsistent between external download and console download #556

Closed

Bug 1715001: If extracted from payload, 'oc version' reports payload version #88

Bug 1715001: If extracted from payload, 'oc version' reports payload version #88

Uh oh!

Conversation

sallyom commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Sep 10, 2019

Uh oh!

sallyom commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Sep 10, 2019

Uh oh!

openshift-ci-robot commented Sep 10, 2019

Uh oh!

sallyom commented Sep 10, 2019

Uh oh!

wking commented Sep 10, 2019

Uh oh!

wking commented Sep 10, 2019

Uh oh!

sallyom commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Sep 10, 2019

Uh oh!

Uh oh!

sallyom commented Sep 10, 2019

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

openshift-ci-robot commented Sep 11, 2019

Uh oh!

openshift-ci-robot commented Sep 11, 2019

Uh oh!

openshift-ci-robot commented Sep 12, 2019

Uh oh!

sallyom commented Sep 23, 2019

Uh oh!

openshift-ci-robot commented Sep 23, 2019

Uh oh!

openshift-ci-robot commented Oct 7, 2019

Uh oh!

openshift-ci-robot commented Oct 7, 2019

Uh oh!

soltysh Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

soltysh commented Oct 7, 2019

Uh oh!

openshift-ci-robot commented Oct 7, 2019

Uh oh!

openshift-ci-robot commented Oct 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sallyom commented Sep 10, 2019 •

edited

Loading

sallyom commented Sep 10, 2019 •

edited

Loading

sallyom commented Sep 10, 2019 •

edited

Loading