Skip to content
44 changes: 41 additions & 3 deletions drivers/LinstorSR.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <https://www.gnu.org/licenses/>.

from sm_typing import Any, Optional, override
from sm_typing import Any, Optional, override, Literal

from constants import CBTLOG_TAG
from constants import CBTLOG_TAG, LINSTOR_AUTO_BACKUP_DELAY

try:
from linstorcowutil import LinstorCowUtil, MultiLinstorCowUtil
Expand Down Expand Up @@ -817,6 +817,24 @@ def is_master(self):

return self._is_master

@override
def check_sr(self, sr_uuid) -> None:
# Note: check_sr is called on all hosts by the health check mechanism
# not by regular xapi calls such as scans.
# Applied only on the Linstor Controller, for reasons -> listed below.
if not LinstorVolumeManager.is_controller():
Comment thread
klmp200 marked this conversation as resolved.
return
# Validate and clean previous backups if necessary.
# -> Needs access to backup files, available only on the Controller.
LinstorVolumeManager.database_invalidation()
# check_sr is launched on *all* hosts, but it turns out that
# we do not want all of them to blindly generate concurrencing backups.
# 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.get_database_backup_age() > LINSTOR_AUTO_BACKUP_DELAY:
self.database_backup("auto")

@override
@_locked_load
def vdi(self, uuid) -> VDI.VDI:
Expand Down Expand Up @@ -1570,6 +1588,22 @@ def _kick_gc(self):
util.SMlog('Kicking GC')
cleanup.start_gc_service(self.uuid)

def database_backup(self, name: Literal["auto", "create", "delete", "snapshot"]):
"""
Generate a new database backup file.
This operation should not prevent the underlying action to be successful.
Hence all Exceptions are caught and logged.
"""
Comment thread
Millefeuille42 marked this conversation as resolved.
if not self._linstor:
self._reconnect()
try:
self._linstor.database_backup(name) # type: ignore

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

except Exception as e:
util.SMlog(
f"[database_backup] Error during creation: {e}",
priority=util.LOG_ERR,
)
Comment thread
Millefeuille42 marked this conversation as resolved.

# ==============================================================================
# LinstorSr VDI
# ==============================================================================
Expand Down Expand Up @@ -1753,6 +1787,8 @@ def create(self, sr_uuid, vdi_uuid, size) -> str:
self.ref = self._db_introduce()
self.sr._update_stats(self.size)

self.sr.database_backup("create")
Comment thread
klmp200 marked this conversation as resolved.

return VDI.VDI.get_params(self)

@override
Expand Down Expand Up @@ -1800,7 +1836,8 @@ def delete(self, sr_uuid, vdi_uuid, data_only=False) -> None:
# TODO: Check size after delete.
self.sr._update_stats(-self.size)
self.sr._kick_gc()
return super(LinstorVDI, self).delete(sr_uuid, vdi_uuid, data_only)
super(LinstorVDI, self).delete(sr_uuid, vdi_uuid, data_only)
self.sr.database_backup("delete")
Comment thread
klmp200 marked this conversation as resolved.

@override
def attach(self, sr_uuid, vdi_uuid) -> str:
Expand Down Expand Up @@ -2382,6 +2419,7 @@ def _do_snapshot(self, sr_uuid, vdi_uuid, snapType,
finally:
self.disable_leaf_on_secondary(vdi_uuid, secondary=secondary)
blktap2.VDI.tap_unpause(self.session, sr_uuid, vdi_uuid, secondary)
self.sr.database_backup("snapshot")
Comment thread
klmp200 marked this conversation as resolved.

def _snapshot(self, snap_type, cbtlog=None, cbt_consistency=None):
util.SMlog(
Expand Down
2 changes: 2 additions & 0 deletions drivers/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,5 @@
# Ref counting for VDI's: we need a ref count for LV activation/deactivation
# on the master.
NS_PREFIX_LVM: Final = "lvm-"

LINSTOR_AUTO_BACKUP_DELAY = 3600
118 changes: 117 additions & 1 deletion drivers/linstorvolumemanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@
import time
import util
import uuid
from datetime import datetime
from pathlib import Path
import contextlib
import zipfile

# Persistent prefix to add to RAW persistent volumes.
PERSISTENT_PREFIX = 'xcp-persistent-'
Expand All @@ -42,7 +46,11 @@
DATABASE_SIZE = 1 << 30 # 1GB.
DATABASE_PATH = '/var/lib/linstor'
DATABASE_MKFS = 'mkfs.ext4'

DATABASE_BACKUP_DIR_MAIN = Path(DATABASE_PATH)
DATABASE_BACKUP_DIR_SPARE = Path("/var/lib/linstor.d/db-backups")
DATABASE_BACKUP_NAME_FORMAT = "linstor_database_backup-{}-{}"
DATABASE_BACKUP_RETENTION = 10
DATABASE_BACKUP_DATE_FORMAT = "%Y%m%d_%H%M%S"
LINSTOR_SATELLITE_PORT = 3366

REG_DRBDADM_PRIMARY = re.compile("([^\\s]+)\\s+role:Primary")
Expand Down Expand Up @@ -251,6 +259,8 @@ def __init__(self, message, code=ERR_GENERIC):
def code(self):
return self._code

class LinstorDatabaseBackupError(Exception):
pass

# ==============================================================================

Expand Down Expand Up @@ -1774,6 +1784,51 @@ def get_database_path(self):
"""
return self._request_database_path(self._linstor, activate=True)

@classmethod
def is_controller(cls):
return cls._is_mounted(DATABASE_PATH)

@classmethod
def get_database_backup_age(cls):
"""
Return the latest backup age in seconds.
If not called on the Controller, since backups are not available,
returns a huge value (a timestamp of now).
"""
return (datetime.now() - cls._get_latest_database_backup()[1]).total_seconds()

def database_backup(self, name=""):
Comment thread
klmp200 marked this conversation as resolved.
# Create new backup
date = datetime.now().strftime(DATABASE_BACKUP_DATE_FORMAT)
filename = DATABASE_BACKUP_NAME_FORMAT.format(date, name)
self._linstor.controller_backupdb(filename)
# Relative path are ok for a secondary backup filename:
# 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(f"../linstor.d/db-backups/{filename}")
util.SMlog(f"[database_backup] Created: {filename}", priority=util.LOG_INFO)

@classmethod
def database_invalidation(cls):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, so maybe something like validate_and_prune_backups?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, while keeping the database_backup prefix: database_backup_validate_and_prune

"""
Removes old backup based on two criterias:
- Validity of the zipfile done by self._check_database_backup.
- Number of valid files found, only the nth latest are kept.
"""
for directory in (DATABASE_BACKUP_DIR_MAIN, DATABASE_BACKUP_DIR_SPARE):
valid_backup_count = 0
# Validate file and apply retention
for database_backup_path, _ in cls._get_sorted_database_backup(directory):
try:
cls._check_database_backup(database_backup_path)
valid_backup_count += 1
if valid_backup_count < DATABASE_BACKUP_RETENTION:
continue
except LinstorDatabaseBackupError as error:
util.SMlog(f"[database_backup] Check failed `{error}` [{database_backup_path}]",
priority=util.LOG_ERR)
with contextlib.suppress(OSError):
os.unlink(database_backup_path)

@classmethod
def get_all_group_names(cls, base_name):
"""
Expand Down Expand Up @@ -2651,6 +2706,67 @@ def _get_volume_properties(self, volume_uuid):
properties.namespace = self._build_volume_namespace(volume_uuid)
return properties

@classmethod
def _list_database_backups(cls, database_backup_dir):
"""
List all visible backup files in database_backup_dir.
DATABASE_BACKUP_DIR_MAIN is only available on the Linstor Controller.
DATABASE_BACKUP_DIR_SPARE will list backups previously made when the Host was the Linstor Controller.
This may not be useful information if it is not the Controller anymore.
"""
for path in database_backup_dir.glob(DATABASE_BACKUP_NAME_FORMAT.format(
"[0-9]" * 8 + "_" + "[0-9]" * 6, "*") + ".zip"):
try:
yield path, datetime.strptime(path.name.split("-")[1], DATABASE_BACKUP_DATE_FORMAT)
except (ValueError, IndexError):
continue
Comment thread
Millefeuille42 marked this conversation as resolved.

@classmethod
def _get_sorted_database_backup(cls, database_backup_dir):
"""
Return list of backups in database_backup_dir, alongside their creation date.
Sorted by date from the more recent to the older one.
"""
return sorted(cls._list_database_backups(database_backup_dir),
reverse=True,
key=lambda p: p[1])

@classmethod
def _get_latest_database_backup(cls):
"""
Return the latest backup in DATABASE_BACKUP_DIR_MAIN, and its creation date.
Returns (None, timestamp(0)) when none are found.
None will be found if it is not called on the Linstor Controller.
(cf _list_database_backups)
"""
return max(cls._list_database_backups(DATABASE_BACKUP_DIR_MAIN),
default=(None, datetime.fromtimestamp(0)),
key=lambda p: p[1])
Comment thread
Millefeuille42 marked this conversation as resolved.

@classmethod
def _check_database_backup(cls, database_backup_path):
"""
Make some validation of a database backup zip-file.
Check its a valid zipfile, and CRC-test its content.
Check it contains a non-empty linstordb.mv.db file.
Always raises a LinstorDatabaseBackupError if checks failed.
"""
try:
with zipfile.ZipFile(database_backup_path, mode="r") as archive:
if archive.testzip() is not None:
raise LinstorDatabaseBackupError("zip archive CRC failed")
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")
if linstordb.file_size == 0:
raise LinstorDatabaseBackupError("linstordb.mv.db is empty")
except (FileNotFoundError, zipfile.BadZipFile, zipfile.LargeZipFile) as e:
raise LinstorDatabaseBackupError(e) from e

@classmethod
def _build_sr_namespace(cls):
return '/{}/'.format(cls.NAMESPACE_SR)
Expand Down