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

k8s CRI Support added to bucketbench #31

Merged
merged 3 commits into from Nov 6, 2017
Merged

Conversation

kunalkushwaha
Copy link
Contributor

This PR is for adding support of CRI interface of k8s to bucketbench.

  • There are few not-so-clear approach in this PR and would like to have review for that too.

//cc @estesp @runcom @Random-Liu

PS: Sorry for this creating this big PR, didn't found any other way to break into smaller one.

@runcom
Copy link

runcom commented Oct 29, 2017

Looks fine overall, ping @estesp

@@ -0,0 +1,54 @@
{
"metadata": {
"name": "powertest",

Choose a reason for hiding this comment

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

s/powertest/bucketbench/ ?

@estesp
Copy link
Owner

estesp commented Oct 30, 2017

Thanks @kunalkushwaha ; will do a complete review over the next day or so. Looks good in general and will answer specific approach questions in review.

Copy link
Owner

@estesp estesp left a comment

Choose a reason for hiding this comment

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

All comments are fairly minor. Thanks, it looks good overall @kunalkushwaha!

I'm thinking maybe binary entry in YAML should change to clientpath? That way overloading it for binary paths and UNIX socket paths doesn't seem to crazy as both fit the description of "client path", whether a direct binary for drivers that use it, or socket path for gRPC-based drivers. What do you think?

"security.alpha.kubernetes.io/seccomp/pod": "unconfined"
},
"linux": {
"cgroup_parent": "/Burstable/pod_123-456",
Copy link
Owner

Choose a reason for hiding this comment

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

is this /Burstable/pod_123-456 useful/necessary, or just copied from somewhere else? Maybe it doesn't matter, but would be nice for this to be as "default"/vanilla as can be.

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 copied it from CRI-O 's contrib files. Will try to make it more generic.

@@ -101,6 +101,11 @@ func (c *ContainerdContainer) Detached() bool {
return true
}

//GetPodID return pod-id assosicated with container.
Copy link
Owner

Choose a reason for hiding this comment

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

typo: s/assosicated/associated/

Maybe for all the drivers which will not implement GetPodID add a second sentence to the comment "only used by CRI-based drivers"

@@ -90,6 +90,11 @@ func (c *CtrContainer) Detached() bool {
return true
}

//GetPodID return pod-id assosicated with container.
Copy link
Owner

Choose a reason for hiding this comment

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

same typo as the containerd driver :) Also, same comment about noting this is stubbed out only for use with CRI drivers

driver/cri.go Outdated
)

const (
defaultPodImage = "docker.io/ibmcom/pause:3.0"
Copy link
Owner

Choose a reason for hiding this comment

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

Update to the gcr.io pause image reference per @runcom's comment?

driver/cri.go Outdated
const (
defaultPodImage = "docker.io/ibmcom/pause:3.0"
defaultPodNamePrefix = "pod"
defaultSanboxConfig = "contrib/sandbox_config.json"
Copy link
Owner

Choose a reason for hiding this comment

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

typo: s/defaultSanboxConfig/defaultSandboxConfig

driver/cri.go Outdated
runtimeClient := pb.NewRuntimeServiceClient(conn)
imageClient := pb.NewImageServiceClient(conn)

pconfig, err := loadPodSandboxConfig(defaultSanboxConfig)
Copy link
Owner

Choose a reason for hiding this comment

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

s/defaultSanboxConfig/defaultSandboxConfig

driver/docker.go Outdated
@@ -82,6 +82,11 @@ func (c *DockerContainer) Command() string {
return c.cmdOverride
}

//GetPodID return pod-id assosicated with container.
Copy link
Owner

Choose a reason for hiding this comment

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

same typo as other drivers; and same comment about unused outside of CRI implementation

driver/driver.go Outdated
@@ -19,6 +19,8 @@ const (
// Null driver represents an empty driver for use by benchmarks that
// require no driver
Null
//CRI driver represents k8s Container Runtime Interface
CRI
Copy link
Owner

Choose a reason for hiding this comment

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

can you add this above Null so that Null is the last driver in the list? Not a big deal, but.. :)

driver/driver.go Outdated
@@ -95,6 +100,8 @@ func New(dtype Type, path string) (Driver, error) {
return NewCtrDriver(path)
case Null:
return nil, nil
case CRI:
Copy link
Owner

Choose a reason for hiding this comment

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

same comment as above re: Null being the last entry

driver/runc.go Outdated
@@ -91,6 +91,11 @@ func (c *RuncContainer) State() string {
return c.state
}

//GetPodID return pod-id assosicated with container.
Copy link
Owner

Choose a reason for hiding this comment

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

same as other drivers: typo and comment about unused :)

- Support for CRI driver type added to helper functions
  of Driver Interface

Signed-off-by: Kunal Kushwaha <kushwaha_kunal_v7@lab.ntt.co.jp>
- config file added for CRI
- Examples yaml file added for CRI

Signed-off-by: Kunal Kushwaha <kushwaha_kunal_v7@lab.ntt.co.jp>
Signed-off-by: Kunal Kushwaha <kushwaha_kunal_v7@lab.ntt.co.jp>
@kunalkushwaha
Copy link
Contributor Author

clientpath will be perfect :).

Have incorporated all suggestion with this update. Please check once more.

@estesp
Copy link
Owner

estesp commented Nov 6, 2017

LGTM

Merging as the tip gofmt is not a requirement; going to switch travis to only test 1.9.x with a separate PR

@estesp estesp merged commit 24ab741 into estesp:master Nov 6, 2017
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.

None yet

4 participants