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: add fs-version annotation #497

Merged
merged 1 commit into from
Jun 17, 2022
Merged

Conversation

yawqi
Copy link
Contributor

@yawqi yawqi commented Jun 15, 2022

Add an annotation to the image manifest for detecting the bootstrap fs
version without reading the magic number from bootstrap.

fix #496

Signed-off-by: Qi Wang wangqi@linux.alibaba.com

@yawqi yawqi changed the title nydusify: add fs-version annotation WIP nydusify: add fs-version annotation Jun 15, 2022
@yawqi yawqi force-pushed the annotaion branch 3 times, most recently from ae23437 to 8a7c54a Compare June 15, 2022 07:51
@yawqi yawqi changed the title WIP nydusify: add fs-version annotation nydusify: add fs-version annotation Jun 15, 2022
@@ -129,6 +129,7 @@ func (cache *Cache) recordToLayer(record *Record) (*ocispec.Descriptor, *ocispec
Size: record.NydusBootstrapDesc.Size,
Annotations: map[string]string{
utils.LayerAnnotationNydusBootstrap: "true",
utils.LayerAnnotationNydusFsVersion: cache.opt.Version,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be cache.opt.FsVersion, how this option be passed to the cache image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked, it is cache.opt.Version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// Opt configures Nydus cache
type Opt struct {
	// Maximum records(bootstrap layer + blob layer) in cache image.
	MaxRecords uint
	Version    string
	// Make cache image manifest compatible with the docker v2 media
	// type defined in github.com/containerd/containerd/images.
	DockerV2Format bool
	// The blob layer record will not be written to cache image if
	// the backend be specified, because the blob layer will be uploaded
	// to backend.
	Backend backend.Backend
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tested on converting alpine:latest with nydusify for both v5 and v6, and checked their manifests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@yawqi This Version field is used to indicate the version of cache image (not fs version), nydusify will use cache records depending on the version. :)

Copy link
Collaborator

@imeoer imeoer Jun 16, 2022

Choose a reason for hiding this comment

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

We can add some comments to distinguish these two fields:

// Opt configures Nydus cache
type Opt struct {
	...
	// Version of cache image, we need to discard cache layers when
        // the required version (specified by `--build-cache-version`)
	// is unmatched with the cache image version, for example nydus
	// bootstrap format has a minor upgrade.
	Version    string
	// Bootstrap's RAFS version of cache image, we need to discard cache
	// layers when the required version (specified by `--fs-version`) is
	// unmatched with the fs version recorded in cache image, for example
	// we can't use rafs v5 cache layers for rafs v6 image.
	FsVersion string
	...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@yawqi yawqi force-pushed the annotaion branch 2 times, most recently from 96f4878 to 1999069 Compare June 16, 2022 02:37
@yawqi yawqi requested a review from imeoer June 16, 2022 05:47
// Discard the cache if mismatched RAFS FsVersion
if manifest.Annotations[utils.LayerAnnotationNydusFsVersion] != cache.opt.FsVersion {
return fmt.Errorf(
"unmatched cache image version %s, required to be %s",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe unmatched fs version %s of cache image, required to be %s.

if manifest.Annotations[utils.ManifestNydusCache] != cache.opt.Version {
return fmt.Errorf(
"unmatched cache image version %s, required to be %s",
manifest.Annotations[utils.ManifestNydusCache], cache.opt.Version,
)
}

// Discard the cache if mismatched RAFS FsVersion
if manifest.Annotations[utils.LayerAnnotationNydusFsVersion] != cache.opt.FsVersion {
Copy link
Collaborator

Choose a reason for hiding this comment

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

For manifest.Annotations[utils.LayerAnnotationNydusFsVersion] == "" (e.g. old cache image), we should consider it as 5.

Add an annotation to the image manifest for detecting the bootstrap fs
version without reading the magic number from bootstrap.

fix dragonflyoss#496

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

@imeoer imeoer left a comment

Choose a reason for hiding this comment

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

LGTM!

@imeoer imeoer merged commit 5093dc2 into dragonflyoss:master Jun 17, 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.

nydusify: append fs version annotation in image manifest
2 participants