Add support for config models #873

barthisrael · 2023-11-28T17:34:47Z

Description

This PR adds support for config models in Barman. The main idea of this
feature is to make it easier to integrate Barman with HA solutions, e.g.
Patroni.

Besides the usual Barman server configuration, we now add support for Barman
model configuration. The model acts as a set of overrides for configuration
options for a given Barman server.

The servers configuration now have an additional configuration option:

cluster: it defines the cluster name for a given Barman server. If not set,
defaults to the server name -- for backward compatibility.

In order to say to Barman that a section of the configuration file is a model,
you need to set the configuration option model = true in such section. Also,
as explained for the Barman servers, you need to specify a cluster option in
the model. That is what makes it possible to apply config models to servers:
they need to belong to the same cluster.

The models currently support overriding all server configuration options, except
for:

Path related options:
- backup_directory
- basebackups_directory
- errors_directory
- incoming_wals_directory
- streaming_wals_directory
- wals_directory
Hook related options:
- post_archive_retry_script
- post_archive_script
- post_backup_retry_script
- post_backup_script
- post_delete_script
- post_delete_retry_script
- post_recovery_retry_script
- post_recovery_script
- post_wal_delete_script
- post_wal_delete_retry_script
- pre_archive_retry_script
- pre_archive_script
- pre_backup_retry_script
- pre_backup_script
- pre_delete_script
- pre_delete_retry_script
- pre_recovery_retry_script
- pre_recovery_script
- pre_wal_delete_script
- pre_wal_delete_retry_script

Note: we did it that way because those options can be easily destructive for
a Barman server, with the most concerning ones being the directories related as
they specify where to store files.

Example

As an example, let's assume you have a PostgreSQL cluster composed of 3 nodes,
say A (primary), B (standby), and C (standby), and assume you are taking
backups from A through a Barman server named my_cluster. You could create
a couple config models which would override the conninfo options of that
Barman server. So, in the event of a failover in the cluster, you could ask
Barman to apply a given model on top of the Barman server config, overriding
the values that would reference the failed node.

Please note that you can either have no active model, or have at most one
active model at a given point in time. If you have no active model, only the
options from the Barman server take place. If you have an active model, the
options defined in the model will override the corresponding options from the
Barman server config.

In order to switch configuration models, the user can use the new CLI
command barman config-switch. You need to call the command specifying
the server name and the model name. That command can also be used with
the --reset flag, so the active model is reset, allowing only the server options
to take place.

Technical details

Previous to this PR we used to have a ServerConfig class in the code.
It used to have the logic for handling the configuration of the Barman
servers. While introducing the models implementation we noticed some overlap
between how we would handle server and model configuration. With that in mind
we created a BaseConfig class which handles the similarities, and created a
couple subclasses from it:

ServerConfig: the usual ServerConfig which existed before, with an
additional cluster option being supported. Besides that, it now also
tracks a few things related with the server models:
- The hidden file .active-model.auto, which is written under the
  Barman server directory when a model is active for that server, if any;
- It also exposes a method apply_model so both Barman and the user
  can request a given model to be applied
- to_json method has been changed, so it correctly points the source
  file for an option that is override by a model
ModelConfig: a new class to handle configuration of Barman models. It
exposes a few handy methods:
- get_override_options: yields the options and values which are set by
  the model, and which override the Barman server config;
- to_json: so we can put models information in the diagnose output

We had to change the logic for parsing the Barman configuration, so it is
now able to distinguish between sections for Barman servers and for Barman
models. The same function which was used to parse and cache the servers
from config files is not in charge of parsing and caching the config models.
At the very end it will apply any configuration models which are active based
on the hidden files that are maintained by Barman inside each server directory.

When parsing config models, Barman will log warning messages about options
which are not supported in models. Also, it will deactivate Barman servers
if they attempt to apply an invalid model.

References: BAR-122.

mikewallace1979

This looks like a good approach to me - I like the fact that everything is handled in the configuration so all the code in Barman which consumes config remains unchanged, and the implementation is really clean and easy to follow.

I had one minor comment about naming the auto conf file but it's just bikeshedding really.

mikewallace1979 · 2023-11-29T14:21:30Z

barman/config.py

@@ -750,6 +764,78 @@ def __init__(self, config, name):
            if value is not None and value == "" or value == "None":
                value = None
            setattr(self, key, value)
+        self._active_model_file = os.path.join(
+            self.backup_directory, ".active-model.auto.conf"


I had assumed we'd store this in the config directory but using backup_directory like this seems like a better idea - the active model feels more like state rather than configuration so belongs in the server directory. I wonder if we should remove the .conf suffix so that it looks less like a config file (if it looks like a config file users are more likely to want to edit it directly which we don't want happening here).

Thanks for your thoughts 🙌 . I think that makes sense, yes. I'll perform the following changes:

Change the "active" file name

Move the models stuff out of the servers -> SERVER -> config key in the barman diagnose output

Adjust the unit tests, and include the ones which are missing

.# Description and examples This commit adds support for config models in Barman. The main idea of this feature is to make it easier to integrate Barman with HA solutions, e.g. Patroni. Besides the usual Barman server configuration, we now add support for Barman model configuration. The model acts as a set of overrides for configuration options for a given Barman server. The servers configuration now have an additional configuration option: * `cluster`: it defines the cluster name for a given Barman server. The name must be unique across different servers configured in Barman. If not filled, it defaults to the server name. In order to say to Barman that a section of the configuration file is a model, you need to set the configuration option `model = true` in such section. Also, as explained for the Barman servers, you need to specify a `cluster` option in the model. That is what ties up the Barman servers with their models. The models currently support only overrides for these configuration options: * `conninfo`; * `primary_conninfo`; * `streaming_conninfo`. As an example, let's assume you have a PostgreSQL cluster composed of 3 nodes, say `A` (primary), `B` (standby), and `C` (standby), and assume you are taking backups from `A` through a Barman server named `my_cluster`. You could create a couple config models which would override the `*conninfo` options of that Barman server. So, in the event of a failover in the cluster, you could ask Barman to apply a given model on top of the Barman server config, overriding the values that would reference the failed node. Please note that you can either have no active model, or have at most one active model at a given point in time. If you have no active model, only the options from the Barman server take place. If you have an active model, the options defined in the model will override the corresponding options from the Barman server config. .# Technical details Previous to this commit we used to have a `ServerConfig` class in the code. It used to have the logic for handling the configuration of the Barman servers. While introducing the models implementation we noticed some overlap between how we would handle server and model configuration. With that in mind we created a `BaseConfig` class which handles the similarities, and created a couple subclasses from it: * `ServerConfig`: the usual `ServerConfig` which existed before, with an additional `cluster` option being supported. Besides that, it now also tracks a few things related with the server models: * The hidden file `.active-model.auto.conf`, which is written under the Barman server directory when a model is active for that server, if any; * A mapping of models which are tied with this Barman server, and which can be activated by the user. * It also exposes a method `apply_model`, which will be used in a future commit, where we will make it possible for users to activate given config model for a server * `to_json` method has been changed, so it correctly points the source file for an option that is override by a model * `ModelConfig`: a new class to handle configuration of Barman models. It exposes a few handy methods: * `get_override_options`: yields the options and values which are set by the model, and which override the Barman server config; * `to_json`: so we can put models information in the diagnose output We had to change the logic for parsing the Barman configuration, so it is now able to distinguish between sections for Barman servers and for Barman models. Besides that, as we need exclusive cluster names among the Barman servers, the function that parses the configuration also checks for that and quits with an error if it finds conflicting names. If everything is fine in regards to cluster names, it will append the config models for each server near the end of parsing. At the very end it will apply any configuration models which are active based on the hidden files that are maintained by Barman inside each server directory. When parsing config models, Barman will log warning messages about options which are not supported in models. Also, it will deactivate Barman servers if they have models which are not properly configured and that are missing the required keys. References: BAR-122. Signed-off-by: Israel Barth Rubio <[email protected]>

The new command is basically a wrapper around the already implemented code for dealing with switches of configuration through config models. The command will call the `apply_model` method of the server after performing some validations. References: BAR-122. Signed-off-by: Israel Barth Rubio <[email protected]>

This commit removes bogus attributes from `servers -> SERVER -> config` in the barman diagnose output: * `_active_model_file`: removed * `active_model`: moved to `servers -> SERVER -> active_model` * `models`: moved to `servers -> SERVER -> models` We also changed the `to_json` method of `ModelConfig` to export only configuration options, not class attributes. References: BAR-122. Signed-off-by: Israel Barth Rubio <[email protected]>

We don't want users to touch that file, so we use an extension which is less likely to lead to that situation: we remove the `.conf` extension. References: BAR-122. Signed-off-by: Israel Barth Rubio <[email protected]>

References: BAR-122. Signed-off-by: Israel Barth Rubio <[email protected]>

Signed-off-by: Israel Barth Rubio <[email protected]>

mikewallace1979

This looks like a really clean implementation which does exactly what the PR descriptions says. I made a few suggestions and observations in the comments.

mikewallace1979 · 2023-12-04T10:46:10Z

barman/diagnose.py

+        diagnosis["servers"][name]["active_model"] = server.config.active_model
+        diagnosis["servers"][name]["models"] = {}
+        for model in server.config.models:
+            diagnosis["servers"][name]["models"][model.name] = model.to_json(


This is failing for me currently because model is a string - the for loop looks like it should probably be iterating models.values or models.items.

Good catch!
Given the recent chatting that we had, we might want to move diagnosis["servers"][name]["models"] to diagnosis["models"] too, as they may apply to many Barman servers.

Just submitted a commit with these changes.

mikewallace1979 · 2023-12-04T11:01:36Z

barman/cli.py

+
+    if server:
+        if server.config.active_model == args.model_name:
+            output.info(


This message is really helpful feedback as it makes it clear to the user that the desired configuration is already applied.

I think it would also be nice to have a message for the first time a model is applied - i.e. when running config-switch on a new configuration when there is not already an active model. Currently, if the first config-switch results in a change of values then the output will show those changes but if the first applied model happens to match the values in the base configuration then there will be no output all.

Good point, I think that makes sense.

This will be covered by the new behavior of def apply_model(self, name, from_cli=False): -- I'll submit a commit soon.

Just submitted a commit with these changes.

mikewallace1979 · 2023-12-04T11:26:46Z

barman/cli.py

+)
+def config_switch(args):
+    """
+    Switch a Barman server configuration by applying some model on top of it.


Alternative wording suggestion: Change the active configuration for a server by applying a named model on top of it.

Just submitted a commit with these changes.

mikewallace1979 · 2023-12-04T11:38:13Z

barman/config.py

+        "conninfo",
+        "model",
+        "primary_conninfo",
+        "streaming_conninfo",


We'll want to add the config vars for WAL streaming here though it depends on merge order as to whether that happens in #868 or this PR.

Right, I was about to mention it somewhere too. Whatever PR gets merged later needs to take this into account.

mikewallace1979 · 2023-12-04T11:42:23Z

barman/config.py

+        """
+        For each Barman server, check for a pre-existing active model.
+
+        If a hidden file with an pre-existing active model file exists, apply


with an pre-existing -> with a pre-existing

Just submitted a commit with these changes.

mikewallace1979 · 2023-12-04T11:52:01Z

barman/cli.py

+            "model_name",
+            help="specifies the model name for the command ",
+        ),
+    ]


Should we add a --reset (or similarly named) option here which would cause Barman to delete the .active-model.audo file?

This could be useful in scenarios where a user removes a model from the configuration while it is still the active model - currently the only way to resolve this case would be to delete the file manually or to config-swtich to a different model for the server.

Good point, makes total sense.

Just submitted a commit with these changes.

mikewallace1979 · 2023-12-04T11:58:42Z

barman/config.py

+                    "from '%s' to '%s' through the model '%s'"
+                ) % (option, self.name, old_value, value, model.name)
+
+                _logger.debug(message)


Is it worth making this info level? I can imagine wanting to see all such changes with the default barman logging configuration, and I would expect the frequency of them to be reasonably low most of the time.

I hadn't understood there were two contexts in which this was called when I first made this comment so I see now why this is logging at debug - it's going to get logged every time the config is loaded, as long as the var in the active model differs from the base config.

Could this be logged at info when the model is applied in the context of barman config-switch, but debug the rest of the time?

Right, I guess that makes sense. We can change the level based on the caller, yes.

Just submitted a commit with these changes.

mikewallace1979 · 2023-12-04T12:18:37Z

barman/config.py

+
+                setattr(self, option, value)
+
+        with open(self._active_model_file, "w") as f:


It looks like the active model gets written back to the file every time the config is generated, even though the value of name may have been obtained by reading it from that same file a few instructions ago in _apply_models. It feels like there are really two different responsibilities here

Applying the model to the live configuration.

Making the named model the active model by writing it to the file.

When running barman config-switch both jobs must be carried out, but in all other cases we only need the first. Does that make sense?

Good point, yes, that makes sense. The CLI should "apply and persist to disk", while Barman should only "apply".

Just submitted a commit with these changes.

Commit with changes suggested in the PR review: - Fix failing `barman diagnose` command: it was failing because we were iterating through the keys of a dictionary instead of through its values - Have messages printed when applying config models, independently if changes have been detected or not. They should go as `INFO` when a user runs `barman config-switch`, and as `DEBUG` when Barman is doing that on its own - Slight change on `barman config-switch` description - Fix typos - Add a `--reset` option to `barman config-switch` command, so users can recover from an active model that doesn't exist anymore without manual intervention on the files - Avoid that Barman internal calls of `apply_model` attempt to rewrite the active model file. That file should only be written when the user runs `barman config-switch` command, when there is an actual change of its value Unit tests were changed accordingly. References: BAR-122. Signed-off-by: Israel Barth Rubio <[email protected]>

barthisrael · 2023-12-04T17:37:34Z

Thanks for the new review! I've applied most of your suggestions -- except for the one which depends on the order of merging between this PR and #868 .

I'll now work on adjusting the oversight in regards to cluster vs servers relationship -- it should be 1 to many, not 1 to 1.

Previous to this commit we had a wrong implementation of the config models: it was considering that cluster x server was a 1-to-1 relationship, and consequently servers x models was a 1-to-many relationship. However, in practice we should be able to have cluster x server as a 1-to-many relationship, and cluster x model as a 1-to-many relationship, in such a way a model could be used for different servers in the same cluster, resulting in a server x model many-to-many relationship. This commit fixes the implementation in that regard. The following changes have been performed: * Implement `manage_model_command`, `get_model` and `get_model_list` in `barman.cli`: this is the equivalent feature from server handling in the CLI, but for models * Move models config from `servers - SERVER -> config -> models` to `models` in the `barman diagnose` output: as cluster is not supposed to be a 1 to 1 relationship with server, it makes more sense to have the models on the root of the output given the same model can be used by different servers * Refactor model related functions from `ServerConfig` and `Config` classes in `barman.config` -- it doesn't make sense anymore to cache models in the `ServerConfig` object, so: * `ServerConfig.add_model` method has been removed * `ServerConfig.apply_model` method now receives a `ModelConfig` instead of a model name * `ServerConfig.to_json` has been adjusted to not expect the `models` attribute * `Config._models` will cache the models * `Config._populate_servers` was renamed to `_populate_servers_and_models`, and it is now responsible for caching the models in `Config`. Also, it doesn't check anymore for cluster exclusiveness among servers as that's not supported to be a 1-to-1 relationship Unit tests were updated accordingly. References: BAR-122. Signed-off-by: Israel Barth Rubio <[email protected]>

barman/cli.py

barman/config.py

gcalacoci · 2023-12-06T14:57:25Z

barman/config.py

@@ -447,6 +504,7 @@ class ServerConfig(object):
        "basebackup_retry_times",
        "basebackups_directory",
        "check_timeout",
+        "cluster",


cannot find a good point to add this comment, so adding this here: maybe we want to move this list to the baseconfig class

That works, but I guess we should keep it into ServerConfig because it's not used at all in the BaseConfig class. I only left PARSERS in BaseConfig because that's used in the invoke_parser method.

Maybe we could "re-create" KEYS in ModelConfig using "set of KEYS from ModelConfig except for non-allowed keys", which I guess to be aligned with your other comment about the whitelist vs blacklist in Slack.

What do you think?

References: BAR-122. Signed-off-by: Israel Barth Rubio <[email protected]>

…fig` Previous to this commit we were using a whitelist approach to define the configuration options that could be override through config models, and they were: * `conninfo` * `primary_conninfo` * `streaming_conninfo` While discussing within the team we think it would be better if it were more flexible, so we turned that whitelist into a blacklist instead, where we disallow overriding only the value of configuration options which are related with paths for the Barman server, or with hooks for the Barman server, namely: * Path related: * `backup_directory` * `basebackups_directory` * `errors_directory` * `incoming_wals_directory` * `streaming_wals_directory` * `wals_directory` * Hook related: * `post_archive_retry_script` * `post_archive_script` * `post_backup_retry_script` * `post_backup_script` * `post_delete_script` * `post_delete_retry_script` * `post_recovery_retry_script` * `post_recovery_script` * `post_wal_delete_script` * `post_wal_delete_retry_script` * `pre_archive_retry_script` * `pre_archive_script` * `pre_backup_retry_script` * `pre_backup_script` * `pre_delete_script` * `pre_delete_retry_script` * `pre_recovery_retry_script` * `pre_recovery_script` * `pre_wal_delete_script` * `pre_wal_delete_retry_script` That should make it easier to introduce new settings to Barman, as well as make config models more useful for users. References: BAR-122. Signed-off-by: Israel Barth Rubio <[email protected]>

barthisrael · 2023-12-06T18:51:04Z

@gcalacoci I just submitted a commit which changes the behavior of ModelConfig, so it allows overrides based on a blacklist instead of a whitelist on the server config options.

edb-sonar-app · 2023-12-06T18:53:04Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
9 Code Smells

94.1% Coverage
0.0% Duplication

barthisrael self-assigned this Nov 28, 2023

barthisrael marked this pull request as draft November 28, 2023 17:34

barthisrael force-pushed the dev/BAR-122-add-config-models branch 2 times, most recently from 083a77b to a9edd51 Compare November 28, 2023 18:10

barthisrael requested review from mikewallace1979 and gcalacoci November 28, 2023 19:24

mikewallace1979 reviewed Nov 29, 2023

View reviewed changes

barthisrael force-pushed the dev/BAR-122-add-config-models branch from a86f516 to ec19219 Compare November 29, 2023 18:32

barthisrael added 5 commits November 30, 2023 14:16

Rename .active-model.auto.conf

152d14e

We don't want users to touch that file, so we use an extension which is less likely to lead to that situation: we remove the `.conf` extension. References: BAR-122. Signed-off-by: Israel Barth Rubio <[email protected]>

Extend unit tests suite due to implementation of config models

ef7dabb

References: BAR-122. Signed-off-by: Israel Barth Rubio <[email protected]>

barthisrael force-pushed the dev/BAR-122-add-config-models branch from ee8c49b to ef7dabb Compare November 30, 2023 17:17

barthisrael marked this pull request as ready for review November 30, 2023 17:23

Please flake8

64cc57d

Signed-off-by: Israel Barth Rubio <[email protected]>

mikewallace1979 reviewed Dec 4, 2023

View reviewed changes

barthisrael requested a review from mikewallace1979 December 5, 2023 16:22

gcalacoci reviewed Dec 6, 2023

View reviewed changes

barman/cli.py Outdated Show resolved Hide resolved

barman/config.py Show resolved Hide resolved

barman/config.py Outdated Show resolved Hide resolved

gcalacoci reviewed Dec 6, 2023

View reviewed changes

barthisrael added 2 commits December 6, 2023 13:25

PR feedback: rename get_model_list as get_models_list

adbd422

References: BAR-122. Signed-off-by: Israel Barth Rubio <[email protected]>

barthisrael requested a review from gcalacoci December 7, 2023 14:27

barthisrael mentioned this pull request Dec 11, 2023

Add documentation for HA cluster support features #880

Merged

gcalacoci approved these changes Dec 11, 2023

View reviewed changes

barthisrael merged commit 896312d into master Dec 11, 2023
9 checks passed

barthisrael deleted the dev/BAR-122-add-config-models branch December 11, 2023 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for config models #873

Add support for config models #873

barthisrael commented Nov 28, 2023 •

edited

Loading

mikewallace1979 left a comment

mikewallace1979 Nov 29, 2023

barthisrael Nov 29, 2023

mikewallace1979 left a comment

mikewallace1979 Dec 4, 2023

barthisrael Dec 4, 2023

barthisrael Dec 4, 2023

mikewallace1979 Dec 4, 2023

barthisrael Dec 4, 2023

barthisrael Dec 4, 2023

barthisrael Dec 4, 2023

mikewallace1979 Dec 4, 2023

barthisrael Dec 4, 2023

mikewallace1979 Dec 4, 2023

barthisrael Dec 4, 2023

mikewallace1979 Dec 4, 2023

barthisrael Dec 4, 2023

mikewallace1979 Dec 4, 2023

barthisrael Dec 4, 2023

barthisrael Dec 4, 2023

mikewallace1979 Dec 4, 2023

barthisrael Dec 4, 2023

barthisrael Dec 4, 2023

mikewallace1979 Dec 4, 2023

barthisrael Dec 4, 2023 •

edited

Loading

barthisrael Dec 4, 2023

barthisrael commented Dec 4, 2023

gcalacoci Dec 6, 2023

barthisrael Dec 6, 2023 •

edited

Loading

barthisrael commented Dec 6, 2023

edb-sonar-app bot commented Dec 6, 2023


		setattr(self, option, value)

		with open(self._active_model_file, "w") as f:

Add support for config models #873

Add support for config models #873

Conversation

barthisrael commented Nov 28, 2023 • edited Loading

Description

Example

Technical details

mikewallace1979 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikewallace1979 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barthisrael Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barthisrael commented Dec 4, 2023

Choose a reason for hiding this comment

barthisrael Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

barthisrael commented Dec 6, 2023

edb-sonar-app bot commented Dec 6, 2023

barthisrael commented Nov 28, 2023 •

edited

Loading

barthisrael Dec 4, 2023 •

edited

Loading

barthisrael Dec 6, 2023 •

edited

Loading