Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds concept of a primary network interface #1143

Merged
merged 2 commits into from
Jun 3, 2022
Merged

Conversation

bnaecker
Copy link
Collaborator

@bnaecker bnaecker commented Jun 1, 2022

  • Adds the is_primary column to the network_interface table, and a
    corresponding field primary in the database model and external
    NetworkInterface objects. Primary interfaces are used for NAT and
    appear in DNS records.
  • Updates the InsertNetworkInterfaceQuery to automatically decide if
    this interface should be considered the primary. It considers the new
    NIC primary iff there are zero existing NICs for the instance it's to
    be attached to. That means that the first NIC added to an instance,
    either during a provision or later, is the primary. Future work could
    allow changing which NIC is the primary.
  • Adds a new query for deleting a network interface from an instance,
    with improved validation. This now checks that the instance is stopped
    inside the query, fixing a TOCTOU bug. It also verifies that the
    instance either has exactly 1 interface (which must be the primary) or
    that the instance has 2 or more (we're deleting a secondary). This
    means that the primary interface cannot be deleted until all secondary
    interfaces are deleted. The reason for this restriction is that
    instances must have a primary interface, and it's not clear how to
    pick a new primary from the remaining secondaries if we allow deletion
    of the primary. We force the client to make the choice.
  • Adds a special error type for handling the above validation failures.
  • Adds tests for this deletion behavior to the instance integration
    tests

@bnaecker
Copy link
Collaborator Author

bnaecker commented Jun 1, 2022

This is intended to resolve #960.

- Adds the `is_primary` column to the `network_interface` table, and a
  corresponding field `primary` in the database model and external
  `NetworkInterface` objects. Primary interfaces are used for NAT and
  appear in DNS records.
- Updates the `InsertNetworkInterfaceQuery` to automatically decide if
  this interface should be considered the primary. It considers the new
  NIC primary iff there are zero existing NICs for the instance it's to
  be attached to. That means that the first NIC added to an instance,
  either during a provision or later, is the primary. Future work could
  allow changing which NIC is the primary.
- Adds a new query for deleting a network interface from an instance,
  with improved validation. This now checks that the instance is stopped
  inside the query, fixing a TOCTOU bug. It also verifies that the
  instance either has exactly 1 interface (which must be the primary) or
  that the instance has 2 or more (we're deleting a secondary). This
  means that the primary interface cannot be deleted until all secondary
  interfaces are deleted. The reason for this restriction is that
  instances _must_ have a primary interface, and it's not clear how to
  pick a new primary from the remaining secondaries if we allow deletion
  of the primary. We force the client to make the choice.
- Adds a special error type for handling the above validation failures.
- Adds tests for this deletion behavior to the instance integration
  tests
@bnaecker bnaecker force-pushed the mark-nic-primary branch from a213668 to d83abad Compare June 1, 2022 17:33
Copy link
Collaborator

@smklein smklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Couple questions, but otherwise LGTM!

nexus/src/db/datastore.rs Outdated Show resolved Hide resolved
nexus/src/db/queries/network_interface.rs Outdated Show resolved Hide resolved
nexus/src/db/queries/network_interface.rs Outdated Show resolved Hide resolved
nexus/src/db/queries/network_interface.rs Show resolved Hide resolved
nexus/src/db/queries/network_interface.rs Outdated Show resolved Hide resolved
nexus/src/db/queries/network_interface.rs Show resolved Hide resolved
nexus/src/external_api/http_entrypoints.rs Show resolved Hide resolved
nexus/src/db/queries/network_interface.rs Show resolved Hide resolved
- Cleanup comments
- Simplify NIC query/error type names
- Remove stale test
@bnaecker bnaecker merged commit e2bd1e8 into main Jun 3, 2022
@bnaecker bnaecker deleted the mark-nic-primary branch June 3, 2022 18:00
leftwo pushed a commit that referenced this pull request Feb 9, 2024
Crucible changes:
Remove unused fields in IOop (#1149)
New downstairs clone subcommand. (#1129)
Simplify the do_work_task loop (#1150)
Move `Guest` stuff into a module (#1125)
Bump nix to 0.27.1 and use new safer Fd APIs (#1110)
Move `FramedWrite` work to a separate task (#1145)
Use fewer borrows in ExtentInner API (#1147)
Update Rust crate reedline to 0.28.0 (#1141)
Update Rust crate tokio to 1.36 (#1143)
Update Rust crate slog-bunyan to 2.5.0 (#1139)
Update Rust crate rayon to 1.8.1 (#1138)
Update Rust crate itertools to 0.12.1 (#1137)
Update Rust crate byte-unit to 5.1.4 (#1136)
Update Rust crate base64 to 0.21.7 (#1135)
Update Rust crate async-trait to 0.1.77 (#1134)
Discard deferred msgs (#1131)
Minor Downstairs cleanup (#1127)
Update test_fail_live_repair to support pstop (#1128)
Ignore client messages after stopping the IO task (#1126)
Move client IO task into a struct (#1124)
Bump Rust to 1.75 and fix new Clippy lints (#1123)

Propolis changes:
PHD: convert to async (#633)
PHD: assume specialized Windows images (#636)
propolis-standalone-config needn't be a crate
standalone: Use tar for snapshot/restore
phd: use latest "lab-2.0-opte" target, not a specific version (#637)
PHD: add tests for migration of running processes (#623)
PHD: fix `cargo xtask phd` tidy not doing anything (#630)
PHD: add documentation for `cargo xtask phd` (#629)
standalone: improve virtual device creation errors (#632)
phd: add Windows Server 2019 guest adapter (#627)
PHD: add `cargo xtask phd` to make using PHD nicer (#619)
leftwo added a commit that referenced this pull request Feb 9, 2024
Crucible changes:
Remove unused fields in IOop (#1149)
New downstairs clone subcommand. (#1129)
Simplify the do_work_task loop (#1150)
Move `Guest` stuff into a module (#1125)
Bump nix to 0.27.1 and use new safer Fd APIs (#1110) Move `FramedWrite`
work to a separate task (#1145) Use fewer borrows in ExtentInner API
(#1147)
Update Rust crate reedline to 0.28.0 (#1141)
Update Rust crate tokio to 1.36 (#1143)
Update Rust crate slog-bunyan to 2.5.0 (#1139)
Update Rust crate rayon to 1.8.1 (#1138)
Update Rust crate itertools to 0.12.1 (#1137)
Update Rust crate byte-unit to 5.1.4 (#1136)
Update Rust crate base64 to 0.21.7 (#1135)
Update Rust crate async-trait to 0.1.77 (#1134)
Discard deferred msgs (#1131)
Minor Downstairs cleanup (#1127)
Update test_fail_live_repair to support pstop (#1128) Ignore client
messages after stopping the IO task (#1126) Move client IO task into a
struct (#1124)
Bump Rust to 1.75 and fix new Clippy lints (#1123)

Propolis changes:
PHD: convert to async (#633)
PHD: assume specialized Windows images (#636)
propolis-standalone-config needn't be a crate
standalone: Use tar for snapshot/restore
phd: use latest "lab-2.0-opte" target, not a specific version (#637)
PHD: add tests for migration of running processes (#623) PHD: fix `cargo
xtask phd` tidy not doing anything (#630) PHD: add documentation for
`cargo xtask phd` (#629) standalone: improve virtual device creation
errors (#632) phd: add Windows Server 2019 guest adapter (#627)
PHD: add `cargo xtask phd` to make using PHD nicer (#619)

Co-authored-by: Alan Hanson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants