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.

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

Fixes #35157 - Enable Pulp's content caching on Katello #794

Merged
merged 1 commit into from Jul 5, 2022

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Jul 5, 2022

Like 4155c66 but for the Katello scenario.

@ehelms
Copy link
Member

ehelms commented Jul 5, 2022

This commentary does not block this change, as it is the current state of our code. Following up from a discussion on IRC, this setting seems to now be required by Katello and therefore is a not optional. There was then some follow on discussion around whether this option should be removed and hard set to avoid user's inadvertently breaking their setups. @evgeni do you recall if that discussion went further or if we arrived at an outcome?

@evgeni
Copy link
Member

evgeni commented Jul 5, 2022

I think we (@ianballou and I) reached a "uuuh, not sure" point in that IRC discussion.

The only real dependency of Katello needing Pulp with caching enabled is the code in ping.rb.

I think what broke us is the change in pulp/pulpcore#1752 which made the Pulp status endpoint report "redis_connection": {"connected": False} when the cache is disabled instead if "redis_connection": None as it was before. The older result is correctly caught by if json['redis_connection'] in ping.rb.

I'd argue this is a bug in Pulp, reporting a Redis connection which is not configured as "disconnected" vs "not configured".

(The change in this PR is still good, we do want cache enabled by default)

Edit: I pinged in #pulp-dev to look at this comment.

@fao89
Copy link

fao89 commented Jul 5, 2022

The only real dependency of Katello needing Pulp with caching enabled is the code in ping.rb.

I think what broke us is the change in pulp/pulpcore#1752 which made the Pulp status endpoint report "redis_connection": {"connected": False} when the cache is disabled instead if "redis_connection": None as it was before. The older result is correctly caught by if json['redis_connection'] in ping.rb.

I agree with your assessment, but I wonder if changing it now would break other users

@ianballou
Copy link
Contributor

Yeah, I'd agree that, ideally, the connected: False would only pop up if it was specifically configured as such. We might be able to drop this check entirely in Katello, but we'd need to test without Redis. I'll add a redmine to check up on that.

With the current state of Katello's code though, I'd say let's get this merged to avoid more user issues.

@ehelms ehelms merged commit b6305f7 into theforeman:develop Jul 5, 2022
@ekohl ekohl deleted the 35157-enable-pulp-content-caching branch July 5, 2022 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants