server: ensure check_config is called during app instantiation
ClosedPublic
Actions

Authored by olasd on Mar 4 2022, 3:39 PM.

Details

Reviewers

vlorentz

Maniphest Tasks

Restricted Maniphest Task

Commits

rDVAU7397110d444b: server: ensure check_config is called during app instantiation
rDVAU456eb8f41967: server tests: refactor config fixtures to match production
rDVAU432bfdcdea52: conftest: drop deprecated args from objstorage initializers

Summary

check_config converts the "stored" configuration (with toplevel keys for
most services) into a config suitable for the `get_vault` function.

Ensure all tests use the same codepath to make sure it is properly
exercized, and that the vault backend object can be properly initialized
in all cases.

Also contains a couple of refactoring commits to unconfuse the configuration
generation via fixtures.

Supersedes D7294

Test Plan

pytested + tested that the addition of check_config makes the staging
deployment happy

Diff Detail

Repository

rDVAU Software Heritage Vault

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

olasd created this revision.Mar 4 2022, 3:39 PM

Build is green

Patch application report for D7296 (id=26402)

Rebasing onto df35ac0601...

Current branch diff-target is up to date.

Changes applied before test

commit 7397110d444b89e5f452cf8f6de4ccb43f91dae3
Author: Nicolas Dandrimont <nicolas@dandrimont.eu>
Date:   Fri Mar 4 15:36:09 2022 +0100

    server: ensure check_config is called during app instantiation
    
    check_config converts the "stored" configuration (with toplevel keys for
    most services) into a config suitable for the ``get_vault`` function.
    
    Ensure all tests use the same codepath to make sure it is properly
    exercized, and that the vault backend object can be properly initialized
    in all cases.

commit 456eb8f419670902d0e5f30493027c02cc4ee809
Author: Nicolas Dandrimont <nicolas@dandrimont.eu>
Date:   Fri Mar 4 15:34:24 2022 +0100

    server tests: refactor config fixtures to match production
    
    This makes the server "fixtured" config use the production structure,
    with storage, objstorage, cache etc. set at the top level instead of
    within the vault configuration key.

commit 432bfdcdea522124350f8cb37ccdbf22b191d066
Author: Nicolas Dandrimont <nicolas@dandrimont.eu>
Date:   Fri Mar 4 15:25:57 2022 +0100

    conftest: drop deprecated args from objstorage initializers

See https://jenkins.softwareheritage.org/job/DVAU/job/tests-on-diff/225/ for more details.

Harbormaster completed remote builds in B27289: Diff 26402.Mar 4 2022, 3:41 PM

olasd requested review of this revision.Mar 4 2022, 3:41 PM

vlorentz added inline comments.Mar 4 2022, 3:55 PM

swh/vault/tests/test_server.py
27 ↗	(On Diff #26402)	why are items of `swh_vault_config` outside the `"vault"` dict? The naming doesn't make sense to me. And why does the `"db"` key need to be inside the `"vault"` key in `swh_vault_server_config`, but is outside of it in `swh_vault_config`?

olasd added inline comments.Mar 4 2022, 4:10 PM

swh/vault/tests/test_server.py
27 ↗	(On Diff #26402)	There's no `"vault"` key in `swh_vault_config`, only a set of valid arguments for the `swh.vault.get_vault()` function. The RPC server config file has `storage`, `scheduler`, etc. keys at the top level. I believe that's the schema that we've agreed upon, so that config files can be shared across multiple services without repetition. I'm therefore moving the relevant arguments there.

olasd added inline comments.Mar 4 2022, 4:13 PM

swh/vault/tests/test_server.py
27 ↗	(On Diff #26402)	See the configs in the `swh-environment/docker/conf` directory for examples of what we need to support and test here.

vlorentz added inline comments.Mar 4 2022, 4:30 PM

swh/vault/tests/test_server.py
27 ↗	(On Diff #26402)	Each package reads its own toplevel key when it is started directly, yes. But when a package needs to initialize a class from a different package, configs are nested (eg. see `objstorage` in `swh-environment/docker/conf/storage.yml`), because it makes more sense this way. For example, if running an swh-objstorage on a host (`cls: pathslicer`), you would want an swh-storage to access it remotely (`cls: remote`). And the vault does need its `storage` config in exactly the same way the storage needs the `objstorage` config

olasd added inline comments.Mar 4 2022, 4:36 PM

swh/vault/tests/test_server.py
27 ↗	(On Diff #26402)	We're talking about the vault here, not about the storage. We've been (slowly) migrating away from deeply nested configs in favor of reusing a top level config value if it makes sense. See how the workers and how the vault are currently being configured in docker, or how they're being configured in puppet. I am not rolling this back for "consistency".