oxidecomputer / oxidecomputer/omicron

VPC create saga node `svc_create_vpc` is not idempotent

Open
#6,097 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
572
Forks
97
Avg merge
2d 12h
Merged PRs (30d)
96

Description

While attempting to fix #6069, I added a test that calls test_helpers::actions_succeed_idempotently() to ensure that the new changes I made were idempotent. This revealed that the first node, svc_create_vpc is not idempotent, because the method DataStore::project_create_vpc() is not idempotent. Attempting to insert the same VPC more than once results in this stack trace:

bnaecker@shale : ~/omicron $ RUST_BACKTRACE=1 cargo nextest run vpc_create::test::test_actions_succeed_idempotently
info: experimental features enabled: setup-scripts
    Blocking waiting for file lock on build directory
    Finished `test` profile [unoptimized + debuginfo] target(s) in 8.15s
    Starting 1 test across 133 binaries (1365 skipped; run ID: 4310706f-309e-4eb4-9a46-2f80b35667b3, nextest profile: default)
       SETUP [      1/1] crdb-seed: cargo run -p crdb-seed --profile test
             [ 00:00:00] [                                                                                                                                       ]    0/1366:          Blocking waiting for file lock on package cache
    Blocking waiting for file lock on package cache
    Blocking waiting for file lock on package cache
    Finished `test` profile [unoptimized + debuginfo] target(s) in 1.15s
     Running `target/debug/crdb-seed`
Jul 16 17:16:19.807 INFO Using existing CRDB seed tarball: `/tmp/crdb-base-bnaecker/961b8ac67f16f2b01e46f3a25d34a9d3ce6c1b29d2b2e0f444689ad1463e62b6.tar`
  SETUP PASS [      1/1] crdb-seed: cargo run -p crdb-seed --profile test
        FAIL [  11.887s] omicron-nexus app::sagas::vpc_create::test::test_actions_succeed_idempotently

--- STDOUT:              omicron-nexus app::sagas::vpc_create::test::test_actions_succeed_idempotently ---

running 1 test
test app::sagas::vpc_create::test::test_actions_succeed_idempotently ... FAILED

failures:

failures:
    app::sagas::vpc_create::test::test_actions_succeed_idempotently

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 82 filtered out; finished in 11.70s


--- STDERR:              omicron-nexus app::sagas::vpc_create::test::test_actions_succeed_idempotently ---
log file: /tmp/omicron_nexus-e5c6b021751ee71f-test_actions_succeed_idempotently.5433.0.log
note: configured to log to "/tmp/omicron_nexus-e5c6b021751ee71f-test_actions_succeed_idempotently.5433.0.log"
DB URL: postgresql://root@[::1]:34432/omicron?sslmode=disable
DB address: [::1]:34432
log file: /tmp/omicron_nexus-e5c6b021751ee71f-test_actions_succeed_idempotently.5433.2.log
note: configured to log to "/tmp/omicron_nexus-e5c6b021751ee71f-test_actions_succeed_idempotently.5433.2.log"
log file: /tmp/omicron_nexus-e5c6b021751ee71f-test_actions_succeed_idempotently.5433.3.log
note: configured to log to "/tmp/omicron_nexus-e5c6b021751ee71f-test_actions_succeed_idempotently.5433.3.log"
thread 'app::sagas::vpc_create::test::test_actions_succeed_idempotently' panicked at nexus/src/app/sagas/test_helpers.rs:285:10:
Saga should have succeeded: ObjectAlreadyExists { type_name: Vpc, object_name: "my-vpc" }
stack backtrace:
   0: rust_begin_unwind
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/panicking.rs:645:5
   1: core::panicking::panic_fmt
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/panicking.rs:72:14
   2: core::result::unwrap_failed
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/result.rs:1654:5
   3: core::result::Result<T,E>::expect
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/result.rs:1034:23
   4: {async_fn#0}
             at ./src/app/sagas/test_helpers.rs:280:5
   5: {async_fn#0}
             at ./src/app/sagas/vpc_create.rs:854:64
   6: {async_block#0}
             at ./src/app/sagas/vpc_create.rs:832:5
   7: poll<&mut dyn core::future::future::Future<Output=()>>
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/future/future.rs:123:9
   8: poll<&mut core::pin::Pin<&mut dyn core::future::future::Future<Output=()>>>
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/future/future.rs:123:9
   9: {closure#0}<core::pin::Pin<&mut core::pin::Pin<&mut dyn core::future::future::Future<Output=()>>>>
             at /home/bnaecker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/scheduler/current_thread/mod.rs:659:57
  10: with_budget<core::task::poll::Poll<()>, tokio::runtime::scheduler::current_thread::{impl#8}::block_on::{closure#0}::{closure#0}::{closure_env#0}<core::pin::Pin<&mut core::pin::Pin<&mut dyn core::future::future::Future<Output=()>>>>>
             at /home/bnaecker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/coop.rs:107:5
  11: budget<core::task::poll::Poll<()>, tokio::runtime::scheduler::current_thread::{impl#8}::block_on::{closure#0}::{closure#0}::{closure_env#0}<core::pin::Pin<&mut core::pin::Pin<&mut dyn core::future::future::Future<Output=()>>>>>
             at /home/bnaecker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/coop.rs:73:5
  12: {closure#0}<core::pin::Pin<&mut core::pin::Pin<&mut dyn core::future::future::Future<Output=()>>>>
             at /home/bnaecker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/scheduler/current_thread/mod.rs:659:25
  13: tokio::runtime::scheduler::current_thread::Context::enter
             at /home/bnaecker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/scheduler/current_thread/mod.rs:404:19
  14: {closure#0}<core::pin::Pin<&mut core::pin::Pin<&mut dyn core::future::future::Future<Output=()>>>>
             at /home/bnaecker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/scheduler/current_thread/mod.rs:658:36
  15: tokio::runtime::scheduler::current_thread::CoreGuard::enter::{{closure}}
             at /home/bnaecker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/scheduler/current_thread/mod.rs:737:68
  16: tokio::runtime::context::scoped::Scoped<T>::set
             at /home/bnaecker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/context/scoped.rs:40:9
  17: tokio::runtime::context::set_scheduler::{{closure}}
             at /home/bnaecker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/context.rs:180:26
  18: try_with<tokio::runtime::context::Context, tokio::runtime::context::set_scheduler::{closure_env#0}<(alloc::boxed::Box<tokio::runtime::scheduler::current_thread::Core, alloc::alloc::Global>, core::option::Option<()>), tokio::runtime::scheduler::current_thread::{impl#8}::enter::{closure_env#0}<tokio::runtime::scheduler::current_thread::{impl#8}::block_on::{closure_env#0}<core::pin::Pin<&mut core::pin::Pin<&mut dyn core::future::future::Future<Output=()>>>>, core::option::Option<()>>>, (alloc::boxed::Box<tokio::runtime::scheduler::current_thread::Core, alloc::alloc::Global>, core::option::Option<()>)>
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/thread/local.rs:284:16
  19: std::thread::local::LocalKey<T>::with
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/thread/local.rs:260:9
  20: tokio::runtime::context::set_scheduler
             at /home/bnaecker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/context.rs:180:9
  21: tokio::runtime::scheduler::current_thread::CoreGuard::enter
             at /home/bnaecker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/scheduler/current_thread/mod.rs:737:27
  22: tokio::runtime::scheduler::current_thread::CoreGuard::block_on
             at /home/bnaecker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/scheduler/current_thread/mod.rs:646:19
  23: {closure#0}<core::pin::Pin<&mut dyn core::future::future::Future<Output=()>>>
             at /home/bnaecker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/scheduler/current_thread/mod.rs:175:28
  24: tokio::runtime::context::runtime::enter_runtime
             at /home/bnaecker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/context/runtime.rs:65:16
  25: block_on<core::pin::Pin<&mut dyn core::future::future::Future<Output=()>>>
             at /home/bnaecker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/scheduler/current_thread/mod.rs:167:9
  26: tokio::runtime::runtime::Runtime::block_on
             at /home/bnaecker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.38.0/src/runtime/runtime.rs:347:47
  27: test_actions_succeed_idempotently
             at ./src/app/sagas/vpc_create.rs:832:5
  28: omicron_nexus::app::sagas::vpc_create::test::test_actions_succeed_idempotently::{{closure}}
             at ./src/app/sagas/vpc_create.rs:835:6
  29: core::ops::function::FnOnce::call_once
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/ops/function.rs:250:5
  30: core::ops::function::FnOnce::call_once
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
WARN: dropped CockroachInstance without cleaning it up first (there may still be a child process running and a temporary directory leaked)
WARN: temporary directory leaked: "/tmp/.tmpiZegmB"
        If you would like to access the database for debugging, run the following:

        # Run the database
        cargo run --bin omicron-dev db-run --no-populate --store-dir "/tmp/.tmpiZegmB/data"
        # Access the database. Note the port may change if you run multiple databases.
        cockroach sql --host=localhost:32221 --insecure
WARN: dropped ClickHouseInstance without cleaning it up first (there may still be a child process running and a temporary directory leaked)
failed to clean up ClickHouse data dir:
- /tmp/omicron_nexus-e5c6b021751ee71f-test_actions_succeed_idempotently.5433.1-clickhouse-58bt6o: File exists (os error 17)
WARN: dropped DendriteInstance without cleaning it up first (there may still be a child process running and a temporary directory leaked)
WARN: dendrite temporary directory leaked: /tmp/.tmpIy8D4i
WARN: dropped DendriteInstance without cleaning it up first (there may still be a child process running and a temporary directory leaked)
WARN: dendrite temporary directory leaked: /tmp/.tmpM2VIpv
WARN: dropped MgdInstance without cleaning it up first (there may still be a child process running and a temporary directory leaked)
WARN: mgd temporary directory leaked: /tmp/.tmpf84k3s
WARN: dropped MgdInstance without cleaning it up first (there may still be a child process running and a temporary directory leaked)
WARN: mgd temporary directory leaked: /tmp/.tmpvacSOx

   Canceling due to test failure
------------
     Summary [  13.169s] 1 test run: 0 passed, 1 failed, 1365 skipped
        FAIL [  11.887s] omicron-nexus app::sagas::vpc_create::test::test_actions_succeed_idempotently
error: test run failed

That DataStore method needs to return the newly-created or reinserted VPC, so we could either catch this conflict and fetch it again, or make the query itself more complicated to either insert or return the existing row. We could catch this conflict in the saga node only, if that makes more sense.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with nexus/src/app/sagas/vpc_create.rs and the idempotency helper in nexus/src/app/sagas/test_helpers.rs, then inspect DataStore::project_create_vpc(). Run cargo nextest run vpc_create::test::test_actions_succeed_idempotently to reproduce the ObjectAlreadyExists failure. Done means repeated VPC creation returns the existing or newly created VPC and this test succeeds.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
api, backend, databases
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.