Skip to content

Commit

Permalink
Fix failures in barman-cloud-backup-delete related with incrementals
Browse files Browse the repository at this point in the history 
Through Barman 3.11.0 we introduced native PG17+ incremental backups.
Some changes have been made to the backup metadata to support that.

As part of the changes, we introduced a few properties to the class
`LocalBackupInfo`. However, there was an oversight in some code paths
which are shared between local and cloud backups.

For example, when running `barman-cloud-backup-delete`, that command
reuses the retention policy classes. These, in their turn, attempt to
access the aforementioned properties.

This commit fixes the issue by moving the new properties to the base
`BackupInfo` class, so they are available both for local and for
cloud backups.

The properties `has_children` and `is_incremental` will always return
`False` for cloud backups. Analagously, `deduplication_ratio` and
`backup_type` are not expected to be used in any code path related
with cloud backups. With that in mind, moving these properties to the
base classe should not cause a problem.

References: BAR-284.

Signed-off-by: Israel Barth Rubio <[email protected]>
  • Loading branch information
@barthisrael
barthisrael committed Aug 22, 2024
1 parent 9e8ea91 commit f720ccb
Show file tree
Hide file tree
Showing 2 changed files with 112 additions and 90 deletions.
154 changes: 88 additions & 66 deletions barman/infofile.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,94 @@ def set_attribute(self, key, value):
"""
setattr(self, key, value)

@property
def is_incremental(self):
"""
Only checks if the backup_info is an incremental backup
.. note::
This property only makes sense in the context of local backups stored in the
Barman server. However, this property is used for retention policies
processing, code which is shared among local and cloud backups. As this
property always returns ``False`` for cloud backups, it can safely be reused
in their code paths as well, though.
:return bool: ``True`` if this backup has a parent, ``False`` otherwise.
"""
return self.parent_backup_id is not None

@property
def has_children(self):
"""
Only checks if the backup_info has children
.. note::
This property only makes sense in the context of local backups stored in the
Barman server. However, this property is used for retention policies
processing, code which is shared among local and cloud backups. As this
property always returns ``False`` for cloud backups, it can safely be reused
in their code paths as well, though.
:return bool: ``True`` if this backup has at least one child, ``False`` otherwise.
"""
return self.children_backup_ids is not None

@property
def backup_type(self):
"""
Returns a string with the backup type label.
.. note::
Even though this property is available in this base class, it is not
expected to be used in the context of cloud backups.
The backup type can be one of the following:
- ``snapshot``: If the backup mode starts with ``snapshot``.
- ``rsync``: If the backup mode starts with ``rsync``.
- ``incremental``: If the mode is ``postgres`` and the backup is incremental.
- ``full``: If the mode is ``postgres`` and the backup is not incremental.
:return str: The backup type label.
"""
if self.mode.startswith("snapshot"):
return "snapshot"
elif self.mode.startswith("rsync"):
return "rsync"
return "incremental" if self.is_incremental else "full"

@property
def deduplication_ratio(self):
"""
Returns a value between and including ``0`` and ``1`` related to the estimate
deduplication ratio of the backup.
.. note::
For ``rsync`` backups, the :attr:`size` of the backup, which is the sum of
all file sizes in basebackup directory, is used to calculate the
ratio. For ``postgres`` backups, the :attr:`cluster_size` is used, which contains
the estimated size of the Postgres cluster at backup time.
We perform this calculation to make an estimation of how much network and disk
I/O has been saved when taking an incremental backup through ``rsync`` or through
``pg_basebackup``.
We abuse of the term "deduplication" here. It makes more sense to ``rsync`` than to
``postgres`` method. However, the idea is the same in both cases: get an estimation
of resources saving.
.. note::
Even though this property is available in this base class, it is not
expected to be used in the context of cloud backups.
:return float: The backup deduplication ratio.
"""
size = self.cluster_size
if self.backup_type == "rsync":
size = self.size
if size and self.deduplicated_size:
return 1 - (self.deduplicated_size / size)
return 0

def to_dict(self):
"""
Return the backup_info content as a simple dictionary
Expand Down Expand Up @@ -733,72 +821,6 @@ def __init__(self, server, info_file=None, backup_id=None, **kwargs):
e,
)

@property
def is_incremental(self):
"""
Only checks if the backup_info is an incremental backup
:return bool: ``True`` if this backup has a parent, ``False`` otherwise.
"""
return self.parent_backup_id is not None

@property
def has_children(self):
"""
Only checks if the backup_info has children
:return bool: ``True`` if this backup has at least one child, ``False`` otherwise.
"""
return self.children_backup_ids is not None

@property
def backup_type(self):
"""
Returns a string with the backup type label.
The backup type can be one of the following:
- ``snapshot``: If the backup mode starts with ``snapshot``.
- ``rsync``: If the backup mode starts with ``rsync``.
- ``incremental``: If the mode is ``postgres`` and the backup is incremental.
- ``full``: If the mode is ``postgres`` and the backup is not incremental.
:return str: The backup type label.
"""
if self.mode.startswith("snapshot"):
return "snapshot"
elif self.mode.startswith("rsync"):
return "rsync"
return "incremental" if self.is_incremental else "full"

@property
def deduplication_ratio(self):
"""
Returns a value between and including ``0`` and ``1`` related to the estimate
deduplication ratio of the backup.
.. note::
For ``rsync`` backups, the :attr:`size` of the backup, which is the sum of
all file sizes in basebackup directory, is used to calculate the
ratio. For ``postgres`` backups, the :attr:`cluster_size` is used, which contains
the estimated size of the Postgres cluster at backup time.
We perform this calculation to make an estimation of how much network and disk
I/O has been saved when taking an incremental backup through ``rsync`` or through
``pg_basebackup``.
We abuse of the term "deduplication" here. It makes more sense to ``rsync`` than to
``postgres`` method. However, the idea is the same in both cases: get an estimation
of resources saving.
:return float: The backup deduplication ratio.
"""
size = self.cluster_size
if self.backup_type == "rsync":
size = self.size
if size and self.deduplicated_size:
return 1 - (self.deduplicated_size / size)
return 0

def get_list_of_files(self, target):
"""
Get the list of files for the current backup
Expand Down
48 changes: 24 additions & 24 deletions tests/test_infofile.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,24 @@ def test_backup_info_from_backup_id(self, tmpdir):
assert b_info.tablespaces[0].oid == 16384
assert b_info.tablespaces[0].location == "/fake_tmp/tbs"

@pytest.mark.parametrize(
("mode", "parent_backup_id", "expected_backup_type"),
[
("rsync", None, "rsync"),
("postgres", "some_id", "incremental"),
("postgres", None, "full"),
("snapshot", None, "snapshot"),
],
)
def test_backup_type(self, mode, parent_backup_id, expected_backup_type):
"""
Ensure :meth:`LocalBackupInfo.backup_type` returns the correct backup type label.
"""
backup_info = build_test_backup_info(parent_backup_id=parent_backup_id)
backup_info.mode = mode

assert backup_info.backup_type == expected_backup_type

def test_backup_info_save(self, tmpdir):
"""
Test the save method of a BackupInfo object
Expand Down Expand Up @@ -947,7 +965,7 @@ def test_get_backup_manifest_path(self, mock_get_data_dir, backup_info):
mock_get_data_dir.return_value = "/some/random/path"
assert backup_info.get_backup_manifest_path() == expected

@patch("barman.infofile.LocalBackupInfo.is_incremental", new_callable=PropertyMock)
@patch("barman.infofile.BackupInfo.is_incremental", new_callable=PropertyMock)
def test_get_parent_backup_info_no_parent(self, mock_is_incremental, backup_info):
"""
Ensure :meth:`LocalBackupInfo.get_parent_backup_info` returns ``None`` if the
Expand All @@ -956,7 +974,7 @@ def test_get_parent_backup_info_no_parent(self, mock_is_incremental, backup_info
mock_is_incremental.return_value = False
assert backup_info.get_parent_backup_info() is None

@patch("barman.infofile.LocalBackupInfo.is_incremental", new_callable=PropertyMock)
@patch("barman.infofile.BackupInfo.is_incremental", new_callable=PropertyMock)
def test_get_parent_backup_info_empty_parent(
self, mock_is_incremental, backup_info
):
Expand All @@ -972,7 +990,7 @@ def test_get_parent_backup_info_empty_parent(
assert backup_info.get_parent_backup_info() is None
mock.assert_called_once_with(backup_info.server, backup_id="SOME_ID")

@patch("barman.infofile.LocalBackupInfo.is_incremental", new_callable=PropertyMock)
@patch("barman.infofile.BackupInfo.is_incremental", new_callable=PropertyMock)
def test_get_parent_backup_info_parent_ok(self, mock_is_incremental, backup_info):
"""
Ensure :meth:`LocalBackupInfo.get_parent_backup_info` returns the backup info
Expand Down Expand Up @@ -1189,7 +1207,7 @@ def provide_child_backup_info(server, backup_id):
("off", "off", "on", "on", False),
),
)
@patch("barman.infofile.LocalBackupInfo.is_incremental", new_callable=PropertyMock)
@patch("barman.infofile.BackupInfo.is_incremental", new_callable=PropertyMock)
def test_is_checksum_consistent(
self, mock_is_incremental, conf_root, conf_inc1, conf_inc2, conf_inc3, expected
):
Expand Down Expand Up @@ -1221,7 +1239,7 @@ def test_is_checksum_consistent(
walk_mock.return_value = (incremental2, incremental1, root_backup)
assert incremental3.is_checksum_consistent() is expected

@patch("barman.infofile.LocalBackupInfo.is_incremental", new_callable=PropertyMock)
@patch("barman.infofile.BackupInfo.is_incremental", new_callable=PropertyMock)
def test_true_is_full_and_eligible_for_incremental(self, mock_is_incremental):
"""
Test that the function applies the correct conditions for a full backup
Expand Down Expand Up @@ -1256,7 +1274,7 @@ def test_true_is_full_and_eligible_for_incremental(self, mock_is_incremental):
("rsync", "off", False),
],
)
@patch("barman.infofile.LocalBackupInfo.is_incremental", new_callable=PropertyMock)
@patch("barman.infofile.BackupInfo.is_incremental", new_callable=PropertyMock)
def test_false_is_full_and_eligible_for_incremental(
self, mock_is_incremental, backup_method, summarize_wal, is_incremental
):
Expand All @@ -1279,24 +1297,6 @@ def test_false_is_full_and_eligible_for_incremental(

assert not backup_info.is_full_and_eligible_for_incremental()

@pytest.mark.parametrize(
("mode", "parent_backup_id", "expected_backup_type"),
[
("rsync", None, "rsync"),
("postgres", "some_id", "incremental"),
("postgres", None, "full"),
("snapshot", None, "snapshot"),
],
)
def test_backup_type(self, mode, parent_backup_id, expected_backup_type):
"""
Ensure :meth:`LocalBackupInfo.backup_type` returns the correct backup type label.
"""
backup_info = build_test_backup_info(parent_backup_id=parent_backup_id)
backup_info.mode = mode

assert backup_info.backup_type == expected_backup_type


class TestSyntheticBackupInfo:
"""
Expand Down

0 comments on commit f720ccb

Please sign in to comment.