Skip to content
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

Delete deprecated registries config #8161

Closed
wants to merge 1 commit into from
Closed

Delete deprecated registries config #8161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion completions/fish/crio.fish
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ complete -c crio -n '__fish_crio_no_subcommand' -f -l profile-mem -r -d 'Write a
complete -c crio -n '__fish_crio_no_subcommand' -f -l profile-port -r -d 'Port for the pprof profiler.'
complete -c crio -n '__fish_crio_no_subcommand' -f -l rdt-config-file -r -d 'Path to the RDT configuration file for configuring the resctrl pseudo-filesystem.'
complete -c crio -n '__fish_crio_no_subcommand' -f -l read-only -d 'Setup all unprivileged containers to run as read-only. Automatically mounts the containers\' tmpfs on \'/run\', \'/tmp\' and \'/var/tmp\'.'
complete -c crio -n '__fish_crio_no_subcommand' -f -l registry -r -d 'Registry to be prepended when pulling unqualified images. Can be specified multiple times.'
complete -c crio -n '__fish_crio_no_subcommand' -f -l registry -r -d 'Registry to be prepended when pulling unqualified images. Can be specified multiple times. This option is deprecated, and it has no effect.'
complete -c crio -n '__fish_crio_no_subcommand' -l root -s r -r -d 'The CRI-O root directory.'
complete -c crio -n '__fish_crio_no_subcommand' -l runroot -r -d 'The CRI-O state directory.'
complete -c crio -n '__fish_crio_no_subcommand' -f -l runtimes -r -d 'OCI runtimes, format is \'runtime_name:runtime_path:runtime_root:runtime_type:privileged_without_host_devices:runtime_config_path:container_min_memory\'.'
Expand Down
7 changes: 0 additions & 7 deletions contrib/test/ci/setup.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,5 @@
[crio]
storage_driver = "overlay"

- 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" ]

Comment on lines -74 to -80
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@bitoku bitoku May 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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"

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

jk I think failures are not flakes from this #8167

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

- name: build parallel
include_tasks: "build/parallel.yml"
2 changes: 1 addition & 1 deletion docs/crio.8.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ crio [GLOBAL OPTIONS] command [COMMAND OPTIONS] [ARGUMENTS...]

**--read-only**: Setup all unprivileged containers to run as read-only. Automatically mounts the containers' tmpfs on '/run', '/tmp' and '/var/tmp'.

**--registry**="": Registry to be prepended when pulling unqualified images. Can be specified multiple times.
**--registry**="": Registry to be prepended when pulling unqualified images. Can be specified multiple times. This option is deprecated, and it has no effect.

**--root, -r**="": The CRI-O root directory. (default: "/var/lib/containers/storage")

Expand Down
2 changes: 1 addition & 1 deletion internal/criocli/criocli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,7 @@ func getCrioFlags(defConf *libconfig.Config) []cli.Flag {
&cli.StringSliceFlag{
Name: "registry",
Value: cli.NewStringSlice(defConf.Registries...),
Usage: "Registry to be prepended when pulling unqualified images. Can be specified multiple times.",
Usage: "Registry to be prepended when pulling unqualified images. Can be specified multiple times. This option is deprecated, and it has no effect.",
EnvVars: []string{"CONTAINER_REGISTRY"},
},
&cli.StringFlag{
Expand Down
2 changes: 2 additions & 0 deletions pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

@saschagrunert saschagrunert May 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Would this stand in for deprecation?

This has been defunct for about two years now.

Copy link
Member

Choose a reason for hiding this comment

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

Somehow, yes.

Copy link
Member

Choose a reason for hiding this comment

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

We can:

  1. 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.

  2. 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?

// Temporary directory for big files
BigFilesTemporaryDir string `toml:"big_files_temporary_dir"`
Expand Down
Loading