Skip to content

Conversation

@runcom
Copy link
Contributor

@runcom runcom commented Jul 20, 2016

Implements a basic and raw pull image functionality (that works though). I left a lot of TODO(s) in the code just to remind us what still needs to be fixed/changed - mainly containers/storage integration and some other stuff with containers/image.

# make sure /var/lib/ocid exists and you can write there (well, ocid can write there)

$ ./ocid &

# we need a way from k8s to tell us more about the image
# in ocid we can just assume is a docker:// image always for now
# btw, I put this on the client just to show this off

# with the above I mean "docker://" should be provided by the Kubelet API
$ ./ocic pullimage docker://busybox

$ ls -l /var/lib/ocid/images/ 
total 4
drwxr-xr-x. 2 amurdaca amurdaca 4096 Jul 20 10:42 busybox:latest

$ ls -l /var/lib/ocid/images/busybox:latest 
total 660
-rw-rw-r--. 1 amurdaca amurdaca   1459 Jul 20 10:42 2b8fd9751c4c0f5dd266fcae00707e67a2545ef34f9a29354585f93dac906749.tar
-rw-rw-r--. 1 amurdaca amurdaca 667590 Jul 20 10:42 8ddc19f16526912237dd8af81971d5e4dd0587907234be2b83e249518d5b673f.tar
-rw-r--r--. 1 amurdaca amurdaca    505 Jul 20 10:42 manifest.json

Signed-off-by: Antonio Murdaca runcom@redhat.com

@runcom
Copy link
Contributor Author

runcom commented Jul 20, 2016

/cc @nalind @mrunalp @mtrmac

Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@runcom
Copy link
Contributor Author

runcom commented Jul 20, 2016

This PR is based on #13 to fix the initial Godep imports

Last commit pulls in containers/image dep (I'll squash it with the second commit to not break git bisect). A notable thing about this is that it pulls in "k8s.io/kubernetes" because in containers/image we import k8s with that path while mrunalp/ocid import k8s as "github.com/kubernetes/kubernetes"

Please review individual commits

server/image.go Outdated
if err != nil {
return nil, err
}
src, err := tr.NewImageSource("", false)
Copy link

Choose a reason for hiding this comment

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

This disables TLS verification! We need that either hardcoded-on, or figure out a configuration story.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's just a POC to describe the raw flow.. I'll update accordingly after we figure that out..

Copy link

Choose a reason for hiding this comment

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

At least file an issue in ocid or add a loud comment to the code, then, please; we will all forget about this line comment in a day or two.

Copy link
Owner

Choose a reason for hiding this comment

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

We can add TODOs and create issues on the repo to make sure that it is addressed.

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'll open some issues later this evening (as well as adding some other todo)

@mtrmac
Copy link

mtrmac commented Jul 20, 2016

Overall I’d prefer to move the skopeo copy.go core into containers/image, have ocid just prepare the source and destination containers/image/types.ImageReference values, and then call into the copy library, instead of copy&pasting and separately maintaining the 60 or so lines (which will be much more if we decide to do any kinds of format conversion).

@mtrmac
Copy link

mtrmac commented Jul 20, 2016

… all of which will require building a bit more infrastructure in containers/image, like the ImageContext and context.Context. So this PR does work as a POC / prototype implementation, I just hope that it is not the final state.

@runcom
Copy link
Contributor Author

runcom commented Jul 20, 2016

@mtrmac it isn't, I'm working for the imagecontext story and we'll figure out copy inside containers/image

@mrunalp
Copy link
Owner

mrunalp commented Jul 20, 2016

@mtrmac We are making progress on the bits that we know. None of this is expected to be final state but will continue evolving. Working on these bits will help us identify what is missing in libraries and upstream so we fix things there and improve the integration here.

@runcom
Copy link
Contributor Author

runcom commented Jul 21, 2016

addressed @mtrmac's comments - added some other TODOs and explanations (now back to containers/image)


err = PullImage(client, context.Args().Get(0))
if err != nil {
return fmt.Errorf("pulling image failed failed: %v", err)
Copy link
Owner

Choose a reason for hiding this comment

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

s/failed failed/failed/

Copy link
Contributor Author

@runcom runcom Jul 21, 2016

Choose a reason for hiding this comment

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

fixed - wanted to be clear about the fact the operation failed 😄 (joking)

Copy link
Owner

Choose a reason for hiding this comment

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

Heh :laugh:

runcom added 2 commits July 21, 2016 18:49
Signed-off-by: Antonio Murdaca <runcom@redhat.com>
Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@mrunalp
Copy link
Owner

mrunalp commented Jul 21, 2016

Tested. LGTM.

@mrunalp mrunalp merged commit 5e643c1 into mrunalp:master Jul 21, 2016
@runcom runcom deleted the raw-pullimage branch July 21, 2016 17:49
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.

3 participants