canonical-ubuntu-qa team mailing list archive

[Skia <mp+472678@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> diff --git a/charms/focal/autopkgtest-web/reactive/autopkgtest_web.py b/charms/focal/autopkgtest-web/reactive/autopkgtest_web.py
> index fe3cae7..b4900d0 100644
> --- a/charms/focal/autopkgtest-web/reactive/autopkgtest_web.py
> +++ b/charms/focal/autopkgtest-web/reactive/autopkgtest_web.py
> @@ -40,9 +40,11 @@ SWIFT_WEB_CREDENTIALS_PATH = os.path.expanduser(
>  )
>  API_KEYS_PATH = "/home/ubuntu/external-web-requests-api-keys.json"
>  CONFIG_DIR = pathlib.Path("/home/ubuntu/.config/autopkgtest-web/")
> +if not CONFIG_DIR.exists():
> +    set_flag("autopkgtest-web.config-needs-writing")
>  for parent in reversed(CONFIG_DIR.parents):
> -    parent.mkdir(mode=0o770, exist_ok=True)
> -CONFIG_DIR.mkdir(mode=0o770, exist_ok=True)
> +    parent.mkdir(mode=0o777, exist_ok=True)

775 would probably be a better choice here. Why would you give world write access to your HOME config directory?

> +CONFIG_DIR.mkdir(mode=0o777, exist_ok=True)
>  ALLOWED_REQUESTOR_TEAMS_PATH = CONFIG_DIR / "allowed-requestor-teams"
>  
>  PUBLIC_SWIFT_CREDS_PATH = os.path.expanduser("~ubuntu/public-swift-creds")
> diff --git a/charms/focal/autopkgtest-web/webcontrol/db-backup b/charms/focal/autopkgtest-web/webcontrol/db-backup
> index b03100b..7f2d83f 100755
> --- a/charms/focal/autopkgtest-web/webcontrol/db-backup
> +++ b/charms/focal/autopkgtest-web/webcontrol/db-backup
> @@ -47,40 +46,9 @@ 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():

It's a bit of a shame to loose the compression. I know the database compresses very well, and this would save a lot of storage, bandwidth, and memory while running the operation.
If you reintroduce it, would you consider zstd as the compression algorithm? It's way more efficient in every way compared to gzip.

> -    """
> -    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
> +    with io.open(DB_BACKUP_PATH, "w") as bkp:
> +        for line in db_con.iterdump():
> +            bkp.write(f"{line}\n")
>  
>  
>  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 7b22012..9524473 100644
> --- a/charms/focal/autopkgtest-web/webcontrol/helpers/utils.py
> +++ b/charms/focal/autopkgtest-web/webcontrol/helpers/utils.py
> @@ -241,4 +242,44 @@ def get_test_id(db_con, release, arch, src):
>          return test_id
>  
>  
> +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
> +
> +
> +def is_db_empty(db_con):
> +    # maybe we need to check the db path exists also?

Extra comments

> +    # for id_, name, filename in db_con.execute("PRAGMA database_list"):
> +    #     if name == "main" and filename is not None:
> +    #         path = filename
> +    #     elif name == "main" and filename is None:
> +    # if pathlib.Path()
> +    cursor = db_con.cursor()
> +    cursor.execute("SELECT name FROM sqlite_master WHERE type='table';")
> +    tables = cursor.fetchall()
> +    if len(tables) == 0:
> +        return True
> +    for table in tables:
> +        cursor.execute(f"SELECT * FROM {table[0]};")
> +        entries = cursor.fetchall()
> +        if len(entries) > 0:
> +            return False
> +    return True
> +
> +
>  get_test_id._cache = {}
> diff --git a/charms/focal/autopkgtest-web/webcontrol/publish-db b/charms/focal/autopkgtest-web/webcontrol/publish-db
> index a621f8a..6c4ffd6 100755
> --- a/charms/focal/autopkgtest-web/webcontrol/publish-db
> +++ b/charms/focal/autopkgtest-web/webcontrol/publish-db
> @@ -28,11 +29,29 @@ components = ["main", "restricted", "universe", "multiverse"]
>  
>  def init_db(path, path_current, path_rw):
>      """Create DB if it does not exist, and connect to it"""
> -
>      db = sqlite3.connect(path)
>      db_rw = sqlite3.connect("file:%s?mode=ro" % path_rw, uri=True)
>  
> -    # Copy r/w database over
> +    # checks if db is empty
> +    if is_db_empty(db_rw):
> +        logging.warning(
> +            (
> +                "Looks like this unit has been recently deployed - publish-db will"

I think this message is too high-level, and thus makes assumptions that could lead us, the admins, into errors. The real message to give from this script's point of view, is that the DB is empty. The reason for that cannot really be guessed here. Same for mentioning `sqlite-writer`: this is bound to become untrue after the next refactor.

For easier long term maintenance, each script should be absolutely pedantic on its mission, but never assume anything of the whole system. At least that's true here for autopkgtest-cloud, that is architectured with a lot of independent units chained together.

> +                " exit and wait for the sqlite-writer to restore the db from a backup"
> +            )
> +        )
> +        sys.exit(0)
> +
> +    # if no db, we need to copy /home/ubuntu/autopkgtest.db to /home/ubuntu/public/autopkgtest.db?
> +    if not os.path.exists(path_current):
> +        logging.warning(
> +            f"Looks like there's no pre-existing db at {path_current}, copying..."
> +        )
> +        public_db_con = sqlite3.connect(path_current)
> +        db_rw.backup(public_db_con)
> +        public_db_con.close()
> +
> +    logging.info(f"backing up {path_rw} to {path}")
>      with db:
>          db_rw.backup(db)
>      db_rw.close()
> diff --git a/charms/focal/autopkgtest-web/webcontrol/sqlite-writer b/charms/focal/autopkgtest-web/webcontrol/sqlite-writer
> index d0ec23a..bdd66e2 100755
> --- a/charms/focal/autopkgtest-web/webcontrol/sqlite-writer
> +++ b/charms/focal/autopkgtest-web/webcontrol/sqlite-writer
> @@ -38,14 +44,15 @@ def amqp_connect():
>      return amqp_con
>  
>  
> -def db_connect():
> -    """Connect to SQLite DB"""
> +def get_db_path():

This is a great candidate for utils.

>      cp = configparser.ConfigParser()
>      cp.read(os.path.expanduser("~ubuntu/autopkgtest-cloud.conf"))
> +    return cp["web"]["database"]
>  
> -    db_con = init_db(cp["web"]["database"])
>  
> -    return db_con
> +def db_connect():

As discussed, useless function, no real value there beside an indirection.

> +    """Connect to SQLite DB"""
> +    return init_db(get_db_path())
>  
>  
>  def check_msg(queue_msg):
> @@ -109,9 +116,59 @@ def msg_callback(msg, db_con):
>      checkpoint_db_if_necessary(db_con)
>  
>  
> +def restore_db_from_backup(db_con: sqlite3.Connection):

As discussed:
- Weird API: consuming db_con and returning a new one in a "restore_db" function
- No need for low level `os` stuff playing with files, just restore to the `db_con`

> +    backups_container = "db-backups"
> +    new_db_path = f"{get_db_path()}.new"
> +    logging.info(f"Creating new db: {new_db_path}")
> +    if os.path.isfile(new_db_path):
> +        os.remove(new_db_path)
> +    os.mknod(new_db_path)
> +    logging.info(f"Connecting to new db {new_db_path}")
> +    new_db = sqlite3.connect(new_db_path)
> +    logging.info("Connecting to swift")
> +    swift_conn = init_swift_con()
> +    logging.info(
> +        f"Connected to swift! Getting backups from container: {backups_container}"
> +    )
> +    _, objects = swift_conn.get_container(container=backups_container)
> +    latest = objects[-1]
> +    _, db_dump = swift_conn.get_object(
> +        container=backups_container, obj=latest["name"]
> +    )
> +    logging.info(
> +        (
> +            f"Restoring db {new_db_path} from swift - "
> +            f"container: {backups_container} - object: {latest['name']}"
> +        )
> +    )
> +    for line in db_dump.splitlines():
> +        try:
> +            new_db.execute(line.decode("utf-8"))
> +        except sqlite3.OperationalError as e:
> +            logging.warning(
> +                f"Running sql command: `{line.decode('utf-8')}` failed with {e}"
> +            )
> +    # checkpoint old db
> +    db_con.execute("PRAGMA wal_checkpoint(TRUNCATE);")
> +    # remove it
> +    os.remove(get_db_path())
> +    # move new db to old db location
> +    os.rename(new_db_path, get_db_path())
> +    # reinit db_con - with the new db after it's been moved, and return it
> +    db_con.close()
> +    logging.info("db restored from backup!")
> +    return db_connect()
> +
> +
>  def main():
>      logging.basicConfig(level=logging.INFO)
>      db_con = db_connect()
> +    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")
> +        db_con = 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.