Re-attempt GrowFilesystem when partition already matches disk size - #464
Closed
neddp wants to merge 1 commit into
Closed
Re-attempt GrowFilesystem when partition already matches disk size#464neddp wants to merge 1 commit into
neddp wants to merge 1 commit into
Conversation
When a disk is resized, ResizeSinglePartition and GrowFilesystem are called in sequence. If GrowFilesystem fails transiently, subsequent deploys detect the partition is already full-size and skip the grow entirely, leaving the filesystem permanently smaller than its partition with no visible error. Fix: always call GrowFilesystem in the else branch after Format. resize2fs and xfs_growfs are idempotent — they exit 0 when the filesystem already fills the partition — so this is safe on every run and surfaces any persistent grow failure on the next deploy.
Contributor
Walkthrough
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a persistent-disk resize edge case in the Linux platform layer where a previously failed filesystem grow could be silently skipped on subsequent deploys when the partition already spans the full disk.
Changes:
- Calls
GrowFilesystemin the “no partition resize needed” branch afterFormat, so incomplete prior grows are retried and failures surface. - Adds unit specs asserting grow is attempted in that branch and that grow failures are returned.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| platform/linux_platform.go | Re-attempt filesystem grow even when partition resize is not required. |
| platform/linux_platform_test.go | Adds specs for grow invocation and error propagation in the non-resize path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1455
to
+1458
| // Grow is idempotent — re-attempt on every run to catch incomplete prior grows. | ||
| if err = p.diskManager.GetFormatter().GrowFilesystem(firstPartitionPath); err != nil { | ||
| return bosherr.WrapError(err, "Failed to grow filesystem") | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform/linux_platform_test.go`:
- Around line 3375-3397: Add an XFS-specific context around
AdjustPersistentDiskPartitioning that configures the filesystem type and
verifies the partition is mounted before GrowFilesystem runs and unmounted
afterward. Assert the xfs_growfs invocation receives the mounted path, while
preserving the existing ext4 coverage and error assertions.
In `@platform/linux_platform.go`:
- Around line 1455-1457: The unpartitioned-disk path must mount the freshly
formatted XFS partition before growing it, then clean up the mount using the
same flow as the resize branch; update the logic around GrowFilesystem in
platform/linux_platform.go:1455-1457 without changing retry behavior. Add
coverage for this XFS mount-before-grow-and-cleanup sequence in
platform/linux_platform_test.go:3375-3397.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 441072b2-d577-4ebd-9eb9-cc4138dbc6a4
📒 Files selected for processing (2)
platform/linux_platform.goplatform/linux_platform_test.go
Comment on lines
+3375
to
+3397
|
|
||
| Context("when the partition already exists and does not need resizing", func() { | ||
| It("attempts to grow the filesystem", func() { | ||
| err := platform.AdjustPersistentDiskPartitioning(diskSettings, mntPoint) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
|
|
||
| Expect(formatter.GrowFilesystemCalled).To(BeTrue()) | ||
| Expect(formatter.GrowFilesystemPartitionPath).To(Equal("fake-real-device-path1")) | ||
| }) | ||
|
|
||
| Context("when growing the filesystem fails", func() { | ||
| BeforeEach(func() { | ||
| formatter.GrowFilesystemError = errors.New("resize2fs refused") | ||
| }) | ||
|
|
||
| It("returns an error", func() { | ||
| err := platform.AdjustPersistentDiskPartitioning(diskSettings, mntPoint) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("Failed to grow filesystem")) | ||
| Expect(err.Error()).To(ContainSubstring("resize2fs refused")) | ||
| }) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add explicit XFS coverage.
These tests exercise the default ext4 path only. They do not detect that Line 1456 invokes xfs_growfs without a mount. Add an XFS case that verifies the partition is mounted before growth and unmounted afterward. xfs_growfs requires a mounted filesystem. (man7.org)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/linux_platform_test.go` around lines 3375 - 3397, Add an
XFS-specific context around AdjustPersistentDiskPartitioning that configures the
filesystem type and verifies the partition is mounted before GrowFilesystem runs
and unmounted afterward. Assert the xfs_growfs invocation receives the mounted
path, while preserving the existing ext4 coverage and error assertions.
Source: MCP tools
Comment on lines
+1455
to
+1457
| // Grow is idempotent — re-attempt on every run to catch incomplete prior grows. | ||
| if err = p.diskManager.GetFormatter().GrowFilesystem(firstPartitionPath); err != nil { | ||
| return bosherr.WrapError(err, "Failed to grow filesystem") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)platform/(linux_platform|linux_platform_test)\.go$' || true
echo "== outline linux_platform.go relevant symbols =="
ast-grep outline platform/linux_platform.go --match adjust --view expanded || true
echo "== locate relevant code =="
rg -n "GrowFilesystem|GrowPersistent|AdjustPersistentDiskPartitioning|xfs|fstype|Mount|Unmount" platform/linux_platform.go platform/linux_platform_test.go || true
echo "== lines around production =="
sed -n '1400,1490p' platform/linux_platform.go
echo "== lines around tests =="
sed -n '3330,3410p' platform/linux_platform_test.goRepository: cloudfoundry/bosh-agent
Length of output: 46649
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate formatter implementations =="
git ls-files | rg 'formatter|disk|linux_formatter|xfs|Filesystem' | head -200
echo "== Search GrowFilesystem definition / xfs handling =="
rg -n "GrowFilesystem|xfs_growfs|FilesystemType|File System|XFS|Xfs" . --glob '!vendor/**' --glob '!third_party/**' | head -300
echo "== Check formatter file sizes =="
while IFS= read -r f; do wc -l "$f"; done < <(git ls-files | rg '.*formatter.*|.*disk.*|.*fs.*|.*xfs.*' | head -100)
echo "== Read likely formatter files =="
for f in $(git ls-files | rg '.*formatter.*|.*disk.*|.*fs.*|.*xfs.*' | head -80); do
if rg -q 'GrowFilesystem|xfs_growfs' "$f"; then
echo "--- $f ---"
sed -n '1,240p' "$f"
fi
doneRepository: cloudfoundry/bosh-agent
Length of output: 10596
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== linux_formatter.go outline =="
ast-grep outline platform/disk/linux_formatter.go --view expanded || true
echo "== linux_formatter.go relevant lines =="
sed -n '1,120p' platform/disk/linux_formatter.go
echo "== linux_formatter_test.go GrowFilesystem context =="
sed -n '227,310p' platform/disk/linux_formatter_test.go
echo "== platform/linux_platform.go resize branch and partition block context =="
sed -n '1375,1470p' platform/linux_platform.go
echo "== disk/types =="
sed -n '1,80p' platform/disk/formatter_interface.go
sed -n '360,385p' platform/disk/partition_strategy.go 2>/dev/null || trueRepository: cloudfoundry/bosh-agent
Length of output: 10627
Mount the fresh XFS partition before calling GrowFilesystem.
The unpartitioned disk branch creates, formats, and calls GrowFilesystem(firstPartitionPath) directly. For XFS, this runs xfs_growfs /dev/... instead of xfs_growfs -d /dev/..., bypassing the supported unmounted-root-device-only growth path. Use the same mount-before-grow-and-cleanup flow as the resize branch, and add coverage for the XFS path in platform/linux_platform_test.go.
📍 Affects 2 files
platform/linux_platform.go#L1455-L1457(this comment)platform/linux_platform_test.go#L3375-L3397
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/linux_platform.go` around lines 1455 - 1457, The unpartitioned-disk
path must mount the freshly formatted XFS partition before growing it, then
clean up the mount using the same flow as the resize branch; update the logic
around GrowFilesystem in platform/linux_platform.go:1455-1457 without changing
retry behavior. Add coverage for this XFS mount-before-grow-and-cleanup sequence
in platform/linux_platform_test.go:3375-3397.
Source: MCP tools
neddp
marked this pull request as draft
August 6, 2026 06:22
Member
Author
|
Not a good implementation. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this change about?
When
AdjustPersistentDiskPartitioningis called after a disk resize, it either resizes the partition and grows the filesystem (if the partition is smaller than the disk), or falls into anelsebranch that only partitions and formats (if the partition already spans the disk). Theelsebranch never calledGrowFilesystem.This means: if a previous deploy resized the partition but
GrowFilesystemfailed, every subsequent deploy would see the partition already at full size, skip the grow silently, and succeed - leaving the filesystem permanently smaller than its partition with no visible error.Fix: call
GrowFilesystemunconditionally in theelsebranch afterFormat.resize2fsandxfs_growfsare idempotent and exit 0 when the filesystem already fills the partition, so this is safe on every deploy. If the grow fails, the error is returned and the deployment fails, forcing the operator to resolve it.Please provide contextual information.
Discovered during an investigation into a
resize2fs: Permission deniedfailure. Three nodes had pre-existing filesystem corruption from a storage write-error event. The partition resize succeeded butGrowFilesystemfailed. Subsequent deploys silently succeeded, leaving those nodes with 98G filesystems on 1T volumes.What tests have you run against this PR?
platformunit test suite: 373 passed, 0 failedHow should this change be described in bosh-agent release notes?
When a persistent disk's partition already matches the disk size but the filesystem is smaller (due to a previously failed grow), the agent now re-attempts the filesystem grow on each deploy instead of silently skipping it.
Does this PR introduce a breaking change?
No.
resize2fs/xfs_growfsare idempotent on already-full filesystems. The only behaviour change is that a previously silent failure now surfaces as a deploy error.