controller/network: add unit tests for network controller#2647
Open
shreyanshjain7174 wants to merge 2 commits intomicrosoft:mainfrom
Open
controller/network: add unit tests for network controller#2647shreyanshjain7174 wants to merge 2 commits intomicrosoft:mainfrom
shreyanshjain7174 wants to merge 2 commits intomicrosoft:mainfrom
Conversation
shreyanshjain7174
force-pushed
the
controller/network-tests
branch
from
e7512e0 to
604cb57
Compare
shreyanshjain7174
force-pushed
the
controller/network-tests
branch
from
604cb57 to
94fc862
Compare
shreyanshjain7174
force-pushed
the
controller/network-tests
branch
from
94fc862 to
8a793b9
Compare
rawahars
requested changes
Apr 27, 2026
Contributor
rawahars
left a comment
rawahars
left a comment
There was a problem hiding this comment.
Thanks for adding the tests. There are various scenarios which are missing such as-
- Half setup and then teardown
- Half setup, failure in teardown and then success teardown
- Teardown called again after Teardown
- Setup called again after Setup
shreyanshjain7174
force-pushed
the
controller/network-tests
branch
from
8a793b9 to
91976f0
Compare
5 tasks
Contributor
Author
|
@rawahars all 8 review items addressed in 91976f0 (force-pushed). Summary: Removed (per "use existing repo convention"):
Fixed:
Added (the 4 missing teardown-after-failure scenarios you flagged):
Filed #2707 to track the All checks: |
rawahars
requested changes
Apr 27, 2026
Adds gomock mocks (in mocks/) and unit tests for the narrow vmNetworkManager, capabilitiesProvider, and platform-specific guestNetwork interfaces. Mocks are committed without a //go:generate directive to match existing repo convention; standardised mock generation and CI drift validation will be addressed in a separate PR. Tests cover the production failure paths: Shared (network_test.go): - New(): namespace-add capability is queried once and cached; nil capabilities do not panic the constructor. - Setup(): rejects calls in any non-NotConfigured state without mutating state; an empty namespace ID transitions the controller to StateInvalid via the deferred error handler. - Teardown(): no-ops from StateNotConfigured and StateTornDown so containerd's StopPodSandbox retries are safe. LCOW (network_lcow_test.go): - addEndpointToGuestNamespace: host-then-guest ordering, host-only when caps say no, host-failure leaves NIC untracked, guest-failure-after-host keeps the NIC tracked so Teardown can unwind. - removeEndpointFromGuestNamespace: guest-then-host ordering; guest failure short-circuits host removal; host-only path when guest lacks namespace support. - Teardown: full happy path, partial-failure cleanup chain (failed NIC stays tracked, others are removed, state moves to Invalid), host-side RemoveNIC failure path. - Half-setup recovery: host AddNIC succeeds + guest Add fails + Teardown unwinds the host-side device end-to-end; first Teardown fails on guest removal -> StateInvalid -> retry from StateInvalid drains the endpoint and reaches StateTornDown. WCOW (network_wcow_test.go): - addEndpointToGuestNamespace: full PreAdd -> AddNIC -> Add sequence, pre-add failure short-circuits, host failure leaves NIC untracked, final-add failure leaves NIC tracked. - removeEndpointFromGuestNamespace: guest-then-host ordering; guest failure short-circuits host removal. - Teardown: happy path (no-namespace variant) and partial-failure cleanup chain. - Half-setup recovery: finalize-Add failure + Teardown unwinds host; retry-from-StateInvalid succeeds. Signed-off-by: Shreyansh Sancheti <shsancheti@microsoft.com>
shreyanshjain7174
force-pushed
the
controller/network-tests
branch
from
91976f0 to
530518b
Compare
…ntract test Each platform had two tests covering the same half-setup recovery path: TestLCOW_AddEndpoint_HostOK_GuestFails_StillTracked TestLCOW_AddEndpoint_HostOK_GuestFails_TeardownUnwindsHost TestWCOW_AddEndpoint_FinalAddFails_StillTracked TestWCOW_AddEndpoint_FinalAddFails_TeardownUnwindsHost The "_StillTracked" tests asserted the in-memory contract that a NIC remains tracked after a guest-side failure. The "_TeardownUnwindsHost" tests proved the same contract end-to-end by then running Teardown and verifying the host-side device gets unwound. These are not independent contracts: tracking the NIC is the precondition for Teardown unwinding. Splitting them into two tests duplicates setup and hides the relationship. Merge by removing each "_StillTracked" test; the "_TeardownUnwindsHost" tests already perform the tracked-state assertion as their precondition before exercising Teardown. Net result: identical coverage, 2 fewer tests, 1 file with the full half-setup recovery story per platform. Signed-off-by: Shreyansh Sancheti <shsancheti@microsoft.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #2633 — must merge first.
Adds unit tests for the network controller (
internal/controller/network/). 22 tests across LCOW and WCOW, biased toward resource cleanup chains and state corruption rather than guard-check permutations.Teardown: continue-on-error when one NIC removal fails (remaining NICs still attempted, failed NICs stay tracked for retry), retry from Invalid state, idempotent when TornDown, no-op when NotConfigured, full Configured→TornDown lifecycle. Setup rejects duplicate calls.
LCOW endpoint ops: host AddNIC before guest AddLCOWNetworkInterface (ordering enforced via
gomock.InOrder),BuildLCOWNetworkAdapterstruct verified, host-fail→not tracked, guest-fail→still tracked for Teardown cleanup, guest-remove-fails→host RemoveNIC not called, nil capabilities disables guest ops.WCOW endpoint ops: 3-phase add (PreAdd → host AddNIC → guest Add), PreAdd failure→not tracked, guest-finalize failure→still tracked, remove ordering (guest before host), namespace add with/without guest support.
Mocks generated for
vmNetworkManager,linuxGuestNetworkManager,windowsGuestNetworkManager, andcapabilitiesProvider.GuestDefinedCapabilitiesmock reused frominternal/gcs/mock/.