Add other small functions #53

Random-Liu · 2017-05-26T02:28:43Z

Add Version, UpdateRuntimeConfig and Status.

Based on #41, #46, #48 and #52. Only last 2 commits are new.

We could basically achieve our alpha goal after this PR is merged, and more tests are needed after code freeze. @yujuhong @dchen1107 @mikebrow
/cc @abgworrall @matchstick

With this PR, we could pass half CRI validation test now:

Ran 36 of 36 Specs in 93.276 seconds
FAIL! -- 18 Passed | 18 Failed | 0 Pending | 0 Skipped --- FAIL: TestE2ECRI (93.28s)
FAIL

Ginkgo ran 1 suite in 1m33.350842999s
Test Suite Failed
exit status 1

Failed tests are caused by:

Container logging
Exec
Other unsupported features: HostPort, Schema 1 image support etc.

yujuhong · 2017-05-30T21:11:27Z

pkg/server/status.go

+	}
+	networkReady := &runtime.RuntimeCondition{
+		Type:   runtime.NetworkReady,
+		Status: true,


Add a TODO to check whether the plugins are ready or they exist? I didn't review the networking code, so this may not make sense.

cri-containerd doesn't support cider allocation now (we are directly using cri-o cni plugin package now).

The networking team is also planing to deprecate kubenet. In the future, the cider should be handled by a daemonset outside of the container.

/cc @freehan

It doesn't have to be CIDR. Network could be not ready if cri-containerd cannot find any network plugins.

True, will update.

yujuhong · 2017-05-30T21:11:44Z

pkg/server/status.go

 // Status returns the status of the runtime.
 func (c *criContainerdService) Status(ctx context.Context, r *runtime.StatusRequest) (*runtime.StatusResponse, error) {
-	return nil, errors.New("not implemented")
+	runtimeReady := &runtime.RuntimeCondition{


Do we want to check whether containerd is alive? E.g., call containerd version().

Will do in this PR.

… gomock. Signed-off-by: Lantao Liu <[email protected]>

Random-Liu · 2017-05-31T17:36:53Z

@yujuhong Addressed comments.

mikebrow · 2017-05-31T18:03:04Z

pkg/server/status.go

 // Status returns the status of the runtime.
 func (c *criContainerdService) Status(ctx context.Context, r *runtime.StatusRequest) (*runtime.StatusResponse, error) {
-	return nil, errors.New("not implemented")
+	runtimeCondition := &runtime.RuntimeCondition{


maybe check the health of the meta stores? For example, ListPodSandbox()

@mikebrow What do you mean by "meta store"?

that which implements a MetadataStore..

SandboxStore, ImageMetadataStore, ...

Here, it's checking whether containerd is working or not.

mikebrow · 2017-05-31T18:16:31Z

pkg/server/update_runtime_config.go

 // UpdateRuntimeConfig updates the runtime config. Currently only handles podCIDR updates.
 func (c *criContainerdService) UpdateRuntimeConfig(ctx context.Context, r *runtime.UpdateRuntimeConfigRequest) (*runtime.UpdateRuntimeConfigResponse, error) {
-	return nil, errors.New("not implemented")
+	return &runtime.UpdateRuntimeConfigResponse{}, nil


We don't need this function now. In current dockershim, this is used to update pod CIDR, which we don't support now and may not support in the future.
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/dockershim/docker_service.go#L304-L315

We may have a separate daemonset to update the CIDR in the future.

maybe a comment or todo to investigate if / when we need to update.. Returning no error here with no update if they passed in something to be updated could lead the user astray?

mikebrow · 2017-05-31T18:26:24Z

pkg/server/version.go

+	containerdAPIVersion = "0.0.0"
+	containerdVersion    = "0.0.0"
+	// kubeAPIVersion is the api version of kubernetes.
+	kubeAPIVersion = "0.1.0"


Couple questions.. 1) Should / could this be set at build vs being hard coded? 2) Why kubeAPIVersion and not kubelet and / or kubelet/cri?

Copied from dockershim.

This field doesn't make too much sense to me now, so just copied the code.

mikebrow · 2017-05-31T18:29:24Z

pkg/server/version.go

+		// Containerd doesn't return semver because of a bug.
+		// TODO(random-liu): Replace this with version from containerd.
+		RuntimeVersion: containerdVersion,
+		// Containerd doesn't have an api version now.


so containerd isn't going to follow the new docker/moby model for using the date to set the version and let the api version be the more traditional major change tag?

containerd won't use date to set version I think.

They should expose api version, but I think it's to early to them to talk about api version now, given that they are changing api everyday before GA.

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

mikebrow

LGTM

Random-Liu · 2017-05-31T21:31:41Z

@mikebrow Thanks for reiviewing!
@yujuhong Are you ok with the new commits?

yujuhong · 2017-05-31T21:42:41Z

LGTM

Random-Liu · 2017-05-31T21:53:33Z

@yujuhong Thanks for reviewing!

Random-Liu · 2017-05-31T21:54:22Z

Apply LGTM based on #53 (review) and #53 (comment).

…tions Add other small functions

update to golang 1.13.4

k8s-ci-robot added the cncf-cla: yes label May 26, 2017

Random-Liu force-pushed the add-other-small-functions branch from 2bbfe8e to 2259b61 Compare May 26, 2017 02:54

Random-Liu assigned yujuhong and mikebrow and unassigned yujuhong May 26, 2017

Random-Liu force-pushed the add-other-small-functions branch from 2259b61 to 18227aa Compare May 30, 2017 16:51

Random-Liu mentioned this pull request May 30, 2017

Add container logging support. #56

Merged

Random-Liu added this to the v0.1.0-rc1 milestone May 30, 2017

yujuhong reviewed May 30, 2017

View reviewed changes

Update containerd to 2562aca1a3c070bc9f571dec44bfb10993c326d6 and add…

bc5dfda

… gomock. Signed-off-by: Lantao Liu <[email protected]>

Random-Liu force-pushed the add-other-small-functions branch 2 times, most recently from 026f5a1 to be565d8 Compare May 31, 2017 17:35

mikebrow reviewed May 31, 2017

View reviewed changes

Random-Liu added 2 commits May 31, 2017 19:11

Add Version, UpdateRuntimeConfig and Status.

7c1a4c1

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

Add unit test.

2df96e1

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

Random-Liu force-pushed the add-other-small-functions branch from be565d8 to 2df96e1 Compare May 31, 2017 19:12

mikebrow approved these changes May 31, 2017

View reviewed changes

Random-Liu added the lgtm label May 31, 2017

Random-Liu merged commit a4e067c into containerd:master May 31, 2017

Random-Liu deleted the add-other-small-functions branch May 31, 2017 21:54

lanchongyizu pushed a commit to lanchongyizu/cri-containerd that referenced this pull request Sep 3, 2017

Merge pull request containerd#53 from Random-Liu/add-other-small-func…

5a1f5ea

…tions Add other small functions

jstarks pushed a commit to jstarks/cri that referenced this pull request Jan 30, 2020

Merge pull request containerd#53 from katiewasnothere/golang1.13.4

de5f0a2

update to golang 1.13.4

Add other small functions #53

Add other small functions #53

Uh oh!

Conversation

Random-Liu commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu commented May 31, 2017

Uh oh!

mikebrow May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebrow May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Random-Liu commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yujuhong commented May 31, 2017

Uh oh!

Random-Liu commented May 31, 2017

Uh oh!

Random-Liu commented May 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Random-Liu commented May 26, 2017 •

edited

Loading

mikebrow May 31, 2017 •

edited

Loading

Random-Liu May 31, 2017 •

edited

Loading

mikebrow May 31, 2017 •

edited

Loading

Random-Liu May 31, 2017 •

edited

Loading

Random-Liu commented May 31, 2017 •

edited

Loading