Proxy initial commit by bergwolf · Pull Request #1 · kata-containers/proxy

bergwolf · 2017-11-15T15:26:29Z

No description provided.

jodh-intel · 2017-11-16T09:14:08Z

.travis.yml

+  - sudo apt-get install -y -qq automake
+
+install:
+  - cd ${TRAVIS_BUILD_DIR}


Nit: for consistency with the script section below, you could do this on one line using &&.

jodh-intel · 2017-11-16T09:22:58Z

Makefile

+	go build proxy.go
+	make -C test
+
+test: all


This is slightly unusual in that it's the "other way round" to normal (atleast in my mind ;). It might be clearer to have:

all: build test build: go build proxy.go test: make -C test test

I would prefer make command w/o arguments to just build the project rather than running tests altogether. That is also how most Makefiles work.

Ah - yes, sorry. It hadn't registered that all is the default rule. I guess it's fine as-is or you could make it:

default: build all: default test

jodh-intel · 2017-11-16T09:25:06Z

Makefile

@@ -0,0 +1,10 @@
+all:


Just a general comment - could you add standard copyright headers to all the files? I've just noticed that https://github.com/clearcontainers/proxy/blob/master/Makefile is actually missing one, but it should really match the comment header here for example: https://github.com/clearcontainers/runtime/blob/master/Makefile.

Do we need to have apache 2 licensing statement in every file? There is already an apche LICENSE file in the top directory and I don't think we will use other licenses in the project, right?

OTOH, if it is an Intel open source project policy, I'm willing to follow it.

To add to that - I'll note that we are looking at using 'SPDX' style license headers for these new projects - a much smaller and cleaner method than placing the full or part license text in each file.
For reference:
https://github.com/kata-containers/ksm-throttler/pull/1/files#diff-360cbfd0e0c83e21065226e59696f652R5
https://spdx.org/using-spdx

SPDX sounds good to me.

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

proxy.go

+	"github.com/hashicorp/yamux"
+)
+
+// @channel is the unix socket address we want to multiplex


Looks like this comment might have become detached from the variable it describes (in main()?)

Yep, it was left over from previous versions. I'll just drop it.

Great - thanks.

jodh-intel · 2017-11-16T09:27:30Z

proxy.go

+	"net"
+	"sync"
+
+	"github.com/golang/glog"


It might be worth looking at https://github.com/sirupsen/logrus as a very powerful alternative to glog which we're using in all projects currently.

All hyper projects use glog instead. We should make a decision on which log system to use for kata and migrate to it.

Agreed. What's your view @sameo, @grahamwhaley? Do we need to document this somewhere maybe?

I've taken the question to the slack channel. Let's discuss it there.

jodh-intel · 2017-11-16T09:31:39Z

proxy.go

+
+func main() {
+	channel := flag.String("s", "/tmp/target.sock", "unix socket to multiplex on")
+	proxyAddr := flag.String("l", "/tmp/proxy.sock", "unix socket to listen at")


Do we need these default paths? Also, it would be useful if the options accepted full URIs here (unix:///foo/bar.sock).

Good point, I'll add URI support here. And I agree that default paths are useless.

jodh-intel · 2017-11-16T09:34:41Z

test/test-cmd.sh

+pkill server
+pkill proxy
+
+rm -f /tmp/proxy.sock /tmp/target.sock


It might be better to define a variable for these sockets to avoid the duplication.

jodh-intel · 2017-11-16T09:35:11Z

test/test-cmd.sh

+pkill proxy
+
+rm -f /tmp/proxy.sock /tmp/target.sock
+echo test result is $?


I think this line is redundant as it will only return the value of the rm?

I was using it as a indicator that the test is done because otherwise there is no output. But you do have a valid point that $? is redundant here.

jodh-intel · 2017-11-16T09:38:47Z

test/test-cmd.sh

+# start server
+./server &
+# sleep a bit to let server spin up
+sleep 2


This should certainly be sufficient time for the server to start but at some point, it would be better long-term if we had a shell function that performs a check repeatedly (but with a timeout) here (and below).

I tend to think that busy pulling with a timeout is a bit over-skill just for this little test script.

Yes, it's a very simple script at present, so I don't think this is a "blocker" to get this landed. It's just something we need to think about going forward as the script grows to avoid odd failures on (very ;) slow systems.

Overall the proxy program will be very simple & small. But yeah, who knows ;)

Yeah, we have seen problems with fixed timeouts (even quite large timeouts) when running CI tests on cloud based VM machines - so we've become wary of fixed timeouts and try to avoid them when we can. Flaky CI failures can become the bane of our lives otherwise :-)

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

test/test-cmd.sh

+		output=$(./client -f ${f})
+		result=$(echo ${output}|grep SUCCESS 2>/dev/null || true)
+		if [ x"${result}" == "x" ]; then
+			echo test failed with ${output}


Maybe add >&2 to denote an error message?

The error message from grep is useless. We just need to know if grep succeeds or not.

Signed-off-by: Peng Tao <bergwolf@gmail.com>

bergwolf · 2017-11-20T11:14:24Z

updated and CI green. @jodh-intel @grahamwhaley PTAL.

Signed-off-by: Peng Tao <bergwolf@gmail.com>

sameo · 2017-11-27T15:44:58Z

@bergwolf I assume you're going to rebase this PR based on the final gRPC definition? Or do you want us to give it a final review and merge it without gRPC support first?

bergwolf · 2017-11-27T15:51:51Z

@sameo please review it as is. The proxy does not rely on gRPC support.

sameo · 2017-11-27T21:11:59Z

@bergwolf ah right, your implementation copies all frames coming from the shim into the yamux endpoint, so it's not a gRPC server itself. I guess doing otherwise would be more secure but also much more complex.

sameo

Overall, this looks pretty good. I just have a few minor comments.
One missing piece here are unit tests. The 2 test programs could be implemented as unit tests, which would be easier to use and CI.

sameo · 2017-11-27T21:13:07Z

Gopkg.toml

+  name = "github.com/golang/glog"
+
+[[constraint]]
+  branch = "master"


Could we stick to a specific revision/SHA1?

sameo · 2017-11-27T21:30:29Z

proxy.go

+
+func main() {
+	var channel, proxyAddr string
+	flag.StringVar(&channel, "s", "", "unix socket to multiplex on")


Could we give a clearer name for that option? -mux-socket for example.

sameo · 2017-11-27T21:31:22Z

proxy.go

+func main() {
+	var channel, proxyAddr string
+	flag.StringVar(&channel, "s", "", "unix socket to multiplex on")
+	flag.StringVar(&proxyAddr, "l", "", "unix socket to listen on")


Ditto. -shim-socket or -listen-socket

sameo · 2017-11-27T21:41:34Z

proxy.go

+	"github.com/hashicorp/yamux"
+)
+
+func serv(servConn io.ReadWriteCloser, proto, addr string) error {


nit: serve instead of serv?

sameo · 2017-11-27T21:42:58Z

proxy.go

+		glog.Error("channel and proxy address must be set")
+		return
+	}
+


This block should be the first check we run (before the prefix/scheme ones).

sameo · 2017-11-27T21:45:23Z

proxy.go

+	if strings.HasPrefix(proxyAddr, unixURI) {
+		proxyAddr = proxyAddr[len(unixURI):]
+	}
+


I think you want to use the net/url to safely parse (url.Parse()) the URI you get as an input and verify that we either have a unix scheme or no scheme at all.

sameo · 2017-11-27T22:05:59Z

proxy.go

+			glog.Errorf("fail to accept new connection: %s", err)
+			return err
+		}
+		stream, err := session.Open()


Could you please put the whole stream opening, stream copy and sync logic into a dedicated function and call a go routine on it for each accept? I think this would make the code more readable.

WeiZhang555 · 2017-11-29T04:11:51Z

Oh, please, logrus 👍
glog 👎

I can see some glog lost log problems due to it's buffer/cache, prefer "logrus" much more

Signed-off-by: Peng Tao <bergwolf@gmail.com>

Replace test dir with go testing UT. Signed-off-by: Peng Tao <bergwolf@gmail.com>

In future we'll use a wrapper above logrus to print glog format logs. Signed-off-by: Peng Tao <bergwolf@gmail.com>

Signed-off-by: Peng Tao <bergwolf@gmail.com>

bergwolf · 2017-11-29T08:24:15Z

updated to address @sameo's comments and added unit tests. glog is also replaced with logrus. We'll add a wrapper above logrus to print glog format logs when need.

sameo · 2017-11-29T09:13:23Z

@bergwolf Thanks for the update.
LGTM

do no merge - only to check move to go 1.10 works note: make issue number Fixes: kata-containers#1 Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>

Fixes: kata-containers#57 Signed-off-by: Kaly Xin <Kaly.Xin@arm.com>

Fixes kata-containers#57 Signed-off-by: Kaly Xin <Kaly.Xin@arm.com>

Add a fake fixes line as well ;-) Fixes: kata-containers#1 Signed-off-by: Graham Whaley <graham.whaley@intel.com>

bergwolf closed this Nov 15, 2017

bergwolf reopened this Nov 15, 2017

bergwolf force-pushed the proxy branch from 5a4554f to b7baddf Compare November 15, 2017 15:43

jodh-intel reviewed Nov 16, 2017

View reviewed changes

bergwolf added 2 commits November 20, 2017 18:39

initial commit

94a07ff

Signed-off-by: Peng Tao <bergwolf@gmail.com>

support unix socket URI address

d0c966f

Signed-off-by: Peng Tao <bergwolf@gmail.com>

bergwolf force-pushed the proxy branch 3 times, most recently from 5dab6ae to 1234db5 Compare November 20, 2017 11:10

bergwolf added 4 commits November 27, 2017 16:02

add vendor

f2a3618

Signed-off-by: Peng Tao <bergwolf@gmail.com>

add travis ci

19fedc7

Signed-off-by: Peng Tao <bergwolf@gmail.com>

add SPDX license footage

2428453

Signed-off-by: Peng Tao <bergwolf@gmail.com>

add gitignore

860774a

Signed-off-by: Peng Tao <bergwolf@gmail.com>

bergwolf force-pushed the proxy branch from 1234db5 to 860774a Compare November 27, 2017 08:06

sameo reviewed Nov 27, 2017

View reviewed changes

bergwolf added 2 commits November 29, 2017 08:05

address comments

adb61b7

Signed-off-by: Peng Tao <bergwolf@gmail.com>

add unit tests

b00c30c

Replace test dir with go testing UT. Signed-off-by: Peng Tao <bergwolf@gmail.com>

bergwolf force-pushed the proxy branch from 6411fe2 to b00c30c Compare November 29, 2017 08:06

bergwolf added 2 commits November 29, 2017 16:17

change glog to logrus

d7d955d

In future we'll use a wrapper above logrus to print glog format logs. Signed-off-by: Peng Tao <bergwolf@gmail.com>

update vendor

a1cd9a9

Signed-off-by: Peng Tao <bergwolf@gmail.com>

sameo merged commit 4520f46 into kata-containers:master Nov 29, 2017

jcvenegas mentioned this pull request Apr 25, 2018

ci: move go 1.10 #68

Closed

kalyxin02 added a commit to kalyxin02/proxy that referenced this pull request Aug 3, 2018

Fire a test pr for Arm jenkins test kata-containers#1

fa4cc70

Fixes: kata-containers#57 Signed-off-by: Kaly Xin <Kaly.Xin@arm.com>

kalyxin02 added a commit to kalyxin02/proxy that referenced this pull request Aug 8, 2018

Fire a test pr for Arm jenkins test kata-containers#1

46f54ff

Fixes kata-containers#57 Signed-off-by: Kaly Xin <Kaly.Xin@arm.com>

grahamwhaley pushed a commit to grahamwhaley/kata-containers-proxy that referenced this pull request Mar 19, 2019

CI: zuul: add fake ZUUL.md to check Zuul CI

89c8bf8

Add a fake fixes line as well ;-) Fixes: kata-containers#1 Signed-off-by: Graham Whaley <graham.whaley@intel.com>

Conversation

bergwolf commented Nov 15, 2017

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!

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!

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!

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!

bergwolf commented Nov 20, 2017

Uh oh!

sameo commented Nov 27, 2017

Uh oh!

bergwolf commented Nov 27, 2017

Uh oh!

sameo commented Nov 27, 2017

Uh oh!

sameo left a comment

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!

WeiZhang555 commented Nov 29, 2017 •

edited

Loading