Add disk quota support#171
Open
parkervcp wants to merge 19 commits into
Open
Conversation
This uses the built in quota management services to manage quotas for EXT4. May potentially work on XFS but has not been tested. See the docs for more information. https://pelican.dev/docs/guides/disk-quotas/about
builds were failing due to un-used err
This uses the built in quota management services to manage quotas for EXT4. May potentially work on XFS but has not been tested. See the docs for more information. https://pelican.dev/docs/guides/disk-quotas/about
builds were failing due to un-used err
This uses the built in quota management services to manage quotas for EXT4. May potentially work on XFS but has not been tested. See the docs for more information. https://pelican.dev/docs/guides/disk-quotas/about
builds were failing due to un-used err
update internal name for the panel id to Pid to avoid overlap with ID as the uuid moves quota checking earlier to avoid an out of order problem add logging to quota management update detection of supported filesystems update how directory xattr is handled
use the updated and recommended "golang.org/x/sys/unix" package per https://go.googlesource.com/proposal/+/refs/heads/master/design/freeze-syscall.md
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an EXT4/XFS quota subsystem with CLI startup validation, module updates, quota APIs (Add/Set/Get/Del), EXFS project management with atomic manifest writes, and xattr/ioctl helpers; also migrates syscall usages to golang.org/x/sys/unix. ChangesQuota subsystem and integration
Sequence Diagram(s)sequenceDiagram
participant Manager as Server Manager
participant Server as Server Instance
participant Quotas as quotas package
participant FS as Filesystem (EXT4/XFS)
Manager->>Server: InitServer()
Server->>Quotas: AddQuota(pid, uuid)
Quotas->>FS: Statfs -> detect type (EXT4/XFS/BTRFS/ZFS)
FS-->>Quotas: supported / unsupported
Quotas-->>Server: success / error
Server->>Quotas: SetQuota(limit, uuid) (on Sync)
Quotas->>FS: write projid/projects, set xattr, apply quota
FS-->>Quotas: applied / error
Router->>Quotas: DelQuota(uuid) (on deletion, async)
Quotas-->>Router: success / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
server/filesystem/stat_linux.go (1)
18-21: Remove duplicatedStat_tbranch inCTime().The second type-assertion block repeats the first one exactly and is unreachable as a distinct fallback.
♻️ Proposed cleanup
func (s *Stat) CTime() time.Time { if st, ok := s.Sys().(*unix.Stat_t); ok { // Do not remove these "redundant" type-casts, they are required for 32-bit builds to work. return time.Unix(int64(st.Ctim.Sec), int64(st.Ctim.Nsec)) } - if st, ok := s.Sys().(*unix.Stat_t); ok { - // Do not remove these "redundant" type-casts, they are required for 32-bit builds to work. - return time.Unix(int64(st.Ctim.Sec), int64(st.Ctim.Nsec)) - } return time.Time{} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/filesystem/stat_linux.go` around lines 18 - 21, In CTime() remove the redundant second type-assertion branch that repeats the first (the block checking s.Sys().(*unix.Stat_t) and returning time.Unix(int64(st.Ctim.Sec), int64(st.Ctim.Nsec))); keep the original single branch (including the existing 32-bit build comment) and any fallback behavior after it, so only one Stat_t type-assertion and return remains in the function.router/router_server.go (1)
287-290: Use a goroutine-local error variable for quota cleanup.Avoid assigning to outer
errfrom inside the goroutine.♻️ Proposed refactor
if config.Get().System.Quotas.Enabled { - if err = quotas.DelQuota(s.Config().Uuid); err != nil { - log.WithFields(log.Fields{"server_id": s.Config().Pid, "error": err}). + if qerr := quotas.DelQuota(s.Config().Uuid); qerr != nil { + log.WithFields(log.Fields{"server_id": s.Config().Pid, "error": qerr}). Warn("failed to remove quota during deletion process") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/router_server.go` around lines 287 - 290, The goroutine is assigning to the outer variable err when calling quotas.DelQuota(s.Config().Uuid); change that to use a goroutine-local variable (e.g., qerr := quotas.DelQuota(s.Config().Uuid)) and log or handle qerr inside the goroutine (log.WithFields(..., "error": qerr).Warn(...)); if the caller needs the result, send qerr back via a channel or sync.WaitGroup rather than reassigning the outer err to avoid race/variable-shadowing on err.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/filesystem/quotas/exfs.go`:
- Around line 15-16: exfsProjects (type exfsProject) is shared mutable state and
must be synchronized: add a package-level sync.RWMutex (e.g. exfsMu) and use
exfsMu.RLock/RUnlock around readers and exfsMu.Lock/Unlock around writers that
mutate exfsProjects; for file writes (the code paths that write /etc/projid and
/etc/projects) take the lock, make a shallow copy/snapshot of exfsProjects while
holding the lock, then release the lock and perform the actual disk I/O using
the snapshot to avoid holding the mutex during blocking file operations; update
all places that read or modify exfsProjects to use the new mutex.
- Around line 153-167: The writeTemplate function currently returns early on
t.Execute failure and leaks the open file f; modify writeTemplate (which accepts
*template.Template, file string, data interface{}) to always close f by
deferring a close right after successful os.Create, and ensure any Close error
is propagated or merged with the existing err (e.g., if Execute fails, preserve
that error but still attempt to Close and return the first non-nil error). Use a
defer that captures and assigns the Close error into the named return variable
so f is closed on all paths.
In `@server/filesystem/quotas/functions.go`:
- Around line 35-79: In IsSupportedFS/getFSType logging, fix the typos in
filesystem labels: change the FSBTRFS case's log field value from "brtfs" to
"btrfs" and update the FSZFS branch error message (currently "btrfs is not
supported") to correctly state "zfs is not supported"; ensure the log.WithField
keys remain "fs-type" and "path" and update only the string literals in the
FSBTRFS case and the FSZFS error branch to keep messages consistent with
FSEXT4/FSXFS branches.
- Around line 107-115: SetQuota currently casts the int64 limit to uint64 which
will wrap negative values into huge quotas; before calling getProject or
fsProject.setQuota, validate that the incoming limit is non-negative and return
a sensible error (e.g., invalid argument) if limit < 0. Update SetQuota to check
the limit, reject negatives, and only call fsProject.setQuota(uint64(limit))
when the check passes; reference SetQuota, getProject and fsProject.setQuota to
locate where to add the guard.
In `@server/filesystem/quotas/syscall_xattr.go`:
- Around line 49-55: The fsXAttr struct is too small compared to the kernel ABI
(28 bytes) and is missing the CowExtSize field; update the struct definition for
fsXAttr by adding a CowExtSize uint32 field and expanding FSXPad to provide the
remaining padding so the total struct size is 28 bytes (match kernel ABI used by
FS_IOC_FSGETXATTR and FS_IOC_FSSETXATTR), ensuring the field order remains
XFlags, ExtSize, CowExtSize, NextENTs, ProjectID, and then padding to reach 28
bytes.
In `@server/power.go`:
- Around line 190-203: The quota branch wrongly treats unlimited-disk servers as
over quota because it compares used >= s.DiskSpace() when DiskSpace() <= 0;
modify the quota check in the block guarded by
config.Get().System.Quotas.Enabled so that you first handle the unlimited-disk
case (if s.DiskSpace() <= 0) by skipping the quota check and
returning/continuing normally, or after getting used from
quotas.GetQuota(s.Config().Uuid) explicitly ignore the comparison when
s.DiskSpace() <= 0; ensure you keep calls like s.PublishConsoleOutputFromDaemon
and the error logging around quotas.GetQuota unchanged except for this
additional guard so unlimited servers are not blocked from starting.
In `@server/resources.go`:
- Around line 38-44: The code currently calls quotas.GetQuota and
unconditionally calls atomic.StoreInt64(&s.resources.Disk, used), which
overwrites the disk value with 0 on errors; change the flow in the block guarded
by config.Get().System.Quotas.Enabled so that you only update s.resources.Disk
(via atomic.StoreInt64) when quotas.GetQuota returns nil error (i.e., on
success), and on error leave the existing s.resources.Disk value unchanged (or
optionally use a documented fallback) while still logging the error from
quotas.GetQuota with s.ID() for diagnostics.
---
Nitpick comments:
In `@router/router_server.go`:
- Around line 287-290: The goroutine is assigning to the outer variable err when
calling quotas.DelQuota(s.Config().Uuid); change that to use a goroutine-local
variable (e.g., qerr := quotas.DelQuota(s.Config().Uuid)) and log or handle qerr
inside the goroutine (log.WithFields(..., "error": qerr).Warn(...)); if the
caller needs the result, send qerr back via a channel or sync.WaitGroup rather
than reassigning the outer err to avoid race/variable-shadowing on err.
In `@server/filesystem/stat_linux.go`:
- Around line 18-21: In CTime() remove the redundant second type-assertion
branch that repeats the first (the block checking s.Sys().(*unix.Stat_t) and
returning time.Unix(int64(st.Ctim.Sec), int64(st.Ctim.Nsec))); keep the original
single branch (including the existing 32-bit build comment) and any fallback
behavior after it, so only one Stat_t type-assertion and return remains in the
function.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
cmd/root.goconfig/config.gogo.modinternal/ufs/error.gorouter/router_server.gorouter/router_server_files.gorouter/router_transfer.goserver/activity.goserver/configuration.goserver/crash.goserver/filesystem/quotas/exfs.goserver/filesystem/quotas/functions.goserver/filesystem/quotas/syscall_xattr.goserver/filesystem/stat_linux.goserver/manager.goserver/power.goserver/resources.goserver/server.goserver/transfer/archive.gosystem/system.go
💤 Files with no reviewable changes (1)
- server/activity.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.1, linux, amd64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.24.7, linux, amd64)
🧰 Additional context used
🧬 Code graph analysis (9)
server/filesystem/quotas/functions.go (1)
config/config.go (1)
Get(445-453)
server/server.go (3)
config/config.go (1)
Get(445-453)system/system.go (1)
System(61-68)server/filesystem/quotas/functions.go (1)
SetQuota(107-117)
router/router_server.go (2)
config/config.go (1)
Get(445-453)server/filesystem/quotas/functions.go (1)
DelQuota(95-104)
cmd/root.go (3)
config/config.go (1)
Get(445-453)system/system.go (1)
System(61-68)server/filesystem/quotas/functions.go (1)
IsSupportedFS(55-82)
server/power.go (2)
config/config.go (1)
Get(445-453)server/filesystem/quotas/functions.go (1)
GetQuota(120-129)
server/manager.go (2)
config/config.go (1)
Get(445-453)server/filesystem/quotas/functions.go (1)
AddQuota(85-92)
server/filesystem/quotas/syscall_xattr.go (1)
internal/ufs/error.go (1)
NewSyscallError(54-56)
server/resources.go (5)
config/config.go (1)
Get(445-453)system/system.go (1)
System(61-68)server/filesystem/quotas/functions.go (1)
GetQuota(120-129)server/filesystem/errors.go (1)
Error(25-37)server/filesystem/filesystem.go (1)
Filesystem(24-34)
server/filesystem/quotas/exfs.go (1)
server/filesystem/quotas/syscall_xattr.go (1)
FS_XFLAG_PROJINHERIT(24-24)
🔇 Additional comments (12)
server/crash.go (1)
102-102: No action needed — formatting-only change.This hunk does not introduce behavioral changes.
router/router_server_files.go (1)
243-243: No action needed — formatting-only change.No semantic delta in this hunk.
router/router_transfer.go (1)
149-151: No action needed — formatting/alignment-only changes.No functional behavior changed in these segments.
Also applies to: 210-210, 219-219, 222-222, 230-230, 238-238, 243-243, 245-245
server/transfer/archive.go (1)
58-77: Backup selection flow looks good.The explicit set-based filtering and early return when no matching backup files exist improves clarity and avoids unnecessary work.
server/configuration.go (1)
49-52: PID wiring is clean and thread-safe.Adding the
Pidfield and exposing it via a locked accessor is consistent with the configuration access pattern in this file.Also applies to: 115-119
config/config.go (1)
217-221: Quotas config block is well-scoped.The new
quotas.enabledsetting is explicit and defaults to disabled, which is a safe rollout posture.system/system.go (1)
188-190: Statfs migration preserves behavior.The updated calls keep the existing disk-to-mountpoint matching logic intact.
Also applies to: 194-195
server/server.go (1)
273-279: Quota-aware disk limit switching inSync()looks good.The enabled/disabled branch keeps behavior explicit and preserves the non-quota fallback.
cmd/root.go (1)
171-180: Good startup preflight for quota capability.Failing fast before manager initialization is the right behavior here.
server/resources.go (1)
44-47: Good fallback path for non-quota environments.Preserving the cached usage path when quotas are disabled keeps previous runtime behavior stable.
server/manager.go (2)
204-209: Quota provisioning placement looks good.Conditionally calling
quotas.AddQuotaduring init and failing fast on error is a solid integration point.
258-277: Closure capture is safe — Go 1.24 handles loop variable semantics correctly.The codebase targets Go 1.24.0, which uses the Go 1.22+ range semantics that re-creates loop variables on each iteration. The
datavariable captured in the worker closure at line 259 is safe without explicit rebinding.
Comment on lines
+38
to
+44
| if config.Get().System.Quotas.Enabled { | ||
| used, err := quotas.GetQuota(s.ID()) | ||
| if err != nil { | ||
| log.WithFields(log.Fields{"server-uuid": s.ID(), "error": err.Error()}).Error("there was an issue getting the used disk space") | ||
| } | ||
| atomic.StoreInt64(&s.resources.Disk, used) | ||
| } else { |
There was a problem hiding this comment.
Don’t overwrite Disk with zero when quota lookup fails.
On quota read failure, used remains 0 and is still stored, which can falsely report empty disk usage. Keep the previous value or use a fallback only on success.
Suggested fix
if config.Get().System.Quotas.Enabled {
used, err := quotas.GetQuota(s.ID())
if err != nil {
log.WithFields(log.Fields{"server-uuid": s.ID(), "error": err.Error()}).Error("there was an issue getting the used disk space")
- }
- atomic.StoreInt64(&s.resources.Disk, used)
+ } else {
+ atomic.StoreInt64(&s.resources.Disk, used)
+ }
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if config.Get().System.Quotas.Enabled { | |
| used, err := quotas.GetQuota(s.ID()) | |
| if err != nil { | |
| log.WithFields(log.Fields{"server-uuid": s.ID(), "error": err.Error()}).Error("there was an issue getting the used disk space") | |
| } | |
| atomic.StoreInt64(&s.resources.Disk, used) | |
| } else { | |
| if config.Get().System.Quotas.Enabled { | |
| used, err := quotas.GetQuota(s.ID()) | |
| if err != nil { | |
| log.WithFields(log.Fields{"server-uuid": s.ID(), "error": err.Error()}).Error("there was an issue getting the used disk space") | |
| } else { | |
| atomic.StoreInt64(&s.resources.Disk, used) | |
| } | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/resources.go` around lines 38 - 44, The code currently calls
quotas.GetQuota and unconditionally calls atomic.StoreInt64(&s.resources.Disk,
used), which overwrites the disk value with 0 on errors; change the flow in the
block guarded by config.Get().System.Quotas.Enabled so that you only update
s.resources.Disk (via atomic.StoreInt64) when quotas.GetQuota returns nil error
(i.e., on success), and on error leave the existing s.resources.Disk value
unchanged (or optionally use a documented fallback) while still logging the
error from quotas.GetQuota with s.ID() for diagnostics.
adds coxextsize and limits fsxpad to 8bytes corrects some errors on filesystem type detection handles when limits are lower than 0. Treats them as unlimited. now locks exfs projects when working with them to avoid any issues
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
server/filesystem/quotas/exfs.go (1)
170-177:⚠️ Potential issue | 🟠 MajorPropagate
Closeerrors inwriteTemplate.Line 170 closes the file via defer but ignores close failures, which can hide partial write/flush errors for
/etc/projidand/etc/projects.Suggested patch
func writeTemplate(t *template.Template, file string, data interface{}) (err error) { f, err := os.Create(file) if err != nil { return err } - defer f.Close() + defer func() { + if cerr := f.Close(); err == nil && cerr != nil { + err = cerr + } + }() err = t.Execute(f, data) if err != nil { return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/filesystem/quotas/exfs.go` around lines 170 - 177, writeTemplate currently defers f.Close() and ignores its error which can hide write/flush failures for /etc/projid and /etc/projects; modify writeTemplate so the deferred close captures its error and is propagated: capture the error returned by t.Execute into a local variable (err), then in a deferred closure call cerr := f.Close() and if err == nil && cerr != nil set/return cerr, else if err != nil and cerr != nil wrap both (e.g., using fmt.Errorf("%v; close error: %w", err, cerr)) so the function returns the most relevant error; reference the writeTemplate function, the file variable f, and the t.Execute call when making the change.
🧹 Nitpick comments (1)
server/filesystem/quotas/syscall_xattr.go (1)
56-65: Add a compile-time ABI size check forfsXAttr.This struct is ioctl ABI-sensitive; a compile-time size assertion prevents accidental regressions.
💡 Proposed hardening
type fsXAttr struct { XFlags uint32 ExtSize uint32 NextENTs uint32 ProjectID uint32 CowExtSize uint32 FSXPad [8]byte } + +const fsXAttrSize = 28 + +var ( + _ [fsXAttrSize - int(unsafe.Sizeof(fsXAttr{}))]struct{} + _ [int(unsafe.Sizeof(fsXAttr{})) - fsXAttrSize]struct{} +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/filesystem/quotas/syscall_xattr.go` around lines 56 - 65, Add a compile-time ABI size check to ensure fsXAttr remains the expected size (28 bytes): add a build-time assertion using unsafe.Sizeof(fsXAttr{}) compared against 28 (for example via a const/var zero-length array or a type alias trick) so the compiler will fail if fsXAttr's size changes; reference the fsXAttr type in the assertion to locate the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 3: The go.mod declares "go 1.25.0" but CI and Docker files are pinned to
1.24.7 (and push matrix contains outdated/wrong entries); update the toolchain
refs to match go.mod by changing the release workflow's go-version to 1.25.0 (or
1.25.1), update the Dockerfile FROM to golang:1.25.0-alpine (or 1.25.1-alpine),
and fix the push workflow matrix to ["1.25.0","1.25.1"] (or a single consistent
1.25.x value) so that go.mod's "go 1.25.0" aligns with CI and image builds.
In `@server/filesystem/quotas/exfs.go`:
- Around line 114-124: The deletion routine takes exfs.lock, mutates
exfs.projects, then calls writeEXFSProjects() while still holding exfs.lock,
which deadlocks because writeEXFSProjects also tries to lock the same mutex; fix
by releasing the lock before calling writeEXFSProjects (unlock exfs.lock right
after the append/remove and before calling writeEXFSProjects), or alternatively
add an internal unexported helper (e.g., writeEXFSProjectsLocked) that assumes
the lock is already held and have writeEXFSProjects call that under its own
lock—use exfs.lock, exfs.projects, writeEXFSProjects and ensure no path attempts
to re-acquire the same mutex.
- Around line 98-103: addProject currently appends q to exfs.projects while
holding exfs.lock, then releases the lock and calls writeEXFSProjects; if
writeEXFSProjects fails the in-memory state remains inconsistent with /etc. Fix
by performing the persistence first or by rolling back the in-memory change on
write failure: inside addProject (the function that manipulates exfs.projects),
keep the lock while appending, call writeEXFSProjects while still holding or
immediately after appending but detect an error and if writeEXFSProjects returns
an error remove the appended entry from exfs.projects (or restore the previous
slice) before returning, ensuring exfs.lock protects both mutation and rollback
so memory and on-disk state stay consistent.
In `@server/filesystem/quotas/functions.go`:
- Around line 109-112: The code currently resets limit to 0 before logging so
requested_limit is always 0; preserve the original negative value by capturing
it into a temporary variable (e.g., originalLimit) or log the existing limit
before assigning limit = 0, then call
log.WithFields(log.Fields{"requested_limit": originalLimit}).Error(...) so the
log shows the actual negative input; update the block around the limit check
(the variable limit and the log.WithFields call) to use the preserved value.
- Around line 65-68: The call to
fsquota.ProjectQuotasSupported(config.Get().System.Data) returns an error that
is immediately returned with no logging; update the error path so it logs the
error with context before returning. Specifically, in the block handling the
result of fsquota.ProjectQuotasSupported (the supported, err = ...; if err !=
nil { ... } section) log a descriptive message including the data path
(config.Get().System.Data) and the returned err (for example via the file's
existing logger/processLogger) and then return the error as before.
In `@server/filesystem/quotas/syscall_xattr.go`:
- Around line 89-97: The setXAttr function is casting projectID (int) to uint32
without validation which can wrap negative or out-of-range values; update
setXAttr to validate projectID before assigning to fxattr.ProjectID (e.g., check
projectID >= 0 and <= math.MaxUint32) and return a descriptive error when
invalid so fxattr.ProjectID is only set with a safe uint32 value; reference
symbols: function setXAttr, parameter projectID, and field fxattr.ProjectID.
---
Duplicate comments:
In `@server/filesystem/quotas/exfs.go`:
- Around line 170-177: writeTemplate currently defers f.Close() and ignores its
error which can hide write/flush failures for /etc/projid and /etc/projects;
modify writeTemplate so the deferred close captures its error and is propagated:
capture the error returned by t.Execute into a local variable (err), then in a
deferred closure call cerr := f.Close() and if err == nil && cerr != nil
set/return cerr, else if err != nil and cerr != nil wrap both (e.g., using
fmt.Errorf("%v; close error: %w", err, cerr)) so the function returns the most
relevant error; reference the writeTemplate function, the file variable f, and
the t.Execute call when making the change.
---
Nitpick comments:
In `@server/filesystem/quotas/syscall_xattr.go`:
- Around line 56-65: Add a compile-time ABI size check to ensure fsXAttr remains
the expected size (28 bytes): add a build-time assertion using
unsafe.Sizeof(fsXAttr{}) compared against 28 (for example via a const/var
zero-length array or a type alias trick) so the compiler will fail if fsXAttr's
size changes; reference the fsXAttr type in the assertion to locate the code.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.modserver/filesystem/quotas/exfs.goserver/filesystem/quotas/functions.goserver/filesystem/quotas/syscall_xattr.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Test (ubuntu-22.04, 1.24.7, linux, amd64)
- GitHub Check: Analyze (go)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: parkervcp
Repo: pelican-dev/wings PR: 171
File: server/power.go:190-203
Timestamp: 2026-03-02T13:53:08.995Z
Learning: In `server/power.go`, when quotas are enabled via `config.Get().System.Quotas.Enabled`, the disk space check using `used >= s.DiskSpace()` does not need a special guard for unlimited-disk servers (DiskSpace() <= 0) because the disk quota is managed by the filesystem itself, which handles such cases appropriately.
📚 Learning: 2026-03-02T13:53:08.995Z
Learnt from: parkervcp
Repo: pelican-dev/wings PR: 171
File: server/power.go:190-203
Timestamp: 2026-03-02T13:53:08.995Z
Learning: In the server package, when quotas are enabled via config.Get().System.Quotas.Enabled, the disk space check using used >= s.DiskSpace() does not require a special guard for unlimited-disk scenarios (DiskSpace() <= 0). The filesystem handles such cases, so the existing check is sufficient. Apply this pattern to similar quota-related disk checks in the server package and ensure tests/docs reflect that unlimited-disk behavior is governed by the filesystem, not by an extra guard in code.
Applied to files:
server/filesystem/quotas/exfs.goserver/filesystem/quotas/syscall_xattr.goserver/filesystem/quotas/functions.go
🔇 Additional comments (2)
server/filesystem/quotas/syscall_xattr.go (2)
58-65:fsXAttrlayout update looks correct.Including
CowExtSizeand[8]bytepadding aligns with the intendedstruct fsxattrshape.
42-45: The ioctl request codes are actually architecture-independent.FS_IOC_FSGETXATTRandFS_IOC_FSSETXATTRencode to identical values (0x801c581f and 0x401c5820) across amd64, arm64, arm, and ppc64le because the underlyingstruct fsxattris composed of fixed-width types (__u32fields with explicit padding), not variable-width types likelong. UnlikeFS_IOC_GETFLAGS/SETFLAGS(which uselongand thus differ between 32-bit and 64-bit ABIs), these constants are stable across all architectures. The hard-coded values are correct and require no changes.
reorganize handling limits being below 0 handle if the project id is outside the range
There was a problem hiding this comment.
♻️ Duplicate comments (4)
server/filesystem/quotas/exfs.go (3)
167-180:⚠️ Potential issue | 🟠 MajorPropagate
Closeerrors inwriteTemplate.
defer f.Close()at Line 173 drops close failures, which can hide final writeback errors for/etcfiles.✅ Suggested error-safe close handling
func writeTemplate(t *template.Template, file string, data interface{}) (err error) { f, err := os.Create(file) if err != nil { return err } - defer f.Close() + defer func() { + if cerr := f.Close(); err == nil && cerr != nil { + err = cerr + } + }() - err = t.Execute(f, data) - if err != nil { + if err = t.Execute(f, data); err != nil { return err } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/filesystem/quotas/exfs.go` around lines 167 - 180, The function writeTemplate currently defers f.Close() and drops any Close error; update it to capture and propagate Close errors by using the named return error variable (err) and in the deferred close check assign err = first non-nil error between err and the result of f.Close() so that a failing file close after t.Execute (e.g., writeback to /etc) is returned; reference the writeTemplate function, the t.Execute call, the os.File f and f.Close() in your changes.
98-103:⚠️ Potential issue | 🔴 CriticalRelease
exfs.lockbefore callingwriteEXFSProjectsto avoid self-deadlock.Line 98 and Line 117 hold
exfs.lock, then callwriteEXFSProjects(Line 102 / Line 126), which locks again at Line 143. This can deadlock the goroutine.🛠️ Minimal fix
func (q exfsProject) addProject() (err error) { @@ exfs.lock.Lock() - defer exfs.lock.Unlock() exfs.projects = append(exfs.projects, q) + exfs.lock.Unlock() if err = writeEXFSProjects(); err != nil { log.WithError(err).Error("failed to write exfs projects") return @@ func (q exfsProject) removeProject() (err error) { exfs.lock.Lock() - defer exfs.lock.Unlock() for pos, project := range exfs.projects { if project.Name == q.Name { exfs.projects = append(exfs.projects[:pos], exfs.projects[pos+1:]...) break } } - - err = writeEXFSProjects() - return + exfs.lock.Unlock() + return writeEXFSProjects() }Also applies to: 117-127, 142-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/filesystem/quotas/exfs.go` around lines 98 - 103, The code currently holds exfs.lock while calling writeEXFSProjects, which itself acquires the same lock and can deadlock; update the callers (where exfs.lock is acquired around appending to exfs.projects, e.g., the block that uses exfs.lock.Lock()/defer exfs.lock.Unlock() and the similar block at lines that append/remove projects) to release the lock before invoking writeEXFSProjects: perform the mutation while holding exfs.lock, unlock it (remove or move the defer unlock as needed), then call writeEXFSProjects afterwards so writeEXFSProjects can safely acquire the lock; ensure no shared state is accessed after unlocking except via safe copies or re-locking if required.
100-105:⚠️ Potential issue | 🟠 MajorRollback in-memory append if
/etcpersistence fails inaddProject.Line 100 mutates
exfs.projectsbefore disk write. If Line 102 fails, in-memory and/etc/projid//etc/projectsdiverge.🧩 Suggested rollback on failure
exfs.projects = append(exfs.projects, q) exfs.lock.Unlock() if err = writeEXFSProjects(); err != nil { log.WithError(err).Error("failed to write exfs projects") + exfs.lock.Lock() + for i := len(exfs.projects) - 1; i >= 0; i-- { + if exfs.projects[i].Name == q.Name { + exfs.projects = append(exfs.projects[:i], exfs.projects[i+1:]...) + break + } + } + exfs.lock.Unlock() return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/filesystem/quotas/exfs.go` around lines 100 - 105, The code appends q to exfs.projects before persisting and doesn't revert if writeEXFSProjects() fails, causing in-memory/des on-disk divergence; in addProject, capture the prior slice length (or copy the slice), append q to exfs.projects, then if writeEXFSProjects() returns an error remove the appended entry (restore the slice to its previous length or replace with the saved copy) before returning—i.e., rollback the in-memory mutation on failure to keep exfs.projects consistent with disk.server/filesystem/quotas/functions.go (1)
65-68:⚠️ Potential issue | 🟡 MinorAdd data-path context to quota-support probe failures.
When
fsquota.ProjectQuotasSupported(...)fails, the log currently omits the mount path, which makes diagnosis harder.✏️ Small logging improvement
supported, err = fsquota.ProjectQuotasSupported(config.Get().System.Data) if err != nil { - log.WithError(err).Error("error checking for quota support") + log.WithError(err).WithField("path", config.Get().System.Data).Error("error checking for quota support") return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/filesystem/quotas/functions.go` around lines 65 - 68, The log for quota-support probe omits the data mount path; modify the error branch where fsquota.ProjectQuotasSupported(config.Get().System.Data) returns an error to include the data path (config.Get().System.Data) in the log message — e.g., change the log.WithError(err).Error call to include context about the mount path so the error reads something like "error checking for quota support on data path <path>" and still attaches the error via WithError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@server/filesystem/quotas/exfs.go`:
- Around line 167-180: The function writeTemplate currently defers f.Close() and
drops any Close error; update it to capture and propagate Close errors by using
the named return error variable (err) and in the deferred close check assign err
= first non-nil error between err and the result of f.Close() so that a failing
file close after t.Execute (e.g., writeback to /etc) is returned; reference the
writeTemplate function, the t.Execute call, the os.File f and f.Close() in your
changes.
- Around line 98-103: The code currently holds exfs.lock while calling
writeEXFSProjects, which itself acquires the same lock and can deadlock; update
the callers (where exfs.lock is acquired around appending to exfs.projects,
e.g., the block that uses exfs.lock.Lock()/defer exfs.lock.Unlock() and the
similar block at lines that append/remove projects) to release the lock before
invoking writeEXFSProjects: perform the mutation while holding exfs.lock, unlock
it (remove or move the defer unlock as needed), then call writeEXFSProjects
afterwards so writeEXFSProjects can safely acquire the lock; ensure no shared
state is accessed after unlocking except via safe copies or re-locking if
required.
- Around line 100-105: The code appends q to exfs.projects before persisting and
doesn't revert if writeEXFSProjects() fails, causing in-memory/des on-disk
divergence; in addProject, capture the prior slice length (or copy the slice),
append q to exfs.projects, then if writeEXFSProjects() returns an error remove
the appended entry (restore the slice to its previous length or replace with the
saved copy) before returning—i.e., rollback the in-memory mutation on failure to
keep exfs.projects consistent with disk.
In `@server/filesystem/quotas/functions.go`:
- Around line 65-68: The log for quota-support probe omits the data mount path;
modify the error branch where
fsquota.ProjectQuotasSupported(config.Get().System.Data) returns an error to
include the data path (config.Get().System.Data) in the log message — e.g.,
change the log.WithError(err).Error call to include context about the mount path
so the error reads something like "error checking for quota support on data path
<path>" and still attaches the error via WithError.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/filesystem/quotas/exfs.goserver/filesystem/quotas/functions.goserver/filesystem/quotas/syscall_xattr.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Build and Test (ubuntu-22.04, 1.24.7, linux, amd64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.1, linux, amd64)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: parkervcp
Repo: pelican-dev/wings PR: 171
File: server/power.go:190-203
Timestamp: 2026-03-02T13:53:08.995Z
Learning: In `server/power.go`, when quotas are enabled via `config.Get().System.Quotas.Enabled`, the disk space check using `used >= s.DiskSpace()` does not need a special guard for unlimited-disk servers (DiskSpace() <= 0) because the disk quota is managed by the filesystem itself, which handles such cases appropriately.
📚 Learning: 2026-03-02T13:53:08.995Z
Learnt from: parkervcp
Repo: pelican-dev/wings PR: 171
File: server/power.go:190-203
Timestamp: 2026-03-02T13:53:08.995Z
Learning: In the server package, when quotas are enabled via config.Get().System.Quotas.Enabled, the disk space check using used >= s.DiskSpace() does not require a special guard for unlimited-disk scenarios (DiskSpace() <= 0). The filesystem handles such cases, so the existing check is sufficient. Apply this pattern to similar quota-related disk checks in the server package and ensure tests/docs reflect that unlimited-disk behavior is governed by the filesystem, not by an extra guard in code.
Applied to files:
server/filesystem/quotas/functions.goserver/filesystem/quotas/exfs.goserver/filesystem/quotas/syscall_xattr.go
🧬 Code graph analysis (2)
server/filesystem/quotas/functions.go (1)
config/config.go (1)
Get(445-453)
server/filesystem/quotas/exfs.go (2)
config/config.go (1)
Get(445-453)system/system.go (1)
System(61-68)
🔇 Additional comments (2)
server/filesystem/quotas/syscall_xattr.go (1)
59-66:fsXAttrABI shape andprojectIDguard look good.The struct layout and range check before
uint32cast are solid in this revision.Also applies to: 91-93
server/filesystem/quotas/functions.go (1)
110-113: Negative-limit handling here looks correct.Logging the original value and clamping to zero avoids unsafe
int64 -> uint64wraparound before quota application.
Better error handling Only compiles quota management for linux better path handling mutex to make sure quota update happens at a time atomic file handling for project files uses better syscall option more logging
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/filesystem/quotas/syscall_xattr.go (2)
14-15: 💤 Low valueDuplicate comment.
Remove one of the duplicated lines.
🤖 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 `@server/filesystem/quotas/syscall_xattr.go` around lines 14 - 15, The file syscall_xattr.go contains two identical header comments "// Pulled definitions from /usr/include/linux/fs.h" at the top; remove the duplicate so only a single instance of that comment remains (locate the repeated comment lines in syscall_xattr.go and delete one of them).
85-88: 💤 Low value32-bit Linux build compatibility isn’t part of the current targets
The repo’s build/CI targets onlyGOARCH=amd64andGOARCH=arm64(Makefile and.github/workflows/*), so theintvsmath.MaxUint32portability issue won’t affect supported builds. If 32-bit support is added later, switch to the portable cast below.Optional portability tweak
- if projectID < 0 || projectID > math.MaxUint32 { + if projectID < 0 || uint64(projectID) > uint64(math.MaxUint32) {🤖 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 `@server/filesystem/quotas/syscall_xattr.go` around lines 85 - 88, In setXAttr, replace the current range check that compares an int to math.MaxUint32 with a portable two-step check: first test projectID < 0, then compare uint64(projectID) > math.MaxUint32 (the negative check prevents wrapping), and return the same error; this avoids int-to-constant overflow on 32-bit builds while keeping the intent of setXAttr intact.
🤖 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.
Nitpick comments:
In `@server/filesystem/quotas/syscall_xattr.go`:
- Around line 14-15: The file syscall_xattr.go contains two identical header
comments "// Pulled definitions from /usr/include/linux/fs.h" at the top; remove
the duplicate so only a single instance of that comment remains (locate the
repeated comment lines in syscall_xattr.go and delete one of them).
- Around line 85-88: In setXAttr, replace the current range check that compares
an int to math.MaxUint32 with a portable two-step check: first test projectID <
0, then compare uint64(projectID) > math.MaxUint32 (the negative check prevents
wrapping), and return the same error; this avoids int-to-constant overflow on
32-bit builds while keeping the intent of setXAttr intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9af01d4-75c4-4733-9e95-f7b1b3ac97aa
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
cmd/root.gogo.modserver/filesystem/quotas/exfs.goserver/filesystem/quotas/functions.goserver/filesystem/quotas/syscall_xattr.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/root.go
- server/filesystem/quotas/exfs.go
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-02T13:53:08.995Z
Learnt from: parkervcp
Repo: pelican-dev/wings PR: 171
File: server/power.go:190-203
Timestamp: 2026-03-02T13:53:08.995Z
Learning: In the server package, when quotas are enabled via config.Get().System.Quotas.Enabled, the disk space check using used >= s.DiskSpace() does not require a special guard for unlimited-disk scenarios (DiskSpace() <= 0). The filesystem handles such cases, so the existing check is sufficient. Apply this pattern to similar quota-related disk checks in the server package and ensure tests/docs reflect that unlimited-disk behavior is governed by the filesystem, not by an extra guard in code.
Applied to files:
server/filesystem/quotas/syscall_xattr.goserver/filesystem/quotas/functions.go
🪛 OSV Scanner (2.3.8)
go.mod
[HIGH] 15-15: github.com/docker/docker 28.5.2+incompatible: Moby has an Off-by-one error in its plugin privilege validation in github.com/docker/docker
(GO-2026-4883)
[HIGH] 15-15: github.com/docker/docker 28.5.2+incompatible: Moby has AuthZ plugin bypass when provided oversized request bodies in github.com/docker/docker
(GO-2026-4887)
[HIGH] 15-15: github.com/docker/docker 28.5.2+incompatible: Moby has an Off-by-one error in its plugin privilege validation
[HIGH] 15-15: github.com/docker/docker 28.5.2+incompatible: Docker: Race condition in docker cp allows bind mount redirection to host path
[HIGH] 15-15: github.com/docker/docker 28.5.2+incompatible: Docker: Race condition in docker cp allows creation of arbitrary empty files on the host via symlink swap
[HIGH] 15-15: github.com/docker/docker 28.5.2+incompatible: Moby has AuthZ plugin bypass when provided oversized request bodies
[HIGH] 15-15: github.com/docker/docker 28.5.2+incompatible: Docker: PUT /containers/{id}/archive executes container binary on the host
🔇 Additional comments (14)
server/filesystem/quotas/functions.go (5)
1-14: LGTM!Also applies to: 16-23
27-78: LGTM!
80-88: LGTM!
90-107: LGTM!
109-131: LGTM!go.mod (4)
12-12: LGTM!Also applies to: 16-17, 19-35, 39-49, 51-94, 96-192
50-50: ⚡ Quick wingolang.org/x/sys v0.45.0 is a valid module version.
golang.org/x/sys v0.45.0exists on the Go module proxy (.../@v/v0.45.0.inforeturns version metadata), so the migration target isn’t invalid due to a nonexistent version.
15-15: 🏗️ Heavy liftUpgrade
github.com/docker/docker(v28.5.2+incompatible) due to multiple security advisories
- GitHub Security Vulnerabilities data shows HIGH issues affecting this version, including:
- AuthZ plugin bypass for versions < 29.3.1 (patched in 29.3.1)
docker cprace condition (bind mount redirection / host path impact) for <= 28.5.2 (no patched version listed in the advisory feed)PUT /containers/{id}/archivehost binary execution for <= 28.5.2 (no patched version listed in the advisory feed)- The “plugin privilege validation off-by-one” advisory is MODERATE in the feed (not HIGH).
Also: the
github.com/parkervcp/fsquotapseudo-version future timestamp and thegolang.org/x/sys v0.45.0version validity still need independent checks before marking them addressed.
38-38: ⚡ Quick winUpdate:
github.com/parkervcp/fsquotapseudo-version is valid; required APIs exist
go.modpinsgithub.com/parkervcp/fsquota v0.0.0-20260506201313-20951fbdc96c; commit20951fbdc96cexists upstream.- No tagged releases/tags are available for
parkervcp/fsquota, so switching to a tag isn’t currently possible.- The APIs wings depends on are present at that ref:
LookupProject,LookupProjectID(inutil_linux.go)SetProjectQuota,GetProjectInfo,ProjectQuotasSupported(infsquota.go)server/filesystem/quotas/syscall_xattr.go (5)
1-12: LGTM!
17-50: LGTM!
52-70: LGTM!
72-82: LGTM!
90-100: LGTM!
need to update due to syscall.Stat_T being required.
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
2 participants
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.
This adds disk quota support for the EXT4 filesystem. May also work on XFS but that has not been tested yet.
Requires setup called out in the docs. https://pelican.dev/docs/guides/disk-quotas/about
Quotas will hard stop any file writes when they would go beyond the allocated space.
This also replaces the "syscall" package with "golang.org/x/sys/unix". Syscall was deprecated in 2014.
Summary by CodeRabbit
New Features
Configuration
Bug Fixes / Stability