-
Notifications
You must be signed in to change notification settings - Fork 66
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 #13451 - enables lazy sync #122
Conversation
Does Pulp turn this on by default as well? |
Its off by default in pulp. We need it on for katello though. |
d32779b
to
f388be4
Compare
684b4d2
to
56b88c3
Compare
@@ -67,7 +67,7 @@ | |||
$email_from = "no-reply@${::domain}" | |||
$email_enabled = false | |||
|
|||
$lazy_enabled = false | |||
$lazy_enabled = true |
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.
If Pulp doesn;t enable this by default, neither should the module. This should be up to users of the module such as puppet-katello to enable.
@@ -97,4 +97,22 @@ | |||
refreshonly => true, | |||
} | |||
} | |||
|
|||
if $pulp::lazy_enabled { | |||
class { '::squid3': |
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.
Is squid the Pulp supported option for lazy sync? @cristifalcas would you need a parameter here such as manage_squid
or is this setup and configuration of squid if lazy sync is enabled OK?
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.
its not the only supported option, any http proxy would work i would imagine, but they provide same configs for two different proxies, squid and one more that isn't in the base OS.
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.
also pulp removed the lazy_enabled option from their config in the latest 2.8 nightly: pulp/pulp@0f100db
so maybe it makes more sense to just move to to a manage_squid option
8bb6db1
to
6a5e937
Compare
@@ -67,7 +67,7 @@ | |||
$email_from = "no-reply@${::domain}" | |||
$email_enabled = false | |||
|
|||
$lazy_enabled = false | |||
$manage_squid = true |
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 think we want this to be false by default, and turn it on here:
https://github.com/Katello/puppet-katello/blob/master/manifests/init.pp#L96-L119
and
https://github.com/Katello/puppet-capsule/blob/master/manifests/init.pp#L163-L187
and
https://github.com/Katello/puppet-katello_devel/blob/master/manifests/init.pp#L126-L144
and yes there has been discussion on consolidating these parts :)
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.
If lazy is simply just "on" in Pulp now, and it requires a proxy like this squid setup to be configured then having this set to true by default is fine. I equate this to how it manages and configures mongo and apache by default.
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.
Its not that its 'on' or 'off'. Its more that if you create a repo with a download policy of 'on_demand' or 'background' it requires squid to be configured and running.
I believe pulp is still using the 'immediate' policy by default which means normal pulp users not using the new feature won't need squid
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.
Does it just continue to work as before if you use those settings but do not have squid?
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.
it does, yes
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.
if you use the immediate setting i mean, if you use on_demand or background, requests from clients will get a 404 i believe
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.
Seems like a bad user experience, but whatever we can go this route and flip the bit if a user of the module comes along and wants it flipped.
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.
works4me
6c8480e
to
8288860
Compare
http_access => [ 'allow localhost', 'deny to_localhost', 'deny all' ], | ||
cache => [ 'allow all' ], | ||
maximum_object_size => '5 GB', | ||
maximum_object_size_in_memory => '100 MB', |
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 might would make this smaller, I'm thinking somewhere closer to 1MB. I'm okay leaving this as is for now and adjusting later, but I think it needs to be tested for memory consumption at some point.
ACK - thanks @cfouant ! |
Fixes #13451 - enables lazy sync
Fixes #13451 - enables lazy sync
No description provided.