fix(linstor): Database Backup when Linstor Controller is not the Pool Master. by Ythogtha · Pull Request #142 · xcp-ng/sm

Ythogtha · 2026-06-10T09:46:34Z

Added secondary backup location outside of Linstor's DRBD mounts.
Throttling is restricted to backup on the Linstor Controller (needs access to files).
check_sr operates a regular backup only if it runs on the Linstor Controller, to allow throttling.
Retention is enforced by check_sr on Controller, with access to actual files.
Retention checks all backup.zip files for consistency :
Valid zip with a non-empty linstordb.mv.db file inside,
Reporting errors, and keeping only valid files.

Millefeuille42

Only small changes / clarification. Not all comments are actually blocking though.

Wescoeur · 2026-06-17T15:58:19Z

+        """Generate a new database backup file.
+        This operation should not prevent the underlying action to be successful.
+        Hence all Exceptions are caught and re-raised only if asked to.
+        controller: operate only if the current host is the Linstor Controller.
+        """


Suggested change

"""Generate a new database backup file.

This operation should not prevent the underlying action to be successful.

Hence all Exceptions are caught and re-raised only if asked to.

controller: operate only if the current host is the Linstor Controller.

"""

"""

Generate a new database backup file.

This operation should not prevent the underlying action to be successful.

Hence all Exceptions are caught and re-raised only if asked to.

controller: operate only if the current host is the Linstor Controller.

"""

Wescoeur · 2026-06-17T16:07:59Z

+        except Exception as e:
+            raise LinstorDatabaseBackupError(e) from e


Can you catch ZIP exception instead? Or add a comment about exceptions here?

That'd be (FileNotFoundError, zipfile.BadZipFile).
I'm somehow unhappy with what will happen if we don't catch something:
Retention fails, backup accumulate without hope for cleanup, and later along the road, everything breaks with a full /var/lib/linstor/
That is if the file is bad in a way we haven't anticipated.
I'm unsure here, of what is best.

Well, according to https://docs.python.org/3/library/zipfile.html there are only two zipfile Exceptions.
I'va added zipfile.LargeZipFile, because if that exception is raised, it is very strange, and may be the result of a malformed or malicious zipfile.
It feels better to handle the situation, and remove it.
I'm ok with this code.

Wescoeur · 2026-06-17T16:16:05Z

+
+    def database_invalidation(self):
+        for directory in (Path(DATABASE_PATH), DATABASE_BACKUP_LOGDIR):
+            file_ok = 0


Suggested change

file_ok = 0

valid_backup_count = 0

Wescoeur · 2026-06-17T16:16:28Z

+                    util.SMlog("[database_backup] Check failed: `{}` [{}]".format(
+                        error, database_backup_file), priority=util.LOG_ERR)


You can use f-string here.

Wescoeur · 2026-06-18T13:13:09Z

+        # Automatic backup if there were no backups for the last hour.
+        # Let it fail if needed, so full Traceback is on SMLog.
+        # Launch it only if we are on the controller.
+        self.database_backup("auto", delay=3600, fail=True, controller=True)


I think we should remove the controller flag, and test if we are on a host with a running controller before the database backup call.

Same for the fail flag, IMHO it adds complexity.

Wescoeur · 2026-06-18T13:21:59Z

-DATABASE_BACKUP_DIR_MAIN = Path(DATABASE_PATH)
-DATABASE_BACKUP_DIR_SPARE = Path('/var/lib/linstor.d/db-backups')
+DATABASE_BACKUP_LOGDIR = Path('/var/lib/linstor.d/db-backups')
+DATABASE_BACKUP_RELATIVE = Path("../linstor.d/db-backups")


IMHO, the use of RELATIVE in the variable name seems to me to be more an effect of the path content than an approach to the semantics and what this variable represents.

Suggested change

DATABASE_BACKUP_RELATIVE = Path("../linstor.d/db-backups")

DATABASE_BACKUP_DIR_SECONDARY = Path("../linstor.d/db-backups")

Let's say I'd be glad to use DATABASE_BACKUP_DIR_SPARE.relative_to(DATABASE_BACKUP_DIR_MAIN, walk_up=True), but Python 3.6 is in the way.
Fixed.

Wescoeur · 2026-06-18T13:25:13Z

 DATABASE_MKFS = 'mkfs.ext4'
-DATABASE_BACKUP_DIR_MAIN = Path(DATABASE_PATH)
-DATABASE_BACKUP_DIR_SPARE = Path('/var/lib/linstor.d/db-backups')
+DATABASE_BACKUP_LOGDIR = Path('/var/lib/linstor.d/db-backups')


I don't understand the var name: this constant is used to list backups and to manipulate logs.

I removed the logs and changed names according to new way of doing things.

Wescoeur · 2026-06-18T13:28:09Z

+    def is_controller(self):
+        """Checks if the current host is the Linstor Controller.
+        This is done by checking that the Linstor database path is a mountpoint.
+        Which should only be the case on the Linstor Controller."""
+        return os.path.ismount(DATABASE_PATH)


Should be merged with existing code like cls._is_mounted(DATABASE_PATH).

Wescoeur · 2026-06-18T13:28:39Z

+        # https://github.com/LINBIT/linstor-server/blob/3e9306a9d8215606544c64c50ced150625ee4926/controller/src/main/java/com/linbit/linstor/api/rest/v1/Controller.java#L408
+        self._linstor.controller_backupdb(str(DATABASE_BACKUP_RELATIVE / filename))
+        self._log_database_backup(date, name)
+        util.SMlog("[database_backup] Created: {}".format(filename), priority=util.LOG_INFO)


You can use f-strings.

Wescoeur · 2026-06-18T13:33:23Z

I don't understand the choice of using a log file rather than directly listing the logs, either by using:

The file dates (not the most robust method if they are modified).
Or by using a clear and robust naming convention, in which case there are no problems.

Ythogtha · 2026-06-18T17:21:27Z

I don't understand the choice of using a log file rather than directly listing the logs, either by using:
* The file dates (not the most robust method if they are modified).

* Or by using a clear and robust naming convention, in which case there are no problems.

Because we can't use throttling with filenames on SR operations (not check_sr), because we don't know if we have access to the files.
So in order to have a usable delay parameter, another solutions needs to be found.
I used a logfile.
But that's a bad one, and instead I removed the delay parameter, since only check_sr needs it, and it can be run on the Controller, therefore have access to the files, and use them as reference.

Ythogtha · 2026-06-18T17:44:47Z

Also, I converted some method into @classmethod since only the actual backup requires a linstor connection, and an instance of LinstorVolumeManager.

- Retention of 10 backups from operations, and check_sr always saves on latest. - Provides a secondary backup location, with the same retention policy. Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>

… Master. * Added secondary backup location outside of Linstor's DRBD mounts. * Throttling uses a logfile for backup operations. * sr_scan operates a regular backup only if it runs on the Linstor Controller. * Retention is enforced by sr_scan on controller, with access to actual files. * Exceptions are raised only on backup errors from sr_scan ; Other operations will only report in SMlog. * Retention checks all backup.zip files for consistency : Valid zip with a non-empty linstordb.mv.db file inside, Reporting errors, and keeping only valid files. Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>

Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>

check_sr works only on the Controller to have access to the backup files. It applies Validation, Retention, and Throttling for the automatic backup. All three of which requires access to the backup files, on the Controller. Removal of the logfile to manage throttling for normal operation. Hence removal of the delay parameter. Restricted caught Exceptions in backup validation to (FileNotFoundError, zipfile.BadZipFile). Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>

Don't except/raise LinstorDatabaseBackupError in _check_database_backup(). Simpler glob search for backup files, as the date is properly validated by datetime. Fix bug in database_backup_age(). Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>

Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>

Wescoeur · 2026-07-29T12:05:21Z

+        if not self._linstor:
+            self._reconnect()
+        try:
+            self._linstor.database_backup(name)  # type: ignore


You can import cast helper from sm_typing here.

Suggested change

self._linstor.database_backup(name) # type: ignore

self._linstor.database_backup(cast(str, name))

The # type: ignore is not because of name, but of _linstor, defines that way:
_linstor: Optional["LinstorVolumeManager"] = None

And since I've added a type for the name parameter, the linter checks types inside the function.
And AttributeError: 'NoneType' object has no attribute 'database_backup', because _linstor could be None.

But we just _reconnect(), and here I'm not caring about why something might fail: I log it and raise nothing.
Because the backup is a sub-product of an action which was a success, and should be reported as such.

So I am covered, by the reconnection, and the generic try/except, I'm ok having the AttributeError raised here, it is explicit enough for debugging purposes.
And I don't want to ignore a _linstor is None, that is an error which ought to be reported as such.

So I ask the linter to ignore its type.

Oh ok! I naively thought that old mypy version or something similar had an issue with the str type and Literal.
So you can use this before the call and without explicit message:

assert self._linstor

It's common to do that, we apply the classic usage of assert: "It must be true; it's a contract. If not, I messed up the code somewhere else."

Oh, great, I'll remember that, I like it.

Wescoeur · 2026-07-29T14:40:54Z

+        util.SMlog(f"[database_backup] Created: {filename}", priority=util.LOG_INFO)
+
+    @classmethod
+    def database_invalidation(cls):


Regarding description, I think we can change this name to: remove_old_database_backups.

It removes old database backups, but also checks existing files for validation (proper non-empty zipfile with a non-empty linstordb.mv.db.
That's why I used invalidation, but I'm open to a better name.

I agree, so maybe something like validate_and_prune_backups?

Ok, while keeping the database_backup prefix: database_backup_validate_and_prune

Wescoeur · 2026-07-29T21:31:29Z

+                   key=lambda p: p[1])
+
+    @classmethod
+    def _check_database_backup(cls, database_backup_file):


I suggest this:

Suggested change

def _check_database_backup(cls, database_backup_file):

def _check_database_backup(cls, database_backup_path):

It hasn't been formally documented yet, but:

A path suffix is useful for indicating a string or a Path.

A file suffix is useful for indicating that a file descriptor is involved.

Fine by me, thanks!

Wescoeur · 2026-07-29T21:41:20Z

+        # Applied only on the Linstor Controller, for reasons -> listed below.
+        if not LinstorVolumeManager.is_controller():
+            return
+        # Start database invalidation.


I think this comment needs to be modified. I added a note where the function is declared.

Suggested change

# Start database invalidation.

# Clean previous backups if necessary.

Wescoeur · 2026-07-29T21:46:05Z

+        # Hence we must choose one, either one is good, but there must be only one.
+        # Apply throttling: only backup if last one is >1h old.
+        # -> Needs access to backup files, available only on the Controller.
+        if LinstorVolumeManager.database_backup_age() > 3600:


IMHO we should use a constant here for maintenance.

That is very True.

Wescoeur · 2026-07-29T21:49:30Z

+                linstordb = [f for f in archive.filelist if f.filename == "linstordb.mv.db"]
+                if not linstordb:
+                    raise LinstorDatabaseBackupError("cannot find linstordb.mv.db")
+                linstordb = linstordb[0]


Suggested change

linstordb = [f for f in archive.filelist if f.filename == "linstordb.mv.db"]

if not linstordb:

raise LinstorDatabaseBackupError("cannot find linstordb.mv.db")

linstordb = linstordb[0]

linstordb = next((f for f in archive.filelist if f.filename == "linstordb.mv.db"), None)

if not linstordb:

raise LinstorDatabaseBackupError("cannot find linstordb.mv.db")

Small suggestion with small perf improvement.

It might end faster if there are several files and linstordb.mv.db is not the last one?
Well True, but in a normal situation, the zipfile contains only one file.
But that's elegant, so Ok ;)

Wescoeur · 2026-07-29T22:04:49Z

+        return cls._is_mounted(DATABASE_PATH)
+
+    @classmethod
+    def database_backup_age(cls):


Suggested change

def database_backup_age(cls):

def get_database_backup_age(cls):

Because it's not a property and also we use this prefix for getters.

Wescoeur · 2026-07-29T22:09:58Z

        return properties

+    @classmethod
+    def _list_database_backup(cls, database_backup_dir):


Suggested change

def _list_database_backup(cls, database_backup_dir):

def _list_database_backups(cls, database_backup_dir):

database_backup_path instead of database_backup_file because it is a str|Path. Throttling delay converted to Constant. Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>

Wescoeur · 2026-07-30T17:47:26Z

+        util.SMlog(f"[database_backup] Created: {filename}", priority=util.LOG_INFO)
+
+    @classmethod
+    def database_invalidation(cls):


I agree, so maybe something like validate_and_prune_backups?

Wescoeur · 2026-07-30T21:10:19Z

+        if not self._linstor:
+            self._reconnect()
+        try:
+            self._linstor.database_backup(name)  # type: ignore


Oh ok! I naively thought that old mypy version or something similar had an issue with the str type and Literal.
So you can use this before the call and without explicit message:

assert self._linstor

It's common to do that, we apply the classic usage of assert: "It must be true; it's a contract. If not, I messed up the code somewhere else."

…le None self._linstor Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>

Ythogtha force-pushed the agf-linstor_backupdb_base branch from 0e5dcb8 to f5e5bfc Compare June 10, 2026 09:59

Ythogtha changed the base branch from 3.2.12-8.3 to 3.2.12-8.3-cowutil-qcow2 June 10, 2026 10:04

Wescoeur changed the base branch from 3.2.12-8.3-cowutil-qcow2 to 3.2.12-8.3-devel June 10, 2026 12:58

Ythogtha force-pushed the agf-linstor_backupdb_base branch from b18a3f2 to 46bc6e3 Compare June 16, 2026 10:05

Ythogtha changed the title ~~Linstor: (fix) Remove all operations on backup files…~~ fix(linstor): Database Backup when Linstor Controller is not the Pool Master. Jun 16, 2026

Wescoeur requested a review from a team June 16, 2026 15:15

Ythogtha mentioned this pull request Jun 17, 2026

Linstor backup database basic checks. #138

Closed

Millefeuille42 requested changes Jun 17, 2026

View reviewed changes

Comment thread drivers/LinstorSR.py Outdated

Comment thread drivers/LinstorSR.py Outdated

Comment thread drivers/linstorvolumemanager.py Outdated

Comment thread drivers/linstorvolumemanager.py Outdated

Millefeuille42 requested a review from a team June 17, 2026 08:54

Wescoeur requested changes Jun 18, 2026

View reviewed changes

Ythogtha requested review from Millefeuille42 and Wescoeur June 18, 2026 17:21

Ythogtha force-pushed the agf-linstor_backupdb_base branch 2 times, most recently from 9126138 to 6e1ab3d Compare June 18, 2026 17:43

Ythogtha force-pushed the agf-linstor_backupdb_base branch 2 times, most recently from b090447 to b6b7676 Compare June 19, 2026 07:45

Millefeuille42 requested changes Jun 24, 2026

View reviewed changes

Comment thread drivers/LinstorSR.py

Comment thread drivers/LinstorSR.py

Comment thread drivers/linstorvolumemanager.py

Comment thread drivers/linstorvolumemanager.py

Ythogtha force-pushed the agf-linstor_backupdb_base branch 2 times, most recently from 3dba5e3 to 09c9925 Compare June 25, 2026 12:04

Ythogtha requested a review from Millefeuille42 June 26, 2026 14:45

Millefeuille42 approved these changes Jun 26, 2026

View reviewed changes

Nambrok force-pushed the 3.2.12-8.3-devel branch from e712aa5 to fce2d13 Compare June 29, 2026 07:36

Ythogtha force-pushed the agf-linstor_backupdb_base branch from 09c9925 to 6c0d0a3 Compare June 29, 2026 14:02

Wescoeur requested a review from klmp200 July 2, 2026 14:18

Nambrok force-pushed the 3.2.12-8.3-devel branch from 6607beb to c4dce9e Compare July 2, 2026 14:25

Ythogtha and others added 2 commits July 7, 2026 10:38

Ythogtha added 3 commits July 7, 2026 10:38

fix(linstor): Clarifications, documentations, and more explicit work.

4e6336f

Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>

Ythogtha force-pushed the agf-linstor_backupdb_base branch from 6c0d0a3 to b42bcd2 Compare July 7, 2026 08:42

klmp200 requested changes Jul 7, 2026

View reviewed changes

Comment thread drivers/LinstorSR.py Outdated

Comment thread drivers/LinstorSR.py

Comment thread drivers/LinstorSR.py

Comment thread drivers/LinstorSR.py

Comment thread drivers/linstorvolumemanager.py

Comment thread drivers/LinstorSR.py

Ythogtha requested a review from klmp200 July 9, 2026 09:01

Ythogtha force-pushed the agf-linstor_backupdb_base branch 7 times, most recently from 3ed3cbd to b2d00c7 Compare July 20, 2026 13:03

Clarifying comments, type hint for database_backup#name parameter.

b2d00c7

Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>

klmp200 approved these changes Jul 29, 2026

View reviewed changes

Wescoeur requested changes Jul 29, 2026

View reviewed changes

Better names, comments and a small but elegant optimization.

0a77cc2

database_backup_path instead of database_backup_file because it is a str|Path. Throttling delay converted to Constant. Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>

Ythogtha requested a review from Wescoeur July 30, 2026 15:37

Wescoeur requested changes Jul 30, 2026

View reviewed changes

Better name for backup invalidation and mypy handling of a (im)possib…

14961f8

…le None self._linstor Signed-off-by: Arnaud Garcia-Fernandez <arnaud.garcia-fernandez@vates.tech>

Ythogtha requested a review from Wescoeur July 31, 2026 15:23

		except Exception as e:
		raise LinstorDatabaseBackupError(e) from e

		util.SMlog("[database_backup] Check failed: `{}` [{}]".format(
		error, database_backup_file), priority=util.LOG_ERR)

	DATABASE_BACKUP_RELATIVE = Path("../linstor.d/db-backups")
	DATABASE_BACKUP_DIR_SECONDARY = Path("../linstor.d/db-backups")

	self._linstor.database_backup(name) # type: ignore
	self._linstor.database_backup(cast(str, name))

	def _check_database_backup(cls, database_backup_file):
	def _check_database_backup(cls, database_backup_path):

	# Start database invalidation.
	# Clean previous backups if necessary.

	def database_backup_age(cls):
	def get_database_backup_age(cls):

	def _list_database_backup(cls, database_backup_dir):
	def _list_database_backups(cls, database_backup_dir):

Uh oh!

Conversation

Ythogtha commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Millefeuille42 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wescoeur commented Jun 18, 2026

Uh oh!

Ythogtha commented Jun 18, 2026

Uh oh!

Ythogtha commented Jun 18, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Ythogtha commented Jun 10, 2026 •

edited

Loading