Skip to content

fix: change default artifact temp dir to storage partition to prevent…#246

Open
Jxt-Eli wants to merge 1 commit into
TencentCloud:masterfrom
Jxt-Eli:tempfix
Open

fix: change default artifact temp dir to storage partition to prevent…#246
Jxt-Eli wants to merge 1 commit into
TencentCloud:masterfrom
Jxt-Eli:tempfix

Conversation

@Jxt-Eli
Copy link
Copy Markdown

@Jxt-Eli Jxt-Eli commented May 14, 2026

… disk full errors during extraction
Fixes #240

What this PR does:
This PR addresses the issue where template creation fails due to /tmp running out of space.

Instead of defaulting to os.TempDir() (which would cause issues when disk space isn't enough), artifactWorkRootDir() in template_image.go will now dynamically default to creating a tmp folder inside the configured artifactStoreRootDir unless an environment variable is explicitly set by the user.

Why this is better:

  1. It prevents root partition disk exhaustion.
  2. It improves performance by keeping intermediate files on the same disk as the final storage, avoiding cross-device copy penalties.

(Note: This is my first open-source contribution! Let me know if any changes are needed to the code style or structure.)

Copilot AI review requested due to automatic review settings May 14, 2026 08:52
Copy link
Copy Markdown

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

This PR changes CubeMaster’s rootfs artifact work directory default so template image extraction uses the artifact storage partition instead of /tmp, reducing root partition space pressure during template creation.

Changes:

  • Defaults artifactWorkRootDir() to a tmp/cubemaster-rootfs-artifacts directory under the configured artifact store root.
  • Documents a storage free-space recommendation for template creation in the README.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
README.md Adds a note recommending at least 20GB free on the CubeMaster storage partition.
CubeMaster/pkg/templatecenter/template_image.go Changes the default artifact work directory from OS temp to the artifact store partition.

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

Comment on lines +2008 to +2009
storeRoot := artifactStoreRootDir()
return filepath.Join(storeRoot, "tmp", "cubemaster-rootfs-artifacts")
@fslongjin
Copy link
Copy Markdown
Member

/cubebot review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 14, 2026

Claude finished @fslongjin's task in 2m 14s —— View job


Review of #246

Thanks for the PR, @Jxt-Eli! This is a well-motivated change — moving temp artifacts off the root partition to prevent disk-full failures during template extraction is a solid improvement.

Change Summary

  • template_image.go: artifactWorkRootDir() now defaults to a tmp/cubemaster-rootfs-artifacts directory under the configured artifact store root (/data/CubeMaster/storage) rather than os.TempDir().
  • README.md: Adds a note recommending at least 20GB free on the storage partition.

Code Quality

The implementation is clean and follows the existing patterns well. A few observations:

1. Copilot reviewer's concern — fallback path mismatch

The copilot reviewer correctly identified that artifactWorkRootDir() doesn't participate in the fallback logic that resolveArtifactStoreDir() uses. If /data/CubeMaster/storage is unavailable:

  • resolveArtifactStoreDir() falls back to os.TempDir() + "/cubemaster-rootfs-artifacts-store"works
  • artifactWorkRootDir() still returns /data/CubeMaster/storage/tmp/cubemaster-rootfs-artifactsfails
  • os.MkdirAll(rootfsDir) at line 1368 fails before the fallback store can be used

Suggested fix: Derive the work directory from the resolved store root so that both paths stay on the same filesystem. In buildRootfsArtifact, move resolveArtifactStoreDir before the work directory computation:

func buildRootfsArtifact(ctx context.Context, record *models.RootfsArtifact, ...) {
    storeDir, err := resolveArtifactStoreDir(ctx, record.ArtifactID)
    if err != nil {
        return nil, nil, err
    }
    // Derive work dir from resolved store root so extraction stays
    // on the same filesystem as the final artifact, even on fallback.
    storeRoot := filepath.Dir(storeDir)
    workDir := filepath.Join(storeRoot, "tmp", "cubemaster-rootfs-artifacts", record.ArtifactID)
    rootfsDir := filepath.Join(workDir, "rootfs")
    storeRootfsDir := filepath.Join(storeDir, "rootfs")
    ...
}

This keeps the work directory on the same filesystem as the final store in both primary and fallback scenarios.

2. Minor: README note placement

The README note about storage space is useful, but it might be more discoverable if placed closer to the template creation instructions (around line 196 or 215) rather than in the general storage note section. Consider moving it.

3. Cross-device copy optimization

The PR description correctly notes that keeping intermediate files on the same disk avoids cross-device copies. This works because relocateRootfsToArtifactStore (line 2340) first tries os.Rename() which is atomic on the same filesystem. This is a nice performance win — the existing design already handles this well.

Testing Notes

This is difficult to unit-test locally since it depends on filesystem state. In production/staging environments, verify:

  • Template creation succeeds when /data/CubeMaster/storage is writable (the happy path — should be unchanged)
  • If CUBEMASTER_ROOTFS_ARTIFACT_DIR is set, it still takes precedence (env var logic is unchanged)
  • If possible, simulate the fallback scenario (e.g., by mounting a tmpfs over /data/CubeMaster/storage to fill it) and verify the work directory also falls back

Overall this is a well-structured first contribution. The code is concise, the motivation is clear, and the change is scoped appropriately. Nice work!

@fslongjin
Copy link
Copy Markdown
Member

Hi @Jxt-Eli,

Thanks for the PR! The DCO (Developer Certificate of Origin) check is failing. You need to add a Signed-off-by line to your commit message.

You can fix this by amending your commit:

git commit --amend --signoff
git push --force

This will append a Signed-off-by: Your Name <your-email@example.com> line to the commit message, which is required by the project's DCO policy. See DCO for details.

@fslongjin
Copy link
Copy Markdown
Member

Hi @Jxt-Eli,

Thanks for the PR. I think the current approach works around the issue rather than addressing the root cause. Silently changing the default temp dir to the storage partition changes behavior in a way that may not be expected.

I think a more robust solution would be to add disk space detection — check available space before template creation and fail early with a clear error message if there isn't enough room, rather than letting it fail halfway with a confusing error. This way users know exactly what to fix (e.g., free up space or configure CUBEMASTER_ROOTFS_ARTIFACT_DIR).

Would you be interested in exploring this approach?

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.

[Bug Report] 临时目录空间不足导致模版构建失败

3 participants