Bug 1778882: pkg/cli/admin/release/extract_tools: Pass []replacements to copyAndReplace #198
Conversation
…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
openshift-ci-robot
added
the
bugzilla/invalid-bug
|
@openshift-cherrypick-robot: This pull request references Bugzilla bug 1775875, 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. |
openshift-ci-robot
added
the
size/XL
|
/retitle Bug 1778882: pkg/cli/admin/release/extract_tools: Pass []replacements to copyAndReplace |
openshift-ci-robot
changed the title
openshift-ci-robot
added
the
bugzilla/valid-bug
|
@openshift-cherrypick-robot: This pull request references Bugzilla bug 1778882, 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. |
openshift-ci-robot
removed
the
bugzilla/invalid-bug
|
@soltysh, here's the 4.3 backport to approve :) |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: openshift-cherrypick-robot, soltysh 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 |
openshift-ci-robot
added
the
approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@openshift-cherrypick-robot: All pull requests linked via external trackers have merged. Bugzilla bug 1778882 has been moved to the MODIFIED state. 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. |
This is an automated cherry-pick of #167
/assign wking