Redesign classes to allow split deployments #308

ekohl · 2019-11-30T11:21:51Z

Every class that provides an aspect of the deployment is now standalone. This means:

candlepin
pulp
qpid
application

They can just be included and work by themselves. All class parameters are tunables that users will commonly modify.

There's also shared parameters which are common among the standalone classes. These are meant to be changed via katello::params. Either via Hiera or by including them in a profile class before the instance classes themselves.

The katello::globals class has globals that a user via the installer might want to change. The katello::params class is not intended to be exposed via the installer.

Currently a draft because I want to do testing with rewriting the changes in the installer. Having this in a PR makes that easier.

Includes:

Implement automatic dependency chaining #306
Drop old workarounds #307 (only the drop unused variable)

ehelms · 2019-12-02T13:33:05Z

I'm trying to follow the over arching design goals of this change and I am having trouble. I think what I need to understand more is the larger goal that we're driving towards. I have for a while thought the end goals would be something akin to:

Make Katello like any other plugin in puppet-foreman
Center backend configuration into either puppet-foreman_proxy_content or the installer
Ensure we don't break user workflows or over complicate orchestrated configuration

ekohl · 2019-12-09T16:09:59Z

Rebased so it's easier to see. Also submitted #309 as something that can be merged beforehand.

This is not final yet because I think I can rewrite init.pp to keep backwards compatible while already delivering the new split classes. That should allow experimentation with split deployments while still doing a monolithic deployment in the installer.

ekohl · 2019-12-10T00:23:24Z

This is not final yet because I think I can rewrite init.pp to keep backwards compatible while already delivering the new split classes. That should allow experimentation with split deployments while still doing a monolithic deployment in the installer.

This is done now. I've still added the monolithic change on top of it, but viewing the change should be easy now. It does drop some parameters, but I'd argue they didn't make sense for most users anyway.

ekohl · 2019-12-10T16:04:22Z

I think this is now ready for review. It is backwards incompatible because it drops 6 parameters from init.pp:

$user
$group
$user_groups
$qpid_interface
$qpid_hostname
$package_names

I believe these shouldn't be configurable anyway so there's no loss of functionality here for most users.

@ehelms please have a look. Ideally we'd merge this soon so it has some time to stablize. It should also
@laugmanuel please also have a look. While it doesn't fully solve your use case yet, I'd like to get to a point where we provide suitable profiles (according to the roles/profiles pattern) that can be deployed standalone.

laugmanuel

@ekohl thanks for the rework of the module.
We are currently reworking our internal module to use split deployment for pulp, qpid, candlepin, etc. For that we are using the code from this PR and will test it in our release environment.

In general I like the changes but stumbled across some small things as seen by the comments. If these changes are included, I will run the code against our env and report back.

@dgoetz FYI

laugmanuel · 2019-12-11T08:22:58Z

manifests/params.pp

+  Stdlib::Httpsurl $candlepin_url = "https://${facts['fqdn']}:8443/candlepin",
+  Stdlib::Httpsurl $pulp_url = "https://${facts['fqdn']}/pulp/api/v2/",
+  Stdlib::Httpsurl $crane_url = "https://${facts['fqdn']}:5000",
+  String $qpid_url = "amqp:ssl:${katello::globals::qpid_hostname}:5671",


It would be necessary to expose qpid_hostname as well. This is needed if Candlepin and QPID are running on different machines. At the moment katello::candlepin and katello::qpid use the default from globals which is localhost.
Given the qpid_url is only used in one template we could remove this variable completely and construct the URL in application.pp.

Another way would be to use the qpid_url in the candlepin.pp / qpid.pp directly but this would require rework of the ::candlepin class and the URL seems to be in a different format.

I think the first way would be much easier.

I was also considering this. Given it was a parameter in init.pp before, it makes sense to keep it. Will have a look.

I've pushed a separate commit to make review easier but should be squashed on merge. Please have a look.

Change looks good to me.

laugmanuel · 2019-12-11T10:01:19Z

manifests/pulp.pp

+#   The Apache options to use on the `/pub` resource
+#
+# @param repo_export_dir
+#   Create a repository export directory for Katello to use
 class katello::pulp (


I would appreciate if we could add the following parameters to support split deployments (as mentioned in #304):

manage_httpd (boolean)

https_cert

https_key

ca_cert

These values can default to undef / false but should be passed to pulp class.

My goal would for this PR is to just refactor without adding new params. Hopefully merge it quickly and then iterate on this in a separate PR.

Sounds good to me. If I can support you on getting the PR ready for merge just let me know.

laugmanuel · 2019-12-11T16:40:53Z

manifests/init.pp

@@ -40,6 +40,10 @@
 #
 # $qpid_wcache_page_size:: The size (in KB) of the pages in the write page cache
 #
+# $qpid_interface::     The interface qpidd listens to.


nitpick: listens on

laugmanuel · 2019-12-13T14:10:59Z

@ekohl now we have some good news for you!

We have successfully upgraded our test environment as far as we can get it using the code in this PR.
What we cannot test at the moment is the upgrade of Pulp and Candlepin because we need the followup PR regarding Certs and manage_httpd (Candlepin is also affected by a different Cert locations / setup).

Next week I will configure a client to talk to the upgraded environment and test some things.

In my perspective this PR is ready.

ehelms · 2019-12-13T17:17:59Z

manifests/init.pp


+  class { 'katello::params':


Never seen calling params and configuring it as a design convention.

It's unconventional and I might clean this up a bit more later as I gain more experience with splitting up into cleaner roles and profiles.

ehelms · 2019-12-13T17:19:24Z

What if any effect will this have on the installer currently? Just dropping a few parameters?

ekohl · 2019-12-13T17:24:22Z

What if any effect will this have on the installer currently? Just dropping a few parameters?

It should only drop a few parameters. Any other change would be considered a regression.

ehelms · 2019-12-18T13:44:31Z

Will need a rebase, after that I think I am good with this change.

laugmanuel · 2019-12-18T14:02:39Z

@ekohl I have one more thing regarding the Pulp configuration:
We do include ::foreman here: https://github.com/theforeman/puppet-katello/blob/master/manifests/pulp.pp#L37
This basically results in the installation/configuration of a complete stack which does not make any sense if the components are split to different servers.

The only reason for including this class is to be able to access $foreman::servername. This variable is mapped to $::fqdn anyway, which is basicly the same as the default for $pulp::server_name.
And to be able to use foreman::config::passenger::fragment of course.

What are your thoughts?

ekohl · 2019-12-18T14:14:44Z

That's the hard part about our vhost handling. I'd say we need some handling that's roughly:

if $manage_vhost {
  include foreman
  $servername = $foreman::servername
  foreman::config::apache::fragment { 'pulp':
    # ...
  }
} else {
  $servername = undef
}

class { 'pulp':
  servername   => $servername,
  manage_httpd => manage_vhost,
  # ...
}

Every class that provides an aspect of the deployment is now standalone. This means: * candlepin * pulp * qpid * application They can just be included and work by themselves. All class parameters are tunables that users will commonly modify. There's also shared parameters which are common among the standalone classes. These are meant to be changed via katello::params. Either via Hiera or by including them in a profile class before the instance classes themselves. The katello::globals class has globals that a user via the installer might want to change. The katello::params class is not intended to be exposed via the installer. The main katello class remains as a compatibility class. In the future this should probably be converted into a role but perhaps it is no needed at all.

ekohl · 2019-12-18T14:21:26Z

Rebased to resolve the conflict. I've also moved $katello::params::qpid_interface to $katello::qpid::interface since it's only used in that class.

ekohl · 2019-12-19T12:46:28Z

Thanks for the reviews. Merged now and I'm looking forward to iterating on this.

laugmanuel · 2019-12-20T10:22:00Z

Thanks @ekohl !
I did already prepare the follow up and will open a PR as soon as I've tested everything.

theforeman-bot added Needs testing Not yet reviewed labels Nov 30, 2019

ekohl mentioned this pull request Nov 30, 2019

Allow management of single products and add manage_httpd parameter for pulp #304

Closed

ekohl force-pushed the globals branch 2 times, most recently from 95171ad to 98a3e1a Compare December 2, 2019 15:33

ekohl mentioned this pull request Dec 2, 2019

Refactor Katello entry points theforeman/foreman-installer#421

Draft

ekohl force-pushed the globals branch from 98a3e1a to a5cc75f Compare December 9, 2019 16:06

ekohl force-pushed the globals branch from a5cc75f to cbf12d5 Compare December 10, 2019 00:21

ekohl added the Backwards incompatible label Dec 10, 2019

ekohl force-pushed the globals branch from cbf12d5 to 8137416 Compare December 10, 2019 15:57

ekohl marked this pull request as ready for review December 10, 2019 15:57

ekohl force-pushed the globals branch from 8137416 to 7b9251d Compare December 10, 2019 16:07

laugmanuel reviewed Dec 11, 2019

View reviewed changes

ehelms reviewed Dec 13, 2019

View reviewed changes

ekohl force-pushed the globals branch from b34a148 to ac38043 Compare December 18, 2019 14:20

ekohl merged commit ac38043 into theforeman:master Dec 19, 2019

ekohl deleted the globals branch December 19, 2019 12:41

laugmanuel mentioned this pull request Dec 20, 2019

Use Stdlib::Httpurl datatype to support setting a proxy protocol #311

Merged

laugmanuel mentioned this pull request Dec 23, 2019

Support manage_httpd to allow Pulp in a standalone setup #312

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign classes to allow split deployments #308

Redesign classes to allow split deployments #308

ekohl commented Nov 30, 2019

ehelms commented Dec 2, 2019

ekohl commented Dec 9, 2019

ekohl commented Dec 10, 2019

ekohl commented Dec 10, 2019

laugmanuel left a comment

laugmanuel Dec 11, 2019

ekohl Dec 11, 2019

ekohl Dec 11, 2019

laugmanuel Dec 11, 2019

laugmanuel Dec 11, 2019

ekohl Dec 11, 2019

laugmanuel Dec 11, 2019

laugmanuel Dec 11, 2019

laugmanuel commented Dec 13, 2019

ehelms Dec 13, 2019

ekohl Dec 13, 2019

ehelms commented Dec 13, 2019

ekohl commented Dec 13, 2019

ehelms commented Dec 18, 2019

laugmanuel commented Dec 18, 2019 •

edited

Loading

ekohl commented Dec 18, 2019

ekohl commented Dec 18, 2019

ekohl commented Dec 19, 2019

laugmanuel commented Dec 20, 2019

Redesign classes to allow split deployments #308

Redesign classes to allow split deployments #308

Conversation

ekohl commented Nov 30, 2019

ehelms commented Dec 2, 2019

ekohl commented Dec 9, 2019

ekohl commented Dec 10, 2019

ekohl commented Dec 10, 2019

laugmanuel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laugmanuel commented Dec 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehelms commented Dec 13, 2019

ekohl commented Dec 13, 2019

ehelms commented Dec 18, 2019

laugmanuel commented Dec 18, 2019 • edited Loading

ekohl commented Dec 18, 2019

ekohl commented Dec 18, 2019

ekohl commented Dec 19, 2019

laugmanuel commented Dec 20, 2019

laugmanuel commented Dec 18, 2019 •

edited

Loading