Initial push #1

sameo · 2017-11-15T13:16:59Z

Mostly a refactor of the @clearcontainers/proxy KSM code, with:

Initial gRPC protocol defined
gRPC service implemented

TODO

grahamwhaley

As an initial import, lgtm
Minor queries, and obviously we'll grow docs and build/install instr etc.

grahamwhaley · 2017-11-15T13:35:06Z

pkg/grpc/ksm.proto

+
+import "google/protobuf/empty.proto";
+
+// To generate hyperstart.pb.go, run:


hyperstart.pb.go? - presume that should be ksm.pb.go ?

grahamwhaley · 2017-11-15T13:36:28Z

pkg/grpc/ksm.proto

@@ -0,0 +1,14 @@
+syntax = "proto3";
+
+package ksm;


nit: early days - but I presume we will add a high level description in here.
I only notice as I think there is a place in ksm.pb.go where it would be copied to on generation.

grahamwhaley · 2017-11-15T13:40:36Z

ksm.go

@@ -0,0 +1,481 @@
+//
+// Copyright (c) 2017 Intel Corporation


A thought, as this is effectively a new project - maybe we should start to use the SPDX copyright commenting method rather than the full blown long winded one?
https://spdx.org/licenses/
https://patchwork.kernel.org/patch/10016189/

It is becoming more common and accepted.

Only Kick for now. Signed-off-by: Samuel Ortiz <[email protected]>

Mostly copy pasted from clearcontainers/proxy Signed-off-by: Samuel Ortiz <[email protected]>

The throttler creates the gRPC service on a UNIX socket, and kicks the KSM throttling routine when receiving the Kick command. Signed-off-by: Samuel Ortiz <[email protected]>

Signed-off-by: Samuel Ortiz <[email protected]>

With locked revisions. Signed-off-by: Samuel Ortiz <[email protected]>

Signed-off-by: Samuel Ortiz <[email protected]>

Again, copied from the original proxy KSM throttling implemenation. Signed-off-by: Samuel Ortiz <[email protected]>

Signed-off-by: Samuel Ortiz <[email protected]>

coveralls · 2017-11-15T14:51:27Z

Changes Unknown when pulling a8ba9e3 on sameo:topic/initial-push into ** on kata-containers:master**.

Signed-off-by: Samuel Ortiz <[email protected]>

jodh-intel

lgtm.

jodh-intel · 2017-11-16T09:03:26Z

.ci/go-lint.sh

+# limitations under the License.
+#
+
+#!/bin/bash


This line is redundant as it's isn't the first - same comment applies to go-test.sh.

jodh-intel · 2017-11-16T09:05:40Z

.travis.yml

+go_import_path: github.com/kata-containers/ksm-throttler
+
+before_install:
+- go get github.com/mattn/goveralls


Just an idea, but how about trying out codecov.io instead? clearcontainers/jenkins#28 Or we could run both and see how they compare?

For now it is just a wrapper around the Kick gRPC method. Signed-off-by: Samuel Ortiz <[email protected]>

kicker only sends a Kick() call and returns. Signed-off-by: Samuel Ortiz <[email protected]>

Let's give it a try. Signed-off-by: Samuel Ortiz <[email protected]>

throttler.go:143: arg logLevel for printf verb %s of wrong type: *string Signed-off-by: Samuel Ortiz <[email protected]>

Signed-off-by: Samuel Ortiz <[email protected]>

codecov · 2017-11-16T10:03:26Z

Codecov Report

❗ No coverage uploaded for pull request base (master@fea7374). Click here to learn what that means.
The diff coverage is 47.16%.

@@            Coverage Diff            @@
##             master       #1   +/-   ##
=========================================
  Coverage          ?   47.16%           
=========================================
  Files             ?        2           
  Lines             ?      405           
  Branches          ?        0           
=========================================
  Hits              ?      191           
  Misses            ?      175           
  Partials          ?       39

Impacted Files	Coverage Δ
ksm.go	`59.1% <59.1%> (ø)`
throttler.go	`6.52% <6.52%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fea7374...a4008f2. Read the comment docs.

coveralls · 2017-11-16T10:04:06Z

Changes Unknown when pulling 90dece9 on sameo:topic/initial-push into ** on kata-containers:master**.

coveralls · 2017-11-16T10:04:06Z

Changes Unknown when pulling 90dece9 on sameo:topic/initial-push into ** on kata-containers:master**.

coveralls · 2017-11-16T10:04:06Z

Changes Unknown when pulling 90dece9 on sameo:topic/initial-push into ** on kata-containers:master**.

coveralls · 2017-11-16T10:05:21Z

Changes Unknown when pulling d86c1cf on sameo:topic/initial-push into ** on kata-containers:master**.

coveralls · 2017-11-16T14:26:01Z

Changes Unknown when pulling 8e8d3b7 on sameo:topic/initial-push into ** on kata-containers:master**.

coveralls · 2017-11-16T14:26:02Z

Changes Unknown when pulling 8e8d3b7 on sameo:topic/initial-push into ** on kata-containers:master**.

coveralls · 2017-11-16T14:31:12Z

Changes Unknown when pulling 6ea86f1 on sameo:topic/initial-push into ** on kata-containers:master**.

coveralls · 2017-11-16T14:31:13Z

Changes Unknown when pulling 6ea86f1 on sameo:topic/initial-push into ** on kata-containers:master**.

jodh-intel · 2017-11-16T16:57:37Z

.ci/setup.sh

+set -e
+
+cidir=$(dirname "$0")
+tests_repo="github.com/clearcontainers/tests"


Presumably this is just a temporary URL until we populate https://github.com/kata-containers/tests?

jodh-intel · 2017-11-16T16:59:33Z

.pullapprove.yml

+  title_regex: 'WIP'
+  labels:
+    - do-not-merge
+    - wip


FYI: I've just added these labels.

jodh-intel · 2017-11-16T17:04:39Z

Makefile

@@ -0,0 +1,122 @@
+VERSION := 0.1+
+
+PACKAGE       = github.com/kata-containers/ksm-throttler


The github URL is used in a lot of places. It's not a blocker for this PR of course, but I wonder if we could standardise on something like .ci/self-url that container this string and then have the Makefile read that file. make can then generate the value used in ksm-throttler.service.in, and so on.

Signed-off-by: Samuel Ortiz <[email protected]>

We don't need it for now. Signed-off-by: Samuel Ortiz <[email protected]>

coveralls · 2017-11-16T18:14:15Z

Changes Unknown when pulling 8f30bc1 on sameo:topic/initial-push into ** on kata-containers:master**.

coveralls · 2017-11-16T18:14:15Z

Changes Unknown when pulling 8f30bc1 on sameo:topic/initial-push into ** on kata-containers:master**.

coveralls · 2017-11-16T18:14:15Z

Changes Unknown when pulling 8f30bc1 on sameo:topic/initial-push into ** on kata-containers:master**.

coveralls · 2017-11-16T18:19:39Z

Changes Unknown when pulling b79782c on sameo:topic/initial-push into ** on kata-containers:master**.

sboeuf · 2017-11-16T18:46:07Z

.ci/go-lint.sh

@@ -0,0 +1,46 @@
+# Copyright (c) 2017 Intel Corporation


Do we want to keep that ?

sboeuf · 2017-11-16T18:56:04Z

@sameo that's a nice and huge first commit ! Even if there are some flaws (I am not saying there are some :) ), we can manage this later in follow up pull requests.
LGTM

sameo · 2017-11-16T19:05:19Z

This is complete now.

Signed-off-by: Samuel Ortiz <[email protected]>

coveralls · 2017-11-16T19:14:10Z

Changes Unknown when pulling f446d8d on sameo:topic/initial-push into ** on kata-containers:master**.

coveralls · 2017-11-16T19:14:12Z

Changes Unknown when pulling a4008f2 on sameo:topic/initial-push into ** on kata-containers:master**.

coveralls · 2017-11-16T19:14:12Z

Changes Unknown when pulling a4008f2 on sameo:topic/initial-push into ** on kata-containers:master**.

coveralls · 2017-11-16T19:14:12Z

Changes Unknown when pulling a4008f2 on sameo:topic/initial-push into ** on kata-containers:master**.

sboeuf · 2017-11-16T19:15:17Z

@sameo you're gonna have to approve your own PR since it introduces the proxy team as approvers too.

sameo · 2017-11-16T22:04:42Z

LGTM

sameo requested review from grahamwhaley, mcastelino, jodh-intel and sboeuf November 15, 2017 13:18

grahamwhaley approved these changes Nov 15, 2017

View reviewed changes

sameo force-pushed the topic/initial-push branch from bdaacd2 to 1f6c0c1 Compare November 15, 2017 14:08

Samuel Ortiz added 5 commits November 15, 2017 15:17

grpc: Initial protobuf description

8c83a88

Only Kick for now. Signed-off-by: Samuel Ortiz <[email protected]>

throttler: Initial code split

89d9804

Mostly copy pasted from clearcontainers/proxy Signed-off-by: Samuel Ortiz <[email protected]>

throttler: Initial gRPC server implementation

21084a1

The throttler creates the gRPC service on a UNIX socket, and kicks the KSM throttling routine when receiving the Kick command. Signed-off-by: Samuel Ortiz <[email protected]>

.travis: Initial Travis CI scripts

81c8bf2

Signed-off-by: Samuel Ortiz <[email protected]>

vendor: Initial vendoring

05d2b5a

With locked revisions. Signed-off-by: Samuel Ortiz <[email protected]>

sameo force-pushed the topic/initial-push branch from 1f6c0c1 to 05d2b5a Compare November 15, 2017 14:26

Samuel Ortiz added 3 commits November 15, 2017 15:29

travis: Only test on the last 2 Golang releases

dcaf3ef

Signed-off-by: Samuel Ortiz <[email protected]>

ksm: Add unit tests

89cf6d1

Again, copied from the original proxy KSM throttling implemenation. Signed-off-by: Samuel Ortiz <[email protected]>

README: Add coveralls, travis, goreport and godoc badges

a8ba9e3

Signed-off-by: Samuel Ortiz <[email protected]>

Samuel Ortiz added 3 commits November 15, 2017 16:23

client: Initial client API

c18cf70

Signed-off-by: Samuel Ortiz <[email protected]>

throttler: Factorize the server listener

47d21ef

Signed-off-by: Samuel Ortiz <[email protected]>

throttler: Add uri, version and log command line options

ac6e1e9

Signed-off-by: Samuel Ortiz <[email protected]>

jodh-intel approved these changes Nov 16, 2017

View reviewed changes

Samuel Ortiz added 5 commits November 16, 2017 10:41

client: Add client package

3eee0da

For now it is just a wrapper around the Kick gRPC method. Signed-off-by: Samuel Ortiz <[email protected]>

input: Add kicker

b9ef158

kicker only sends a Kick() call and returns. Signed-off-by: Samuel Ortiz <[email protected]>

travis: Add codecov support

371788c

Let's give it a try. Signed-off-by: Samuel Ortiz <[email protected]>

throttler: Fix govet issue

90dece9

throttler.go:143: arg logLevel for printf verb %s of wrong type: *string Signed-off-by: Samuel Ortiz <[email protected]>

ci: Fix copyright headers and bash line

d86c1cf

Signed-off-by: Samuel Ortiz <[email protected]>

sameo force-pushed the topic/initial-push branch 4 times, most recently from dd1357c to 692db52 Compare November 16, 2017 16:46

sameo changed the title ~~[WIP] Initial push~~ Initial push Nov 16, 2017

jodh-intel approved these changes Nov 16, 2017

View reviewed changes

Makefile: Pass proxy socket and install systemd service files

8f30bc1

Signed-off-by: Samuel Ortiz <[email protected]>

sameo force-pushed the topic/initial-push branch from 692db52 to 8f30bc1 Compare November 16, 2017 18:09

ci: Remove setup.sh

b79782c

We don't need it for now. Signed-off-by: Samuel Ortiz <[email protected]>

sboeuf reviewed Nov 16, 2017

View reviewed changes

.ci/go-lint.sh

@@ -0,0 +1,46 @@

# Copyright (c) 2017 Intel Corporation

Copy link

sboeuf Nov 16, 2017

Choose a reason for hiding this comment

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

Do we want to keep that ?

Samuel Ortiz added 2 commits November 16, 2017 20:08

README: Fill with architectural overall and build instructions

3576587

Signed-off-by: Samuel Ortiz <[email protected]>

input: Rename to trigger

a4008f2

Signed-off-by: Samuel Ortiz <[email protected]>

sameo force-pushed the topic/initial-push branch from f446d8d to a4008f2 Compare November 16, 2017 19:08

sameo merged commit 74cca56 into kata-containers:master Nov 16, 2017


		import "google/protobuf/empty.proto";

		// To generate hyperstart.pb.go, run:

		@@ -0,0 +1,122 @@
		VERSION := 0.1+

		PACKAGE = github.com/kata-containers/ksm-throttler

Initial push #1

Initial push #1

Conversation

sameo commented Nov 15, 2017 • edited Loading

TODO

grahamwhaley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 15, 2017 • edited Loading

jodh-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 16, 2017 • edited Loading

Codecov Report

coveralls commented Nov 16, 2017

coveralls commented Nov 16, 2017

coveralls commented Nov 16, 2017

coveralls commented Nov 16, 2017 • edited Loading

coveralls commented Nov 16, 2017

coveralls commented Nov 16, 2017 • edited Loading

coveralls commented Nov 16, 2017

coveralls commented Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 16, 2017

coveralls commented Nov 16, 2017

coveralls commented Nov 16, 2017

coveralls commented Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

sboeuf commented Nov 16, 2017

sameo commented Nov 16, 2017

coveralls commented Nov 16, 2017 • edited Loading

coveralls commented Nov 16, 2017

coveralls commented Nov 16, 2017

coveralls commented Nov 16, 2017

sboeuf commented Nov 16, 2017

sameo commented Nov 16, 2017

sameo commented Nov 15, 2017 •

edited

Loading

coveralls commented Nov 15, 2017 •

edited

Loading

codecov bot commented Nov 16, 2017 •

edited

Loading

coveralls commented Nov 16, 2017 •

edited

Loading

coveralls commented Nov 16, 2017 •

edited

Loading

coveralls commented Nov 16, 2017 •

edited

Loading

coveralls commented Nov 16, 2017 •

edited

Loading

coveralls commented Nov 16, 2017 •

edited

Loading