-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pkg/archive: Use map of long names avoiding max path limits #8449
base: main
Are you sure you want to change the base?
Conversation
b57808a
to
288d017
Compare
dd4eec5
to
3dcd023
Compare
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
3dcd023
to
a2699e7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8449 +/- ##
==========================================
+ Coverage 58.98% 59.00% +0.01%
==========================================
Files 368 368
Lines 39000 39017 +17
==========================================
+ Hits 23004 23021 +17
Misses 14532 14532
Partials 1464 1464 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
||
// If the target is longer than maxPathLength, we'll use the sha256 of the header name as the filename. | ||
// https://github.com/vmware-tanzu/velero/issues/8434 | ||
if len(target) > maxPathLength { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See here and here, header.Name
contains more than one fs separators, which results in the generation of more than one sub dirs after calling e.fs.MkdirAll
.
If we replace it with a hash string, the separators will be eliminated, as a result, the followed logics, like Parse
will be affected.
Is the restore still functioning with this PR? Or anything I misunderstood?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will double check that restore Parse is working and report back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hash string is saved in a map, and separators are stored. When parser encounters a sha256 in a map, it gets back the original
I believe per unit test in pkg/archive/parser_test.go show that it is functioning correctly on long dir name and long file name.
We're only generating hash for name of dir or name of file, not a path containing separators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At here, we change the target to a hash string of header.Name
:
target = filepath.Join(dir, shortSha256Name)
Per my understanding header.Name
contains file system separators. So the separators are missing in target
Then at here, we create dirs according to target
:
err := e.fs.MkdirAll(target, header.FileInfo().Mode())
As a result, the separators in the original header.Name
are eliminated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is low priority let's bump this one out. Perhaps I can scope the hashing to if header.Name has no separators
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See
velero/pkg/archive/filesystem.go
Line 34 in 6ac38cd
// GetVersionedItemFilePath returns an item's file path once extracted from a Velero backup archive, with version included. |
header.Name
has separators in most cases.
Signed-off-by: Tiger Kaovilai tkaovila@redhat.com
Thank you for contributing to Velero!
Please add a summary of your change
pkg/archive: Use map of long names avoiding max path limits
Test image:
ghcr.io/kaovilai/velero:maxpathlimits-a2699e765
for amd64, arm64Does your change fix a particular issue?
Fixes #8434
Please indicate you've done the following:
make new-changelog
) or comment/kind changelog-not-required
on this PR.site/content/docs/main
.