Add automatic volume creation for local input source #4961
Conversation
- Change the spec value for local provider from 'localDirectory' to 'local' - Introduce CreateAs param to handle non-existent volumes - Support creation of both files and directories based on source path - Add inference logic to determine appropriate creation strategy - Improve error messaging Currently if a local input source (file or directory) specified in the Job spec does not exist on a compute node, it will not try to bid for the execution. This changes the behavior to allow compute nodes to bid if they can create an empty directory or file at the specified path. When such a bid is accepted, the compute node will create an empty file or directory and mount it as normal. This change also introduces a new parameter to the Job spec input source 'CreateAs', which allows the user to specify how they want the input source to be created. By default the system will try to infer the volume type (file/directory) based on the provided path.
WalkthroughThe changes refactor and enhance the local storage subsystem by consolidating the previous Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant StorageProvider
participant Filesystem
Client->>StorageProvider: PrepareStorage(Source, ReadWrite, CreateAs)
alt Volume exists
StorageProvider->>Filesystem: Check existence
StorageProvider-->>Client: Return volume
else Volume does not exist & ReadWrite true
StorageProvider->>Filesystem: Create volume (dir/file/infer)
Filesystem-->>StorageProvider: Success/Error
StorageProvider-->>Client: Return volume or error
else Volume does not exist & ReadOnly
StorageProvider-->>Client: Return error (cannot create)
end
Assessment against linked issues
Poem
🪧 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: 3
🧹 Nitpick comments (4)
pkg/storage/local/types.go (1)
45-57: Improved error handling and CreateStrategy conversion.The code now properly declares the error variable upfront and returns immediately on decode failure. The additional logic to convert the string representation of
CreateAsto aCreateStrategytype is well-implemented, with appropriate error handling.Consider adding a comment above line 54 to clarify why the conversion from string to CreateStrategy is necessary, such as:
// here c.CreateAs is a string, try to convert it to CreateStrategy c.CreateAs, err = CreateStrategyFromString(c.CreateAs.String())pkg/storage/local/storage.go (1)
178-183: Use.String()to future‑proof hint messageThe hint embeds the
Dir/Fileconstants directly. Relying on the implicit%sstring conversion of a custom type works today but will break if the underlying representation changes (e.g. to an enum). Safer to call.String()explicitly.- WithHint(fmt.Sprintf("If you want the job to create the volume, set the CreateAs property to either '%s' or '%s'", Dir, File)) + WithHint(fmt.Sprintf("If you want the job to create the volume, set the CreateAs property to either '%s' or '%s'", Dir.String(), File.String()))pkg/storage/local/create_strategy.go (1)
65-67: Spelling: “Additionally”Minor typo in comment – improves readability.
- // Additinally, we can look at Target value to see if it gives more insight on whether we should create a file or directory. + // Additionally, we can look at the Target value to see if it gives more insight on whether we should create a file or directory.pkg/storage/local/storage_test.go (1)
520-531: Propagate errors fromCreateStrategyFromStringin helperThe helper ignores the error, which would hide test failures for invalid
createAsvalues. Failing fast makes the suite safer.- createStrategy, _ := CreateStrategyFromString(createAs) + createStrategy, err := CreateStrategyFromString(createAs) + require.NoError(s.T(), err)(Import
requireor bubble the error.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
cmd/util/opts/storage_specconfig.go(1 hunks)pkg/bidstrategy/semantic/export_test.go(1 hunks)pkg/bidstrategy/semantic/input_locality_test.go(1 hunks)pkg/bidstrategy/semantic/storage_installed_test.go(1 hunks)pkg/executor/util/utils.go(2 hunks)pkg/models/constants.go(1 hunks)pkg/storage/local/create_strategy.go(1 hunks)pkg/storage/local/create_strategy_test.go(1 hunks)pkg/storage/local/storage.go(7 hunks)pkg/storage/local/storage_test.go(1 hunks)pkg/storage/local/types.go(4 hunks)pkg/storage/local_directory/storage_test.go(0 hunks)pkg/storage/noop/noop.go(1 hunks)pkg/test/compute/utils_test.go(1 hunks)pkg/test/devstack/jobselection_test.go(1 hunks)pkg/test/scenario/storage.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/storage/local_directory/storage_test.go
🧰 Additional context used
🧬 Code Graph Analysis (8)
pkg/test/compute/utils_test.go (4)
pkg/models/input_source.go (1)
InputSource(13-23)pkg/storage/local/types.go (1)
Source(15-19)pkg/models/spec_config.go (1)
SpecConfig(14-20)pkg/models/constants.go (1)
StorageSourceLocal(61-61)
pkg/bidstrategy/semantic/export_test.go (2)
pkg/storage/local/types.go (2)
Source(15-19)NewSpecConfig(62-72)pkg/models/constants.go (1)
StorageSourceLocal(61-61)
pkg/executor/util/utils.go (3)
pkg/models/constants.go (2)
StorageSourceLocal(61-61)StorageSourceLocalDirectory(60-60)pkg/storage/local/storage.go (2)
NewStorageProvider(26-33)StorageProviderParams(19-21)pkg/storage/local/types.go (1)
ParseAllowPaths(107-113)
pkg/bidstrategy/semantic/storage_installed_test.go (2)
pkg/storage/local/types.go (2)
Source(15-19)NewSpecConfig(62-72)pkg/models/constants.go (1)
StorageSourceLocal(61-61)
pkg/bidstrategy/semantic/input_locality_test.go (2)
pkg/storage/local/types.go (1)
Source(15-19)pkg/models/constants.go (1)
StorageSourceLocal(61-61)
pkg/storage/noop/noop.go (1)
pkg/models/constants.go (1)
StorageSourceLocal(61-61)
pkg/storage/local/types.go (3)
pkg/storage/local/create_strategy.go (2)
CreateStrategy(11-11)CreateStrategyFromString(39-57)pkg/models/spec_config.go (1)
SpecConfig(14-20)pkg/models/constants.go (2)
StorageSourceLocal(61-61)StorageSourceLocalDirectory(60-60)
pkg/storage/local/create_strategy.go (2)
pkg/bacerrors/error.go (1)
Newf(105-107)pkg/bacerrors/codes.go (1)
ValidationError(17-17)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Container Tests / Run
- GitHub Check: Integration Tests / Run
- GitHub Check: Unit Tests / Run
🔇 Additional comments (21)
cmd/util/opts/storage_specconfig.go (1)
15-15: Import path updated as part of package renaming.The import path has been updated to reflect the renaming of the
local_directorypackage tolocal, which aligns with the PR's objective of renaming the storage source type while maintaining backward compatibility.pkg/test/devstack/jobselection_test.go (1)
18-18: Import path updated for consistency with package renaming.The import path has been updated from
local_directorytolocalto match the package renaming, while maintaining the same alias (storage_local). This ensures that all test code using this import continues to function properly.pkg/test/scenario/storage.go (1)
17-17: Import path updated to use the renamed local storage package.The import path has been updated from
local_directorytolocalto align with the package renaming. All function calls tostorage_local.NewSpecConfig()throughout this file will now use the implementation from the renamed package.pkg/bidstrategy/semantic/input_locality_test.go (1)
31-31: Storage source type updated fromStorageSourceLocalDirectorytoStorageSourceLocal.The test now uses the new
StorageSourceLocalconstant instead of the deprecatedStorageSourceLocalDirectory, which aligns with the PR's objective of renaming the storage source type. This change ensures consistent use of the new storage type across the codebase.pkg/bidstrategy/semantic/storage_installed_test.go (1)
25-25: Correct update to use the new storage source type.The change from
StorageSourceLocalDirectorytoStorageSourceLocalis consistent with the PR objectives to rename the local storage provider. This ensures the test uses the new standardized constant for local storage.pkg/storage/noop/noop.go (1)
102-102: Properly updated storage source type in noop implementation.The
Uploadmethod now correctly returns aSpecConfigwith the updatedStorageSourceLocaltype, which aligns with the source type standardization across the codebase.pkg/test/compute/utils_test.go (2)
11-11: Correctly updated import path for local storage package.The import path has been properly updated to point to the new package location with an appropriate alias.
17-19: Updated storage type and parameter struct correctly.The code now uses
models.StorageSourceLocaland the correspondingstorage_local.Sourcestruct, which aligns with the package renaming and maintains functionality.pkg/bidstrategy/semantic/export_test.go (1)
24-24: Properly updated storage source type in test helper.The test helper function now correctly uses
models.StorageSourceLocalinstead of the deprecatedStorageSourceLocalDirectory, ensuring consistency across the codebase.pkg/executor/util/utils.go (3)
19-19: Appropriate use of import alias for the local storage package.The change from
localdirectorytolocal_storageimport alias accurately reflects the package renaming while maintaining a descriptive alias that clearly indicates its purpose.
65-69: Use ofIsNotDisabledwith the new storage source type is correct.The code correctly updates the check to use
models.StorageSourceLocalinstead of the deprecatedmodels.StorageSourceLocalDirectory, aligning with the package renaming strategy.
74-76: Well-implemented backward compatibility approach.Excellent approach to maintain backward compatibility by registering the same storage provider instance under both the new
models.StorageSourceLocaland deprecatedmodels.StorageSourceLocalDirectorynames. This ensures existing code continues to work while facilitating migration to the new name.pkg/models/constants.go (2)
60-61: Good use of deprecation comment and consistent naming.The code properly marks
StorageSourceLocalDirectoryas deprecated with a clear comment indicating to useStorageSourceLocalinstead. The new constant follows the same naming convention and has a cleaner, more concise value.
68-68: Correctly added new source type to StoragesNames slice.Adding
StorageSourceLocalto theStoragesNamesslice ensures it's recognized throughout the system while maintaining backward compatibility by keeping the deprecatedStorageSourceLocalDirectory.pkg/storage/local/create_strategy_test.go (3)
10-15: Comprehensive test for allowed create strategies.The test correctly verifies that all valid strategies are included in the
AllowedCreateStrategiesfunction result, providing good coverage for this fundamental aspect of the new functionality.
17-77: Well-structured test for CreateStrategyFromString.This test thoroughly covers all valid inputs, including empty string defaulting, as well as error handling for invalid inputs. The table-driven test approach provides clear test cases and expectations.
79-128: Comprehensive test for path-based strategy inference.The test covers a wide range of path patterns including absolute/relative paths, with/without trailing slashes, with/without extensions, and empty paths. Each case correctly validates the expected strategy inference logic.
pkg/storage/local/types.go (4)
1-1: Package renamed correctly.The package has been appropriately renamed from
localdirectorytolocalas part of the broader refactoring effort.
18-18: Good addition of CreateStrategy field.The
CreateAsfield of typeCreateStrategyhas been properly added to theSourcestruct, enabling the new functionality for specifying how local storage volumes should be created.
26-34: Well-implemented storage source type compatibility.The code now correctly accepts both
models.StorageSourceLocalandmodels.StorageSourceLocalDirectoryin theDecodeSpecfunction, supporting backward compatibility while encouraging use of the new type. The error message has been improved for clarity usingfmt.Sprintf.
69-69: Updated return type for NewSpecConfig.The function now correctly returns a
SpecConfigwithTypeset tomodels.StorageSourceLocalinstead of the deprecatedmodels.StorageSourceLocalDirectory.
- Some renamings - Spellcheck fixes - Better handling of nocreate strategy
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/storage/local/create_strategy.go (1)
47-65: Parse is strict‑case and whitespace‑sensitive – consider being more forgiving
CreateStrategyFromStringcurrently compares the raw string literal against the four canonical spellings.
A single accidental capital letter ("Dir") or a trailing space ("dir "), both fairly common in YAML/JSON, will trigger a validation error even though the intent is unambiguous.-func CreateStrategyFromString(s string) (CreateStrategy, error) { - switch s { +func CreateStrategyFromString(raw string) (CreateStrategy, error) { + s := strings.TrimSpace(strings.ToLower(raw)) switch s {The change trims leading/trailing whitespace and normalises the word to lower‑case before the switch, improving UX without sacrificing type‑safety.
(Add a quick unit‑test increate_strategy_test.goto lock the behaviour in.)pkg/storage/local/storage.go (2)
31-33: Wrong format verb prints garbled slice
log.Debug().Msgf("Local driver created with allowedPaths: %s", storageHandler.allowedPaths)
uses%s, butstorageHandler.allowedPathsis a slice of structs, not a string.
At runtime this produces something like[%!s(MISSING)], hurting debuggability.- log.Debug().Msgf("Local driver created with allowedPaths: %s", storageHandler.allowedPaths) + log.Debug().Msgf("Local driver created with allowedPaths: %+v", storageHandler.allowedPaths)
68-81: Duplicate “may‑create” logic – extract helper?
HasStorageLocallyandGetVolumeSizenow contain identical checks:source.ReadWrite && source.CreateAs != NoCreateCentralising this in a small
func (s Source) creationAllowed() bool { … }would:
- prevent future drift (e.g. if a new permissive strategy is added),
- clarify intent at the call‑site.
Not critical, but would improve maintainability.
pkg/storage/local/storage_test.go (1)
119-123: Tiny typo in comment
"file doesn't exist and does not allow read-write permission, dirver does not allow read-write permission"→driver.Purely cosmetic – ignore if the suite is about to be re‑worked.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
pkg/storage/local/create_strategy.go(1 hunks)pkg/storage/local/create_strategy_test.go(1 hunks)pkg/storage/local/storage.go(7 hunks)pkg/storage/local/storage_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/storage/local/create_strategy_test.go
🧰 Additional context used
🧠 Learnings (1)
pkg/storage/local/storage.go (2)
Learnt from: Olg-K
PR: bacalhau-project/bacalhau#4961
File: pkg/storage/local/storage.go:0-0
Timestamp: 2025-04-23T06:18:01.350Z
Learning: In the Bacalhau project's storage package, when creating empty files, it's preferable to use `os.OpenFile()` with `O_CREATE|O_EXCL|O_RDONLY` flags rather than `os.Create()` to avoid race conditions and potential data loss in multi-process environments.
Learnt from: Olg-K
PR: bacalhau-project/bacalhau#4961
File: pkg/storage/local/storage.go:0-0
Timestamp: 2025-04-23T06:18:01.350Z
Learning: In the Bacalhau project's storage package, when creating empty files, it's preferable to use `os.OpenFile()` with `O_CREATE|O_EXCL|O_RDONLY` flags rather than `os.Create()` to avoid race conditions and potential data loss in multi-process environments.
🧬 Code Graph Analysis (1)
pkg/storage/local/create_strategy.go (2)
pkg/bacerrors/error.go (1)
Newf(105-107)pkg/bacerrors/codes.go (1)
ValidationError(17-17)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Coverage / Report
- GitHub Check: Container Tests / Run
🔇 Additional comments (3)
pkg/storage/local/create_strategy.go (1)
70-85: Inference logic silently assumes POSIX paths
InferCreateStrategyFromPathrelies on/semantics andfilepath.Splitheuristics.
On Windows a path likeC:\data\is still recognised (back‑slash is handled), butC:\data(no trailing slash) will be treated as a file, which is probably not what the user meant.
The docstring hints at the limitation for dotted directories, but you may also want to:
- mention the Windows caveat in the comment, and/or
- add a short platform‑specific test verifying the current behaviour so that future refactors don’t introduce surprises.
No code change required – this is just a heads‑up.
pkg/storage/local/storage.go (2)
50-56: 👍 Creation‑permission check now guards biddingThe additional
source.CreateAs != NoCreateclause closes the loophole previously flagged where a node would bid on a job even though creation was explicitly disabled.
Looks good.
219-224: File created withO_RDONLY– double‑check on Windows
os.OpenFile(filePath, os.O_CREATE|os.O_EXCL|os.O_RDONLY, …)works fine on Linux, but on WindowsO_RDONLYforbids deleting an open handle, which can trip later clean‑up in tests.
Opening withos.O_WRONLY(we never read the file) avoids that quirk while still preventing accidental truncation.Up to you – just flagging the cross‑platform nuance.
Local Storage Provider Enhancements
localDirectoryinput source renamed tolocal. The old name remains valid and supports all new functionalityCreateAsproperty to Local InputSource Params to control file/directory creation behaviorBackground
Currently, when a local input source (file or directory) specified in a Job spec doesn't exist on a compute node, the node will not bid for execution. This change allows compute nodes to bid if they can create an empty directory or file at the specified path. When such a bid is accepted, the compute node will create the empty file/directory and mount it normally.
CreateAsParameterA new optional
CreateAsproperty can now be used in InputSource Params with the following values:infer(default): The driver will attempt to determine whether to create a file or directory based on the source pathdir: The driver will create an empty directoryfile: The driver will create an empty filenocreate: The driver will not attempt to create anythingBoth files and directories are created with
-rwx------@permissions.Important: To enable automatic creation,
ReadWritemust be set totrue.Example:
Volume Type Inference
The inference is currently based on source path characteristics. The driver determines if a path represents a file or directory based on common conventions - for example, paths with trailing slashes are treated as directories. This is a best-effort approach; for precise control, users should explicitly set the
CreateAsproperty to either"dir"or"file".Addresses #3922
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests