chunked: skip file metadata for composefs-like links#1862
chunked: skip file metadata for composefs-like links#1862openshift-merge-bot[bot] merged 4 commits intocontainers:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
LGTM |
|
Apart from the other comment, this seems sane; are there no integration tests that run with composefs enabled on this repo? It seems like it wouldn't be too hard to add a test for this if so. (And if not, probably need integration coverage for chunked/composefs here) |
mtrmac
left a comment
There was a problem hiding this comment.
BTW, the code to check whether the hardlink destination matches the expected attributes (canDedupFileWithHardLink?) and this code to set/not set the attributes are way too distant for me to be comfortable; I’m not smart enough to believe I would be able to keep the two in sync long-term.
b17932f to
c80d63b
Compare
this is still something we miss, as we don't have any test for composefs |
c80d63b to
2cc31aa
Compare
mtrmac
left a comment
There was a problem hiding this comment.
This is nice.
Apart from comment around internal.ChunkTypeData (to consistently never use the internal fileMetadata for TypeChunk values), all others are just tentative aesthetic suggestions.
|
|
||
| // setFileAttrs sets the file attributes for file given metadata | ||
| func setFileAttrs(dirfd int, file *os.File, mode os.FileMode, metadata *internal.FileMetadata, options *archive.TarOptions, usePath bool) error { | ||
| func setFileAttrs(dirfd int, file *os.File, mode os.FileMode, metadata *fileMetadata, options *archive.TarOptions, usePath bool) error { |
There was a problem hiding this comment.
(Unrelated: if I’m not mistaken, mode is always os.FileMode(metadata.Mode), so the parameter could be eliminated throughout the call stack. Perhaps best to do in a separate PR.)
84b261e to
c047434
Compare
|
thanks, took care of the comments (except #1862 (comment) ) and pushed a new version |
mtrmac
left a comment
There was a problem hiding this comment.
Thanks!
LGTM, feel free to merge as is or add that small cleanup.
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
when it is set, only the file payload is written, but the inode attributes are ignored. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
if a file was deduplicated with a hard link, do not override its metadata. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
if the file is created using the object-store flat directory format, there is no need to set its inodes attributes, as anyway they are ignored when creating the composefs binary blob. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
c047434 to
1126d65
Compare
|
/lgtm |
if the file is created using the object-store flat directory format, there is no need to set its inodes attributes, as anyway they are ignored when creating the composefs binary blob.
Similarly, we do not need to set file attributes when the deduplication happened through a hard link.