Skip to content

fix(chart): make containers and vgpulock dirs writable by non-root workloads - #22

Closed
dttung2905 wants to merge 1 commit into
Project-HAMi:mainfrom
dttung2905:fix-mkdir-permission
Closed

dttung2905 wants to merge 1 commit into
Project-HAMi:mainfrom
dttung2905:fix-mkdir-permission

Conversation

@dttung2905

@dttung2905 dttung2905 commented Aug 8, 2026 •

Copy link
Copy Markdown
Contributor

What type of PR is this?
Chart fix

What this PR does / why we need it:
When I carried out e2e test on kai-scheduler side for item 4 in this kai-scheduler/KAI-Scheduler#1821 , I notice this issue

$ klf czblxbshgw -n kai-axchniwozk
[HAMI-core Msg(1:124643043790848:libvgpu.c:873)]: Initializing.....
[HAMI-core Warn(1:124643043790848:multiprocess_memory_limit.c:1207)]: Fail to create synthesized container dir /usr/local/vgpu/containers/c7dc6208-0f81-4078-bad8-a603903aefba_ubuntu-container: errno=13, using default cache path: /tmp/cudevshr.cache
[HAMI-core Msg(1:124643043790848:libvgpu.c:909)]: Initialized
allocated 67108864 bytes at 0x715c8e000000

libvgpu synthesizes its metrics cache under {CONTAINER_VGPU_MOUNT}/containers/{POD_UID}_{CONTAINER_NAME}. When that host directory is 755 root, non-root workload containers (common in e2e, UID 1000) fail with errno=13 and fall back to in-container /tmp/cudevshr.cache, so kai-vgpu-monitor never sees per-container hami_* metrics.

This change has libsync create containers/ and /tmp/vgpulock up front and chmod 1777 them so non-root pods can create their cache dirs while still writing the shared cache on the host path the monitor scrapes.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Summary by CodeRabbit

  • Bug Fixes
    • Improved library synchronization by ensuring required host directories are created automatically.
    • Applied shared-access permissions to the container and GPU lock directories for more reliable operation.

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@hami-robot

hami-robot Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dttung2905
Once this PR has been reviewed and has the lgtm label, please assign archlitchi for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@hami-robot
hami-robot Bot requested a review from archlitchi August 8, 2026 08:30
@hami-robot hami-robot Bot added the size/XS label Aug 8, 2026
@coderabbitai

coderabbitai Bot commented Aug 8, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

â„đïļ Recent review info
⚙ïļ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 92dd5622-f625-4334-bd7b-d88c43833513

ðŸ“Ĩ Commits

Reviewing files that changed from the base of the PR and between 13af262 and 7c1e965.

📒 Files selected for processing (1)
  • chart/kai-resource-isolator/templates/daemonset.yaml

📝 Walkthrough

Walkthrough

The DaemonSet startup script now creates the host library destination, containers, and vgpulock directories. It applies mode 1777 to the latter two directories. Existing library and preload file handling remains unchanged.

Changes

Library Sync Directory Setup

Layer / File(s) Summary
Create and configure workload directories
chart/kai-resource-isolator/templates/daemonset.yaml
The startup script creates the required directories and applies sticky world-writable permissions to containers and vgpulock.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Suggested labels: enhancement

Suggested reviewers: archlitchi

Poem

A rabbit hops through paths anew,
With sticky bits in fields of blue.
Containers bloom, locks settle tight,
While libraries sync by morning light.
“1777!” I cheer, and twitch my ear.

ðŸšĨ Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: making the containers and vgpulock directories writable by non-root workloads.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
âœĻ Finishing Touches
🧊 Generate unit tests (beta)
  • Create PR with unit tests

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.

âĪïļ Share

Comment @coderabbitai help to get the list of available commands.

chmod 0644 "${DST}/libvgpu.so" "${DST}/ld.so.preload"
# Workload containers often run as non-root (e.g. e2e UID 1000);
# libvgpu must be able to mkdir {POD_UID}_{CONTAINER_NAME} here.
chmod 1777 "${DST}/containers" /host-root/tmp/vgpulock

@moezdil moezdil Aug 10, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/tmp/vgpulock is hardcoded here too, same as vgpuLockPath in webhook/main.go. could drift later, np for now tho

@moezdil

moezdil commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Old PRs and issues are being closed because they are no longer relevant. Or it was closed because of th
inactivity or PR/issues owners.
If you believe the issue is still relevant, please submit feedback.

@moezdil moezdil closed this Sep 22, 2026
hami-robot Bot pushed a commit that referenced this pull request Sep 28, 2026
The libsync DaemonSet only created /usr/local/vgpu. The webhook mounts
{containerVgpuMount}/containers and the host /tmp/vgpulock into workload
containers as DirectoryOrCreate hostPaths, so the kubelet created both
as root 0755. libvgpu then mkdirs its synthesized cache dir
{CONTAINER_VGPU_MOUNT}/containers/{POD_UID}_{CONTAINER_NAME} from inside
the workload container and, running as non-root, fails with errno=13 and
falls back to the container-local /tmp/cudevshr.cache, which
kai-vgpu-monitor cannot see.

Create both directories in the libsync startup script, which already
runs privileged as root with / mounted at /host-root, and leave them
1777 so any workload UID can create its own cache dir on the shared host
path. mkdir -p plus chmod is idempotent, so an in-place upgrade over
existing 0755 directories converts them on the libsync restart.

This is the change proposed in #22 by dttung2905, closed unmerged by a
stale-PR sweep. Required by the maintainer decision in
kai-scheduler/KAI-Scheduler#1821 (libsync creates /tmp/vgpulock on host
at install) and verified by the merged kai-scheduler/KAI-Scheduler#2033,
which asserts 1777 for both directories.

No values, image, mount, securityContext or webhook change; the rendered
manifest differs from main only inside the libsync command block.

Signed-off-by: blackdragoon26 <sankalp.jha9643@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants