Delete deprecated registries config #8161

bitoku · 2024-05-10T00:59:33Z

What type of PR is this?

/kind cleanup
/kind deprecation

What this PR does / why we need it:

Since #4455, registries option has been no effect.

Lines 765 to 770 in 9712c53

    
           // Registries are deprecated in cri-o.conf and turned into a NOP. 
        
           // Users should use registries.conf instead, so let's log it. 
        
           if len(t.Crio.Image.Registries) > 0 { 
        
           	t.Crio.Image.Registries = nil 
        
           	logrus.Warnf("Support for the 'registries' option has been dropped but it is referenced in %q.  Please use containers-registries.conf(5) for configuring unqualified-search registries instead.", path) 
        
           }

However, there are some places that don't mention explicitly that the option has been deprecated.
This PR changes the messages which don't mention the deprecation, and removes the remained configuration.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Since the deprecation has already been released and there was a release note, I'm not sure if we should need a new release-note for this.
I made it None for now.

Does this PR introduce a user-facing change?

None

openshift-ci · 2024-05-10T00:59:46Z

Hi @bitoku. Thanks for your PR.

I'm waiting for a cri-o 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-sigs/prow repository.

openshift-ci · 2024-05-10T00:59:55Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bitoku
Once this PR has been reviewed and has the lgtm label, please assign giuseppe for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

codecov · 2024-05-10T01:06:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.56%. Comparing base (9712c53) to head (0a65e7b).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8161      +/-   ##
==========================================
- Coverage   49.58%   49.56%   -0.02%     
==========================================
  Files         153      153              
  Lines       16930    16930              
==========================================
- Hits         8394     8392       -2     
- Misses       7489     7490       +1     
- Partials     1047     1048       +1

Signed-off-by: Ayato Tokubi <atokubi@redhat.com>

kwilczynski · 2024-05-10T09:39:06Z

/ok-to-test

kwilczynski · 2024-05-10T09:40:40Z

pkg/config/config.go

@@ -543,6 +543,8 @@ type ImageConfig struct {
 	// ImageVolumes controls how volumes specified in image config are handled
 	ImageVolumes ImageVolumesType `toml:"image_volumes"`
 	// Registries holds a list of registries used to pull unqualified images
+	// Deprecated: Support for this option has been dropped, and it has no effect.
+	// Please refer to containers-registries.conf(5) for configuring unqualified-search registries.
 	Registries []string `toml:"registries"`


I wonder if we should just be done with it and remove it already? It's been a while since 2021...

@cri-o/cri-o-maintainers, thoughts on removing this completely?

Have we ever deprecated this? If not, then we have to per https://github.com/cri-o/cri-o/blob/main/deprecating_process.md

Would this stand in for deprecation?

https://github.com/cri-o/cri-o/blob/main/pkg/config/config.go#L765-L770

This has been defunct for about two years now.

Somehow, yes.

We can:

Remove it completely, given that it has been defunct for a while, with a relevant log line being produced each time this option has been enabled/used.

Improve messaging about this option being detected in command-line use and documentation, and perhaps even make the log line more explicit.

@cri-o/cri-o-maintainers, thoughts?

bitoku · 2024-05-10T12:45:50Z

/retest

haircommander · 2024-05-10T20:13:50Z

contrib/test/ci/setup.yml

- name: add quay.io and docker.io as default registries
-  copy:
-    dest: /etc/crio/crio.conf.d/01-registries.conf
-    content: |
-      [crio.image]
-      registries = [ "quay.io", "docker.io" ]
-


we do need this config in /etc/containers/registries.conf.d

Do we?
It hasn't been used since #4455.
According to the log, it wasn't used.
https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/cri-o_cri-o/8102/pull-ci-cri-o-cri-o-main-ci-fedora-integration/1786581177407115264/artifacts/fedora-integration/cri-o-gather/artifacts/testout.txt

# time="2024-05-04T02:38:42Z" level=warning msg="Support for the 'registries' option has been dropped but it is referenced in \"/etc/crio/crio.conf.d/01-registries.conf\". Please use containers-registries.conf(5) for configuring unqualified-search registries instead."

I don't know how we configure the registries.conf in prow CIs, but at least for github actions, there is already registries.conf.

cri-o/scripts/github-actions-setup

Line 161 in 9712c53

sudo cp "$REPO_ROOT"/test/registries.conf /etc/containers/registries.conf

cri-o/test/registries.conf

Lines 1 to 4 in 9712c53

unqualified-search-registries = ['quay.io' ,'registry.access.redhat.com', 'registry.fedoraproject.org', 'docker.io']

[aliases]

"image-for-testing" = "registry.crio.test.com/repo"

yeah since we have completely different install paths for github actions and prow, I think we may still need this.

can you try with it to see if the ci failures are due to this?

jk I think failures are not flakes from this #8167

@bitoku @haircommander, what would be the way forward here?

seems like we don't need this anymore because it passed CI?

haircommander · 2024-05-10T20:15:36Z

I would be okay with dropping support already. you're correct in that it's been deprecated for a while, and there is a better replacement with registries.conf

cc @mtrmac

mtrmac · 2024-05-13T13:28:38Z

In general, the better replacement is “don’t do this at all”. unqualified-search-registries is invisible to Kubelet and interacts extremely badly with Kubelet’s code to choose a credential to use in the ImagePull operation (it will always send a docker.io credential for short image names, if one is available to Kubelet, and that credential will be sent to the target registry).

So I’m fully in favor of removing anything which makes this misfeature easy to configure, or indeed, reachable by CRI-O users at all.

Do not add any new users of this option, especially not by default. (Yes, https://github.com/openshift/machine-config-operator/blob/master/templates/master/01-master-container-runtime/_base/files/container-registries.yaml does that currently.)

Users who want a layer of indirection so that their Pod definitions don’t change in different deployment sites should define a “canonical” location for each image, which includes a host name (vendor.example/product/component:version, and use registries.d’s prefix/location redirection mechanism (in OpenShift, ImageContentSourcePolicy / ImageTagMirrorSet / ImageDigestMirrorSet). That works consistently everywhere, and it scales across many different products/components — unlike short names, where different product might result in conflicting short names and no way to resolve that.

haircommander · 2024-05-13T19:27:31Z

great points, which further supports removing this field (so users at least have one less place to be able to incorrectly specify)

It would be cool to drop support for short names generally but given the precedent of following docker's age-old lead, that may be tricky.

haircommander · 2024-05-13T19:28:50Z

/retest

bitoku · 2024-05-14T16:39:26Z

/retest

kwilczynski · 2024-05-15T02:55:21Z

/retest

bitoku · 2024-05-15T14:28:14Z

So is our way to go to drop this feature completely now, not adding more deprecation messages?
Or is this PR ok as the first step to drop this feature in the future?

kwilczynski · 2024-05-15T23:03:13Z

So is our way to go to drop this feature completely now, not adding more deprecation messages? Or is this PR ok as the first step to drop this feature in the future?

@bitoku, we should drop it completely.

It's been a while since this wasn't functional per: #8161 (comment). I also think that @haircommander has nothing against doing so and that it would be fine to retire this code... provided that we don't break things (like the need to have the configuration file; albeit, you pointed out that CI has passed with the file being removed).

@cri-o/cri-o-maintainers, thoughts about removing this old code?

saschagrunert · 2024-05-16T14:50:11Z

Yeah I guess we can remove it. 👍

bitoku · 2024-05-16T20:57:15Z

Thank you all!
Then I will create a new PR and will close this one.

bitoku requested a review from mrunalp as a code owner May 10, 2024 00:59

openshift-ci bot requested review from QiWang19 and wgahnagl May 10, 2024 00:59

bitoku force-pushed the registries-deprecation branch from 8d6d336 to fab5195 Compare May 10, 2024 01:12

delete deprecated registries config

0a65e7b

Signed-off-by: Ayato Tokubi <atokubi@redhat.com>

bitoku force-pushed the registries-deprecation branch from fab5195 to 0a65e7b Compare May 10, 2024 01:20

openshift-ci bot 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 May 10, 2024

kwilczynski assigned kwilczynski and bitoku May 10, 2024

kwilczynski reviewed May 10, 2024

View reviewed changes

haircommander reviewed May 10, 2024

View reviewed changes

bitoku mentioned this pull request May 17, 2024

Remove registries config and --registry option #8194

Open

bitoku closed this May 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete deprecated registries config #8161

Delete deprecated registries config #8161

bitoku commented May 10, 2024

openshift-ci bot commented May 10, 2024

openshift-ci bot commented May 10, 2024

codecov bot commented May 10, 2024 •

edited

kwilczynski commented May 10, 2024

kwilczynski May 10, 2024

saschagrunert May 16, 2024 •

edited

kwilczynski May 16, 2024

saschagrunert May 16, 2024

kwilczynski May 16, 2024

bitoku commented May 10, 2024

haircommander May 10, 2024

bitoku May 11, 2024 •

edited

haircommander May 13, 2024

haircommander May 13, 2024

kwilczynski May 15, 2024

bitoku May 15, 2024

haircommander commented May 10, 2024

mtrmac commented May 13, 2024

haircommander commented May 13, 2024

haircommander commented May 13, 2024

bitoku commented May 14, 2024

kwilczynski commented May 15, 2024

bitoku commented May 15, 2024

kwilczynski commented May 15, 2024

saschagrunert commented May 16, 2024

bitoku commented May 16, 2024

	// Registries are deprecated in cri-o.conf and turned into a NOP.
	// Users should use registries.conf instead, so let's log it.
	if len(t.Crio.Image.Registries) > 0 {
	t.Crio.Image.Registries = nil
	logrus.Warnf("Support for the 'registries' option has been dropped but it is referenced in %q. Please use containers-registries.conf(5) for configuring unqualified-search registries instead.", path)
	}

	unqualified-search-registries = ['quay.io' ,'registry.access.redhat.com', 'registry.fedoraproject.org', 'docker.io']

	[aliases]
	"image-for-testing" = "registry.crio.test.com/repo"

Delete deprecated registries config #8161

Delete deprecated registries config #8161

Conversation

bitoku commented May 10, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

openshift-ci bot commented May 10, 2024

openshift-ci bot commented May 10, 2024

codecov bot commented May 10, 2024 • edited

Codecov Report

kwilczynski commented May 10, 2024

Choose a reason for hiding this comment

saschagrunert May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitoku commented May 10, 2024

Choose a reason for hiding this comment

bitoku May 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haircommander commented May 10, 2024

mtrmac commented May 13, 2024

haircommander commented May 13, 2024

haircommander commented May 13, 2024

bitoku commented May 14, 2024

kwilczynski commented May 15, 2024

bitoku commented May 15, 2024

kwilczynski commented May 15, 2024

saschagrunert commented May 16, 2024

bitoku commented May 16, 2024

codecov bot commented May 10, 2024 •

edited

saschagrunert May 16, 2024 •

edited

bitoku May 11, 2024 •

edited