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
Bug 1773257: pkg/version: Prepare of extract-time oc version injection #2682
Bug 1773257: pkg/version: Prepare of extract-time oc version injection #2682
Conversation
Set a marker for oc to inject the release version 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 425e4ff release image quay.io/openshift-release-dev/ocp-release@sha256:bac62983757570b9b8f8bc84c740782984a255c16372b3e30cfc8b52c0a187b9 The actual oc injection logic is in [1]. [1]: openshift/oc#165
@wking: This pull request references Bugzilla bug 1773257, 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. In response to this:
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. |
/retest |
/hold |
Why the hold? |
…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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhixson74, wking 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:
Approvers can indicate their approval by writing |
…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
…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
|
@abhinavdahiya can we can the final decision here, @wking is asking for, currently oc users extracting installer are getting confusing message:
|
/lgtm |
/bugzilla refresh |
@wking: This pull request references Bugzilla bug 1773257, which is invalid:
Comment In response to this:
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. |
/bugzilla refresh |
@wking: This pull request references Bugzilla bug 1773257, which is valid. In response to this:
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. |
/cherrypick release-4.3 |
@wking: once the present PR merges, I will cherry-pick it on top of release-4.3 in a new PR and assign it to you. In response to this:
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. |
/retest Please review the full test history for this PR and help us cut down flakes. |
9 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@wking: The following test failed, say
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. |
@wking: new pull request created: #2907 In response to this:
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. |
/bugzilla refresh |
Set a marker for oc to inject the release version to avoid potential confusion from:
The actual oc injection logic is in openshift/oc#165.