[OCPNODE-1258] Support both icsp and idms #346

QiWang19 · 2022-09-22T22:05:24Z

Append ImageDigestMirrorSet to current ImageContentSourcePolicy. The OCPNODE-521 will land new CRDs for image mirror configurations to replace ICSP in the future release.

ImageDigestMirrorSet implemented: openshift/machine-config-operator#3037
ImageDigestMirrorSet epic: https://issues.redhat.com/browse/OCPNODE-521

QiWang19 · 2022-10-04T17:42:16Z

/hold
for 4.13 tree to open

QiWang19 · 2022-10-21T14:58:24Z

/hold cancel
4.13 branch is open

dmage · 2022-10-21T15:06:24Z

/hold
master is still 4.12

QiWang19 · 2022-11-04T17:21:44Z

@dmage 4.13 branch is open, this is ready for review. I have rebased this branch, not sure why the ci failed. Could you help take a look?

flavianmissi · 2023-01-09T14:38:05Z

/unassign @dmage
/assign @flavianmissi
/hold cancel

flavianmissi · 2023-01-09T14:39:12Z

@QiWang19 please rebase. Let's see if the test failures persist afterwards.

QiWang19 · 2023-01-10T20:37:00Z

failure TestPullThroughIDMS: https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_image-registry/346/pull-ci-openshift-image-registry-master-e2e-agnostic-image-registry/1612831060893110272
The test might reply on the PR: openshift/machine-config-operator#3037 that hasn't been merged yet.

flavianmissi

Looks pretty good, thanks for doing this.
Left a few comments, please take a look.

pkg/dockerregistry/server/simplelookupicsp.go

flavianmissi · 2023-01-11T15:08:51Z

pkg/dockerregistry/server/simplelookupicsp.go

+
+	if len(icspList.Items) > 0 && len(idmsList.Items) > 0 {
+		klog.Errorf("error finding both icsp and idms resources at the same time: %s", err)
+		return []reference.DockerImageReference{ref.AsRepository()}, nil


I think we should surface an error here so that this doesn't go unnoticed to the caller.

Addressed this in line68 and line73.

pkg/dockerregistry/server/simplelookupicsp_test.go

pkg/dockerregistry/server/simplelookupicsp.go

test/integration/pullthrough/pullthrough_test.go

flavianmissi · 2023-01-11T16:58:52Z

@QiWang19 I'm not sure about your failing test. You can try running it against your own cluster with

KUBECONFIG=<path-to-kubeconfig> GOTEST_FLAGS="-p 1 -mod vendor" DOCKER_API_VERSION=1.24 hack/test-go.sh test/integration/*

QiWang19 · 2023-01-11T17:39:56Z

@QiWang19 I'm not sure about your failing test. You can try running it against your own cluster with
KUBECONFIG=<path-to-kubeconfig> GOTEST_FLAGS="-p 1 -mod vendor" DOCKER_API_VERSION=1.24 hack/test-go.sh test/integration/*

Thanks! I will try running the test to make sure the code in this patch is correct.

QiWang19 · 2023-01-11T18:59:42Z

The error is

--- FAIL: TestPullThroughIDMS (9.79s)
    project.go:42: project image-registry-test-integration-idms is created
    registry.go:505: created ephemeral registry: ephemeral-registry-jznd4-image-registry-test-integration-idms.apps.ci-ln-6g0lzsk-76ef8.origin-ci-int-aws.dev.rhcloud.com (ephemeral-registry-jznd4)
    pullthrough_test.go:542: unexpected status 0: v1.Status{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, ListMeta:v1.ListMeta{SelfLink:"", ResourceVersion:"", Continue:"", RemainingItemCount:(*int64)(nil)}, Status:"Failure", Message:"Internal error occurred: does.not.exist/repo/image@sha256:c1d7a836008fcd9cd6cd2b4b4735bd0c2fef0590b468aedd84abbd6d476032e0: Get \"http://does.not.exist/v2/\": dial tcp: lookup does.not.exist on 172.30.0.10:53: no such host", Reason:"InternalError", Details:(*v1.StatusDetails)(0xc000cb3c80), Code:500}

But this test reuses most of TestPullThroughICSP. I need to look into why it failed.

dmage · 2023-01-11T19:26:40Z

@QiWang19 the error seems to be from image streams, did you implement IDMS for image streams? (they live inside openshift/openshift-apiserver)

QiWang19 · 2023-01-11T22:30:45Z

Thanks. @dmage @flavianmissi
no I did not implement openshift-apiserver. I found here: https://github.com/openshift/openshift-apiserver/blob/94da954fae6d5512de5073e3f30415fcec44152e/pkg/image/apiserver/registry/imagestreamimport/rest.go#L283. I will work on it for idms.

flavianmissi · 2023-01-12T15:58:16Z

@QiWang19 I suspect that TestPullThroughIDMS won't pass until openshift/openshift-apiserver#318 is merged... You might want to validate that by deploying a cluster with this PR and openshift/openshift-apiserver#318 together, then running your test again on that cluster. If the test passes then we just need to wait, otherwise you have an actual test failure to troubleshoot.

Relatedly, I see that openshift/openshift-apiserver#318 adds support for both ImageDigestMirrorSet and ImageTagMirrorSet, but here you only covered ImageDigestMirrorSet. Do we not do pull-through by tag? 🤔

QiWang19 · 2023-01-26T19:33:59Z

The PR openshift/openshift-apiserver#318 was failing to launch a 4.13 cluster, I will continue to retest the integration test.

Relatedly, I see that openshift/openshift-apiserver#318 adds support for both ImageDigestMirrorSet and ImageTagMirrorSet, but here you only covered ImageDigestMirrorSet. Do we not do pull-through by tag? thinking

@flavianmissi Current image-registry only looks up icsp objects that supports only the digest image pull. I didn't know if the image-registry want to add the imageTagMirrorSet for tag pulls, so only added ImageDigestMirrorSet to keep the same image pull behavior as with ICSP. I can update the PR to add support for tag pulls. What do you think?
It seems the openshift-apiserver and image-registry should be in alignment.

QiWang19 · 2023-01-26T21:48:39Z

/retest-required

flavianmissi · 2023-02-21T13:36:20Z

The ICSP will be dropped at least after 4.17 to allow the EUS to EUS upgrade.

that makes sense.

Given that not all CI clusters use default IDMS/ICSP, one way to keep the ICSP test by 4.17 is before the TestPullThroughICSP function creates ICSP, in this PR we add a check to see if there's a default IDMS on the cluster [...]

that seems to be the best course of action for now. can you add it to this PR then?

flavianmissi · 2023-03-08T13:28:22Z

I think master is open to 4.14 development now @QiWang19 🎉

QiWang19 · 2023-03-09T19:49:49Z

@flavianmissi Thanks for letting me know about the development for the next release. ❤️
I have rebased this PR and addressed the previous reviews. This is ready for another round of review.
We will hold this before the ci cluster gets updated with the IDMS object per our discussion above.

QiWang19 · 2023-05-08T19:22:02Z

@flavianmissi I rebased openshift/openshift-apiserver#318, do you know which part of the ci script is related to this PR for the pre-merge test?
I have a draft PR: openshift/release#39122

flavianmissi · 2023-05-09T10:12:50Z

do you know which part of the ci script is related to this PR for the pre-merge test?
@QiWang19 do you mean where are the pre-merge tests for image stream import located? I'm actually not sure 🤔
@dmage do you happen to know? I looked around a bit but it wasn't obvious (to me...)

dmage · 2023-05-09T11:44:39Z

@QiWang19 what job are you talking about? openshift/release has a special logic with ICSP for "disconnected" clusters, which is not specific to the image-registry repo nor openshift-apiserver.

dmage · 2023-05-09T11:44:47Z

@QiWang19 what job are you talking about? openshift/release has a special logic with ICSP for "disconnected" clusters, which is not specific to the image-registry repo nor openshift-apiserver.

QiWang19 · 2023-05-09T19:47:09Z

@dmage This PR failed the test cases creating IDMS objects because the ci cluster has an existing ICSP objects. I am thinking about updating the openshft/release script can change the existing ICSP object to IDMS object so this PR can pass the test.
But I am not sure if changing openshift/release logic from ICSP to IDMS requires IDMS support from this image-registry

which is not specific to the image-registry repo nor openshift-apiserver

So the openshift/release logic with ICSP does not rely on image-registry?

dmage · 2023-05-17T14:36:26Z

@QiWang19 openshift/release is a huge thing, it runs on OCP and uses additional OCP clusters to install and test other OCP clusters; all these clusters have a few instances of the image registry.

The latest test results for this PR are few months old and don't seem to be related to this PR.
/retest

QiWang19 · 2023-05-17T17:31:23Z

@dmage I just remembered your comment we should openshift/openshift-apiserver#318 should get this in first.

Can we merge that or that PR still needs to hold for pre-merge testing? I rebased that 3 weeks ago.

dinhxuanvu · 2023-06-15T16:57:47Z

@QiWang19 FYI, the rebase is merged.

Signed-off-by: Qi Wang <qiwan@redhat.com>

openshift-ci · 2023-07-18T07:17:49Z

@QiWang19: all tests passed!

Full PR test history. Your PR dashboard.

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.

QiWang19 · 2023-07-18T12:48:15Z

@flavianmissi I rebased after openshift/openshift-apiserver#318 merged. Can we get this in?

QiWang19 · 2023-07-18T17:32:05Z

/hold cancel

QiWang19 · 2023-07-20T03:54:56Z

@dmage Can we get this in?

dmage · 2023-07-21T09:32:48Z

/lgtm

openshift-ci · 2023-07-21T09:34:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dmage, QiWang19

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~~ [dmage]

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

openshift-ci bot requested review from dmage and gabemontero September 22, 2022 22:07

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 4, 2022

QiWang19 changed the title ~~[OCPNODE-1258] Migrate icsp to idms~~ [OCPNODE-1258] Support both icsp and idms Oct 21, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 21, 2022

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 21, 2022

QiWang19 force-pushed the icsp2idms branch from 03dc22a to 4701ffd Compare November 4, 2022 17:02

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2023

openshift-ci bot assigned flavianmissi Jan 9, 2023

QiWang19 force-pushed the icsp2idms branch from 4701ffd to a7b9db9 Compare January 10, 2023 15:16

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2023

flavianmissi reviewed Jan 11, 2023

View reviewed changes

test/integration/pullthrough/pullthrough_test.go Outdated Show resolved Hide resolved

QiWang19 force-pushed the icsp2idms branch from a7b9db9 to 8d84b41 Compare January 26, 2023 19:12

QiWang19 force-pushed the icsp2idms branch from 9672e9b to 8d92b1c Compare February 21, 2023 21:42

QiWang19 force-pushed the icsp2idms branch from 8d92b1c to 92ee02d Compare March 9, 2023 19:37

QiWang19 added 2 commits July 18, 2023 01:09

[OCPNODE-1258] Migrate icsp to idms

45da6c3

Signed-off-by: Qi Wang <qiwan@redhat.com>

go mod vendor

35e48eb

Signed-off-by: Qi Wang <qiwan@redhat.com>

QiWang19 force-pushed the icsp2idms branch from 92ee02d to 35e48eb Compare July 18, 2023 05:11

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 18, 2023

openshift-ci bot assigned dmage Jul 21, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2023

openshift-merge-robot merged commit ccfad6f into openshift:master Jul 21, 2023
9 checks passed

stbenjam mentioned this pull request Jul 24, 2023

Revert "[OCPNODE-1258] Support both icsp and idms" #374

Merged

This was referenced Aug 4, 2023

OCPBUGS-17882: Add rbac permission IDMS, ITMS openshift/cluster-image-registry-operator#891

Merged

OCPBUGS-17975: Support both icsp and idms #375

Merged

[OCPNODE-1258] Support both icsp and idms #346

[OCPNODE-1258] Support both icsp and idms #346

Conversation

QiWang19 commented Sep 22, 2022 • edited

QiWang19 commented Oct 4, 2022

QiWang19 commented Oct 21, 2022

dmage commented Oct 21, 2022

QiWang19 commented Nov 4, 2022

flavianmissi commented Jan 9, 2023

flavianmissi commented Jan 9, 2023

QiWang19 commented Jan 10, 2023

flavianmissi left a comment

Choose a reason for hiding this comment

flavianmissi Jan 11, 2023

Choose a reason for hiding this comment

QiWang19 Mar 9, 2023

Choose a reason for hiding this comment

flavianmissi commented Jan 11, 2023 • edited

QiWang19 commented Jan 11, 2023

QiWang19 commented Jan 11, 2023

dmage commented Jan 11, 2023

QiWang19 commented Jan 11, 2023 • edited

flavianmissi commented Jan 12, 2023 • edited

QiWang19 commented Jan 26, 2023 • edited

QiWang19 commented Jan 26, 2023

flavianmissi commented Feb 21, 2023

flavianmissi commented Mar 8, 2023

QiWang19 commented Mar 9, 2023

QiWang19 commented May 8, 2023

flavianmissi commented May 9, 2023

dmage commented May 9, 2023

dmage commented May 9, 2023

QiWang19 commented May 9, 2023

dmage commented May 17, 2023

QiWang19 commented May 17, 2023

dinhxuanvu commented Jun 15, 2023

openshift-ci bot commented Jul 18, 2023

QiWang19 commented Jul 18, 2023

QiWang19 commented Jul 18, 2023

QiWang19 commented Jul 20, 2023

dmage commented Jul 21, 2023

openshift-ci bot commented Jul 21, 2023

QiWang19 commented Sep 22, 2022 •

edited

flavianmissi commented Jan 11, 2023 •

edited

QiWang19 commented Jan 11, 2023 •

edited

flavianmissi commented Jan 12, 2023 •

edited

QiWang19 commented Jan 26, 2023 •

edited