devstack: support node joining existing networks and config passing #4827
Conversation
WalkthroughThe pull request introduces modifications across multiple files ( Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Command
participant Config as Configuration Handler
participant DevStack as DevStack Setup
CLI->>Config: Call SetupConfig with options
Config-->>CLI: Return Bacalhau configuration
CLI->>DevStack: Initialize with configuration
DevStack-->>CLI: Configure nodes and settings
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/devstack/devstack.go (1)
99-102: Consider documenting the port increment logic.The addition of port increment for external orchestrator cases is a subtle change that should be documented to prevent future confusion.
cfg.API.Port = 1234 + i - // add one more if using an external orchestrator to avoid port conflict + // When using an external orchestrator (requesterNodeCount == 0), + // increment the port by 1 to avoid conflicts with the orchestrator's ports if requesterNodeCount == 0 { cfg.API.Port += 1 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/cli/devstack/devstack.go(8 hunks)cmd/util/repo.go(2 hunks)pkg/devstack/devstack.go(1 hunks)
🔇 Additional comments (7)
cmd/util/repo.go (2)
Line range hint
46-86: LGTM! Well-structured configuration handling.The function properly handles multiple configuration sources (files, flags, env vars, values) and merges them in a logical order. The addition of variadic options enhances flexibility while maintaining backward compatibility.
87-94: Verify error handling in configuration chain.While the implementation looks correct, we should ensure proper error propagation when multiple configuration options fail.
Also applies to: 95-104
✅ Verification successful
Error handling in configuration chain is properly implemented
The configuration chain demonstrates robust error handling with proper error propagation from lower-level operations through the entire chain. All error cases are properly handled and no errors are silently ignored.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling patterns in configuration-related code rg -A 3 'if err [!:=]' | grep -B 1 'return.*err'Length of output: 65617
Script:
#!/bin/bash # Search for SetupConfigType and UnmarshalBacalhauConfig implementations echo "=== SetupConfigType implementation ===" ast-grep --pattern 'func SetupConfigType($$$) { $$$ }' echo -e "\n=== UnmarshalBacalhauConfig implementation ===" ast-grep --pattern 'func UnmarshalBacalhauConfig($$$) { $$$ }' echo -e "\n=== Tests for these functions ===" rg -l "TestSetupConfig|TestUnmarshalBacalhauConfig|TestSetupConfigType" echo -e "\n=== Usage of these functions ===" rg "SetupConfig|SetupConfigType|UnmarshalBacalhauConfig" --type goLength of output: 1718
Script:
#!/bin/bash # Get the full implementation of the configuration functions echo "=== SetupConfigType implementation ===" rg -A 20 "^func SetupConfigType" cmd/util/repo.go echo -e "\n=== UnmarshalBacalhauConfig implementation ===" rg -A 20 "^func UnmarshalBacalhauConfig" cmd/util/repo.go echo -e "\n=== Error handling in config package ===" rg "return.*err" pkg/config/Length of output: 8378
pkg/devstack/devstack.go (1)
71-91: Potential port conflict in predictable port mode.When
PREDICTABLE_API_PORTis set, the code uses incremental ports (4222+i, 6222+i). This could lead to port conflicts in large clusters or when multiple devstacks run simultaneously.✅ Verification successful
Port allocation strategy is safe and well-designed
The incremental port allocation (+i) in predictable mode is an intentional design that prevents conflicts within the same devstack instance. The code also provides a fallback to dynamic port allocation when
PREDICTABLE_API_PORTis not set. The feature is primarily used in controlled environments like benchmarks and testing, where port ranges are known and managed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of port allocation that might conflict rg "PREDICTABLE_API_PORT" -A 2 | grep -E "Port = \d{4}"Length of output: 55
Script:
#!/bin/bash # Search for PREDICTABLE_API_PORT usage and surrounding context rg "PREDICTABLE_API_PORT" -B 2 -A 5 # Search for specific port numbers used in predictable mode rg "(4222|6222)" --type go # Search for port allocation patterns in tests fd ".*_test\.go$" --exec grep -l "Port.*=.*[0-9]" {} \;Length of output: 6333
cmd/cli/devstack/devstack.go (4)
50-59: LGTM! Clear and descriptive field naming.The renamed fields (
ComputeNodes,OrchestratorNodes,HybridNodes,BadComputeNodes) provide better clarity about their purpose.
130-171: LGTM! Well-handled backward compatibility.Good approach to maintaining backward compatibility by keeping old flags functional but hidden, while introducing clearer new flags.
195-198: Verify configuration merge behavior.The new configuration merging logic needs careful verification to ensure proper precedence of settings.
✅ Verification successful
Configuration merge behavior is properly implemented and well-tested ✅
The configuration merge behavior is implemented safely using the
MergeNewmethod with proper error handling. The implementation is thoroughly tested through multiple integration and unit tests that verify various override scenarios.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for configuration merging patterns ast-grep --pattern 'WithBacalhauConfigOverride($config)'Length of output: 58
Script:
#!/bin/bash # Search for devstack options and config overrides echo "=== Searching for devstack options ===" rg -l "devstack.*Options" --type go echo -e "\n=== Searching for config override implementations ===" rg "WithBacalhau.*Config.*Override" --type go -A 5 echo -e "\n=== Looking for related tests ===" rg "TestConfig.*Override|TestDevstack.*Config" --type goLength of output: 5552
105-117: Verify logging configuration precedence.The new test config integration changes the logging setup flow. Ensure this doesn't override user-specified logging settings.
✅ Verification successful
Logging configuration precedence is correctly maintained ✅
The test config integration doesn't override user-specified logging settings. The configuration system properly merges values with the following precedence (from lowest to highest):
- Default values
- Test config defaults
- User config files
- Command-line flags
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for logging configuration patterns rg -A 3 'ParseAndConfigureLogging|ParseAndConfigureLoggingLevel'Length of output: 2389
Script:
#!/bin/bash # Check test config creation and setup ast-grep --pattern 'func NewTestConfig()' -A 20 rg -l "SetupConfig" ast-grep --pattern 'func SetupConfig'Length of output: 2161
Script:
#!/bin/bash # Check test overrides and config setup rg -A 10 "testOverrides :=" ast-grep --pattern 'func SetupConfig($$$)' -A 30Length of output: 1904
Script:
#!/bin/bash # Check config precedence and flag handling rg -A 10 "func SetupConfigType" ast-grep --pattern 'func ($$$ *Config) MergeNew' -A 10Length of output: 697
This PR refactors devstack to support two key features:
Allow compute nodes to join an existing orchestrator:
devstack will run compute-only nodes that connect to the external orchestrator
Use test configuration as base:
This allows for: