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

nydusify & nydus-image: v6 image conversion support #473

Merged
merged 21 commits into from
Jun 10, 2022

Conversation

yawqi
Copy link
Contributor

@yawqi yawqi commented Jun 7, 2022

We have adapted the nydusify and nydus-image to support using nydusify directly to build v6 image, to accomplish this purpose, we have introduce several features and fix several bugs.

Features:

  1. Previously, the load_parent_bootstrap is unusable for v6 image, we have adapted it for both V5 and V6.
  2. We have implemented all methods for v6's OndiskInodeWrapper.

Bug fixes:

  1. The dirents_offset will only be used by dir and symlink files, for other files, calculating it may leads to overflow.
  2. Correct chunk info implementation for v6.
  3. Previous, we assume the . and .. are the first and second dirents, which is wrong.
  4. Under some circumstances, the xattr size is wrong.

FYI, we have tested converting all top images in the image_list.txt and checked their bootstraps with fsck.erofs.
#425

rafs/src/metadata/direct_v6.rs Outdated Show resolved Hide resolved
rafs/src/metadata/direct_v6.rs Outdated Show resolved Hide resolved
yawqi and others added 13 commits June 8, 2022 02:31
…mlink file

Only the dir and symlink file of rafsv6 can be of EROFS_INODE_FLAT_INLINE layout,
so we only need to compute dirents offset for these two type of files.
Otherwise, it may overflow.

Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
Signed-off-by: Qi Wang <wangqi@linux.alibaba.com>
…uild

First, we need to change v5 specific interface to universal interface.

Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
Signed-off-by: Qi Wang <wangqi@linux.alibaba.com>
We implement unimplmented methods of v6's OndiskInodeWrapper.

Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
Signed-off-by: Qi Wang <wangqi@linux.alibaba.com>
Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
Signed-off-by: Qi Wang <wangqi@linux.alibaba.com>
Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
Previously, in some methods, we assume the `.` and `..` are
the first two dirents, which is not true, both of them are
globally sorted along with other dirents, we fix that in this
patch.

Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
When build centos and java's v6 image, the conversion
failed due to incorrent xattr index. We fix that in this
patch.

Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
Previously, we have modified the nydus-image to allow specifying
parent bootstrap when converting v6 image, therefore, we add the
image-version argument into nydusify to support v6 image conversion.

Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
…h nydus-image

Signed-off-by: Qi Wang <wangqi@linux.alibaba.com>
Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
@zyfjeff
Copy link
Contributor

zyfjeff commented Jun 8, 2022

@liubogithub @jiangliu @imeoer PTAL

Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
let mut bootstrap = Bootstrap::new()?;
if bootstrap_ctx.layered {
// Merge with lower layer if there's one, do not prepare `prefetch` list during merging.
ctx.prefetch.disable();
bootstrap.build(ctx, &mut bootstrap_ctx, &mut tree)?;
tree = bootstrap.apply(ctx, &mut bootstrap_ctx, bootstrap_mgr, blob_mgr, None)?;
}
// If layered, the bootstrap_ctx.offset will be set in first build, so we need restore it here
bootstrap_ctx.offset = origin_bootstarp_offset;
bootstrap_ctx.layered = false;
Copy link
Collaborator

@liubogithub liubogithub Jun 8, 2022

Choose a reason for hiding this comment

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

This also affects https://github.com/dragonflyoss/image-service/blob/fdee5992103916910dea755a2df3d5d669ea6efb/src/bin/nydus-image/core/bootstrap.rs#L202

where layered is used to check whiteout, have you checked if setting layered = false would cause any difference?

Copy link
Contributor

@zyfjeff zyfjeff Jun 9, 2022

Choose a reason for hiding this comment

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

In fact, the processing of whiteout is already done at the time of the first bootstarp.build, and here is actually the second time to do bootstarp.build

        if bootstrap_ctx.layered {
            // Merge with lower layer if there's one, do not prepare `prefetch` list during merging.
            ctx.prefetch.disable();
            bootstrap.build(ctx, &mut bootstrap_ctx, &mut tree)?;
            tree = bootstrap.apply(ctx, &mut bootstrap_ctx, bootstrap_mgr, blob_mgr, None)?;
        }

BTW, https://github.com/dragonflyoss/image-service/blob/master/contrib/nydusify/tests/texture/image-from-2/Dockerfile#L12. There are some integration tests for whiteout in nydusify, which are currently passed.

for j in 0..entries_count {
let de = self
.get_entry(i as usize, j as usize)
.map_err(err_invalidate_data)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

err_invalidate_data is confusing, would you please change it to err_inval_data in a separate patch?

Copy link
Contributor

@zyfjeff zyfjeff Jun 9, 2022

Choose a reason for hiding this comment

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

Agree it

@@ -109,7 +109,7 @@ fn test(
cache_compressed,
rafs_mode.parse().unwrap(),
"api.sock".into(),
true,
!rafsv6,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please leave a comment either here or in the patch log for why digest validate for v6 is disabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually already has some comments. exists only where it was first used.

            // FIXME: Currently no digest validation is implemented for rafs v6.
            !rafsv6,

@@ -141,20 +181,19 @@ impl RafsInspector {

fn load_meta(f: &mut RafsIoReader) -> Result<(RafsMeta, RafsLayout)> {
let layout_profile = RafsLayout::rafsv5_layout();
match Self::super_block_v5(f, &layout_profile) {
Ok(sb) => {
let mut res = Self::super_block_v5(f, &layout_profile).map(|sb| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

super_block_v5 does RafsV5SuperBlock::load() which doesn't check fs magic or fs version, so it won't return errors if it's a v6 superblock.

Copy link
Contributor

Choose a reason for hiding this comment

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

I modified the super_block_v5 and added a checker for rafs versions.

      if !sb.detect() {
            return Err(anyhow!("invalid rafs v5 version"));
        }

@zyfjeff
Copy link
Contributor

zyfjeff commented Jun 9, 2022

@liubogithub PTAL

Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
Signed-off-by: zyfjeff <zyfjeff@linux.alibaba.com>
@imeoer imeoer merged commit aabe14f into dragonflyoss:master Jun 10, 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