Issues with puppet-katello

ekohl · August 23, 2016, 3:02pm

Hello all,

There are a few things with puppet-katello that I think we can fix and
make it more modular and closer to foreman / a basic foreman plugin.

Odd permissions on katello bundler file

github.com

theforeman/puppet-katello/blob/8b4bb6924c5911fb572bb8c19667186e56c8f71b/manifests/config.pp

# Katello Config
class katello::config {

  $apache_version = $::apache::apache_version

  class { '::katello::config::pulp_client': }

  file { '/usr/share/foreman/bundler.d/katello.rb':
    ensure => file,
    owner  => $katello::user,
    group  => $katello::group,
    mode   => '0644',
  }

  file { "${katello::config_dir}/katello.yaml":
    ensure  => file,
    content => template('katello/katello.yaml.erb'),
    owner   => $katello::user,
    group   => $katello::group,
    mode    => '0644',

This file has been truncated. show original

All other bundler files are owned by root:root and since packaging is
used I think this can be changed to root:root.

Odd config file

github.com

theforeman/puppet-katello/blob/8b4bb6924c5911fb572bb8c19667186e56c8f71b/manifests/config.pp

# Katello Config
class katello::config {

  $apache_version = $::apache::apache_version

  class { '::katello::config::pulp_client': }

  file { '/usr/share/foreman/bundler.d/katello.rb':
    ensure => file,
    owner  => $katello::user,
    group  => $katello::group,
    mode   => '0644',
  }

  file { "${katello::config_dir}/katello.yaml":
    ensure  => file,
    content => template('katello/katello.yaml.erb'),
    owner   => $katello::user,
    group   => $katello::group,
    mode    => '0644',

This file has been truncated. show original

Again this file is owned by foreman while all other configs are owned by
root:root. Here I see no reason either.

Another thing is that this has a before foreman::database and the
migrate but puppet-foreman executes the database class before placing
the config. What's the reasoning behind this?

Katello apache fragment is mostly pulp and candlepin

github.com

theforeman/puppet-katello/blob/8b4bb6924c5911fb572bb8c19667186e56c8f71b/manifests/config.pp

# Katello Config
class katello::config {

  $apache_version = $::apache::apache_version

  class { '::katello::config::pulp_client': }

  file { '/usr/share/foreman/bundler.d/katello.rb':
    ensure => file,
    owner  => $katello::user,
    group  => $katello::group,
    mode   => '0644',
  }

  file { "${katello::config_dir}/katello.yaml":
    ensure  => file,
    content => template('katello/katello.yaml.erb'),
    owner   => $katello::user,
    group   => $katello::group,
    mode    => '0644',

This file has been truncated. show original

This sets up pulp and candlepin for which I'd propose to introduce two
separate fragments.

It also manages keepalive which IMHO belongs in puppet-foreman.

Katello config dir

github.com

theforeman/puppet-katello/blob/8b4bb6924c5911fb572bb8c19667186e56c8f71b/manifests/config.pp

# Katello Config
class katello::config {

  $apache_version = $::apache::apache_version

  class { '::katello::config::pulp_client': }

  file { '/usr/share/foreman/bundler.d/katello.rb':
    ensure => file,
    owner  => $katello::user,
    group  => $katello::group,
    mode   => '0644',
  }

  file { "${katello::config_dir}/katello.yaml":
    ensure  => file,
    content => template('katello/katello.yaml.erb'),
    owner   => $katello::user,
    group   => $katello::group,
    mode    => '0644',

This file has been truncated. show original

Can we get rid of this?

Future

Ideally puppet-katello would be more like a profile module. We'd have a
foreman::plugin::katello that installs just the katello plugin to
Foreman. Then it also combines other modules to have a katello profile.
That means we have to move some things to other modules
(pulp::config::foreman instead of sneaking it into puppet-katello?).

Please give your thoughts on my ramblings.

ehelms · August 23, 2016, 5:40pm

> Hello all,
>
> There are a few things with puppet-katello that I think we can fix and
> make it more modular and closer to foreman / a basic foreman plugin.
>
> # Odd permissions on katello bundler file
>
> https://github.com/Katello/puppet-katello/blob/
> 8b4bb6924c5911fb572bb8c19667186e56c8f71b/manifests/config.pp#L8-L13\

>
> All other bundler files are owned by root:root and since packaging is
> used I think this can be changed to root:root.
>

+1

>
> # Odd config file
>
> https://github.com/Katello/puppet-katello/blob/
> 8b4bb6924c5911fb572bb8c19667186e56c8f71b/manifests/config.pp#L15-L23
>
> Again this file is owned by foreman while all other configs are owned by
> root:root. Here I see no reason either.
>
> Another thing is that this has a before foreman::database and the
> migrate but puppet-foreman executes the database class before placing
> the config. What's the reasoning behind this?
>

This is likely due to some migrations requiring some of the settings in the
config file to work properly. On the Foreman side, this would potentially
affect taxonomies? On the Katello side, this would affect some of our
migrations that unfortunately reach out to backend services. I think its a
change that if we wanted to make would need to be thoroughly tested to see
what if any impact.

>
> # Katello apache fragment is mostly pulp and candlepin
>
> https://github.com/Katello/puppet-katello/blob/
> 8b4bb6924c5911fb572bb8c19667186e56c8f71b/manifests/config.pp#L25-L28
>
> This sets up pulp and candlepin for which I'd propose to introduce two
> separate fragments.
>
> It also manages keepalive which IMHO belongs in puppet-foreman.
>

We could look into splitting the fragments up. I want to say the keepalive
was due to needing it and not having a way (that would get into the code
base in time) for us to set it. This could likely be updated.

>
> # Katello config dir
>
> https://github.com/Katello/puppet-katello/blob/
> 8b4bb6924c5911fb572bb8c19667186e56c8f71b/manifests/config.pp#L30-L35
>
> Can we get rid of this?
>

+1

>
> # Future
>
> Ideally puppet-katello would be more like a profile module. We'd have a
> foreman::plugin::katello that installs just the katello plugin to
> Foreman. Then it also combines other modules to have a katello profile.
> That means we have to move some things to other modules
> (pulp::config::foreman instead of sneaking it into puppet-katello?).
>

This sounds like baking into puppet-pulp and puppet-candlepin, as examples,
knowledge of Katello and Foremanisms. We've largely tried to keep those
modules as independent modules that the community can widely use and in
which we tell them what we need altered and changed. There is currently a
decent chunk of orchestration between katello-pulp-candlepin that makes
ripping them apart harder. Thats the part that would need some thought and
re-work: what we configure and how we orchestrate. Loosening that boundary
as much as possible would help here I think.

Eric

···

On Tue, Aug 23, 2016 at 11:02 AM, Ewoud Kohl van Wijngaarden < ewoud@kohlvanwijngaarden.nl> wrote:

Please give your thoughts on my ramblings.

–
You received this message because you are subscribed to the Google Groups
"foreman-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

–
Eric D. Helms
Red Hat Engineering
Ph.D. Student - North Carolina State University

ekohl · August 25, 2016, 1:42pm

>
> > Hello all,
> >
> > There are a few things with puppet-katello that I think we can fix and
> > make it more modular and closer to foreman / a basic foreman plugin.
> >
> > # Odd permissions on katello bundler file
> >
> > https://github.com/Katello/puppet-katello/blob/
> > 8b4bb6924c5911fb572bb8c19667186e56c8f71b/manifests/config.pp#L8-L13
> >
> > All other bundler files are owned by root:root and since packaging is
> > used I think this can be changed to root:root.
> >
>
> +1

>
> >
> > # Odd config file
> >
> > https://github.com/Katello/puppet-katello/blob/
> > 8b4bb6924c5911fb572bb8c19667186e56c8f71b/manifests/config.pp#L15-L23
> >
> > Again this file is owned by foreman while all other configs are owned by
> > root:root. Here I see no reason either.
> >
> > Another thing is that this has a before foreman::database and the
> > migrate but puppet-foreman executes the database class before placing
> > the config. What's the reasoning behind this?
> >
>
> This is likely due to some migrations requiring some of the settings in the
> config file to work properly. On the Foreman side, this would potentially
> affect taxonomies? On the Katello side, this would affect some of our
> migrations that unfortunately reach out to backend services. I think its a
> change that if we wanted to make would need to be thoroughly tested to see
> what if any impact.

We should dive into this. Other plugins that make database changes might
have similar problems.

>
> >
> > # Katello apache fragment is mostly pulp and candlepin
> >
> > https://github.com/Katello/puppet-katello/blob/
> > 8b4bb6924c5911fb572bb8c19667186e56c8f71b/manifests/config.pp#L25-L28
> >
> > This sets up pulp and candlepin for which I'd propose to introduce two
> > separate fragments.
> >
> > It also manages keepalive which IMHO belongs in puppet-foreman.
> >
>
> We could look into splitting the fragments up. I want to say the keepalive
> was due to needing it and not having a way (that would get into the code
> base in time) for us to set it. This could likely be updated.

It looks like puppetlabs-apache now has proper options for this. We even
have options for this in puppet-foreman[1] but they use a custom
fragment for this. That should get updated.

This means we can at least drop KeepAlive On from the template. Then we
need to decide what to do with $katello::max_keep_alive which attempts
to configure the same value as $foreman::max_keepalive_requests but have
wildly differing defaults (10000 vs 100).

[1] https://github.com/theforeman/puppet-foreman/commit/0a6966ce22111b1ce592b5e215a948b3734680e7

> >
> > # Katello config dir
> >
> > https://github.com/Katello/puppet-katello/blob/
> > 8b4bb6924c5911fb572bb8c19667186e56c8f71b/manifests/config.pp#L30-L35
> >
> > Can we get rid of this?
> >
>
> +1

https://github.com/Katello/puppet-katello/pull/140 removes this and the
unused client.conf.erb.

> >
> > # Future
> >
> > Ideally puppet-katello would be more like a profile module. We'd have a
> > foreman::plugin::katello that installs just the katello plugin to
> > Foreman. Then it also combines other modules to have a katello profile.
> > That means we have to move some things to other modules
> > (pulp::config::foreman instead of sneaking it into puppet-katello?).
> >
>
> This sounds like baking into puppet-pulp and puppet-candlepin, as examples,
> knowledge of Katello and Foremanisms. We've largely tried to keep those
> modules as independent modules that the community can widely use and in
> which we tell them what we need altered and changed. There is currently a
> decent chunk of orchestration between katello-pulp-candlepin that makes
> ripping them apart harder. Thats the part that would need some thought and
> re-work: what we configure and how we orchestrate. Loosening that boundary
> as much as possible would help here I think.

You may be right about this. Having it in foreman::plugin::katello::pulp
and foreman::plugin::katello::candlepin might be better. A start could
be to separate them inside puppet-katello first to get a logical
separation.

···

On Tue, Aug 23, 2016 at 01:40:44PM -0400, Eric D Helms wrote: > On Tue, Aug 23, 2016 at 11:02 AM, Ewoud Kohl van Wijngaarden < > ewoud@kohlvanwijngaarden.nl> wrote:

ekohl · August 25, 2016, 2:00pm

> > >
> > > # Katello apache fragment is mostly pulp and candlepin
> > >
> > > https://github.com/Katello/puppet-katello/blob/
> > > 8b4bb6924c5911fb572bb8c19667186e56c8f71b/manifests/config.pp#L25-L28
> > >
> > > This sets up pulp and candlepin for which I'd propose to introduce two
> > > separate fragments.
> > >
> > > It also manages keepalive which IMHO belongs in puppet-foreman.
> > >
> >
> > We could look into splitting the fragments up. I want to say the keepalive
> > was due to needing it and not having a way (that would get into the code
> > base in time) for us to set it. This could likely be updated.
>
> It looks like puppetlabs-apache now has proper options for this. We even
> have options for this in puppet-foreman[1] but they use a custom
> fragment for this. That should get updated.

> This means we can at least drop KeepAlive On from the template. Then we
> need to decide what to do with $katello::max_keep_alive which attempts
> to configure the same value as $foreman::max_keepalive_requests but have
> wildly differing defaults (10000 vs 100).

···

On Thu, Aug 25, 2016 at 03:42:04PM +0200, Ewoud Kohl van Wijngaarden wrote: > On Tue, Aug 23, 2016 at 01:40:44PM -0400, Eric D Helms wrote: > > On Tue, Aug 23, 2016 at 11:02 AM, Ewoud Kohl van Wijngaarden < > > ewoud@kohlvanwijngaarden.nl> wrote: