Bug 1774642: Avoid hard links in cli-artifacts #171

yselkowitz · 2019-11-20T17:10:18Z

#153 introduced a regression because
oc adm release extract does not handle the hard links well.

openshift-ci-robot · 2019-11-20T17:10:33Z

Hi @yselkowitz. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

crawford · 2019-11-20T17:11:53Z

/ok-to-test

wking · 2019-11-20T17:13:00Z

images/cli-artifacts/Dockerfile.rhel

+COPY --from=builder /go/src/github.com/openshift/oc/_output/bin/linux_amd64/oc /usr/share/openshift/linux_amd64/oc
+COPY --from=builder /go/src/github.com/openshift/oc/_output/bin/linux_arm64/oc /usr/share/openshift/linux_arm64/oc
+COPY --from=builder /go/src/github.com/openshift/oc/_output/bin/linux_ppc64le/oc /usr/share/openshift/linux_ppc64le/oc
+COPY --from=builder /go/src/github.com/openshift/oc/_output/bin/linux_s390x/oc /usr/share/openshift/linux_s390x/oc


I prefer the previous approach, because with this one we still have inconsistent paths for with Windows and Mac binaries. Also, oc.exe for Windows, no? And isn't this doubling the amd64 Linux oc in the image?

The copy-once-and-link approach didn't work for oc adm release extract, as discussed on Slack. This goes back to my original proposal which should be safer.

Just fixed the .exe and repushed.

Yes, regardless of approach, there are duplicate oc binaries for the native arch, because cli-artifacts FROMs cli which includes a native /usr/bin/oc. If it is deemed safe to do so, perhaps cli-artifacts can inherit base instead, but I don't know for sure all the ways cli-artifacts are used. (base would be safe wrt the console-operator usage after my PR there lands, which it has in 4.3+ but not yet in 4.2).

@yselkowitz pointed out out of band that we do need to preserve non-hardlink files at the old locations for compat with existing 4.2.z oc. So that's a good reason to prefer a Dockerfile fix. But we can use hardlinks to allow the console downloads deployment to use the standardized paths. I think. I'm not actually sure how our Dockerfile builds work; if we get a separate tar layer for each of these commands we might have trouble setting up links...

With the CI release from this PR.

$ oc adm release info --image-for=cli-artifacts registry.svc.ci.openshift.org/ci-op-620kiff3/release:latest registry.svc.ci.openshift.org/ci-op-620kiff3/stable@sha256:a5593b42790df013921d8144351886dd6afecf99d558347c5195661714f74c79 $ podman pull registry.svc.ci.openshift.org/ci-op-620kiff3/stable@sha256:a5593b42790df013921d8144351886dd6afecf99d558347c5195661714f74c79 $ podman save --format oci-dir --output cli-artifacts registry.svc.ci.openshift.org/ci-op-620kiff3/stable@sha256:a5593b42790df013921d8144351886dd6afecf99d558347c5195661714f74c79 $ jq -r '.layers[].digest' cli-artifacts/manifest.json $ jq -r '.layers[].digest' cli-artifacts/manifest.json sha256:2cab4440f9070965c8454792eee4b122bce5939cc6d21e554469f01e71712a89 sha256:64f2bd9f473b0ae150f07dc65aafb90beeec7ebaa722625055cb8c1ddede0b09 sha256:bd48b9cc9f65a873bde992c97649ace2895af41307502429527f35f1bebda2c0 sha256:86e61694888a7971fcd711f84b1eff117844813adb4ed388d34bdb2d347fd33d sha256:51224f9c74f3f5df27f9f4a29e97832e6f8c19ea7856fe269dbf747a01cd6026 sha256:fa4f7b9bdc6c8e5a3e5eeb0bb79eaab791123b2ecc1b07f94ea06a1c40db3854 $ tar -tvf cli-artifacts/fa4f7b9bdc6c8e5a3e5eeb0bb79eaab791123b2ecc1b07f94ea06a1c40db3854 drwxr-xr-x 0/0 0 2019-10-08 05:41 usr/ drwxr-xr-x 0/0 0 2019-11-20 09:25 usr/share/ drwxr-xr-x 0/0 0 2019-11-20 09:25 usr/share/openshift/ -rwxr-xr-x 0/0 0 1969-12-31 16:00 usr/share/openshift/.wh..wh..opq drwxr-xr-x 0/0 0 2019-11-20 09:25 usr/share/openshift/linux_amd64/ -rwxr-xr-x 0/0 82720408 2019-11-20 09:22 usr/share/openshift/linux_amd64/oc drwxr-xr-x 0/0 0 2019-11-20 09:25 usr/share/openshift/linux_arm64/ -rwxr-xr-x 0/0 77812608 2019-11-20 09:23 usr/share/openshift/linux_arm64/oc drwxr-xr-x 0/0 0 2019-11-20 09:25 usr/share/openshift/linux_ppc64le/ -rwxr-xr-x 0/0 79059232 2019-11-20 09:24 usr/share/openshift/linux_ppc64le/oc drwxr-xr-x 0/0 0 2019-11-20 09:25 usr/share/openshift/linux_s390x/ -rwxr-xr-x 0/0 81223040 2019-11-20 09:25 usr/share/openshift/linux_s390x/oc drwxr-xr-x 0/0 0 2019-11-20 09:25 usr/share/openshift/mac/ -rwxr-xr-x 0/0 93382160 2019-11-20 09:20 usr/share/openshift/mac/oc drwxr-xr-x 0/0 0 2019-11-20 09:25 usr/share/openshift/windows/ -rwxr-xr-x 0/0 81619968 2019-11-20 09:21 usr/share/openshift/windows/oc.exe

So it looks like they all get squashed down into a single layer. How about:

COPY --from=builder /go/src/github.com/openshift/oc/_output/bin/ /usr/share/openshift/ RUN mkdir /usr/share/openshift/mac /usr/share/openshift/windows && mv /usr/share/openshift/darwin_amd64/oc /usr/share/openshift/mac/oc && ln -s /usr/share/openshift/mac/oc /usr/share/openshift/darwin_amd64/oc && mv /usr/share/openshift/windows_amd64/oc.exe /usr/share/openshift/windows/oc.exe && ln -s /usr/share/openshift/windows/oc.exe /usr/share/openshift/windows_amd64/oc.exe

That would keep the plain-file tar entries where they were before #153, but still allow the downloads deployment to work with the new, standardized paths.

symlink approach I floated in that^ comment is working in #172.

openshift#153 introduced a regression because `oc adm release extract` does not handle the hard links well.

crawford · 2019-11-20T18:54:01Z

/retitle Bug 1774642: Avoid hard links in cli-artifacts

openshift-ci-robot · 2019-11-20T18:54:07Z

@yselkowitz: This pull request references Bugzilla bug 1774642, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1774642: Avoid hard links in cli-artifacts

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.

wking · 2019-11-20T21:48:30Z

e2e-cmd:

level=info msg="Cluster operator console Available is False with DeploymentAvailableInsufficientReplicas: DeploymentAvailable: 0 pods available for console deployment"
...
{"component":"entrypoint","file":"prow/entrypoint/run.go:164","func":"k8s.io/test-infra/prow/entrypoint.Options.ExecuteProcess","level":"error","msg":"Process did not finish before 4h0m0s timeout","time":"2019-11-20T21:14:38Z"}

And because of the 4h timeout (which I don't understand), we have no gathered pod logs to explain why downloads was missing. But it makes me jumpy about this PR as it stands with 2510f47.

crawford · 2019-11-21T00:12:32Z

/retest
/approve

yselkowitz · 2019-11-21T04:45:04Z

/assign @mfojtik

mfojtik · 2019-11-21T16:34:05Z

/lgtm

openshift-ci-robot · 2019-11-21T16:34:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crawford, mfojtik, yselkowitz

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:

~~OWNERS~~ [mfojtik]

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 added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 20, 2019

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 20, 2019

wking reviewed Nov 20, 2019

View reviewed changes

Avoid hard links in cli-artifacts

2510f47

openshift#153 introduced a regression because `oc adm release extract` does not handle the hard links well.

yselkowitz force-pushed the master branch from 928dfe8 to 2510f47 Compare November 20, 2019 17:13

openshift-ci-robot requested review from mfojtik and smarterclayton November 20, 2019 17:15

openshift-ci-robot changed the title ~~Avoid hard links in cli-artifacts~~ Bug 1774642: Avoid hard links in cli-artifacts Nov 20, 2019

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

openshift-ci-robot assigned mfojtik Nov 21, 2019

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

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

openshift-merge-robot merged commit 9d412f4 into openshift:master Nov 21, 2019

yselkowitz mentioned this pull request Nov 21, 2019

Bug 1773807: Enable all Linux arches in cli-artifacts [4.2] #168

Merged

wking mentioned this pull request Dec 5, 2019

manifests/07-downloads-deployment: Serve from standardized paths openshift/console-operator#354

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1774642: Avoid hard links in cli-artifacts #171

Bug 1774642: Avoid hard links in cli-artifacts #171

yselkowitz commented Nov 20, 2019

openshift-ci-robot commented Nov 20, 2019

crawford commented Nov 20, 2019

wking Nov 20, 2019

yselkowitz Nov 20, 2019

wking Nov 20, 2019 •

edited

Loading

wking Nov 20, 2019

wking Nov 21, 2019

crawford commented Nov 20, 2019

openshift-ci-robot commented Nov 20, 2019

wking commented Nov 20, 2019

crawford commented Nov 21, 2019

yselkowitz commented Nov 21, 2019

mfojtik commented Nov 21, 2019

openshift-ci-robot commented Nov 21, 2019

Bug 1774642: Avoid hard links in cli-artifacts #171

Bug 1774642: Avoid hard links in cli-artifacts #171

Conversation

yselkowitz commented Nov 20, 2019

openshift-ci-robot commented Nov 20, 2019

crawford commented Nov 20, 2019

wking Nov 20, 2019

Choose a reason for hiding this comment

yselkowitz Nov 20, 2019

Choose a reason for hiding this comment

wking Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

wking Nov 20, 2019

Choose a reason for hiding this comment

wking Nov 21, 2019

Choose a reason for hiding this comment

crawford commented Nov 20, 2019

openshift-ci-robot commented Nov 20, 2019

wking commented Nov 20, 2019

crawford commented Nov 21, 2019

yselkowitz commented Nov 21, 2019

mfojtik commented Nov 21, 2019

openshift-ci-robot commented Nov 21, 2019

wking Nov 20, 2019 •

edited

Loading