[nexus] Consider renaming some DB objects, for clarity

[smklein]

As discussed with @davepacheco offline, there are a couple renames that might improve clarity within the codebase.

Note: These don't necessarily need to correlate with API changes, but given the combination of internal / external state nexus is handling, it might be worthwhile for readability.

• Service -> ServiceInstance: Within the DB schema, "Service" currently refers to a single Zone, executing somewhere. However, under some circumstances, "Serivce" can refer to a collection of "Service Instances", where each is interchangeable, but provides redundancy. For example, each of the CRDB frontends acts like an "instance" for a "service". (RFD 248 has more detail).
  By naming these "ServiceInstances", the ambiguity is lessened.

• Instance -> VmInstance: Similarly, the thing called "Instance" in Nexus currently refers to an "Instance of a VM". Given that the word "instance" is useful in other contexts, disambiguating here may be useful too.

[david-crespo]

Definitely on board with VmInstance — instance basically means "thing" so it is way too broad.

[ryaeng]

I'll create a separate branch for Instance -> VmInstance.

[ryaeng]

I'm working on the Instance -> VmInstance change. Changing the instance was pretty painless. The trouble comes in when updating a NetworkInterface and a specific query. I keep running into issues HERE.

Here's the full stack backtrace I continue receiving. I suspect that it's an issue with the DB, but I'm not sure how to check it.

ryan@Ryans-MacBook-Air omicron % cargo test --package omicron-nexus --lib -- db::queries::network_interface::tests::test_insert_network_interface_query --exact --nocapture
Finished test [unoptimized + debuginfo] target(s) in 0.25s
Running unittests src/lib.rs (target/debug/deps/omicron_nexus-271b60e066ee2d2f)

running 1 test
log file: "/var/folders/y5/351fq3v90jb8mpn17q1yzp_40000gn/T/omicron_nexus-271b60e066ee2d2f-test_insert_network_interface_query.21423.0.log"
note: configured to log to "/var/folders/y5/351fq3v90jb8mpn17q1yzp_40000gn/T/omicron_nexus-271b60e066ee2d2f-test_insert_network_interface_query.21423.0.log"
thread 'db::queries::network_interface::tests::test_insert_network_interface_query' panicked at 'Requesting an interface with the same name on the same instance should fail', nexus/src/db/queries/network_interface.rs:1697:9
stack backtrace:
0: 0x103a52338 - std::backtrace_rs::backtrace::libunwind::trace::hd4df8e77abf55be6
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/../../backtrace/src/backtrace/libunwind.rs:93:5
1: 0x103a52338 - std::backtrace_rs::backtrace::trace_unsynchronized::ha3c872a49430e7f7
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
2: 0x103a52338 - std::sys_common::backtrace::_print_fmt::h11104fc3337a6872
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/sys_common/backtrace.rs:66:5
3: 0x103a52338 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h4b6cfcc8bff01899
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/sys_common/backtrace.rs:45:22
4: 0x103a715dc - core::fmt::write::hb07daa6d4b732434
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/core/src/fmt/mod.rs:1194:17
5: 0x103a4b9f0 - std::io::Write::write_fmt::h84ddd778c0c2d08d
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/io/mod.rs:1655:15
6: 0x103a53ce8 - std::sys_common::backtrace::_print::h97ffc684a924af56
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/sys_common/backtrace.rs:48:5
7: 0x103a53ce8 - std::sys_common::backtrace::print::h01ae5c749e2c8287
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/sys_common/backtrace.rs:35:9
8: 0x103a53ce8 - std::panicking::default_hook::{{closure}}::h5faac4ca80254748
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/panicking.rs:295:22
9: 0x103a539c4 - std::panicking::default_hook::hcb52bdcc4b5b62af
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/panicking.rs:314:9
10: 0x103a541c0 - std::panicking::rust_panic_with_hook::h0c8ae3594fa0153f
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/panicking.rs:698:17
11: 0x103a5407c - std::panicking::begin_panic_handler::{{closure}}::h258894efb214aed5
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/panicking.rs:586:13
12: 0x103a52814 - std::sys_common::backtrace::__rust_end_short_backtrace::h141018aab2afbe79
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/sys_common/backtrace.rs:138:18
13: 0x103a53e0c - rust_begin_unwind
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/panicking.rs:584:5
14: 0x103aaba40 - core::panicking::panic_fmt::h1ffc8437c42e4ace
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/core/src/panicking.rs:142:14
15: 0x100e2a28c - omicron_nexus::db::queries::network_interface::tests::test_insert_network_interface_query::{{closure}}::h9dba414e24539d5e
at /Users/ryan/Projects/rust/omicron/nexus/src/db/queries/network_interface.rs:1697:9
16: 0x1014093dc - <core::future::from_generator::GenFuture as core::future::future::Future>::poll::h5e90a5c4566d8319
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/core/src/future/mod.rs:91:19
17: 0x102008858 - <core::pin::Pin

as core::future::future::Future>::poll::hc43bf4659dcef3a9
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/core/src/future/future.rs:124:9
18: 0x1022201e4 - tokio::runtime::basic_scheduler::CoreGuard::block_on::{{closure}}::{{closure}}::{{closure}}::ha1b3200286e41b51
at /Users/ryan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.19.2/src/runtime/basic_scheduler.rs:508:48
19: 0x1018eb03c - tokio::coop::with_budget::{{closure}}::hf32fada3d9e7c5d0
at /Users/ryan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.19.2/src/coop.rs:102:9
20: 0x1020dcc34 - std::thread::local::LocalKey::try_with::hf0935b3a50e2f382
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/thread/local.rs:443:16
21: 0x1020be59c - std::thread::local::LocalKey::with::h2c9230db0f8eccb6
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/thread/local.rs:419:9
22: 0x10221edfc - tokio::coop::with_budget::h9bbc909e7416ac17
at /Users/ryan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.19.2/src/coop.rs:95:5
23: 0x10221edfc - tokio::coop::budget::hfb13dc394cfbf965
at /Users/ryan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.19.2/src/coop.rs:72:5
24: 0x10221edfc - tokio::runtime::basic_scheduler::CoreGuard::block_on::{{closure}}::{{closure}}::hf43b04bd922ecd55
at /Users/ryan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.19.2/src/runtime/basic_scheduler.rs:508:25
25: 0x1021e1c6c - tokio::runtime::basic_scheduler::Context::enter::h74031f91f9ee9c7c
at /Users/ryan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.19.2/src/runtime/basic_scheduler.rs:362:19
26: 0x1022180a0 - tokio::runtime::basic_scheduler::CoreGuard::block_on::{{closure}}::hf9dff3d214eda1f2
at /Users/ryan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.19.2/src/runtime/basic_scheduler.rs:507:36
27: 0x102201370 - tokio::runtime::basic_scheduler::CoreGuard::enter::{{closure}}::h391426c26dc762a8
at /Users/ryan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.19.2/src/runtime/basic_scheduler.rs:565:57
28: 0x101ed64f8 - tokio::macros::scoped_tls::ScopedKey::set::h172149502819c830
at /Users/ryan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.19.2/src/macros/scoped_tls.rs:61:9
29: 0x1021fcfd0 - tokio::runtime::basic_scheduler::CoreGuard::enter::ha68e24225a5ee19f
at /Users/ryan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.19.2/src/runtime/basic_scheduler.rs:565:27
30: 0x1022029b0 - tokio::runtime::basic_scheduler::CoreGuard::block_on::h54f4f6a39e7d296a
at /Users/ryan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.19.2/src/runtime/basic_scheduler.rs:498:9
31: 0x1021ca3f4 - tokio::runtime::basic_scheduler::BasicScheduler::block_on::h633644ddb86d3894
at /Users/ryan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.19.2/src/runtime/basic_scheduler.rs:174:24
32: 0x1021b98a8 - tokio::runtime::Runtime::block_on::h3678a8c4f125ada7
at /Users/ryan/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.19.2/src/runtime/mod.rs:480:46
33: 0x10177662c - omicron_nexus::db::queries::network_interface::tests::test_insert_network_interface_query::hf4b9ad6e94a5c532
at /Users/ryan/Projects/rust/omicron/nexus/src/db/queries/network_interface.rs:1851:9
34: 0x100e27f00 - omicron_nexus::db::queries::network_interface::tests::test_insert_network_interface_query::{{closure}}::h158913885338f55b
at /Users/ryan/Projects/rust/omicron/nexus/src/db/queries/network_interface.rs:1541:11
35: 0x101b4ea70 - core::ops::function::FnOnce::call_once::h9a38e9c9239fac9d
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/core/src/ops/function.rs:248:5
36: 0x1023fba78 - core::ops::function::FnOnce::call_once::h59d77ed3ce5d6335
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/core/src/ops/function.rs:248:5
37: 0x1023fba78 - test::__rust_begin_short_backtrace::hbef928b1f8607a2b
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/test/src/lib.rs:573:5
38: 0x1023facec - <alloc::boxed::Box<F,A> as core::ops::function::FnOnce>::call_once::hd3ec8f7fcfc13823
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/alloc/src/boxed.rs:1866:9
39: 0x1023facec - <core::panic::unwind_safe::AssertUnwindSafe as core::ops::function::FnOnce<()>>::call_once::hced88cb49f8c91e2
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/core/src/panic/unwind_safe.rs:271:9
40: 0x1023facec - std::panicking::try::do_call::h748650eea26c0849
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/panicking.rs:492:40
41: 0x1023facec - std::panicking::try::ha9187ec3908ac7f4
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/panicking.rs:456:19
42: 0x1023facec - std::panic::catch_unwind::h4dadeb3fb0de4315
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/panic.rs:137:14
43: 0x1023facec - test::run_test_in_process::h3d56f0307f45812e
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/test/src/lib.rs:596:18
44: 0x1023facec - test::run_test::run_test_inner::{{closure}}::hac7bc5bd2d927917
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/test/src/lib.rs:490:39
45: 0x1023cffec - test::run_test::run_test_inner::{{closure}}::hb4cd264bec378ec9
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/test/src/lib.rs:517:37
46: 0x1023cffec - std::sys_common::backtrace::rust_begin_short_backtrace::h3f4e281e6c9c6b25
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/sys_common/backtrace.rs:122:18
47: 0x1023d5d88 - std::thread::Builder::spawn_unchecked::{{closure}}::{{closure}}::h4c703aedf72497d0
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/thread/mod.rs:498:17
48: 0x1023d5d88 - <core::panic::unwind_safe::AssertUnwindSafe as core::ops::function::FnOnce<()>>::call_once::hce0c8486ba75c958
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/core/src/panic/unwind_safe.rs:271:9
49: 0x1023d5d88 - std::panicking::try::do_call::h58339682b1a39dbb
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/panicking.rs:492:40
50: 0x1023d5d88 - std::panicking::try::ha78865501d934bbb
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/panicking.rs:456:19
51: 0x1023d5d88 - std::panic::catch_unwind::h2310b4a08fb4e387
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/panic.rs:137:14
52: 0x1023d5d88 - std::thread::Builder::spawn_unchecked::{{closure}}::h3a8f2ff310d1e325
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/thread/mod.rs:497:30
53: 0x1023d5d88 - core::ops::function::FnOnce::call_once{{vtable.shim}}::hd303580017f66105
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/core/src/ops/function.rs:248:5
54: 0x103a58d10 - <alloc::boxed::Box<F,A> as core::ops::function::FnOnce>::call_once::he4dc75e43b24ddc8
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/alloc/src/boxed.rs:1866:9
55: 0x103a58d10 - <alloc::boxed::Box<F,A> as core::ops::function::FnOnce>::call_once::h85dbea6e6a941e39
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/alloc/src/boxed.rs:1866:9
56: 0x103a58d10 - std::sys::unix::thread::Thread::new::thread_start::h944e84a760d0c477
at /rustc/082e4ca49770ebc9cb0ee616f3726a67471be8cb/library/std/src/sys/unix/thread.rs:108:17
57: 0x1baf4826c - __pthread_deallocate
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: /var/folders/y5/351fq3v90jb8mpn17q1yzp_40000gn/T/.tmplm2J2F
test db::queries::network_interface::tests::test_insert_network_interface_query ... FAILED

failures:

failures:
db::queries::network_interface::tests::test_insert_network_interface_query

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

error: test failed, to rerun pass '-p omicron-nexus --lib'

[bnaecker]

@ryaeng That test asserts that there's a specific error condition reported. Look at the error condition that actually is returned, which should help you proceed.

[ryaeng]

@bnaecker I came to the conclusion yesterday that I was most frustrated with the tool that I was using (Visual Studio Code). Debugging wasn't giving me anything helpful when looking for an error that was returned. While pondering this, I began to think there may be an issue with the tool I was using.

I began searching for other solutions and came across CLion which one user praised as being easier to use when debugging Rust. I figured I had nothing to lose by giving it a try.

First, when building the project CLion immediately threw up errors that I wasn't receiving when building in VS Code. This shouldn't have anything to do with VS Code as they'd both perform the build with Cargo. I fixed those issues first and then got to the point where the test was failing once again. Debugging in CLion immediately gave me a useful error code. There was a conflict with "network_interface_vm_instance_id_name_key". Searching for that string didn't return any results. However, searching for "network_interface_instance_id_name_key" did. I replaced that string with the aforementioned string and voila, test completed.

I currently face a conflict of emotions. On one hand, I'm elated that the test is now passing. On the other, I'm pissed that the tool I was using hindered me from resolving this problem two days ago. Needless to say my taste for VS Code has slightly soured.

[ryaeng]

Which IDE do you fellas use?

[zephraph]

I don't have a deeply informed opinion on the internal type renames but I feel strongly that we shouldn't rename instance to vm-instance at the API layer. The instance nomenclature is a common reference across cloud service APIs and given it's scoped to a project it feels like the context is implied without needing extra specificity. From a UX perspective our URLs and CLI commands are already ergonomically awkward given their length.

[david-crespo]

To that I add: if you buy that premise, then having the internal name be something other than "Instance" seems likely to create more confusion than it eliminates.

[ryaeng]

This sounds reasonable.

What about renaming Service to ServiceInstance? Should we proceed with this change?

[bnaecker]

For the latter, I would argue for something like “VirtualMachine”, but not sure how others feel about the verbosity.

…

On 10 Jun 2022, at 11:50, Sean Klein ***@***.***> wrote:  As discussed with @davepacheco offline, there are a couple renames that might improve clarity within the codebase. Note: These don't necessarily need to correlate with API changes, but given the combination of internal / external state nexus is handling, it might be worthwhile for readability. Service -> ServiceInstance: Within the DB schema, "Service" currently refers to a single Zone, executing somewhere. However, under some circumstances, "Serivce" can refer to a collection of "Service Instances", where each is interchangeable, but provides redundancy. For example, each of the CRDB frontends acts like an "instance" for a "service". (RFD 248 has more detail). By naming these "ServiceInstances", the ambiguity is lessened. Instance -> VmInstance: Similarly, the thing called "Instance" in Nexus currently refers to an "Instance of a VM". Given that the word "instance" is useful in other contexts, disambiguating here may be useful too. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.