[OCPNODE-553] Add CRD ImageDigestMirrorSet,ImageTagMirrorSet

[QiWang19]

- What I did

• Add support for ImageDigestMirrorSet,ImageTagMirrorSet mirror configuration CRDs.
• Reject the update of one of current icsp and new idms.itms resources if both exist to avoid having both icsp and new CRs on the same cluster.

Close card: https://issues.redhat.com/browse/OCPNODE-553
Epic: https://issues.redhat.com/browse/OCPNODE-521
Dependent PR: openshift/runtime-utils#15

- How to verify it

Apply following CR, check the /etc/containers/registries.conf

apiVersion: config.openshift.io/v1
kind: ImageTagMirrorSet
metadata:
  name: tag-mirror
spec:
  imageTagMirrors:
  - mirrors:
    - example.io/example/ubi-minimal 
    source: registry.access.redhat.com/ubi8/ubi-minimal
  - mirrors:
    - example.io/example/ubi-minimal 
    source: registry.access.redhat.com/ubi8/ubi-minimal-1
    mirrorSourcePolicy: NeverContactSource

/etc/containers/registries.conf

unqualified-search-registries = ["registry.access.redhat.com", "docker.io"]
short-name-mode = ""

[[registry]]
  prefix = ""
  location = "registry.access.redhat.com/ubi8/ubi-minimal"

  [[registry.mirror]]
    location = "example.io/example/ubi-minimal"
    pull-from-mirror = "tag-only"

[[registry]]
  prefix = ""
  location = "registry.access.redhat.com/ubi8/ubi-minimal-1"
  blocked = true

  [[registry.mirror]]
    location = "example.io/example/ubi-minimal"
    pull-from-mirror = "tag-only"

- Description for the changelog

Add support for ImageDigestMirrorSet,ImageDigestMirrorSet CRDs

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[mtrmac]

Just an initial pass (I didn’t actually read the added tests).

[nee1esh]

/retest

[QiWang19]

@yuqi-zhang @mtrmac This PR and dependent PR: openshift/runtime-utils#15 will update the registry.conf. And the runtime-utils PR replaced sysregistriesv2.Registry.MirrorByDigestOnly with new per-mirror field sysregistriesv2.Endpoint.PullFromMirror to edit the registry.conf. So I think this PR should also update isSafeContainerRegistryConfChanges().

machine-config-operator/pkg/daemon/drain.go

Lines 178 to 187 in be77042

	// isSafeContainerRegistryConfChanges looks inside old and new versions of registries.conf file.
	// It compares the content and determines whether changes made are safe or not. This will
	// help MCD to decide whether we can skip node drain for applied changes into container
	// registry.
	// Currently, we consider following container registry config changes as safe to skip node drain:
	// 1. A new mirror is added to an existing registry that has `mirror-by-digest-only=true`
	// 2. A new registry has been added that has `mirror-by-digest-only=true`
	// See https://bugzilla.redhat.com/show_bug.cgi?id=1943315
	//nolint:gocyclo
	func isSafeContainerRegistryConfChanges(oldIgnConfig, newIgnConfig ign3types.Config) (bool, error) {

Not sure how the safe conditions are defined, but looking at https://bugzilla.redhat.com/show_bug.cgi?id=1943315#c1, we could propose the following as safe?

// Currently, we consider following container registry config changes as safe to skip node drain:
// 1. A new mirror is added to an existing registry that has per-mirror setting `pull-from-mirror=digest-only` or registry level setting `mirror-by-digest-only=true`
// 2. A new registry has been added that has `mirror-by-digest-only=true` or per-mirror setting `pull-from-mirror=digest-only` for all of its mirrors

[openshift-ci]

@QiWang19: PR needs rebase.

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.

[mtrmac]

LGTM overall. (FWIW the PR doesn’t currently compile).

[mtrmac]

/lgtm again , at least based on the relevant parts of the diff being the same…

[mtrmac]

/lgtm

[rphillips]

/lgtm

[QiWang19]

@yuqi-zhang could the MCO team review and approve this PR? It is ready to get in.

[yuqi-zhang]

Did a first pass with some question/comments below

Also, the MCO team is doing pre-merge QE testing for 4.13. Is node team doing pre-merge QE testing? Would someone from node QE be able to verify this new feature and apply QE approval?

[yuqi-zhang]

To be clear, this doesn't actually test any pulling, it just checks if ICSP sets up the correct conf?

Would we want to instead test IDSM/ITMS since that's (I assume) the preferred way?

[QiWang19]

This is just to test the ICSP can still work with the addition of new CRDs, before we deprecate the ICSP.

yes, we can add some tests for IDSM/ITMS.

[yuqi-zhang]

would we want to keep the old conditionals? It is possible that someone with a hand-crafted registries.conf could be still using the old method

Or, are we saying that we don't support that, and there isn't any way for ICSP to generate the old style mirror flags anymore?

[QiWang19]

Add some unit test cases with mirror-by-digest-only for the old style. ICSP will no longer generate this old style, but it is still supported when there are hand-crafted registries.conf.

[yuqi-zhang]

I think this generally makes sense to me, approving, thanks for working on this!

Also going to add a

/hold

for QE pre-merge testing and QE approval.

Doesn't have to be part of this PR, but could you provide some docs on how to use IDMS/ITMS? Or maybe some links to other docs are fine as well, thanks!

[yuqi-zhang]

Hmm, does this actually change anything? Just curious

[QiWang19]

These files are also used in the e2e test. To fix the error I saw in the ci e2e test that is image repository name must be lowercase.

[QiWang19]

/retest-required

[rphillips]

/lgtm
/hold for qe review

@QiWang19 Please remove the hold when ready.

[lyman9966]

/label qe-approved

[QiWang19]

for QE pre-merge testing and QE approval.

Doesn't have to be part of this PR, but could you provide some docs on how to use IDMS/ITMS? Or maybe some links to other docs are fine as well, thanks!

@yuqi-zhang This passed the QE approval label. Could you merge this?
The doc can be added in another PR.

[yuqi-zhang]

/hold cancel

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD fc27a2b and 2 for PR HEAD 73e22c3cc054f6a89f55ac48704ebc7cc523e35f in total

[yuqi-zhang]

gcp-op may require a rebase to master which has some e2e test fixes

[QiWang19]

ctrcfg_test.go:283: default configuration value "digest-only" same as values being tested against. Consider updating the test

https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_machine-config-operator/3037/pull-ci-openshift-machine-config-operator-master-e2e-gcp-op/1615814135679815680/build-log.txt the
rebased, e2e-gcp-op result shows the test platform has icsp resources. I can keep the icsp e2e test but have to drop idms,items e2e test. Before the icsp CRD gets deprecated, using idms, items on the same cluster where icsp resource exists is prohibited.

[QiWang19]

/retest-required

[QiWang19]

@yuqi-zhang could you retag lgtm?

[yuqi-zhang]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac, QiWang19, rphillips, yuqi-zhang

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:

• OWNERS [yuqi-zhang]

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]

/retest-required

Remaining retests: 0 against base HEAD 324bc30 and 2 for PR HEAD 7971961 in total

[QiWang19]

/retest-required

[openshift-ci]

@QiWang19: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-openstack	5549a09bf7ea66ba72f19cc29246b67c963e1c0f	link	false	/test e2e-openstack
ci/prow/okd-scos-e2e-aws	73e22c3cc054f6a89f55ac48704ebc7cc523e35f	link	false	/test okd-scos-e2e-aws
ci/prow/e2e-alibabacloud	73e22c3cc054f6a89f55ac48704ebc7cc523e35f	link	false	/test e2e-alibabacloud
ci/prow/okd-scos-e2e-aws-ovn	7971961	link	false	/test okd-scos-e2e-aws-ovn
ci/prow/e2e-alibabacloud-ovn	7971961	link	false	/test e2e-alibabacloud-ovn
ci/prow/e2e-gcp-rt-op	7971961	link	false	/test e2e-gcp-rt-op
ci/prow/okd-scos-e2e-gcp-ovn-upgrade	7971961	link	false	/test okd-scos-e2e-gcp-ovn-upgrade

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.

[QiWang19]

@yuqi-zhang can we skip the tests?

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD 97ddbfe and 1 for PR HEAD 7971961 in total

[yuqi-zhang]

Only e2e-aws-ovn is required, is failing, and being retested, so skipping tests won't really help us either way. The other failures don't block the PR

Or did you mean you wanted me to override e2e-aws-ovn?

[QiWang19]

Yes, override e2e-aws-ovn. The e2e-aws-ovn was retested several times and failed. Can we override it?

[yuqi-zhang]

The latest commit did have a passing run: https://prow.ci.openshift.org/pr-history/?org=openshift&repo=machine-config-operator&pr=3037

So I think it's most likely ok. Still, I am uncomfortable overriding a required job unless the fix is urgent or directly attributed to it. Let's wait until the end of today, and, if there are no passes, I can look to override