-
Notifications
You must be signed in to change notification settings - Fork 48
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.
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
Adding pulp3 container registry config #296
Conversation
afc6eba
to
3701f94
Compare
3701f94
to
507c1a1
Compare
507c1a1
to
74f1a7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too happy about this. By that I don't mean the exact implementation here, but really the core implementation. I would really like to get rid of specifying URLs in the settings file.
Is there a general design that describes how this is supposed to work? Would it make sense to treat this more like we do compute resources: have a database model that allows configuring a remote registry? Otherwise a Smart Proxy plugin that allows to do service discovery.
The reason I'm hesitant in merging this is that with Pulp 3 we have a chance to design it right.
@@ -14,6 +14,7 @@ | |||
String $candlepin_oauth_secret = $katello::candlepin_oauth_secret, | |||
Stdlib::Httpsurl $pulp_url = $katello::pulp_url, | |||
Stdlib::Httpsurl $crane_url = $katello::crane_url, | |||
Stdlib::HTTPUrl $pulp_registry_url = $katello::pulp_registry_url, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have crane_url just above it. Isn't that the same thing? Or is this what the end user connects to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pulp3 acts as its own registry, so this is a separate URL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to not just rename crane_url to pulp_registry_url and just use a single setting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea being that the installer would configure the url based on pulp2 or pulp3 being available for docker.
(Keep in mind you're getting a bit ahead of the game here, we haven't really been adding pulp3 support to the installer yet)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jlsherrill In the current implementation, determining whether the instance supports pulp3 and container content within pulp3 is done in Katello so both URLs are required afaict. Is there another way to go about this?
EDIT: saw your second comment, please disregard this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or that. From the code it looks like at most one will be used. This is exactly what was suggested in #243 and would have avoided this whole rename now.
That said, I'd still like to see this automatically detected rather than in a config file.
I get your concern and it sounds like something that would be beneficial. Though, this does seem like a larger feature that needs to be planned and have devs responsible for it. I'm not sure what else to do except copy the existing pattern at this time.
I know we have pulp3 as a feature of a Smart Proxy, but I don't think you can find the granular information of what repository type is supported, which we would need to know to make sure pulp3 supports container repos so it can be used as the registry. The logic is basically "If pulp3 is present and can sync container repositories, use pulp3 as the registry URL, else use crane" Can what I have here be changed to the new model if that is being developed in the future? |
Of course it can. I do wonder if there's a difference on API implementation between crane and Pulp 3. If not, then you can start with container_registries:
- url: $crane
type: crane
- url: $pulp3
type: pulp3 I should also admit this already bugged me when this was initially added. The reason I mention the smart proxy is that we have a generic framework to pass settings now (https://github.com/theforeman/smart_proxy_pulp/blob/99b7d365cd6f4c111161c0cd37dac40415596cc1/lib/smart_proxy_pulp_plugin/pulp3_plugin.rb#L14-L15). This would allow us to set this all up in the installer at once when installing the pulp + smart proxy and the main Foreman installation doesn't need to know about this up front. We also have capabilities for each content type that are queried from the Pulp 3 API when a Smart Proxy is registered within Foreman. Within Katello this is exposed as content_pulp3_supprt?(content_type). I hope these features can be leveraged to automatically find the right Pulp3 instance or see how we can expand this where needed. |
@ekohl There isn't a difference between the APIs, but the idea is to talk with one or the other based on whether pulp3 is on the system and supports container repositories. Right now we have to make the distinction between the two registries. This line in the Katello PR makes that distinction with the
Are you thinking this info may be better to keep in the Settings rather than the config? Thanks for sharing this info, let me think about other ways to approach this based on what you shared and discuss with the pulp3 team. |
So if it already works based on the Pulp 3 setting, can't we retrieve the Pulp registry URL from there? If it's different than the Pulp URL itself, then it's probably still the same hostname. My reasoning for this is that I want to support a setup where there's:
Or even:
This would making scaling a setup much easier. The installation instructions would be:
So in short:
|
Probably something like theforeman/smart_proxy_pulp#19. Note that you need to refresh the features on the proxy (or reregister using the installer) for Foreman to store the new value. A more advanced example would use a Programmable Settings setup that automatically derives it from the Pulp URL if unset (https://github.com/theforeman/smart-proxy/search?q=load_programmable_settings&unscoped_q=load_programmable_settings) which makes life easier for the user.
I think ekohl/katello@3367309 should do this. Note that I didn't test any of this, but it shows the general principle I'm advocating for. |
This has been merged with a slight change: it's now
After offline discussion with @jlsherrill we decided to be explicit about this since the installer should already be aware of this in the future so there wasn't as much value. |
Closing since we are using the smart proxy now! |
No description provided.