Skip to content
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

Filesystem: refactor estargz support #77

Merged
merged 4 commits into from
Jun 25, 2022

Conversation

imeoer
Copy link
Collaborator

@imeoer imeoer commented Jun 24, 2022

Since the stargz filesystem implementation is very similar
to the nydus filesystem implementation, much of the code
can be shared, for example the shared daemon related codes.

We are going to merge them together, this PR moves codes,
and adjust the code path for stargz support.

Prepare for removing stargz filesystem implementation.

Since the stargz filesystem implementation is very similar
to the nydus filesystem implementation, much of the code
can be shared, for example the shared daemon related codes.

We are going to merge them together, this PR moves codes,
and breaks the current stargz implementation.

Signed-off-by: Yan Song <yansong.ys@antfin.com>
Move pkg/filesystem/stargz to pkg/filesystem/fs/stargz.

And remove stargz related codes from pkg/filesystem/fs,
it's prepare for stargz support in pkg/filesystem/fs
package directly.

Signed-off-by: Yan Song <yansong.ys@antfin.com>
Add stargz handle logic in filesystem implementation,
this commit recovery stargz support.

Signed-off-by: Yan Song <yansong.ys@antfin.com>
@codecov-commenter
Copy link

Codecov Report

Merging #77 (986edf7) into main (615dda8) will increase coverage by 0.66%.
The diff coverage is 7.28%.

@@            Coverage Diff             @@
##             main      #77      +/-   ##
==========================================
+ Coverage   28.78%   29.44%   +0.66%     
==========================================
  Files          20       17       -3     
  Lines        1567     1467     -100     
==========================================
- Hits          451      432      -19     
+ Misses       1049      971      -78     
+ Partials       67       64       -3     
Impacted Files Coverage Δ
pkg/filesystem/fs/blob_manager.go 55.55% <ø> (ø)
pkg/filesystem/fs/resolver.go 0.00% <ø> (ø)
pkg/filesystem/fs/stargz/resolver.go 41.02% <0.00%> (ø)
pkg/filesystem/fs/config.go 3.65% <4.00%> (ø)
pkg/filesystem/fs/fs.go 7.52% <7.52%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 615dda8...986edf7. Read the comment docs.

// So we only need to read the MaxSuperBlockSize size to include both v5 and v6 superblocks
const MaxSuperBlockSize = 8 * 1024
const RafsV6Magic = 0xE0F5E1E2
const ChunkInfoOffset = 1024 + 128 + 24
Copy link
Contributor

Choose a reason for hiding this comment

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

RafsV6ChunkInfoOffset?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, actually, these codes just moved from other place. :)

log.G(ctx).Infof("total nydus prepare data layer duration %d", duration.Milliseconds())
}()

if fs.imageMode == OnDemand {
Copy link
Contributor

Choose a reason for hiding this comment

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

Move it to before the duration calculation? No need to calculate duration for preload mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also it's better to check fs.imageMode == Preload directly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

}
defer readerCloser.Close()

blobFile, err := os.OpenFile(blobPath, os.O_CREATE|os.O_RDWR, 0755)
Copy link
Contributor

Choose a reason for hiding this comment

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

0666?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

return errors.Wrap(err, "failed to create blob file")
}
defer blobFile.Close()
_, err = io.Copy(blobFile, readerCloser)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove blob file upon error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Signed-off-by: Yan Song <yansong.ys@antfin.com>
@jiangliu jiangliu merged commit a1cf218 into containerd:main Jun 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants