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

Get spec from metadata #102

Merged
merged 2 commits into from
Jul 28, 2017

Conversation

Random-Liu
Copy link
Member

Remove spec from our metadata and get spec from containerd container metadata.

This PR also updated cri validation test version to include kubernetes-sigs/cri-tools#115.

@Random-Liu Random-Liu added this to the v0.2.0 milestone Jul 28, 2017
hack/versions Outdated
@@ -1,4 +1,4 @@
RUNC_VERSION=639454475cb9c8b861cc599f8bcd5c8c790ae402
CNI_VERSION=v0.4.0
CONTAINERD_VERSION=8ed1e24ae925b5c6d8195858ee89dddb0507d65f
CRITEST_VERSION=v0.1
CRITEST_VERSION=dbfd7f14a457fc95cf736c5361b244c087cc9688
Copy link
Member

Choose a reason for hiding this comment

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

? Don't see that commit

hack/versions Outdated
@@ -1,4 +1,4 @@
RUNC_VERSION=639454475cb9c8b861cc599f8bcd5c8c790ae402
CNI_VERSION=v0.4.0
CONTAINERD_VERSION=8ed1e24ae925b5c6d8195858ee89dddb0507d65f
CRITEST_VERSION=v0.1
CRITEST_VERSION=dbfd7f14a457fc95cf736c5361b244c087cc9688
Copy link
Member

Choose a reason for hiding this comment

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

Ah the merged commit is .. 74bbd4e142f752f13c648d9dde23defed3e472a2

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, will update.

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.

@@ -55,6 +56,22 @@ func (c *criContainerdService) ExecSync(ctx context.Context, r *runtime.ExecSync
return nil, fmt.Errorf("container %q is in %s state", id, criContainerStateToString(meta.State()))
}

// Get exec process spec.
Copy link
Member

Choose a reason for hiding this comment

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

above comment // Get container config from container store .. should be changed to // Get metadata for container from our container store .. or something like that

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't quite get it.

Copy link
Member

Choose a reason for hiding this comment

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

see the comment above for call to get metadata for the container from the container metadata store

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. Will do.

// Get exec process spec.
cntrResp, err := c.containerService.Get(ctx, &containers.GetContainerRequest{ID: id})
if err != nil {
return nil, fmt.Errorf("failed to get container %q from containerd: %v", id, err)
Copy link
Member

Choose a reason for hiding this comment

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

failed to get container struct.. or container information..

Copy link
Member Author

@Random-Liu Random-Liu Jul 28, 2017

Choose a reason for hiding this comment

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

The interface is called containers.Get and GetContainerRequest. :p

Copy link
Member

Choose a reason for hiding this comment

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

right, just trying to be more specific in the error message.. get container doesn't say much. The response includes the container struct that has the spec in it. Was thinking more detail might help down the road.

Copy link
Member

Choose a reason for hiding this comment

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

iow what's the difference between failing to get the container when pulling an image vs failing to get the container here?

Copy link
Member

Choose a reason for hiding this comment

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

ah well the line number is enough.. either way is ok by me 👍

@Random-Liu Random-Liu force-pushed the get-spec-from-metadata branch from f12a4bc to 69f7c52 Compare July 28, 2017 16:21
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 just a couple nits

@Random-Liu Random-Liu force-pushed the get-spec-from-metadata branch from 69f7c52 to d8ce670 Compare July 28, 2017 16:26
@mikebrow mikebrow added the lgtm label Jul 28, 2017
@Random-Liu Random-Liu merged commit e5d69aa into containerd:master Jul 28, 2017
@Random-Liu Random-Liu deleted the get-spec-from-metadata branch July 28, 2017 16:45
lanchongyizu pushed a commit to lanchongyizu/cri-containerd that referenced this pull request Sep 3, 2017
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.

3 participants