Skip to content

fix: AE-2322: network volume size bounds, forbid extra args#290

Open
jhcipar wants to merge 3 commits intomainfrom
jhcipar/AE-2322/network-volume-validation
Open

fix: AE-2322: network volume size bounds, forbid extra args#290
jhcipar wants to merge 3 commits intomainfrom
jhcipar/AE-2322/network-volume-validation

Conversation

@jhcipar
Copy link
Contributor

@jhcipar jhcipar commented Mar 25, 2026

validation for network volume config:

  • name cannot be an empty string or whitespace-only string
  • forbid extra args (should probably be on base resource config, but i think it's fine to just have it on network volume for now)
  • limit size so failures happen at config time

id: Optional[str] = Field(default=None)
name: Optional[str] = None
size: Optional[int] = Field(default=100, gt=0) # Size in GB
size: Optional[int] = Field(default=100, gt=0, le=4096) # Size in GB
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default should probably be lower, like 20GB? 10GB? Also I believe the minimum is 10GB.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it's 10gb, updated. 20gb seems sensible for a default

@promptless
Copy link

promptless bot commented Mar 25, 2026

📝 Documentation updates detected!

New suggestion: Document valid size range for network volumes


Tip: Configure how Promptless handles changelogs in Agent Settings 📋

Copy link
Contributor

@KAJdev KAJdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just fix the assert in the test looks good

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds stricter validation for NetworkVolume configuration so invalid volume definitions fail fast at model-construction/config time.

Changes:

  • Enforce NetworkVolume name is not empty/whitespace-only and forbid unknown fields via Pydantic config.
  • Introduce size bounds (min 10GB, max 4096GB) and change the default size to 20GB.
  • Update/unit-extend tests and manifest expectations to reflect the new validation and defaults.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/unit/test_p2_remaining_gaps.py Updates a gap-test expectation to reflect new name validation behavior and size constraint wording.
tests/unit/test_p2_gaps.py Updates VOL-* tests for new min-size constraint and new default size.
tests/unit/resources/test_network_volume.py Adds focused unit tests for empty/whitespace names, size bounds, and unknown-field rejection.
tests/unit/cli/commands/build_utils/test_manifest.py Updates manifest extraction test to assert the new default volume size.
src/runpod_flash/core/resources/network_volume.py Implements extra="forbid", adds name validator, and applies min/max size constraints plus default size change.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

id: Optional[str] = Field(default=None)
name: Optional[str] = None
size: Optional[int] = Field(default=100, gt=0) # Size in GB
size: Optional[int] = Field(default=20, ge=10, le=4096) # Size in GB
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default volume size changed from 100GB to 20GB (Field(default=20, ...)). This is a behavior change that isn’t mentioned in the PR description; if intentional, it should be called out (and ideally justified) since it affects generated manifests and runtime defaults for users who omit size.

Copilot uses AI. Check for mistakes.
"name",
}

model_config = ConfigDict(extra="forbid")
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting model_config = ConfigDict(extra="forbid") will cause NetworkVolume(**result) in _create_new_volume() to raise a ValidationError if the RunPod REST API returns any additional keys beyond the model fields (which is common for provider APIs). That would break volume deployment even when the request succeeds. Consider filtering the API response down to known fields before constructing a NetworkVolume, or using a separate model/config for provider responses that allows extra fields while keeping user-input validation strict.

Suggested change
model_config = ConfigDict(extra="forbid")
model_config = ConfigDict(extra="ignore")

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants