Skip to content

Conversation

@ahardin-rh
Copy link
Contributor

Addresses #27612

@ahardin-rh ahardin-rh added branch/enterprise-4.6 branch/enterprise-4.7 branch/enterprise-4.8 issue-burndown Indicates an issue worked on during an "GitHub Issue Burndown" session labels Mar 23, 2021
@ahardin-rh ahardin-rh added this to the Next Release milestone Mar 23, 2021
@ahardin-rh ahardin-rh self-assigned this Mar 23, 2021
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 23, 2021
@netlify
Copy link

netlify bot commented Mar 23, 2021

Deploy preview for osdocs ready!

Built with commit 33f47d1

https://deploy-preview-30805--osdocs.netlify.app

@ahardin-rh
Copy link
Contributor Author

@kikisdeliveryservice @cgwalters @yuqi-zhang Can you please confirm this change? Thank you!

@yuqi-zhang
Copy link

This does seem to match better with the upstream docs, although the containers team probably has more insight into this. CC'ing @vrothberg and @umohnani8 for the review

@ahardin-rh
Copy link
Contributor Author

@yuqi-zhang Thank you!

@umohnani8
Copy link

Any reason we document the MC way of making changes to these low level files? We have the cluster wide Image CR and the ImageContentSourcePolicy CR that users can use to make their registry changes and the controller updates the appropriate files on the node accordingly.

@ahardin-rh
Copy link
Contributor Author

@umohnani8 I'm not sure exactly. Looks like this content was introduced in #25823. I'm not very familiar with this area of content, but I'm happy to update the procedures with your guidance. Thank you!

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Thanks for the ping. Some changes need to be reverted.

The naming of the two directories is unfortunate and very confusing; historical reasons. The rule of thumb is that .conf (TOML) files go into registries.CONF.d. YAMLs for signature policies etc. go into `registries.d.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, the previous path was correct. Signatures go into registries.d.

Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Member

Choose a reason for hiding this comment

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

And here :)

Copy link
Member

Choose a reason for hiding this comment

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

And here.

Choose a reason for hiding this comment

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

registries.conf.d is actually what we need here as it is talking about adding a drop-in file to modify unqualified-search-registries.

Choose a reason for hiding this comment

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

@ahardin-rh looks like this is a bit outdated. In 4.7, we added a new option to the cluster wide Image CR - containerRuntimeSearchRegistries. Users can use that set their configured list of unqualified-search-registries and the controller rolls out the changes to the appropriate nodes. However, we heavily advice against using unqualified-search-registries and it was documented by https://github.com/openshift/openshift-docs/pull/28152/files.

I think we should not document how someone can do this with a MC, we want users to use our CRDs for making such changes to the node. Signatures are not supported yet, so using an MC for that makes sense, but that is something we are looking to combine into the Image CRD in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@umohnani8 Thank you! Should I create a new PR to remove this procedure from the 4.7+ doc set then?

FYI @mburke5678

Choose a reason for hiding this comment

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

Yes please, it should be removed from the 4.7 and later docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@umohnani8 Thank you! I will get this work merged and then remove the procedure from 4.7 and 4.8 in a follow-up PR. Thanks!

sh-4.4# cat /etc/containers/registries.conf.d/99-worker-unqualified-search-registries.conf
unqualified-search-registries = ['registry.access.redhat.com', 'docker.io', 'quay.io']
sh-4.4# exit
----
Copy link
Contributor

@lbarbeevargas lbarbeevargas Mar 24, 2021

Choose a reason for hiding this comment

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

The update LGTM. Should these sh-4.4# commands be separated into individual code blocks? Not sure if there is a reason that they weren't when the original work was done to separate multiple commands and commands from their output. :)

automatically place the file on each node in your cluster. No service
restart is required since policy and `registries.d` files are dynamically
loaded by the container runtime.
loaded by the container runtime.
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this file is just picking up removing extra whitespace at the end of these lines. What do you think about undoing the hard line wraps in this module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we'll need to do some follow-up work there, but that's out of scope for this PR. Thanks!

@lbarbeevargas lbarbeevargas added the peer-review-done Signifies that the peer review team has reviewed this PR label Mar 24, 2021
@lbarbeevargas
Copy link
Contributor

A couple of minor comments, but otherwise LGTM!

@ahardin-rh ahardin-rh merged commit 3370c30 into openshift:master Mar 24, 2021
@ahardin-rh
Copy link
Contributor Author

/cherrypick enterprise-4.6

@ahardin-rh
Copy link
Contributor Author

/cherrypick enterprise-4.7

@ahardin-rh
Copy link
Contributor Author

/cherrypick enterprise-4.8

@openshift-cherrypick-robot

@ahardin-rh: new pull request created: #30869

Details

In response to this:

/cherrypick enterprise-4.6

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-cherrypick-robot

@ahardin-rh: new pull request created: #30870

Details

In response to this:

/cherrypick enterprise-4.7

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-cherrypick-robot

@ahardin-rh: new pull request created: #30871

Details

In response to this:

/cherrypick enterprise-4.8

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.6 branch/enterprise-4.7 branch/enterprise-4.8 issue-burndown Indicates an issue worked on during an "GitHub Issue Burndown" session peer-review-done Signifies that the peer review team has reviewed this PR size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants