Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

Support symlink layer in image import. #727

Merged
merged 1 commit into from
Apr 11, 2018

Conversation

Random-Liu
Copy link
Member

@Random-Liu Random-Liu commented Apr 9, 2018

Fixes #638.

This PR added support for symlink layers. @AkihiroSuda Does this make sense to you?

Now ctr cri load works for k8s.gcr.io/pause-amd64:3.0:

$ sudo ctr cri load test/test.tar 
Loaded image: k8s.gcr.io/pause-amd64:3.0

/cc @hmtai @qiujian16

Signed-off-by: Lantao Liu [email protected]

@@ -60,6 +60,15 @@ func isLayerTar(name string) bool {
return slashes == 2 && strings.HasSuffix(name, "/layer.tar")
}

// followSymlinkLayer returns actual layer name of the symlink layer.
func followSymlinkLayer(name string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

This is not robust, although it is ok as the workaround.

Can we verify the expected name with regexp?
\.\./[a-z0-9]+/layer\.tar

Copy link
Member Author

Choose a reason for hiding this comment

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

How about this?

Copy link
Member

Choose a reason for hiding this comment

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

SGTM but please add a comment about this to the source?

Copy link
Contributor

Choose a reason for hiding this comment

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

We already check with isLayerTar in line 126, maybe remove the check there?

Copy link
Member Author

@Random-Liu Random-Liu Apr 10, 2018

Choose a reason for hiding this comment

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

That is check for the initial layer name, here it is checking the symlink target.

Copy link
Member Author

Choose a reason for hiding this comment

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

@AkihiroSuda will do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@Random-Liu Random-Liu force-pushed the fix-symlink-layer branch 2 times, most recently from 63f2737 to ff9827b Compare April 10, 2018 08:25
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

see nit comments

@@ -60,6 +60,22 @@ func isLayerTar(name string) bool {
return slashes == 2 && strings.HasSuffix(name, "/layer.tar")
}

// followSymlinkLayer returns actual layer name of the symlink layer.
// It returns "deadbeeddeadbeef/layer.tar" if the name is like
Copy link
Member

@mikebrow mikebrow Apr 10, 2018

Choose a reason for hiding this comment

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

nit.. the comments sometimes say deadbeeddeadbeef and sometimes say deadbeefdeadbeef replacing these with foo would take less space and hold the same meaning.

0xDEADBEEF holds historical significance as a marker for freed memory in a heap... if you are pointing to deadbeaf it means you have a bad pointer. This was important back in the early c and asm days when we used kernel debuggers to debug our operating systems. When an object was allocated we'd always memset the block to zero.

// followSymlinkLayer returns actual layer name of the symlink layer.
// It returns "deadbeeddeadbeef/layer.tar" if the name is like
// "../deadbeeddeadbeef/layer.tar", and returns error if the name
// is not in "../xxxx/layer.tar" format.
Copy link
Member

Choose a reason for hiding this comment

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

../foo/layer.tar not xxxx unless you want to use xxxx as the replacement for deadbeeddeadbeef & deadbeefdeadbeef strings...

@Random-Liu
Copy link
Member Author

@mikebrow Done.

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

/LGTM

@Random-Liu Random-Liu merged commit b09489d into containerd:master Apr 11, 2018
@Random-Liu Random-Liu deleted the fix-symlink-layer branch April 11, 2018 01:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants