Skip to content

Bug 1800674: Option to force RepoDigest mirror #298

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.

Sign up for GitHub

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

Jump to bottom
Merged
openshift-merge-robot merged 1 commit into from
Feb 19, 2020
Merged

Bug 1800674: Option to force RepoDigest mirror #298

openshift-merge-robot merged 1 commit into from
Feb 19, 2020

Conversation

@kevinrizza
Copy link
Member

@kevinrizza kevinrizza commented Feb 14, 2020 •
edited
Loading

Currently oc image mirror, when pointed at a
manifest list digest, checks the contents of the manifest and mirrors
just the underlying image if it contains only a single image.

This change adds an option to mirroring to force the repodigest to
mirror in addition to the underlying image digest. Additionally, it
forces oc image mirror and oc adm catalog mirror to always
enable this option

https://bugzilla.redhat.com/show_bug.cgi?id=1800674

@openshift-ci-robot openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Feb 14, 2020
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 14, 2020

@kevinrizza: This pull request references Bugzilla bug 1800674, which is invalid:

  • expected the bug to target the "4.4.0" release, but it targets "---" 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 1800674: Option to force RepoDigest mirror

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 openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 14, 2020
soltysh
soltysh approved these changes Feb 17, 2020
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

This lgtm, but you need to run make update to get the completions updated.
/approve
Also I'd love to have a confirmation from @smarterclayton

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 17, 2020
@ecordell
Copy link
Contributor

ecordell commented Feb 17, 2020

/bugzilla refresh

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Feb 17, 2020
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 17, 2020

@ecordell: This pull request references Bugzilla bug 1800674, 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.

@openshift-ci-robot openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Feb 17, 2020
@benluddy
Copy link
Contributor

benluddy commented Feb 17, 2020

Kevin's out today, so I went ahead and regenerated the shell completions in #301. If this change doesn't need to merge today, I'll close my PR.

@benluddy benluddy mentioned this pull request Feb 17, 2020
Closed
smarterclayton
smarterclayton reviewed Feb 17, 2020
View reviewed changes
pkg/cli/image/mirror/mirror.go Outdated
flag.BoolVar(&o.SkipMount, "skip-mount", o.SkipMount, "Always push layers instead of cross-mounting them")
flag.BoolVar(&o.SkipMultipleScopes, "skip-multiple-scopes", o.SkipMultipleScopes, "Some registries do not support multiple scopes passed to the registry login.")
flag.BoolVar(&o.Force, "force", o.Force, "Attempt to write all layers and manifests even if they exist in the remote repository.")
flag.BoolVar(&o.ForceManifestList, "force-manifest-list", o.ForceManifestList, "Always attempt to mirror manifestlist even if manifestlist contains a single image digest")
Copy link
Contributor

@smarterclayton smarterclayton Feb 17, 2020

Choose a reason for hiding this comment

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

I am a bit concerned here that the default behavior of image mirror is wrong when users specify --filter-by-os=*, regardless of this flag. Mirror doesn't have any issue dealing with manifest lists, so I think ProcessManifestList within mirror should always have the equivalent of force=true. That being said, mirror for release payloads should resolve image streams, so it can't be the default.

That may be a behavior change, but at least on the surface this looks like a bug that mirror by default strips manifest lists.

Copy link
Contributor

@soltysh soltysh Feb 18, 2020

Choose a reason for hiding this comment

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

@smarterclayton so you're basically saying the fix is correct, but we should not expose that outside for users, @kevinrizza would you mind updating?

@kevinrizza
Bug 1800674: Option to force RepoDigest mirror
9b68b41 
Currently `oc image mirror`, when pointed at a
manifest list digest, checks the contents of the manifest and mirrors
just the underlying image if it contains only a single image.

This change adds an option to mirroring to force the repodigest to
mirror in addition to the underlying image digest. Additionally, it
forces `oc image mirror` and `oc adm catalog mirror` to always
enable this option

https://bugzilla.redhat.com/show_bug.cgi?id=1800674
@openshift-ci-robot openshift-ci-robot added bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. and removed bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Feb 18, 2020
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 18, 2020

@kevinrizza: This pull request references Bugzilla bug 1800674, which is invalid:

  • expected the bug to target the "4.4.0" release, but it targets "4.5.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 1800674: Option to force RepoDigest mirror

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.

@kevinrizza
Copy link
Member Author

kevinrizza commented Feb 18, 2020

@smarterclayton @soltysh Updated the PR to remove the command line flag from oc image mirror. Let me know if you have any other thoughts. Thanks!

@kevinrizza
Copy link
Member Author

kevinrizza commented Feb 18, 2020

/bugzilla refresh

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Feb 18, 2020
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 18, 2020

@kevinrizza: This pull request references Bugzilla bug 1800674, which is valid.

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.

@openshift-ci-robot openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Feb 18, 2020
soltysh
soltysh approved these changes Feb 18, 2020
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/retest

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 18, 2020
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 18, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kevinrizza, 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

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 18, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 18, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

6 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 18, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 18, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 18, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 19, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 19, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 19, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 27d7a0d into openshift:master Feb 19, 2020
@soltysh
Copy link
Contributor

soltysh commented Feb 19, 2020

/cherry-pick release-4.4

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Feb 19, 2020
Merged
@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Feb 19, 2020

@soltysh: new pull request created: #304

Details

In response to this:

/cherry-pick release-4.4

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.

@kevinrizza
Copy link
Member Author

kevinrizza commented Feb 19, 2020

/cherry-pick release-4.3

@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Feb 19, 2020

@kevinrizza: #298 failed to apply on top of branch "release-4.3":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	pkg/cli/admin/catalog/mirror.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/cli/admin/catalog/mirror.go
CONFLICT (content): Merge conflict in pkg/cli/admin/catalog/mirror.go
Patch failed at 0001 Bug 1800674: Option to force RepoDigest mirror
Details

In response to this:

/cherry-pick release-4.3

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.

@kevinrizza kevinrizza mentioned this pull request Feb 20, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mfojtik mfojtik Awaiting requested review from mfojtik

2 more reviewers

@smarterclayton smarterclayton smarterclayton left review comments

@soltysh soltysh soltysh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@soltysh soltysh

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@kevinrizza @openshift-ci-robot @ecordell @benluddy @openshift-bot @soltysh @openshift-cherrypick-robot @smarterclayton @openshift-merge-robot