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

builder: support --parent-bootstrap for merge #1137

Merged
merged 3 commits into from
Mar 9, 2023

Conversation

imeoer
Copy link
Collaborator

@imeoer imeoer commented Mar 3, 2023

This option allows merging multiple bootstraps of upper layer with
the bootstrap of a parent image, so that we can implement container
commit operation for nydus image.

Related PR: containerd/nydus-snapshotter#403

@imeoer imeoer requested a review from a team as a code owner March 3, 2023 11:57
@imeoer imeoer requested review from bergwolf, liubogithub and jiangliu and removed request for a team March 3, 2023 11:57
@imeoer imeoer force-pushed the converter-parent-bootstrap branch 2 times, most recently from 82e5ffc to d937eb9 Compare March 8, 2023 06:56
@imeoer imeoer changed the title [WIP] builder: support --parent-bootstrap for merge builder: support --parent-bootstrap for merge Mar 8, 2023
@anolis-bot
Copy link
Collaborator

@imeoer , the title has been updated, so a new test job has been submitted. Please wait in patience. The test job url: https://tone.openanolis.cn/ws/nrh4nnio/test_result/55289

@anolis-bot
Copy link
Collaborator

@imeoer , The CI test is completed, please check result:

Test CaseTest Result
build rust golang image✅ SUCCESS
compile nydusd✅ SUCCESS
compile ctr remote✅ SUCCESS
compile nydus snapshotter✅ SUCCESS
run container with rafs✅ SUCCESS
run container with zran✅ SUCCESS
run container with rafs and compile linux✅ SUCCESS

Congratulations, your test job passed!

}
if !found {
blob_idx_map.push(blob_mgr.len() as u32);
if blob_idx_map.get(&blob.blob_id()).is_none() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

!blob_idx_map.contains(&blob.blob_id())?

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

let blobs = rs.superblock.get_blob_infos();
for blob in blobs.iter() {
// Fix blob id for new images with old converters.
if blob.has_feature(BlobFeatures::INLINED_FS_META) {
blob.set_blob_id_from_meta_path(path.as_ref())?;
fixed = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why removing this backward compatible handling?
If it's not needed any, please help to remove about comments:)

Copy link
Collaborator Author

@imeoer imeoer Mar 9, 2023

Choose a reason for hiding this comment

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

We still need to keep Fix blob id for new images with old converters., but remove Fix blob id for old images with old converters..

In fact, there is no way to check if a separate old bootstrap file was inline to the blob, for example, for an merged bootstrap generated by old converter, we can't set the blob id it references to as the filename, otherwise it will break blob table on loading rafs.

The bug can be reproduced by these steps:

$ nydus-image (v1.0.0) create --blob /blob --bootstrap /old-v5-bootstrap /source

$ nydus-image (v2.2.0) check --bootstrap /old-v5-bootstrap

RAFS filesystem metadata is valid, referenced data blobs:
	 0: old-v5-bootstrap, compressed data size 0x0, compressed file size 0x0, uncompressed file size 0x0, chunks: 0x0, features:
	 1: old-v5-bootstrap, compressed data size 0x0, compressed file size 0x0, uncompressed file size 0x0, chunks: 0x0, features:
	 2: old-v5-bootstrap, compressed data size 0x0, compressed file size 0x0, uncompressed file size 0x0, chunks: 0x0, features:
	 3: old-v5-bootstrap, compressed data size 0x0, compressed file size 0x0, uncompressed file size 0x0, chunks: 0x0, features:

Ths blob ids in output are broken.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we don't need to support the snapshot view capability for old images.

This option allows merging multiple bootstraps of upper layer with
the bootstrap of a parent image, so that we can implement container
commit operation for nydus image.

Signed-off-by: Yan Song <imeoer@linux.alibaba.com>
Signed-off-by: Yan Song <imeoer@linux.alibaba.com>
In fact, there is no way to tell if a separate old bootstrap file
was inline to the blob, for example, for an old merged bootstrap,
we can't set the blob id it references to as the filename, otherwise
it will break blob table on loading rafs.

Signed-off-by: Yan Song <imeoer@linux.alibaba.com>
@anolis-bot
Copy link
Collaborator

@imeoer , the code has been updated, so a new test job has been submitted. Please wait in patience. The test job url: https://tone.openanolis.cn/ws/nrh4nnio/test_result/55522

@anolis-bot
Copy link
Collaborator

@imeoer , The CI test is completed, please check result:

Test CaseTest Result
build rust golang image✅ SUCCESS
compile nydusd✅ SUCCESS
compile ctr remote✅ SUCCESS
compile nydus snapshotter✅ SUCCESS
run container with rafs✅ SUCCESS
run container with zran✅ SUCCESS
run container with rafs and compile linux✅ SUCCESS

Congratulations, your test job passed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants