Review: Needs Fixing
Couple of changes that we discussed, but almost there.
Diff comments:
> diff --git a/charms/focal/autopkgtest-web/webcontrol/db-backup
> b/charms/focal/autopkgtest-web/webcontrol/db-backup
> index b03100b..3cec134 100755
> --- a/charms/focal/autopkgtest-web/webcontrol/db-backup
> +++ b/charms/focal/autopkgtest-web/webcontrol/db-backup
> @@ -47,40 +52,13 @@ def db_connect() -> sqlite3.Connection:
>
>
> def backup_db(db_con: sqlite3.Connection):
> - db_backup_con = sqlite3.connect(DB_BACKUP_PATH)
> - with db_backup_con:
> - db_con.backup(db_backup_con, pages=1)
> - db_backup_con.close()
> -
> -
> -def compress_db():
> - """
> - use gzip to compress database
> - """
> - with open(DB_BACKUP_PATH, "rb") as f_in, gzip.open(
> - "%s.gz" % DB_BACKUP_PATH, "wb"
> - ) as f_out:
> - shutil.copyfileobj(f_in, f_out)
> -
> -
> -def init_swift_con() -> swiftclient.Connection:
> - """
> - Establish connection to swift storage
> - """
> - swift_creds = {
> - "authurl": os.environ["OS_AUTH_URL"],
> - "user": os.environ["OS_USERNAME"],
> - "key": os.environ["OS_PASSWORD"],
> - "os_options": {
> - "region_name": os.environ["OS_REGION_NAME"],
> - "project_domain_name": os.environ["OS_PROJECT_DOMAIN_NAME"],
> - "project_name": os.environ["OS_PROJECT_NAME"],
> - "user_domain_name": os.environ["OS_USER_DOMAIN_NAME"],
> - },
> - "auth_version": 3,
> - }
> - swift_conn = swiftclient.Connection(**swift_creds)
> - return swift_conn
> + zstd = SimpleZstdInterface()
As discussed, no need for this object, with the newly proposed
SimpleZstdInterface API.
> + sql_backup = []
As discussed, remove this in-memory list and just chain all the iterators.
> + for line in db_con.iterdump():
> + sql_backup.append(line)
> + compressed_backup = zstd.compress("\n".join(sql_backup).encode())
> + with open(DB_BACKUP_PATH, "wb") as bkp_file:
> + bkp_file.write(compressed_backup)
>
>
> def create_container_if_it_doesnt_exist(swift_conn: swiftclient.Connection):
> diff --git a/charms/focal/autopkgtest-web/webcontrol/helpers/utils.py
> b/charms/focal/autopkgtest-web/webcontrol/helpers/utils.py
> index 89ea672..c57fb63 100644
> --- a/charms/focal/autopkgtest-web/webcontrol/helpers/utils.py
> +++ b/charms/focal/autopkgtest-web/webcontrol/helpers/utils.py
> @@ -18,10 +19,29 @@ import typing
> from dataclasses import dataclass
>
> import distro_info
> +import swiftclient
>
> sqlite3.paramstyle = "named"
>
>
> +class SimpleZstdInterface:
Proposed user API:
SimpleZstdInterface.compress(data)
SimpleZstdInterface.decompress(data)
Change is: no need to instantiate a SimpleZstdInterface object.
> + def __init__(self):
> + self.COMPRESS_COMMAND = ["zstd", "--compress"]
Those should be class attributes, called like this:
SimpleZstdInterface.COMPRESS_COMMAND
> + self.DECOMPRESS_COMMAND = ["zstd", "--decompress"]
> +
> + def zync_command(self, input: bytes, command: typing.List[str]):
This should be private
> + p = subprocess.run(
> + command, input=input, capture_output=True, check=True
> + )
> + return p.stdout
> +
> + def compress(self, data: bytes) -> bytes:
> + return self.zync_command(data, self.COMPRESS_COMMAND)
> +
> + def decompress(self, data: bytes) -> bytes:
> + return self.zync_command(data, self.DECOMPRESS_COMMAND)
> +
> +
> @dataclass
> class SqliteWriterConfig:
> writer_exchange_name = "sqlite-write-me.fanout"
> diff --git a/charms/focal/autopkgtest-web/webcontrol/sqlite-writer
> b/charms/focal/autopkgtest-web/webcontrol/sqlite-writer
> index d0ec23a..d309311 100755
> --- a/charms/focal/autopkgtest-web/webcontrol/sqlite-writer
> +++ b/charms/focal/autopkgtest-web/webcontrol/sqlite-writer
> @@ -109,9 +109,55 @@ def msg_callback(msg, db_con):
> checkpoint_db_if_necessary(db_con)
>
>
> +def restore_db_from_backup(db_con: sqlite3.Connection):
> + backups_container = "db-backups"
> + db_con.execute("PRAGMA wal_checkpoint(TRUNCATE);")
Shouldn't this be deferred to after the connection to swift, and avoided if the
swift connection cannot be made?
> + logging.info("Connecting to swift")
> + try:
> + swift_conn = init_swift_con()
> + except swiftclient.ClientException as e:
> + logging.warning(
> + (
> + f"Initialising swift connection failed with {e} - "
> + "continuing without restoring db from backup"
> + )
> + )
> + return
> + logging.info(
> + f"Connected to swift! Getting backups from container:
> {backups_container}"
> + )
> + _, objects = swift_conn.get_container(container=backups_container)
> + latest = objects[-1]
> + _, compressed_db_dump = swift_conn.get_object(
> + container=backups_container, obj=latest["name"]
> + )
> + zstd = SimpleZstdInterface()
> + db_dump = zstd.decompress(compressed_db_dump)
> + logging.info(
> + (
> + "Restoring db from swift - "
> + f"container: {backups_container} - object: {latest['name']}"
> + )
> + )
> + for line in db_dump.splitlines():
> + try:
> + db_con.execute(line.decode("utf-8"))
> + except sqlite3.OperationalError as e:
> + logging.warning(
> + f"Running sql command: `{line.decode('utf-8')}` failed with
> {e}"
> + )
> + logging.info("db restored from backup!")
> +
> +
> def main():
> logging.basicConfig(level=logging.INFO)
> - db_con = db_connect()
> + db_con = init_db(get_db_path())
> + if is_db_empty(db_con):
> + logging.info(
> + "DB is empty, indicating this unit has been recently deployed."
> + )
> + logging.info("Restoring database from a swift backup")
> + restore_db_from_backup(db_con)
> amqp_con = amqp_connect()
> status_ch = amqp_con.channel()
> status_ch.access_request("/complete", active=True, read=True,
> write=False)
--
https://code.launchpad.net/~andersson123/autopkgtest-cloud/+git/autopkgtest-cloud/+merge/472678
Your team Canonical's Ubuntu QA is requested to review the proposed merge of
~andersson123/autopkgtest-cloud:charm-fixes into autopkgtest-cloud:master.
--
Mailing list: https://launchpad.net/~canonical-ubuntu-qa
Post to : canonical-ubuntu-qa@lists.launchpad.net
Unsubscribe : https://launchpad.net/~canonical-ubuntu-qa
More help : https://help.launchpad.net/ListHelp
