Repository navigation
fix(server): warn on dangling provider refs at startup - #4661
ericcurtin wants to merge 2 commits into
Conversation
Signed-off-by: Eric Curtin <eric.curtin@docker.com>
|
@johntmyers @elezar PTAL, and |
Signed-off-by: Eric Curtin <eric.curtin@docker.com>
|
Merged main to fix conflicts. @johntmyers @elezar PTAL, and |
|
Is ok |
|
/ok to test 30c1f3b |
|
Label |
|
E2E failures look unrelated to this change: k3s-ha pod gating timeout, ssh_host_identity container exit, and workspace operator |
EmilienM
left a comment
EmilienM
left a comment
There was a problem hiding this comment.
Approved. This does what #4657 asks: a missing provider gets one warning per sandbox, and store errors and the #2962 invalid-composition guard stay fatal. I left three small comments, none blocking: test ordering, the sequential lookups, and the UpdateConfig path that shares this helper. Heads up that my approval doesn't count toward required reviews, so you'll still need a maintainer's.
| ) | ||
| .await? | ||
| .layers; | ||
| // A provider deleted under a sandbox leaves a dangling reference. The |
There was a problem hiding this comment.
This helper also backs the global-policy delete in UpdateConfig (around L4398), so that RPC stops failing on a dangling reference too. I think that's the right call since the sandbox fails closed either way, but it deserves a line in the PR description, and maybe a test next to deleting_global_policy_rejects_reactivated_ambiguous_provider_binding.
| // sandbox already fails closed, so warn and validate the rest. | ||
| let mut records = Vec::with_capacity(provider_names.len()); | ||
| let mut missing = Vec::new(); | ||
| for name in provider_names { |
There was a problem hiding this comment.
Nit: this loads providers one at a time, while load_provider_environment_records fans out with try_join_all (#4320). It's on the startup path before the listener binds, so each extra provider adds two sequential round trips per sandbox. You could try_join_all over load_provider_environment_record and zip the results with provider_names to split found from missing. That keeps the concurrency, and store errors still short-circuit.
| async fn startup_preflight_still_rejects_invalid_policy_beside_dangling_reference() { | ||
| let state = test_server_state().await; | ||
| install_dangling_provider_sandbox(&state).await; | ||
| install_ambiguous_provider_binding(&state, "beside-dangling").await; |
There was a problem hiding this comment.
I don't think this test always exercises the skip. created_at_ms is the insert wall clock, and the list orders by (created_at_ms, name). When both inserts land in the same millisecond (easy on in-memory SQLite), sandbox-beside-dangling sorts before sandbox-dangling, so the invalid sandbox fails first and the test can pass on main too. A suffix that sorts after dangling (say z-beside-dangling) pins the order. A case with both providers on one sandbox would also catch a regression that skips the whole sandbox.
This branch has not been deployed
4 participants
Summary
A deleted provider no longer blocks gateway startup. The preflight warns and keeps validating.
Related Issue
Closes #4657
Changes
load_provider_environment_recordout ofload_provider_environment_recordsto share the lookup.Testing
Ran
cargo clippyand thestartup_preflightunit tests.Checklist