Add NetworkDefault type for compute node autonomy #4897
Conversation
WalkthroughThe changes update the default network configuration across the codebase by replacing instances of Changes
Sequence Diagram(s)sequenceDiagram
participant Job as Job Request
participant Executor as BaseExecutor
participant Docker as Docker Network Setup
Job ->> Executor: Submit job (network type = NetworkDefault)
Executor ->> Docker: PrepareRunArguments()
Docker ->> Docker: Check network type\n(Convert NetworkDefault to NetworkBridge)
Docker ->> Executor: Return network configuration
Executor ->> Job: Execute job with network config
sequenceDiagram
participant Job as Job Request
participant Executor as BaseExecutor
participant WASM as WASM Handler
Job ->> Executor: Submit job (network type = undefined/NetworkDefault)
Executor ->> WASM: loadModules()
WASM ->> WASM: Check network type\n(If nil/NetworkDefault, set to NetworkHost)
WASM ->> Executor: Return network configuration
Executor ->> Job: Execute job with network config
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.64.8){"Issues":[{"FromLinter":"typecheck","Text":"pattern build/: no matching files found","Severity":"","SourceLines":["//go:embed build/"],"Pos":{"Filename":"webui/webui.go","Offset":0,"Line":23,"Column":12},"ExpectNoLint":false,"ExpectedNoLintLinter":""}],"Report":{"Linters":[{"Name":"asasalint"},{"Name":"asciicheck"},{"Name":"bidichk"},{"Name":"bodyclose","Enabled":true},{"Name":"canonicalheader"},{"Name":"containedctx"},{"Name":"contextcheck"},{"Name":"copyloopvar","Enabled":true},{"Name":"cyclop"},{"Name":"decorder"},{"Name":"deadcode"},{"Name":"depguard","Enabled":true},{"Name":"dogsled","Enabled":true},{"Name":"dupl"},{"Name":"dupword"},{"Name":"durationcheck"},{"Name":"errcheck","Enabled":true,"EnabledByDefault":true},{"Name":"errchkjson"},{"Name":"errname"},{"Name":"errorlint"},{"Name":"execinquery"},{"Name":"exhaustive"},{"Name":"exhaustivestruct"},{"Name":"exhaustruct"},{"Name":"exportloopref"},{"Name":"exptostd"},{"Name":"forbidigo","Enabled":true},{"Name":"forcetypeassert"},{"Name":"fatcontext"},{"Name":"funlen","Enabled":true},{"Name":"gci"},{"Name":"ginkgolinter"},{"Name":"gocheckcompilerdirectives"},{"Name":"gochecknoglobals"},{"Name":"gochecknoinits","Enabled":true},{"Name":"gochecksumtype"},{"Name":"gocognit"},{"Name":"goconst","Enabled":true},{"Name":"gocritic"},{"Name":"gocyclo","Enabled":true},{"Name":"godot"},{"Name":"godox"},{"Name":"err113"},{"Name":"gofmt","Enabled":true},{"Name":"gofumpt"},{"Name":"goheader"},{"Name":"goimports","Enabled":true},{"Name":"golint"},{"Name":"mnd","Enabled":true},{"Name":"gomnd"},{"Name":"gomoddirectives"},{"Name":"gomodguard"},{"Name":"goprintffuncname","Enabled":true},{"Name":"gosec","Enabled":true},{"Name":"gosimple","Enabled":true,"EnabledByDefault":true},{"Name":"gosmopolitan"},{"Name":"govet","Enabled":true,"EnabledByDefault":true},{"Name":"grouper"},{"Name":"ifshort"},{"Name":"iface"},{"Name":"importas"},{"Name":"inamedparam"},{"Name":"ineffassign","Enabled":true,"EnabledByDefault":true},{"Name":"interfacebloat"},{"Name":"interfacer"},{"Name":"intrange"},{"Name":"ireturn"},{"Name":"lll","Enabled":true},{"Name":"loggercheck"},{"Name":"maintidx"},{"Name":"makezero"},{"Name":"maligned"},{"Name":"mirror"},{"Name":"misspell"},{"Name":"musttag"},{"Name":"nakedret","Enabled":true},{"Name":"nestif"},{"Name":"nilerr"},{"Name":"nilnesserr"},{"Name":"nilnil"},{"Name":"nlreturn"},{"Name":"noctx","Enabled":true},{"Name":"nonamedreturns"},{"Name":"nosnakecase"},{"Name":"nosprintfhostport"},{"Name":"paralleltest"},{"Name":"perfsprint"},{"Name":"prealloc"},{"Name":"predeclared"},{"Name":"promlinter"},{"Name":"protogetter"},{"Name":"reassign"},{"Name":"recvcheck"},{"Name":"revive"},{"Name":"rowserrcheck"},{"Name":"sloglint"},{"Name":"scopelint"},{"Name":"sqlclosecheck"},{"Name":"spancheck"},{"Name":"staticcheck","Enabled":true,"EnabledByDefault":true},{"Name":"structcheck"},{"Name":"stylecheck","Enabled":true},{"Name":"tagalign"},{"Name":"tagliatelle"},{"Name":"tenv"},{"Name":"testableexamples"},{"Name":"testifylint"},{"Name":"testpackage"},{"Name":"thelper"},{"Name":"tparallel"},{"Name":"typecheck","Enabled":true,"EnabledByDefault":true},{"Name":"unconvert","Enabled":true},{"Name":"unparam"},{"Name":"unused","Enabled":true,"EnabledByDefault":true},{"Name":"usestdlibvars"},{"Name":"usetesting"},{"Name":"varcheck"},{"Name":"varnamelen"},{"Name":"wastedassign"},{"Name":"whitespace","Enabled":true},{"Name":"wrapcheck"},{"Name":"wsl"},{"Name":"zerologlint"},{"Name":"nolintlint","Enabled":true}]}} Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/compute/executor.go (1)
37-40: Documentation notes temporary nature of this solutionThe comment properly documents that this is an interim approach and suggests a future direction for a more comprehensive solution. Consider creating a ticket to track this technical debt if one doesn't already exist.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
cmd/cli/docker/docker_run_cli_test.go(2 hunks)cmd/cli/wasm/wasm_run_cli_test.go(1 hunks)cmd/util/flags/cliflags/task.go(1 hunks)ops/terraform/remote_files/scripts/apply-http-allowlist.sh(1 hunks)pkg/bidstrategy/semantic/networking.go(3 hunks)pkg/bidstrategy/semantic/networking_allowlist_test.go(1 hunks)pkg/bidstrategy/semantic/networking_test.go(1 hunks)pkg/compute/executor.go(2 hunks)pkg/compute/port_allocator_test.go(1 hunks)pkg/config/types/admission_control.go(0 hunks)pkg/executor/docker/network.go(1 hunks)pkg/executor/wasm/handler.go(1 hunks)pkg/models/execution_test.go(1 hunks)pkg/models/network.go(4 hunks)pkg/models/network_string.go(1 hunks)pkg/node/compute.go(3 hunks)pkg/swagger/docs.go(2 hunks)pkg/swagger/swagger.json(1 hunks)pkg/test/devstack/networking_test.go(1 hunks)webui/lib/api/schema/swagger.json(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/config/types/admission_control.go
🧰 Additional context used
🧬 Code Definitions (10)
pkg/bidstrategy/semantic/networking_allowlist_test.go (1)
pkg/models/network.go (1) (1)
NetworkDefault(37-37)
cmd/util/flags/cliflags/task.go (1)
pkg/models/network.go (2) (2)
Network(27-27)NetworkDefault(37-37)
pkg/bidstrategy/semantic/networking.go (1)
pkg/models/network.go (2) (2)
Network(27-27)NetworkDefault(37-37)
pkg/compute/port_allocator_test.go (2)
pkg/models/network.go (3) (3)
NetworkDefault(37-37)NetworkNone(40-40)NetworkHTTP(79-79)pkg/models/execution_test.go (1) (1)
s(17-215)
cmd/cli/wasm/wasm_run_cli_test.go (1)
pkg/models/network.go (1) (1)
NetworkDefault(37-37)
pkg/executor/docker/network.go (1)
pkg/models/network.go (3) (3)
Network(27-27)NetworkDefault(37-37)NetworkBridge(84-84)
pkg/node/compute.go (3)
pkg/models/network.go (2) (2)
NetworkDefault(37-37)NetworkNone(40-40)pkg/config/types/admission_control.go (1) (1)
JobAdmissionControl(7-18)pkg/bidstrategy/semantic/networking.go (1) (1)
NewNetworkingStrategy(16-18)
pkg/test/devstack/networking_test.go (3)
pkg/models/task.go (1) (1)
Task(13-43)pkg/models/network.go (3) (3)
Network(27-27)NetworkConfig(120-124)NetworkDefault(37-37)pkg/config/types/admission_control.go (1) (1)
JobAdmissionControl(7-18)
pkg/models/network_string.go (1)
pkg/models/network.go (6) (6)
NetworkDefault(37-37)NetworkNone(40-40)NetworkHost(44-44)NetworkFull(48-48)NetworkHTTP(79-79)NetworkBridge(84-84)
pkg/compute/executor.go (4)
pkg/models/network.go (2) (2)
Network(27-27)NetworkDefault(37-37)pkg/compute/store/types.go (1) (1)
ExecutionStore(13-44)pkg/models/failure_injection.go (1) (1)
FailureInjectionConfig(3-5)pkg/compute/types.go (1) (1)
PortAllocator(53-64)
🔇 Additional comments (34)
pkg/executor/wasm/handler.go (1)
200-203: Good implementation of the NetworkDefault handlingThis change ensures that when a job has either no network configuration (
nil) or usesmodels.NetworkDefault, the WASM executor defaults to host networking. This implements the PR's objective of allowing compute nodes to determine the appropriate networking setup based on their capabilities.The code properly handles both cases where network might be unspecified (either explicitly as NetworkDefault or implicitly as nil).
cmd/util/flags/cliflags/task.go (1)
78-78: Updated default network type appropriatelyThis change correctly updates the default network type from
NetworkNonetoNetworkDefaultin the CLI task settings. This aligns with the PR's objective of makingNetworkDefaultthe zero value in the network type enumeration, representing cases where networking requirements are unspecified.pkg/bidstrategy/semantic/networking_allowlist_test.go (1)
38-38: Good test case addition for NetworkDefaultThis test case appropriately verifies that jobs with
NetworkDefaultand no specified domains should be allowed to bid. This ensures the bidding strategy correctly handles the new network type.ops/terraform/remote_files/scripts/apply-http-allowlist.sh (1)
12-13: Updated HTTP allowlist script to accept NetworkDefault typeThe script has been properly updated to accept jobs with a
Network.Typeof 'Default' in addition to 'HTTP' and 'None'. The error message has also been updated to reflect this change. This ensures consistency in how the new network type is handled across the system.pkg/bidstrategy/semantic/networking.go (2)
23-23: Good addition of descriptive reason string for undefined network access.This new constant aligns well with the PR objective of distinguishing between explicitly disabled networking and unspecified network requirements.
33-35: Well implemented NetworkDefault handling in the bidding strategy.This code properly implements the PR's goal of handling cases where network requirements are unspecified, allowing compute nodes to determine appropriate networking setup based on their capabilities. The strategy correctly responds with a positive bid and the newly added undefinedReason.
cmd/cli/wasm/wasm_run_cli_test.go (1)
44-44: Correctly updated default network type in test configuration.This change aligns with the PR's goal of using NetworkDefault as the zero value in the network type enumeration. The test now properly reflects the updated default behavior where unspecified network requirements are represented by NetworkDefault instead of NetworkNone.
pkg/executor/docker/network.go (1)
74-77: Good implementation of compute node autonomy with Docker bridge network default.This change exemplifies the PR's core goal - allowing compute nodes to determine appropriate network setup based on their capabilities. For Docker executors, bridge networking is a sensible default when no specific networking is requested, providing isolated networking with controlled access.
pkg/models/execution_test.go (2)
138-143: Good test refactoring to distinguish NetworkDefault from NetworkNone.Renaming the test case to "undefined network config" and updating it to use NetworkDefault properly differentiates between unspecified networking and explicitly disabled networking. This change makes the test more accurate and better documents the new behavior.
144-146: Well-structured addition of explicit NetworkNone test case.Adding a dedicated test case for NetworkNone ensures that both networking states (default and explicitly disabled) are properly tested. This helps maintain backward compatibility while validating the new functionality.
pkg/bidstrategy/semantic/networking_test.go (2)
36-36: Test case added for NetworkDefault without rejectingThe new test case properly validates that jobs with
NetworkDefaultshould be accepted when the networking strategy is not rejecting networked jobs. This aligns with the PR objective of treatingNetworkDefaultas a case where the network configuration is unspecified.
40-40: Test case added for NetworkDefault with rejecting strategyThe test case correctly verifies that jobs with
NetworkDefaultare still accepted even when the networking strategy is set to reject networked jobs. This matches the expected behavior whereNetworkDefaultrepresents an unspecified configuration rather than an explicit network requirement.pkg/node/compute.go (3)
115-122: Clear implementation of network type defaults based on node configurationThis implementation correctly differentiates between explicitly disabled networking and unspecified networking requirements. Setting
defaultNetworkTypetoNetworkDefaultby default, and only changing it toNetworkNonewhen networked jobs are explicitly rejected, aligns perfectly with the PR objectives.The comments effectively explain the reasoning behind this design decision.
134-134: Passing DefaultNetworkType to BaseExecutorThe new parameter correctly passes the determined network type to the base executor, allowing the executor to apply the appropriate networking defaults based on node configuration.
325-325: Simplified networking strategy initializationThe bidder now creates a networking strategy using only the
RejectNetworkedJobsconfiguration, which is a cleaner approach. This simplification maintains the same functionality while making the code more readable.pkg/compute/port_allocator_test.go (2)
41-42: Updated test cases for network typesThe changes correctly rename the test case to "undefined network" with
NetworkDefaulttype and add a new "none network" test case withNetworkNonetype. This properly separates the two different network configurations that were previously combined.
49-49: Updated condition to check for NetworkDefaultThe conditional check is properly updated to test against
models.NetworkDefaultinstead ofmodels.NetworkNone. This change ensures that the test correctly handles the new default networking type.pkg/test/devstack/networking_test.go (2)
128-143: New integration test for NetworkDefault behaviorThis test properly validates that jobs with
NetworkDefaultnetwork configuration can access the network when networked jobs are not rejected. The test uses the bridge networking mode indirectly throughhost.docker.internal, which is the correct approach for these tests.
145-169: New integration test for NetworkDefault when networked jobs are rejectedThis test correctly verifies that when
RejectNetworkedJobsis set to true, jobs withNetworkDefaultnetwork configuration are treated as if they hadNetworkNone(i.e., network access is blocked).The assertions for empty stdout and exit code 7 are appropriate for verifying network failure.
webui/lib/api/schema/swagger.json (2)
1753-1771: Updatemodels.Networkenum with new NetworkDefault type.The enum now starts with the new
"NetworkDefault"element (value 0) followed by"NetworkNone","NetworkHost","NetworkFull","NetworkHTTP", and finally"NetworkBridge". This reordering aligns with the PR objective of disambiguating unspecified networking from explicitly disabled networking. Please verify that the integer mappings remain consistent with other system components and documentation.
2700-2728: Confirm removal of the AcceptNetworkedJobs property.In the
types.JobAdmissionControldefinition, the property"AcceptNetworkedJobs"has been removed as intended, leaving only"RejectNetworkedJobs"(and"RejectStatelessJobs"). Ensure that downstream components and API consumers have been updated accordingly to accommodate this change and that the removal does not disrupt backward compatibility.pkg/swagger/swagger.json (1)
1764-1770: The NetworkDefault addition aligns with PR objectivesThe addition of
NetworkDefaultas the first (zero) value in the Network enum with subsequent reordering of existing values properly supports the PR objective of distinguishing between explicitly disabled networking and unspecified networking configurations.pkg/models/network_string.go (1)
11-21: Auto-generated code updates reflect the Network enum changesThese changes in the auto-generated code correctly reflect the addition of
NetworkDefaultas the first value in the Network enum. The string name and index arrays have been properly updated to include the new "Default" value.cmd/cli/docker/docker_run_cli_test.go (2)
47-49: Updated default network configuration aligns with new NetworkDefault typeThe expected default network configuration now correctly uses
models.NetworkDefaultinstead ofmodels.NetworkNone, reflecting the new zero value for network types.
371-377: Test case updated to verify NetworkDefault behaviorThe test case name change from "with network none default" to "with network undefined default" accurately reflects the semantic difference between unspecified networks and explicitly disabled networks. The assertion now correctly checks for
models.NetworkDefaultwhen no network type is specified.pkg/compute/executor.go (2)
40-40: Addition of DefaultNetworkType parameter enables compute node autonomyAdding the DefaultNetworkType field to BaseExecutorParams allows compute nodes to specify their preferred default network type, which is a key requirement from the PR objectives.
132-135: Implementation correctly handles NetworkDefault fallbackThis is the core implementation that fulfills the PR objective - when a job specifies NetworkDefault (or doesn't specify a network type), the executor will apply its default network type instead. This gives compute nodes the autonomy to determine appropriate networking based on their capabilities.
pkg/swagger/docs.go (1)
1768-1774: NetworkDefault enum added as the zero valueThe enum ordering now starts with
NetworkDefaultas the zero value, withNetworkNonemoved to 1. This aligns with the implementation inpkg/models/network.goand provides clearer semantics for networking specifications.pkg/models/network.go (6)
30-37: Well-documented new NetworkDefault typeThe introduction of
NetworkDefaultas the zero value provides better semantics and autonomy for compute nodes to determine appropriate networking based on executor capabilities. The comprehensive documentation clearly explains the behavior and implications with different executors.
92-96: Good abstraction with the Disabled() methodAdding this helper method improves code organization and encapsulation by moving the network disabling logic to the appropriate type. This will make future networking model changes more maintainable.
101-108: Updated ParseNetwork to handle NetworkDefaultThe function now properly iterates from
NetworkDefaultand returns it as the default value when parsing fails. This maintains consistency with the enum's new zero value.
129-129: Improved encapsulation in NetworkConfig.Disabled()Replacing the direct comparison with a call to
n.Type.Disabled()follows good object-oriented principles by delegating the logic to the appropriate type.
167-167: Updated validation range for network typesThe validation now correctly checks if the network type is within the range starting from
NetworkDefaultinstead ofNetworkNone, consistent with the enum change.
30-168:✅ Verification successful
Verify backward compatibility with existing jobs
This change alters the zero value from
NetworkNonetoNetworkDefault, which could impact existing code relying on the previous behavior. Consider adding integration tests to verify that older jobs withNetworkNonecontinue to function correctly.
🏁 Script executed:
#!/bin/bash # Check for potential issues with the changed network type constants # Look for code that might be affected by the Network zero value change echo "Searching for potential breaking changes related to Network zero value..." # Check for direct comparisons with NetworkNone that might need updating rg "Network(None|Default)" --type go -B 3 -A 3 # Check for code that might be serializing/deserializing Network values rg "json.*Network|Network.*json|marshal.*Network|Network.*marshal" --type go # Check for tests that might be affected rg "NetworkNone.*test|test.*NetworkNone" --type goLength of output: 19355
Ensure Backward Compatibility with Network Constant Updates
After reviewing the code and test coverage, it looks like both the new default value (
NetworkDefault) and the legacy value (NetworkNone) are consistently exercised across multiple tests (see files such aspkg/test/scenario/test_scenarios.go,pkg/test/devstack/networking_test.go, and various executor tests). This confirms that our integration tests already cover scenarios for both undefined and explicit network configurations.Please double-check externally if any legacy jobs depend on the previous zero value behavior, but within our codebase the backward compatibility appears to be maintained.
…4898) ## Description This PR adds network configuration transformation in the orchestrator's NCL message creator to ensure backward compatibility with older compute nodes that don't understand the new network types. The transformation happens only when creating AskForBid messages before they are sent to compute nodes. Two specific transformations are applied: 1. If network type is `NetworkHost`, it is transformed to `NetworkFull` for backward compatibility 2. If network type is `NetworkDefault`, the network configuration is set to `nil` These transformations ensure that older compute nodes can still process jobs regardless of whether they understand the newer network types. ## Implementation Details - Added a `transformNetworkConfig` method to the `NCLMessageCreator` to transform network configurations in-place - Updated the `createAskForBidMessage` method to apply the network transformation before sending ## Testing The implementation has been thoroughly tested with: - Manual testing with v1.6 compute node and latest orchestrator - Unit tests for each type of network transformation (Host→Full, Default→nil) - Tests for edge cases like nil Job or nil Network - Test that other network types (Bridge, None, HTTP) remain unchanged ## Why is this needed? Backward compatibility is critical when introducing new network types to avoid disrupting existing deployments. This approach allows newer orchestrator versions to communicate successfully with older compute nodes, ensuring a smooth upgrade path. This solution is non-intrusive, requiring no changes to the compute nodes themselves, and efficiently implements the transformation only at the message boundary. Related to #4897 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced network configuration adjustments to ensure smoother and compatible bid messaging, with specific network types now transitioning appropriately. - **Tests** - Added comprehensive tests covering various scenarios to validate the updated network configuration handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Network Default Type for Better Node Autonomy
Summary
This PR introduces a new
NetworkDefaulttype as the zero value in our network type iota to clearly distinguish between explicitly disabled networking (NetworkNone) and networking configuration that should be determined by the compute node. This change improves the handling of jobs without explicit network specifications by delegating the decision to compute nodes based on their capabilities and admission control policies.Problem Statement
Previously, we used
NetworkNoneas both the default value and to explicitly disable networking. This created ambiguity in distinguishing between a job that genuinely requires no network access and one where the user simply didn't specify any networking requirements. This limitation made it difficult for compute nodes to apply their own appropriate defaults based on their executor capabilities.Solution
We're introducing
NetworkDefaultas the initial value (iota = 0) in our network type enumeration. This new type:Key Changes
NetworkDefaultas the zero value in the network type enumNetworkDefaultas a valid network typeNetworkDefaultbased on executor type and admission control settingsNetworkDefaultwhen forwarding jobsBenefits
Migration Impact
NetworkNonewill continue to have networking disabled.Nonein the client sideNetworkDefaulttype. Fixed in Add network configuration transformation for backward compatibility #4898Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests