Add $ensure parameter for Bacula packages

[fraenki]

Preface

While testing this module with Bacula 11 packages from bacula.org I've run into an issue.

Bugfixes

These packages have a major difference: the director/storage package is renamed to bacula-postgresql and it already includes the Bacula client, console, etc. So I had to adjust the configuration like this to get both the director/storage and the client running on the same host:

# director/storage/client all on the same host
bacula::storage::packages: [ 'bacula-postgresql' ]
bacula::director::packages: [ 'bacula-postgresql' ]
bacula::client::packages: [ 'bacula-postgresql' ]

Initially this failed, because it lead to a duplicate declaration error due to the way the bacula::client class handled the package installation. After fixing this another issue occured, because the value for $bacula::client::packages was not replaced but instead it was merged, resulting in a value of [ 'bacula-client', 'bacula-postgresql' ], which of course did not work.

This PR fixes these issues and I did not notice any side-effects when using it on Bacula 9.

Disclaimer: I've tested this only with Bacula 11 packages from bacula.org, so I don't know whether or not Linux/BSD distributions have adopted this new package naming scheme.

New Parameter

Furthermore I've noticed that this module does not support updating Bacula. I've added a new $ensure parameter to all packages which defaults to present. It makes it possible to upgrade to a new Bacula release.

The following example demonstrates the upgrade from Bacula 9 to Bacula 11 on CentOS:

# package no longer available in Bacula 11, would prevent upgrade of bacula-client
package { 'bacula-common':
  ensure => absent,
  provider => 'rpm',
  uninstall_options => ['--nodeps'],
}

# upgrade client after removing the old dependency
class { 'bacula::client':
  packages => [ 'bacula-client' ],
  ensure => '11.0.5-1',
}

[smortex]

Interesting… I tried to get the repo information at https://www.bacula.org/bacula-binary-package-download/ but do not receive a anything by mail 😒 Can you share the repo URL? I was not aware of this, but if it works it would make sense to add support for it in the module: indicate which packages should be used, those from the distro or those from bacula.

Since we already have different package lists for each OS, we could add a separate package list when using these repos. This could maybe allow us to avoid ensure_packages() which is a bit "fragile"?

[fraenki]

Can you share the repo URL?

Unfortunately I cannot share the repo URL, this would risk getting our repo access blocked. I'm confident the Bacula.org team will provide an access key following your request.

FYI, this is the repo content for Bacula 11.0.5 on EL8:

[DIR] repodata/                                    2021-06-07 12:29    -   
[   ] bacula-updatedb-11.0.5-1.el8.x86_64.rpm      2021-06-07 12:29   28K  Red Hat Enterprise Linux / Centos 8
[   ] bacula-postgresql-11.0.5-1.el8.x86_64.rpm    2021-06-07 12:29  4.0M  Red Hat Enterprise Linux / Centos 8
[   ] bacula-mysql-11.0.5-1.el8.x86_64.rpm         2021-06-07 12:29  4.0M  Red Hat Enterprise Linux / Centos 8
[   ] bacula-libs-11.0.5-1.el8.x86_64.rpm          2021-06-07 12:29  1.1M  Red Hat Enterprise Linux / Centos 8
[   ] bacula-docker-plugin-11.0.5-1.el8.x86_64.rpm 2021-06-07 12:29   51K  Red Hat Enterprise Linux / Centos 8
[   ] bacula-cloud-storage-11.0.5-1.el8.x86_64.rpm 2021-06-07 12:29  300K  Red Hat Enterprise Linux / Centos 8
[   ] bacula-client-11.0.5-1.el8.x86_64.rpm        2021-06-07 12:29  757K  Red Hat Enterprise Linux / Centos 8
[   ] bacula-aligned-11.0.5-1.el8.x86_64.rpm       2021-06-07 12:29  107K  Red Hat Enterprise Linux / Centos 8

Since we already have different package lists for each OS, we could add a separate package list when using these repos.

Could you elaborate more on this? How could this possibly work?

This could maybe allow us to avoid ensure_packages() which is a bit "fragile"?

I don't think this would solve the duplicate declaration error. What's the issue with ensure_packages()? It's already used in director.pp and storage.pp and I'm not aware of any other way to fix the duplicate declaration error in this case.

[smortex]

Unfortunately I cannot share the repo URL, this would risk getting our repo access blocked. I'm confident the Bacula.org team will provide an access key following your request.

Thanks to the filenames I could get access to a package directory tree (1st result for bacula-postgresql-11.0.5-1.el8 on ddg)…

The package depndencies for Debian are (I have not checked they are the same on RedHat but assumed so):

This imply the director and storage daemon are always installed together and install the file daemon too (unless the sqlite package is used for some reason). This is in fact the exact setup I have on the various bacula infrastructures I manage (through 2 profiles in my control repo: profile::bacula_server and profile::bacula_client), so I assume this is something reasonable for most small / medium sites, and for large sites the overhead of extra files is negligible?

Maybe we can consider this as a baseline and always install the components this way with the module. This would allow switching from OS packages to bacula packages more easily. This might however need some work and may have some unexpected side effects so I am not sure it is the best thing to do…

Since we already have different package lists for each OS, we could add a separate package list when using these repos.

Could you elaborate more on this? How could this possibly work?

Something like:

# module
class bacula (
  Enum['bacula', 'system'] $package_origin = 'system',
  Array[String[1]] $bacula_packages = [...], # probably a static list
  Array[String[1]] $system_packages, # existing packages from hiera
) {
  # ...
  $packages = $package_origin ? {
    bacula => $bacula_packages,
    system => $system_packages,
  }
  package { $packages:
    ensure => installed,
  }
}

# usage:
class { 'bacula':
  package_origin => 'bacula',
}

This could maybe allow us to avoid ensure_packages() which is a bit "fragile"?

I don't think this would solve the duplicate declaration error. What's the issue with ensure_packages()? It's already used in director.pp and storage.pp and I'm not aware of any other way to fix the duplicate declaration error in this case.

Without refactoring we will not be able to avoid it I think. The issue with ensure_packages() is that it is order dependent and we might have surprises if users also try to handle the resource in his code (which he should not be doing but sometimes people do weird things and it is hard for us to find out what is going wrong):

package { 'foo':
  ensure => installed,
}
ensure_packages('foo', ensure => installed)
ensure_packages('foo', ensure => installed)
ensure_packages('foo', ensure => installed) # works

ensure_packages('foo', ensure => installed)
ensure_packages('foo', ensure => installed)
ensure_packages('foo', ensure => installed) # works

ensure_packages('foo', ensure => installed)
ensure_packages('foo', ensure => installed)
ensure_packages('foo', ensure => installed)
package { 'foo':                            # duplicate declaration error
  ensure => installed,
}

[fraenki]

@smortex, thanks for the suggestions.

This might however need some work and may have some unexpected side effects so I am not sure it is the best thing to do…

Interesting idea, but I agree, it's likely to break things. It's also way beyond the scope of this tiny PR.

$packages = $package_origin ? {
bacula => $bacula_packages,
system => $system_packages,

Another good idea. However, I have not seen Bacula 11 or Bacula 13 packages in any Linux distribution, so it's unclear whether or not they will adopt the new Bacula package scheme or stick to the old one. FreeBSD sticked to the old package scheme for Bacula 11 and 13. I would give this more time before adding this complexity to the module. Maybe the idea could be added as a new feature request / issue.

So, as far as I can see, there is no real show-stopper here. Or am I missing something? I'd really like to get this merged, so hopefully it would be part of the first voxpupuli-bacula release :)

[smortex]

You are right, we can have the minimal changes for running with upstream packages merged quickly and maybe a more heavy refactoring allowing an easier integration later.

Would you mind looking at the few little things I noted bellow? Also, can you please create a wiki page that describes how to use upstream packages (maybe add a note to indicate that it is/will be available in 6.0.0).

[fraenki]

Rebased to fix a merge conflict.

[fraenki]

@smortex

Sorry, I can't reproduce the issue sweat_smile. Can you show the output of puppet lookup --explain with and without this part so that I can see what is different on your setup?

Sure. The only difference in our setup is that we've changed the default merge behaviour globally to "deep". See both lookups below (I've removed the entries without a match).

When setting "^bacula::.*::packages" to strategy: first (as proposed in this PR):

$ puppet lookup --explain --node bacula.example.com --facts facts.json bacula::client::packages
Searching for "lookup_options"
...
  Environment Data Provider (hiera configuration version 5)
    Using configuration "/etc/puppetlabs/code/environments/production/hiera.yaml"
    Merge strategy hash
      Hierarchy entry "Set important hiera options"
        Path "/etc/puppetlabs/code/environments/production/data/hiera_options.yaml"
          Original path: "hiera_options.yaml"
          Found key: "lookup_options" value: {
            "^.*" => {
              "merge" => {
                "strategy" => "deep"
              }
            }
          }
...
      Merged result: {
        "^.*" => {
          "merge" => {
            "strategy" => "deep"
          }
        }
      }
...
  Module "bacula" Data Provider (hiera configuration version 5)
    Using configuration "/etc/puppetlabs/code/environments/production/modules/bacula/hiera.yaml"
    Merge strategy hash
      Hierarchy entry "Set important hiera options"
        Path "/etc/puppetlabs/code/environments/production/modules/bacula/data/hiera_options.yaml"
          Original path: "hiera_options.yaml"
          Found key: "lookup_options" value: {
            "^bacula::.*::packages" => {
              "merge" => {
                "strategy" => "first"
              }
            }
          }
...
      Merged result: {
        "^bacula::.*::packages" => {
          "merge" => {
            "strategy" => "first"
          }
        }
      }
...
  Merge strategy hash
    Global and Environment
      Found key: "lookup_options" value: {
        "^.*" => {
          "merge" => {
            "strategy" => "deep"
          }
        }
      }
    Module bacula
      Found key: "lookup_options" value: {
        "^bacula::.*::packages" => {
          "merge" => {
            "strategy" => "first"
          }
        }
      }
    Merged result: {
      "^bacula::.*::packages" => {
        "merge" => {
          "strategy" => "first"
        }
      },
      "^.*" => {
        "merge" => {
          "strategy" => "deep"
        }
      }
    }
...
Using merge options from "lookup_options" hash
Searching for "bacula::client::packages"
...
    Hierarchy entry "Node data"
      Path "/etc/puppetlabs/code/environments/production/data/node/bacula.example.com.yaml"
        Original path: "node/%{::trusted.certname}.yaml"
        Found key: "bacula::client::packages" value: [
          "bacula-postgresql"
        ]

Note that bacula::client::packages was properly replaced (with the value from node data) and now only contains "bacula-postgresql".

Now the same lookup without adding Hiera options to the bacula module:

$ puppet lookup --explain --node bacula.example.com --facts facts.json bacula::client::packages
Searching for "lookup_options"
...
  Environment Data Provider (hiera configuration version 5)
    Using configuration "/etc/puppetlabs/code/environments/production/hiera.yaml"
    Merge strategy hash
      Hierarchy entry "Set important hiera options"
        Path "/etc/puppetlabs/code/environments/production/data/hiera_options.yaml"
          Original path: "hiera_options.yaml"
          Found key: "lookup_options" value: {
            "^.*" => {
              "merge" => {
                "strategy" => "deep"
              }
            }
          }
...
      Merged result: {
        "^.*" => {
          "merge" => {
            "strategy" => "deep"
          }
        }
      }
...
Using merge options from "lookup_options" hash
Searching for "bacula::client::packages"
  Merge strategy deep
...
    Environment Data Provider (hiera configuration version 5)
      Using configuration "/etc/puppetlabs/code/environments/production/hiera.yaml"
      Merge strategy deep
...
        Hierarchy entry "Node data"
          Merge strategy deep
            Path "/etc/puppetlabs/code/environments/production/data/node/bacula.example.com.yaml"
              Original path: "node/%{::fqdn}.yaml"
              Found key: "bacula::client::packages" value: [
                "bacula-postgresql"
              ]
            Merged result: [
              "bacula-postgresql"
            ]
...
    Module "bacula" Data Provider (hiera configuration version 5)
      Using configuration "/etc/puppetlabs/code/environments/production/modules/bacula/hiera.yaml"
      Merge strategy deep
...
        Hierarchy entry "family"
          Path "/etc/puppetlabs/code/environments/production/modules/bacula/data/os/RedHat.yaml"
            Original path: "os/%{facts.osfamily}.yaml"
            Found key: "bacula::client::packages" value: [
              "bacula-client"
            ]
...
        Merged result: [
          "bacula-client"
        ]
    Merged result: [
      "bacula-client",
      "bacula-postgresql"
    ]

As you can see, the contents of the $packages parameter cannot be replaced, it will always be merged. This of course would break the installation in this example.

Locking the merge behaviour for "^bacula::.*::packages" to strategy: first in the bacula module, would ensure the expected behaviour for all $packages parameters. It's a no-op change for many environments, but avoids breakage in environments where special Hiera options are present.

I think setting this option in the bacula module is the right choice. If set elsewhere it would look pretty out of place.

[smortex]

we've changed the default merge behavior globally to "deep".

Haaaa! Everything make sense 😅 I though you where hitting some kind of bug with the default config.

I think setting this option in the bacula module is the right choice. If set elsewhere it would look pretty out of place.

I would not recommend changing the default merge strategy because it will have similar nasty side effects with various modules (just a few examples). If you really need a custom global merge strategy, maybe you can limit its scope to your profile module (if you use the role & profile pattern)? Here is my lookup_options for the common.yaml file in my control-repo:

lookup_options:
  '^profile::.*password':
    convert_to: Sensitive
  '^profile::.*secret':
    convert_to: Sensitive
  profile::admin_accounts::administrators:
    merge: deep
  profile::yubikey::users:
    merge: deep

If that is not feasible and you really need a global merge strategy, what about setting the specific behavior for bacula settings before changing the global behavior? That way, all your site-specific customizations are in your control repo. This seems to have the intended effect on my test setup:

lookup_options:
  # Force a first merge strategy for bacula packages
  # We set it explicitly because we change the default bellow.
  '^bacula::.*::packages':
    merge: first
  # For all the rest, use the deep strategy
  '^.*':
    merge: deep

Does it make sense?

[smortex]

And on a more general note, I would avoid to set this in hiera and would rather pass explicit package parameters in my profiles: overriding modules data form a control-repo seems a smell to me, sometimes it is required, but if avoidable I prefer to avoid it. This makes the site configuration more easily approachable because the only values set in hiera in the control repo are parameters for the classes of the control repo, and you have a global view of all customized settings in one place.

The readme still has some examples that use Hiera to adjust the config… I'll open a PR to replace these old way of tuning the conf using Hiera with actual puppet code that can be put in a profile as is now preferred.

[fraenki]

@smortex I've simplified this PR, now it's only about the new $ensure parameter. Do you think this can be merged?

(Since the bacula.org repo is not public, I think it's best to keep things simple and not add new parameters for it.)