canonical-ubuntu-qa team mailing list archive

[Skia <mp+472678@xxxxxxxxxxxxxxxxxx>]

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.