[RAPTOR-19524] Track executable bit for files - #864
Draft
taras-pokornyy wants to merge 5 commits into
Draft
Conversation
The code-sync manifest only ever tracked hash/size per file, so the executable bit (and any non-default mode) was silently dropped on every sync -- files always land server-side, and round-trip back via checkout, as 0644. Captures mode at the point files are already stat'd for hashing and threads it through the local manifest, the diff engine, and the filesapi wire types (currently server-unpopulated, decodes as the "mode unknown" zero sentinel until the server ships its half). Includes the filesapi.FileMeta/AllFilesItem mode fields since FromFilesAPI's conversion depends on them to compile. Part of a multi-commit fix; upload/download/rollback wiring follows.
The ZipUploader path had a natural carrier for file mode -- zip.FileHeader external attributes -- but never used it, so archived files lost their permission bits before the upload even left the client. addToZip now sets it from the already-captured FileAction.LocalMode.
The per-file multipart upload had no carrier for file mode at all. Adds a mode parameter to filesapi.Client.UploadToStage, forwarded from the already-captured FileAction.LocalMode as an optional `mode` query param (a zero value is omitted entirely rather than sent as `mode=0`, so a server that doesn't understand the param yet sees exactly what it saw before this field existed).
Rollback.Backup and restoreFromDir are pure local-to-local file copies (snapshot before a risky overwrite, restore on failure) that silently normalized mode via os.Create, same bug class as the sync/download paths but with zero server dependency since the true mode is always available via a direct stat. Both now apply it through the shared fileops.ApplyMode helper; a stat failure during restore is treated as non-fatal, since this is a best-effort path where failing the whole rollback over one missed chmod would lose every other file it already restored.
The full mode round trip (commits 9ab15d9, df5c0a4, ea2a400, 761126e) is client-side only until the server (WAPI/IBS) ships its half -- until then the executable bit may still not survive into the built image, and that failure surfaces much later, at container startup, pointing at the runtime rather than the sync. hashEntries now reports which walked files are locally executable; phase2Manifests warns once, naming an example file, mirroring the existing shadow-ignore-file/lockfile warning shapes in this same phase. Remove this warning once server support is confirmed.
taras-pokornyy
changed the title
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
1 participant
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.
NOTES
This PR is CLI-only. The mode is captured, forwarded, and restored wherever this repo can do so,
but nothing here can make the server (Files API / workload-api / IBS) actually apply or return it
-- that's tracked separately; see the handoff spec below for what still needs to happen elsewhere.
Warning
RAPTOR-19524-other-services-agent-brief.md(commit2d18245) is a planning document for theFiles API / workload-api / IBS follow-up work, not part of this repo's shipped code. Squash it
into the branch history or drop it before merge -- it's included here only so reviewers and
whoever picks up the cross-service work have it; it should not land on
main.RATIONALE
RAPTOR-19524 (Critical):
dr workload upand
dr artifact code syncupload the project directory as the server-side image build context,but the sync pipeline never tracked Unix file mode. An executable synced as
0755always landsserver-side (and round-trips back via
dr artifact code checkout) as0644. A Dockerfile thatCOPYs the file without an explicit--chmod=755then produces a container that dies at startupwith
exec: "/path": permission denied-- a failure that names the runtime, not the sync, so it'sexpensive to diagnose. Confirmed reproducible locally: Docker preserves whatever mode the build
context handed it, so the bit is already gone by the time the server has the file.
Root cause: the manifest (
wapi.FileMeta) only ever trackedhash/size; the file walk/hashstep already
os.Stats every file (for size) but threw awayinfo.Mode(); neither uploadtransport carried mode (the zip path has a natural carrier --
zip.FileHeader.SetMode/Mode()--but never called it; the per-file multipart path had no carrier at all); and both download paths
hardcoded
os.Createwith no mode restoration.Scope of this PR: everything fixable within this CLI repo, end to end and backward-compatible.
Whether the mode actually survives in the built image also depends on server-side (Files API /
workload-api / IBS) support that doesn't exist yet -- this repo can't implement or verify that
half, so a sync-time warning stays in place as an interim mitigation until the server side ships
(see the companion agent instructions below).
CHANGES
9ab15d9):fileops.HashFilenow also returns the already-stat'd
os.FileMode; threaded as a newMode uint32field throughwapi.FileMeta(the local.datarobot/workload/manifest.json),sync.FileEntry/FileAction,and
filesapi.FileMeta/AllFilesItem(server-unpopulated for now, decodes as the "mode unknown"zero sentinel). No visible behavior change on its own -- pure plumbing.
df5c0a4): theZipUploaderpath had a natural carrier -- zipexternal attributes -- but never used it.
addToZipnow callszip.FileHeader.SetMode().ea2a400): the per-file multipartStageUploaderpath had nocarrier at all. Adds a
modeparam tofilesapi.Client.UploadToStage, sent as an optional?mode=0755-style query param -- omitted entirely (notmode=0) when unknown, so an old CLI ora server that doesn't understand the param yet sees exactly what it saw before this existed.
761126e, includes the download-side restore): both download paths(
dr artifact code checkoutand the sync engine's own pull-remote-only-files step) hardcodedos.Createwith no mode restoration -- fixed via a new sharedfileops.ApplyModehelper (ano-op when mode is unknown). Same bug also existed in
Rollback.Backup/restoreFromDir(purelocal-to-local copies, same class of bug, zero server dependency) -- fixed with the same helper.
dee1f7b): since the full round trip can't be verified end-to-end yet,warns once per sync (naming an example file) when locally-executable files are uploaded, so the
failure is diagnosable at sync time instead of surfacing later as an opaque container start
failure. Mirrors the existing shadow-ignore-file/lockfile warning shapes in the same phase.
Mode is represented as
uint32holdingos.FileMode.Perm()(0-511 /0000-0777) everywhere.Zero is the sentinel for "unknown" (pre-feature manifest, or a server response without the field
yet) -- unambiguous, since a file that was successfully opened and hashed can never legitimately
have
Perm() == 0.On Windows,
fileops.ApplyModeis a safe no-op beyond the read-only attribute: NTFS has noowner/group/execute concept, so
os.Chmodthere only honors the 0200 (owner-write) bit.PR Automation
Comment-Commands: Trigger CI by commenting on the PR:
/trigger-smoke-testor/trigger-test-smoke- Run smoke tests/trigger-install-testor/trigger-test-install- Run installation testsLabels: Apply labels to trigger workflows:
run-smoke-testsorgo- Run smoke tests on demand (only works for non-forked PRs)Important
For Forked PRs: The
run-smoke-testslabel won't work. A required Smoke Tests check will block merge until a maintainer acts:/approve-smoke-teststo run smoke tests (results will set the check)/skip-smoke-teststo bypass the check without running testsPlease comment requesting a maintainer review if you need smoke tests to run.