add option to opt-out networking #4871
Conversation
WalkthroughThis pull request updates the bid strategy to use a rejection-based approach rather than an acceptance-based one. The Changes
Sequence Diagram(s)sequenceDiagram
participant Compute as Node Compute
participant Config as Job Admission Control
participant NS as NetworkingStrategy
Compute->>Config: Retrieve {RejectNetworkedJobs, AcceptNetworkedJobs}
Config-->>Compute: Return config settings
Compute->>NS: NewNetworkingStrategy(reject)
NS-->>Compute: Constructed strategy with Reject flag
Compute->>NS: ShouldBid()
NS-->>Compute: Return result based on !Reject
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.62.2){"Issues":[{"FromLinter":"typecheck","Text":"pattern build/: no matching files found","Severity":"","SourceLines":["//go:embed build/"],"Replacement":null,"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":"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":"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":"varcheck"},{"Name":"varnamelen"},{"Name":"wastedassign"},{"Name":"whitespace","Enabled":true},{"Name":"wrapcheck"},{"Name":"wsl"},{"Name":"zerologlint"},{"Name":"nolintlint","Enabled":true}]}} ✨ 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pkg/bidstrategy/semantic/networking.go(2 hunks)pkg/bidstrategy/semantic/networking_test.go(1 hunks)pkg/config/types/admission_control.go(1 hunks)pkg/node/compute.go(1 hunks)pkg/swagger/docs.go(2 hunks)pkg/swagger/swagger.json(2 hunks)webui/lib/api/schema/swagger.json(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build / Build Binary (windows, amd64)
- GitHub Check: build / Build Binary (darwin, arm64)
- GitHub Check: build / Build Binary (darwin, amd64)
- GitHub Check: build / Build Binary (linux, armv6)
- GitHub Check: build / Build Binary (linux, armv7)
- GitHub Check: build / Build Binary (linux, arm64)
- GitHub Check: build / Build Binary (linux, amd64)
- GitHub Check: lint / lint
🔇 Additional comments (15)
pkg/config/types/admission_control.go (1)
13-14: Well-documented deprecation notice.Good job clearly communicating that
AcceptNetworkedJobswill be deprecated in v1.7. This helps users prepare for the transition.pkg/swagger/docs.go (3)
2804-2805: Consistent deprecation notice in Swagger documentation.Good job including the deprecation notice in the API documentation. This ensures that API users are aware of the upcoming change.
2823-2826: API documentation for new property is correctly added.The new
RejectNetworkedJobsproperty is properly documented with a clear description of its purpose.
2804-2826:❓ Verification inconclusive
Ensure code handling the transition is implemented.
The API documentation changes look good, but make sure that the implementation properly handles the transition period when both flags could be set. Consider documenting the behavior when both flags are specified.
🌐 Web query:
What are best practices for API deprecation with replacement fields?💡 Result:
API deprecation with replacement fields requires careful planning to minimize disruption and guide users toward updated functionality. Best practices include clear communication, structured documentation, and phased transitions, supported by framework-specific tooling to automate deprecation warnings. Below are key strategies:
1. Transparent Communication
- Announce early: Notify users of deprecation plans via emails, blog posts, or in-app alerts as soon as the decision is made[1][4][10].
- Deprecation headers: Use HTTP headers like
Deprecation: <date>andSunset: <date>to automate warnings in API responses[7][10].- Specification flags: Mark deprecated fields in OpenAPI/Swagger documentation with
deprecated: trueand add migration guidance to thedescriptionfield[2][7][9].
2. Structured Field Replacement
Direct guidance: In code, use attributes like
deprecationReasonto explain replacements. For example:# FastAPI/Pydantic example from pydantic import Field class ExampleModel(BaseModel): legacy_field: str = Field(deprecated=True, description="Use 'new_field' instead") new_field: strThis renders warnings in tools like Swagger UI[2][5][9].
Backward compatibility: Maintain deprecated fields temporarily while directing users to new ones. For example, in REST APIs, return both fields during the transition, but log warnings for deprecated usage[3][8].
3. Documentation and Migration Support
- Versioned docs: Ensure documentation reflects deprecated fields and their replacements, with side-by-side comparisons[10].
- Migration guides: Provide step-by-step instructions, code snippets, and testing checklists to simplify transitions[4][8].
- Deprecation timelines: Use the
sunsetattribute (e.g.,sunset="01/01/2026") in API configurations to specify removal dates[2][7].
4. Framework-Specific Implementation
API Type Deprecation Method REST/OpenAPI Set deprecated: truein schema and useDeprecation/Sunsetheaders[2][7][10].GraphQL Annotate fields with @deprecated(reason: "...")[9].gRPC/Protobuf Use [deprecated=true]for fields and reserve removed field numbers[9].
5. Gradual Phaseout and Monitoring
- Grace periods: Allow 6–12 months for migration, disabling deprecated fields for new users first[1][4][7].
- Usage analytics: Monitor deprecated field traffic and proactively contact high-usage clients[1][6][8].
- Post-deprecation feedback: Survey users to refine future processes[1][6].
By combining clear communication, structured replacements, and automated tooling, teams can retire outdated fields while maintaining user trust and system reliability.
Citations:
- 1: https://blog.treblle.com/best-practices-deprecating-api/
- 2: https://api-platform.com/docs/v2.4/core/deprecations/
- 3: https://liblab.com/blog/api-versioning-best-practices
- 4: https://document360.com/blog/api-deprecation/
- 5: https://www.getorchestra.io/guides/fast-api-field-deprecation-a-comprehensive-guide
- 6: https://github.com/microsoft/FluidFramework/wiki/API-Deprecation
- 7: https://github.com/zalando/restful-api-guidelines/blob/main/chapters/deprecation.adoc
- 8: https://www.getambassador.io/blog/api-versioning-best-practices
- 9: https://apisyouwonthate.com/blog/surviving-deprecations-to-resources-and-properties-on-other-apis/
- 10: https://bump.sh/blog/how-our-api-docs-can-ease-the-pain-of-api-deprecation
Action: Verify Transition Handling for Network Job Flags
The deprecation note in the Swagger spec (marking
AcceptNetworkedJobsfor deprecation in v1.7) is clear. However, please ensure that the implementation correctly manages scenarios where bothAcceptNetworkedJobsandRejectNetworkedJobsmight be set. In line with API deprecation best practices, consider the following:
- Deprecation Marking: Add a
deprecated: trueflag (if supported by your tooling) to clearly indicate thatAcceptNetworkedJobsis being phased out.- Defined Precedence: Document and enforce a clear rule for resolving conflicts when both flags are provided (e.g., specify which flag takes precedence or if a warning is returned).
- User Guidance: Update the documentation to include migration instructions and examples, ensuring that users are well-informed during the transition period.
Please review the business logic to verify that these aspects are implemented and properly documented.
webui/lib/api/schema/swagger.json (1)
2799-2802: Good approach to API evolution with clear deprecation path.The changes properly introduce the new
RejectNetworkedJobsfield while marking the existingAcceptNetworkedJobsfield for deprecation with a clear timeline (v1.7). This approach ensures a smooth transition for users while shifting from an acceptance-based to a rejection-based model for networked jobs.Also applies to: 2819-2822
pkg/node/compute.go (1)
323-324: Solid implementation of transitional logic for the networking strategy.The change implements a backward-compatible transition from an acceptance-based to a rejection-based approach for networked jobs. Using
RejectNetworkedJobs || !AcceptNetworkedJobsensures that:
- Existing configurations using
AcceptNetworkedJobs=falsecontinue to work as expected- New configurations can use the more explicit
RejectNetworkedJobs=true- When both are specified, rejection takes precedence (logical OR)
This approach maintains compatibility while allowing for a clean transition to the new pattern.
pkg/swagger/swagger.json (2)
2800-2802: Good addition of deprecation notice.The description now clearly indicates that
AcceptNetworkedJobswill be deprecated in v1.7 in favor of the new property, which helps users prepare for the transition.
2819-2822: Clear implementation of new option to opt-out networking.The
RejectNetworkedJobsproperty is well-documented and follows a more intuitive "opt-out" model rather than the previous "opt-in" approach. This matches the PR objective of adding an option to opt-out networking.pkg/bidstrategy/semantic/networking.go (3)
9-10: Well-designed inversion of control logic.Changing from
Accept booltoReject boolis a good approach for the opt-out model. This follows the principle of making the default case (accepting networked jobs) the implicit behavior, requiring explicit action only to reject them.
15-16: Constructor properly updated to match field name change.The constructor has been correctly updated to take the new
rejectparameter instead ofaccept, maintaining consistency with the struct field change.
32-32: Logic properly inverted to maintain behavior consistency.The return statement now uses the negation of the
Rejectfield (!s.Reject), correctly implementing the inverted control logic. This ensures that the behavior remains consistent despite the field name change.pkg/bidstrategy/semantic/networking_test.go (4)
19-23: Test case struct correctly updated to reflect the new model.The
networkingStrategyTestCasestruct has been updated from usingaccepttoreject, properly aligning with the main code changes.
25-31: String method appropriately updated for clarity.The
String()method has been updated to clearly indicate that the strategy now "rejects networking" rather than "accepts networking", which improves test output readability.
34-39: Test cases properly updated with clear comments.The test cases and their comments have been updated to reflect the new rejection-based logic. The comments effectively explain each case's expected behavior based on whether the job is local or networked and whether it is rejecting or not.
45-45: Strategy initialization correctly uses the new parameter.The test properly initializes the NetworkingStrategy with the test's
rejectfield, ensuring consistency with the updated constructor.
| // RejectNetworkedJobs indicates whether to reject jobs that require network access. | ||
| RejectNetworkedJobs bool `yaml:"RejectNetworkedJobs,omitempty" json:"RejectNetworkedJobs,omitempty"` |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider clarifying the relationship between the two networking flags.
The addition of RejectNetworkedJobs is good, but you might want to clarify how it interacts with the deprecated AcceptNetworkedJobs flag during the transition period. For example, what happens if both flags are set with conflicting values?
🏁 Script executed:
#!/bin/bash
# Search for code that might handle interaction between the two flags
rg -A 10 -B 10 "RejectNetworkedJobs|AcceptNetworkedJobs" --type go | grep -v "admission_control.go"Length of output: 50056
Clarify Networking Flags Interaction
The addition of the new RejectNetworkedJobs flag is a welcome improvement. However, please add documentation that clearly explains its relationship with the deprecated AcceptNetworkedJobs flag during this transition period. In particular, it would be helpful to explain that the effective behavior is computed as follows:
rejectNetwork = RejectNetworkedJobs || !AcceptNetworkedJobs
This means:
- Networked jobs are rejected if
RejectNetworkedJobsis true, regardless of the value ofAcceptNetworkedJobs. - If
RejectNetworkedJobsis false butAcceptNetworkedJobsis false, networked jobs are still rejected. - Only when
RejectNetworkedJobsis false andAcceptNetworkedJobsis true will networked jobs be accepted.
Action Items:
- pkg/config/types/admission_control.go: Update the inline comments for these flags to include the above explanation.
- pkg/node/compute.go: Add a comment near the computation of the networking strategy (using
semantic.NewNetworkingStrategy(...)) to mention that this logic uses the combined condition. - pkg/config/migrate.go: Verify and document how the migration logic handles these flags so that future maintainers understand the prioritization.
The current behaviour in bacalhau is that networking in disabled by default in jobs and in compute nodes. Users would have to opt-in for networking by setting
JobAdmissionControl.AcceptNetworkedJobsconfig to true. This has caused lots of confusion as jobs that require networking would fail without a clear reason.This PR is a first step towards making networking enabled by default in both compute node admission control and in job spec, and making networking opt-out instead of opt-in.
The PR just introduces the new opt-out config
JobAdmissionControl.RejectNetworkedJobsto allow users to reject networking today and prepare for the change when networking is enabled by default in v1.7Summary by CodeRabbit
New Features
Documentation