Skip to content

Bug 1823143: Implement enhancement for "Add ImageContentSource awareness to oc" - #439

Closed
sallyom wants to merge 6 commits into
openshift:masterfrom
sallyom:icsp_bz1823143
Closed

Bug 1823143: Implement enhancement for "Add ImageContentSource awareness to oc"#439
sallyom wants to merge 6 commits into
openshift:masterfrom
sallyom:icsp_bz1823143

Conversation

@sallyom

@sallyom sallyom commented May 26, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR implements openshift/enhancements#334:

  • writes an ICSP file as part of oc adm release mirror to either:
    • ./config
    • --release-image-icsp-to-dir config subdirectory or --to-dir config subdirectory
  • brings in library-go change to allow lookup of alternative image sources from mirrored registries

To Test - See these test commands: #439 (comment)
set up a secure local registry with this script.

For previous discussions and closed/related PRs:
#427
#395

Includes cherry-picks of commits in #745
Includes fake bump with library-go change: openshift/library-go#939

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 26, 2020
@sallyom sallyom mentioned this pull request May 26, 2020
Merged
@sallyom sallyom changed the title WIP: write ImageContentSourcePolicy to file with oc adm release mirror WIP: Implement enhancement for "Add ImageContentSource awareness to oc" Jun 1, 2020
@sallyom
sallyom force-pushed the icsp_bz1823143 branch 3 times, most recently from c962d12 to fbde626 Compare June 1, 2020 04:44
This was referenced Jun 1, 2020
Closed
Closed
@sallyom sallyom changed the title WIP: Implement enhancement for "Add ImageContentSource awareness to oc" Implement enhancement for "Add ImageContentSource awareness to oc" Jun 5, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 5, 2020
@sallyom

sallyom commented Jun 5, 2020

Copy link
Copy Markdown
Contributor Author

/hold

until enhancement openshift/enhancements#334 merges

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2020
@sallyom
sallyom force-pushed the icsp_bz1823143 branch 3 times, most recently from 6dba197 to 9ac6924 Compare June 9, 2020 23:12
smarterclayton
smarterclayton reviewed Jun 10, 2020
View reviewed changes
Comment thread pkg/cli/admin/release/mirror.go Outdated
smarterclayton
smarterclayton reviewed Jun 10, 2020
View reviewed changes
Comment thread pkg/cli/image/manifest/manifest.go Outdated
smarterclayton
smarterclayton reviewed Jun 10, 2020
View reviewed changes
Comment thread pkg/cli/image/manifest/manifest.go Outdated
smarterclayton
smarterclayton reviewed Jun 10, 2020
View reviewed changes
Comment thread pkg/cli/image/manifest/manifest.go Outdated
smarterclayton
smarterclayton reviewed Jun 10, 2020
View reviewed changes
Comment thread pkg/cli/image/manifest/manifest.go Outdated
smarterclayton
smarterclayton reviewed Jun 10, 2020
View reviewed changes
Comment thread pkg/cli/image/manifest/manifest.go Outdated
smarterclayton
smarterclayton reviewed Jun 10, 2020
View reviewed changes
Comment thread pkg/cli/image/manifest/manifest.go Outdated
smarterclayton
smarterclayton reviewed Jun 10, 2020
View reviewed changes
Comment thread pkg/cli/admin/release/mirror.go Outdated
@sallyom
sallyom force-pushed the icsp_bz1823143 branch 3 times, most recently from f426984 to 8891770 Compare June 15, 2020 03:37
soltysh
soltysh suggested changes Jun 17, 2020
View reviewed changes

@soltysh soltysh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ISCP retrieval is good, but we still need to flesh out the details how to apply it properly to release flow.

Comment thread pkg/cli/admin/release/info.go Outdated
Comment thread pkg/cli/admin/release/mirror.go Outdated
Comment thread pkg/cli/image/manifest/source.go Outdated
soltysh
soltysh reviewed Feb 4, 2021
View reviewed changes

@soltysh soltysh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit, I must have missed before.

Comment thread pkg/cli/admin/release/info.go Outdated
sallyom
sallyom commented Feb 4, 2021
View reviewed changes
Comment thread pkg/cli/image/imagesource/options.go Outdated
@sallyom

sallyom commented Feb 8, 2021

Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@sallyom

sallyom commented Feb 8, 2021

Copy link
Copy Markdown
Contributor Author

/retest

@sallyom

sallyom commented Feb 8, 2021

Copy link
Copy Markdown
Contributor Author

/test e2e-agnostic-cmd

sallyom
sallyom commented Feb 14, 2021
View reviewed changes
Comment thread pkg/cli/admin/release/info.go Outdated
@openshift-ci-robot

openshift-ci-robot commented Feb 20, 2021

Copy link
Copy Markdown

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

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.8.0) matches configured target release for branch (4.8.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
Details

In response to this:

Bug 1823143: Implement enhancement for "Add ImageContentSource awareness to oc"

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.

@mhrivnak mhrivnak mentioned this pull request Mar 26, 2021
Merged
@openshift-ci-robot

openshift-ci-robot commented Mar 27, 2021

Copy link
Copy Markdown

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

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.8.0) matches configured target release for branch (4.8.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @zhouying7780

Details

In response to this:

Bug 1823143: Implement enhancement for "Add ImageContentSource awareness to oc"

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

sallyom commented Mar 27, 2021

Copy link
Copy Markdown
Contributor Author

@smarterclayton most of the changes from the old approach #790 are not required, without the requirement of pulling the original imageReference from a mirrored location using ICSP lookup/replace (with tags or digests). Therefor, the changes here are minimal (still have to do the replace when extracting mirrored releases, since there's no other way for oc to know that the internal referenced images (like tools, installer, oc) should be pulled from the same registry as the release imageRef (fetched from the alternative repo w/ lib-go blobMirrored client) see the readReleaseImageReference fn in release/image_mapper.go, the parsed user-passed imageRef (the o.From usually) must be known to apply the imageRef.ID of a component image to the desired registry.

Note that oc is no longer using any of the ICSP added features to registryclient, only added feature to oc is the switching of lookup from original ref/registry to mirrored ref/registry so image-registry will definitely need to verify that it's all working (however, I did verify it worked w/ the old approach, so I'm pretty confident w/ an ICSP client it will all be working well for them).

@romfreiman

romfreiman commented Apr 5, 2021

Copy link
Copy Markdown

@smarterclayton @sallyom how do we make a progress on this one? We're blocked with the central host management for 4.8 without it

@sallyom

sallyom commented Apr 5, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@smarterclayton another issue I see w/ hiding the replacement imageRef from oc is you cannot do something like

$ oc adm release info localhost:5000/openshift/release:4.7.5-x86_64  --image-for tools
quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:e59e69fa1539693867c2e4ec9c02d0d6091fac35fb6417a8b1e5abc87d326cae

(you can run it, but user will be confused from the output)

UPDATE:
With latest commits, here is the output:

$ oc adm release info localhost:5000/openshift/release:4.7.5-x86_64  --image-for tools -a ~/pull-secret
localhost:5000/openshift/release@sha256:e59e69fa1539693867c2e4ec9c02d0d6091fac35fb6417a8b1e5abc87d326cae

I believe this is the desired output.

@smarterclayton

smarterclayton commented Apr 6, 2021

Copy link
Copy Markdown
Contributor

$ oc adm release info localhost:5000/openshift/release:4.7.5-x86_64 --image-for tools
quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:e59e69fa1539693867c2e4ec9c02d0d6091fac35fb6417a8b1e5abc87d326cae

I think this is the correct behavior. If someone wants to view that image (with oc image info) they have to pass an ICSP.

@sallyom

sallyom commented Apr 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

$ oc adm release info localhost:5000/openshift/release:4.7.5-x86_64 --image-for tools
quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:e59e69fa1539693867c2e4ec9c02d0d6091fac35fb6417a8b1e5abc87d326cae

I think this is the correct behavior. If someone wants to view that image (with oc image info) they have to pass an ICSP.

With this PR, oc can now access the mirrored location rather than only the original. No ICSP is necessary for this, as the user-provided location is simply added to the alternates list (ICSP would be necessary if you needed to oc image info quay.io/openshift...@sha... and expect to behind-the-scenes map that to and access mirrored/location..@sha....That is why oc is no longer using the ICSP, only part oc needs is the simple addition of the user-given location to list of alternates (in the simple strategy lookup from registryclient). If the ICSP feature is desired, I can add that back, but that can be in a follow-up, since the source of the bugs are all that a user simply wants this to work:
oc adm release extract mirrored/location...@sha.... or :tag

This PR solves the above, and with latest commits here is the output of gathering info from a mirrored location:

$ oc adm release info localhost:5000/openshift/release:4.7.5-x86_64  --image-for tools
localhost:5000/openshift/release@sha256:e59e69fa1539693867c2e4ec9c02d0d6091fac35fb6417a8b1e5abc87d326cae

@openshift-ci-robot

openshift-ci-robot commented Apr 14, 2021

Copy link
Copy Markdown

[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:

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

@sallyom

sallyom commented Apr 14, 2021

Copy link
Copy Markdown
Contributor Author

@smarterclayton @soltysh with the added *WithLocation fns in registryclient.client_mirrored, I added the ICSP function back, it is working. See the test cmds for examples: #439 (comment)

@eranco74

eranco74 commented May 3, 2021

Copy link
Copy Markdown

I've tried using the oc from this PR to extract the baremetal-installer from a nightly image (edge-01.edge.lab.eng.rdu2.redhat.com:5000/ocp:nightly-2021-03-16-221720)

Seems that without the changes from this PR, oc knows where to extract from:
Will extract usr/bin/openshift-install from quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:13714f2d3e2db23d44158bad03dcd017971c9674a9c5904b4700f31ff24b67e9
According to the content of release-manifests/image-references in the release image.

But when I use the oc from this PR it tries to extract from the same registry as the release image:
Will extract usr/bin/openshift-install from registry.ci.openshift.org/ocp/release@sha256:13714f2d3e2db23d44158bad03dcd017971c9674a9c5904b4700f31ff24b67e9
and fail with this error:
error: image "registry.ci.openshift.org/ocp/release@sha256:13714f2d3e2db23d44158bad03dcd017971c9674a9c5904b4700f31ff24b67e9" not found: manifest unknown: manifest unknown

@romfreiman

romfreiman commented May 3, 2021

Copy link
Copy Markdown

cc: @soltysh

sallyom added 6 commits May 3, 2021 08:33
@sallyom
Bug 1823143: oc adm release - lookup images from alternative sources
4513ce3 
When working with mirrored release payloads, a release from a mirrored registry,
mylocalregistry/ocp/release:4.5.0-0.nightly-2020-04-18-093630 mirrored from
registry.svc.ci.openshift.org/ocp/release:4.5.0-0.nightly-2020-04-18-093630 -
both reference 'quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:2eb0a51...'.
In case of disconnected, oc will now use 'mylocalregistry/ocp/release' if passed,
instead of 'quay.io/openshift-release-dev/ocp-v4.0-art-dev'
@sallyom
oc adm release mirror: write icsp file to disk with options
bedec7d
@sallyom
FAKE BUMP: go.mod
933ddee
@sallyom
FAKE BUMP: library-go
1ef6668
@sallyom
add ICSP awareness
d4d6e1f
@sallyom
generated completions
0535981
@openshift-ci

openshift-ci Bot commented May 3, 2021

Copy link
Copy Markdown
Contributor

@sallyom: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/verify-deps 0535981 link /test verify-deps
ci/prow/unit 0535981 link /test unit
ci/prow/e2e-agnostic-cmd 0535981 link /test e2e-agnostic-cmd
ci/prow/e2e-aws-serial 0535981 link /test e2e-aws-serial
ci/prow/e2e-aws-upgrade 0535981 link /test e2e-aws-upgrade
ci/prow/e2e-aws 0535981 link /test e2e-aws
ci/prow/e2e-metal-ipi-ovn-ipv6 0535981 link /test e2e-metal-ipi-ovn-ipv6
ci/prow/images 0535981 link /test images

Full PR test history. Your PR dashboard.

Details

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.

@eranco74 eranco74 mentioned this pull request May 11, 2021
Merged
@soltysh

soltysh commented May 20, 2021

Copy link
Copy Markdown
Contributor

This will be split into several PRs, first is #829
/close

@openshift-ci

openshift-ci Bot commented May 20, 2021

Copy link
Copy Markdown
Contributor

@soltysh: Closed this PR.

Details

In response to this:

This will be split into several PRs, first is #829
/close

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

openshift-ci Bot commented May 20, 2021

Copy link
Copy Markdown
Contributor

@sallyom: This pull request references Bugzilla bug 1823143. The bug has been updated to no longer refer to the pull request using the external bug tracker.

Details

In response to this:

Bug 1823143: Implement enhancement for "Add ImageContentSource awareness to oc"

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@deads2k deads2k Awaiting requested review from deads2k
@mfojtik mfojtik Awaiting requested review from mfojtik
@zhouying7780 zhouying7780 Awaiting requested review from zhouying7780
4 more reviewers
@wking wking wking left review comments
@dmage dmage dmage requested changes
@smarterclayton smarterclayton smarterclayton left review comments
@soltysh soltysh soltysh approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@sallyom @openshift-ci-robot @openshift-merge-robot @romfreiman @smarterclayton @eranco74 @soltysh @wking @dmage