refactor(quota): move quota scripts to its own file - #322
Conversation
The quota setup and cleanup scripts are embedded in code and this makes it less testable and readable. This change moves it to its own file and embeds the script into the code at compile time. Signed-off-by: Niladri Halder <niladri.halder26@gmail.com>
There was a problem hiding this comment.
Pull request overview
Refactors the quota helper logic by moving the embedded shell quota apply/cleanup scripts into a standalone quota.sh file and embedding it at compile time, while switching callers to invoke it via generated argv slices. This improves readability and centralizes quota script logic for both helper-pod and node-deployment execution paths.
Changes:
- Added
cmd/provisioner-localpv/app/quota.shand embedded it via//go:embed. - Replaced generated-script-string functions with
QuotaScriptConfig.ApplyArgs()/CleanupArgs()argv builders. - Updated node-deployment and helper-pod call sites to execute the embedded script via argv instead of
sh -c <generated string>.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/provisioner-localpv/app/quota.sh | Introduces a unified quota helper script supporting apply and cleanup modes with flag parsing. |
| cmd/provisioner-localpv/app/quota_scripts.go | Embeds quota.sh and adds argv builders (ApplyArgs/CleanupArgs) plus shared path resolution. |
| cmd/provisioner-localpv/app/local_volume_manager.go | Switches node-deployment quota apply/cleanup execution to use argv from QuotaScriptConfig. |
| cmd/provisioner-localpv/app/helper_hostpath.go | Switches helper-pod quota apply/cleanup execution to use argv from QuotaScriptConfig. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (cfg QuotaScriptConfig) resolvePaths() (parentPath, volumePath string) { | ||
| parentPath = cfg.ParentDir | ||
| volumePath = filepath.Join(cfg.ParentDir, cfg.VolumeDir) | ||
| if cfg.HostPathPrefix != "" { | ||
| parentPath = filepath.Join(cfg.HostPathPrefix, cfg.ParentDir) | ||
| volumePath = filepath.Join(cfg.HostPathPrefix, cfg.ParentDir, cfg.VolumeDir) | ||
| parentPath = filepath.Join(cfg.HostPathPrefix, parentPath) | ||
| volumePath = filepath.Join(cfg.HostPathPrefix, volumePath) | ||
| } |
| // ApplyArgs returns the argv to pass to exec for applying a quota. The | ||
| // returned slice begins with "sh" and includes the embedded script body, | ||
| // so the caller invokes it as e.g. `cmd.Args = cfg.ApplyArgs()`. | ||
| func (cfg QuotaScriptConfig) ApplyArgs() []string { | ||
| parentPath, volumePath := cfg.resolvePaths() | ||
| return []string{ | ||
| "sh", "-c", quotaScript, "quota.sh", "apply", | ||
| "--parent", parentPath, | ||
| "--volume", volumePath, | ||
| "--soft-kb", strings.TrimSuffix(cfg.SoftLimitGrace, "k"), | ||
| "--hard-kb", strings.TrimSuffix(cfg.HardLimitGrace, "k"), | ||
| } | ||
| } | ||
|
|
||
| return fmt.Sprintf(`set -e | ||
|
|
||
| # Path to parent directory (mount point for quota commands) | ||
| PARENT_PATH="%s" | ||
| # Path to volume directory | ||
| VOLUME_PATH="%s" | ||
|
|
||
| # Get filesystem type | ||
| FS=$(stat -f -c %%T "$VOLUME_PATH" 2>/dev/null || echo "unknown") | ||
|
|
||
| if [[ "$FS" == "xfs" ]]; then | ||
| ID=$(xfs_io -c stat "$VOLUME_PATH" 2>/dev/null | awk '/projid/{print $3}' | head -1) | ||
| echo "projid=$ID" | ||
| if [ -n "$ID" ] && [ "$ID" != "0" ]; then | ||
| # Remove projid binding | ||
| xfs_io -c "chproj -R 0" "$VOLUME_PATH" 2>/dev/null || true | ||
| # Remove quota limit | ||
| xfs_quota -x -c "limit -p bsoft=0 bhard=0 $ID" "$PARENT_PATH" 2>/dev/null || true | ||
| fi | ||
| elif [[ "$FS" == "ext2/ext3" ]]; then | ||
| ID=$(lsattr -pd "$VOLUME_PATH"/ 2>/dev/null | awk '{print $1}') | ||
| if [ -n "$ID" ] && [ "$ID" != "0" ]; then | ||
| setquota -P $ID 0 0 0 0 "$PARENT_PATH" 2>/dev/null || true | ||
| fi | ||
| fi | ||
|
|
||
| rm -rf "$VOLUME_PATH"`, | ||
| parentPath, volumePath, | ||
| ) | ||
| // CleanupArgs returns the argv to pass to exec for cleaning up a quota and | ||
| // removing the volume directory. | ||
| func (cfg QuotaScriptConfig) CleanupArgs() []string { | ||
| parentPath, volumePath := cfg.resolvePaths() | ||
| return []string{ | ||
| "sh", "-c", quotaScript, "quota.sh", "cleanup", | ||
| "--parent", parentPath, | ||
| "--volume", volumePath, | ||
| } |
There was a problem hiding this comment.
Just a thought, could we do this in go directly?
There was a problem hiding this comment.
For nodeDeployment mode -- sure.
The helperPod mode runs the openebs/linux-utils container on the hosts where the volume is created/deleted. We'd have to deploy the provisioner-localpv container to run in those cases instead of linux-utils. The proivisoner-localpv container is ~47 MiB, the linux-utils container is ~24MiB.
Or, would it be better if we create a new lighter binary to do this instead?
There was a problem hiding this comment.
We could have a lighter one, but even the localpv one is probably fine?
There was a problem hiding this comment.
I'm also fine with having the provisioner-localpv do it. I'll make the change.
Signed-off-by: Niladri Halder <niladri.halder26@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #322 +/- ##
========================================
Coverage 54.00% 54.00%
========================================
Files 1 1
Lines 474 474
========================================
Hits 256 256
Misses 209 209
Partials 9 9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The quota setup and cleanup scripts are embedded in code and this makes it less testable and readable. This change moves it to its own file and embeds the script into the code at compile time.
Pull Request template
Why is this PR required? What issue does it fix?:
The quota setup and cleanup scripts are embedded in code and this makes it less testable and readable. This change moves it to its own file and embeds the script into the code at compile time.
What this PR does?:
Moves the embedded shell scripts to its own file. The two apply and cleanup functions are triggered via different arguments.
Does this PR require any upgrade changes?:
It's a refactor, shouldn't required upgrade changes.
If the changes in this PR are manually verified, list down the scenarios covered::
I'll update after I do more manual testing. Opening the PR for review in the mean time.
Any additional information for your reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs
Checklist:
<type>(<scope>): <subject>