FC: Add new vsock connection handshake #706

Pennyzct · 2019-12-27T02:45:42Z

Which feature do you think can be improved?
New Firecracker v0.20.0 has changed the host-initiated vsock connection protocol to include a trivial handshake.

The new protocol looks like this:

    - [host] CONNECT <port><LF>
    - [guest/success] OK <assigned_host_port><LF>

See PR firecracker-microvm/firecracker#1472 for detailed info.

codecov · 2019-12-27T02:51:02Z

Codecov Report

Merging #706 into master will decrease coverage by 0.11%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #706      +/-   ##
==========================================
- Coverage   60.82%   60.71%   -0.12%     
==========================================
  Files          17       17              
  Lines        2550     2558       +8     
==========================================
+ Hits         1551     1553       +2     
- Misses        850      857       +7     
+ Partials      149      148       -1

Pennyzct · 2019-12-27T03:07:11Z

/test

jodh-intel

Thanks @Pennyzct.

lgtm

jodh-intel · 2020-01-02T08:27:25Z

protocols/client/client.go

+			return nil, err
+		} else if !strings.Contains(response, "OK") {
+			conn.Close()
+			return nil, errors.New("malformed response code")


I wonder if it would make sense to log response here if debug is enabled?

Hi~ @jodh-intel yep. And since this file is missing logging part, I've added another commit to fulfill it. ;).

Pennyzct · 2020-01-06T02:51:12Z

Hi guys
FC CI is definitely failing here, since this change is included only in v0.20.0. ;).
I've added another PR to do the FC version bump.

devimc · 2020-01-06T17:26:31Z

@Pennyzct thanks, but please add Depends-on: github.com/kata-containers/runtime#2379 in your commit message, that way we can see the CI using the latest version of firecracker

Pennyzct · 2020-01-08T09:29:07Z

Hi~ @devimc
FWIT, even if I add the Depends-on: github.com/kata-containers/runtime#2379, if we don't do the runtime/ vendor update to include this commit, FC CI will still fail. ;).

Pennyzct · 2020-01-08T09:30:15Z

/test

devimc · 2020-01-08T13:48:21Z

@Pennyzct thanks, now I can see that fc 0.20 is installed

[Hypervisor]
  MachineType = ""
  Version = "firecracker 0.20.0"

unfortunately FC CI is failing

Stderr: docker: Error response from daemon: OCI runtime create failed: Failed to check if grpc server is working: rpc error: code = DeadlineExceeded desc = context deadline exceeded: unknown.

Add changes from kata-containers/agent#706. THIS IS ONLY FOR DEBUGGING! Fixes: kata-containers#2378 Signed-off-by: Penny Zheng <penny.zheng@arm.com>

The new release for Firecracker is `v0.20.0`. Fixes: kata-containers#2378 Depends-on: github.com/kata-containers/agent#706 Signed-off-by: Penny Zheng <penny.zheng@arm.com>

Add changes from kata-containers/agent#706. THIS IS ONLY FOR DEBUGGING! Fixes: kata-containers#2378 Depends-on: github.com/kata-containers/agent#706 Signed-off-by: Penny Zheng <penny.zheng@arm.com>

Add changes from kata-containers/agent#706. THIS IS ONLY FOR DEBUGGING! Signed-off-by: Penny Zheng <penny.zheng@arm.com>

Pennyzct · 2020-01-10T07:32:38Z

Updates:
The debugging part should be only running after already getting right response code, since otherwise we may encounter JSON decode error.

dockerd[16053]: time="2020-01-10T03:34:14.461867307Z" level=error msg="Handler for POST 
/v1.38/containers/7a52caa16c60249ceb09ede8b65a8161a0ad7bfe6c1cc206040456ba9737541b/start 
returned error: OCI runtime create failed: unable to retrieve OCI runtime error (invalid character 'i' in 
literal true (expecting 'r')): /usr/local/bin/kata-runtime did not terminate sucessfully

devimc · 2020-01-10T14:43:11Z

@Pennyzct good catch!

jodh-intel · 2020-01-20T09:11:55Z

/test

sboeuf · 2020-01-22T11:33:20Z

LGTM

@jcvenegas @likebreath I think the right move here is to make sure you update Kata to support latest CH from master. This way, I think the random failures will go away, as the PR is effectively reverting the 3 patches we've been reverting so far (through our testing).

@devimc I think you need to bump FC to 0.20.0 so that it does not get broken when this PR is merged.

devimc · 2020-01-22T14:09:55Z

@sboeuf what do you mean? CI installs firecracker 0.20 and is failing

• Failure [152.692 seconds]
docker kill
/tmp/jenkins/workspace/kata-containers-agent-ubuntu-1804-PR-firecracker/go/src/github.com/kata-containers/tests/integration/docker/kill_test.go:79
  killing container
  /tmp/jenkins/workspace/kata-containers-agent-ubuntu-1804-PR-firecracker/go/src/github.com/kata-containers/tests/vendor/github.com/onsi/ginkgo/extensions/table/table.go:92
    with '22'(stopped (tty output)) signal [It]
    /tmp/jenkins/workspace/kata-containers-agent-ubuntu-1804-PR-firecracker/go/src/github.com/kata-containers/tests/vendor/github.com/onsi/ginkgo/extensions/table/table_entry.go:43

    Expected error:
        <*errors.errorString | 0xc000057150>: {
            s: "Timeout reached after 120s",
        }
        Timeout reached after 120s
    not to have occurred

    /tmp/jenkins/workspace/kata-containers-agent-ubuntu-1804-PR-firecracker/go/src/github.com/kata-containers/tests/integration/docker/kill_test.go:144

sboeuf · 2020-01-22T14:42:49Z

@Pennyzct the PR is failing with latest FC 0.20. Are you still trying to make this pass?

sboeuf · 2020-01-22T14:47:33Z

protocols/client/client.go

+		response, err := reader.ReadString('\n')
+		if err != nil {
+			conn.Close()
+			return nil, err


Try removing this line, this way the function will return conn, nil and the GRPC code will handle dialing again.

I'm confused, if reading was failing, we should return error instantly. Then here in func commonDialer

go func() { for { select { case <-cancel: // canceled or channel closed return default: } conn, err := dialFunc() if err == nil { // Send conn back iff timer is not fired // Otherwise there might be no one left reading it if t.Stop() { ch <- conn } else { conn.Close() } return } } }()

we could re-connect(conn, err := dialFunc()) again.

Yes I agree, but this quick re-connection performed by commonDialer() tend to get things to fail. Instead, when relying on the backoff strategy from GRPC (which happens when we return conn, nil), things are more stable.
Just to clarify, I'm not saying this is a long term solution, but while we look actively for the root cause, relying on the backoff strategy from GRPC gives us a more stable CI for both Firecracker and Cloud-Hypervisor.

sboeuf · 2020-01-22T14:47:41Z

protocols/client/client.go

+			return nil, err
+		} else if !strings.Contains(response, "OK") {
+			conn.Close()
+			return nil, errors.New("malformed response code")


Try removing this line, this way the function will return conn, nil and the GRPC code will handle dialing again.

amshinde · 2020-01-22T22:47:53Z

@Pennyzct Can you implement @sboeuf suggestions and see if the CI for firecracker passes with those.

The new release for Firecracker is `v0.20.0`. Fixes: kata-containers#2378 Depends-on: github.com/kata-containers/agent#706 Signed-off-by: Penny Zheng <penny.zheng@arm.com>

… IS ONLY FOR DEBUGGING! Signed-off-by: Penny Zheng <penny.zheng@arm.com>

Pennyzct · 2020-01-27T11:56:35Z

Hi~ @sboeuf @amshinde
Sorry for the late response. 😢. I'm still on the Lunar New Year Holiday, so the response could be slow.;).
I'm still working on these patches, it's odd, the all docker-related integration tests have all passed, but failing on ctr. I'll try to reproduce and debug it.

jcvenegas · 2020-01-29T16:29:55Z

@Pennyzct ping could you rebase this PR?

New Firecracker v0.20.0 has changed the host-initiated vsock connection protocol to include a trivial handshake. The new protocol looks like this: - [host] CONNECT <port><LF> - [guest/success] OK <assigned_host_port><LF> Fixes: kata-containers#705 Signed-off-by: Penny Zheng <penny.zheng@arm.com>

This file is mainly used by kata-runtime and missing logging part. We add one new "agent-client" log field here, and Plz notice that, you should turn on the `kata-runtime` debug option to see the output. Fixes: kata-containers#705 Depends-on: github.com/kata-containers/runtime#2379 Signed-off-by: Penny Zheng <penny.zheng@arm.com>

Pennyzct · 2020-01-30T03:34:14Z

Hi~ guys
Sorry for the delay.😢. I'm still on the Lunar New Year holidays, and the response could be slow.
Thanks for the suggestion from @sboeuf, I've refined the code to use the back-off strategy from grpc. And I've also re-based the code here. ;).

Pennyzct · 2020-01-30T03:34:32Z

/test

Here, We import changes from PR kata-containers/agent#706 to see if it's really working. Fixes: kata-containers#2378 Signed-off-by: Penny Zheng <penny.zheng@arm.com>

We need to include changes in PR github.com/kata-containers/agent#706 (kata-containers/agent#706, to use the new vsock-trivial-handshake scheme implemented in FC v0.20.0. Fixes: kata-containers#2378 Signed-off-by: Penny Zheng <penny.zheng@arm.com>

We need to include changes in PR github.com/kata-containers/agent#706 (kata-containers/agent#706, to use the new vsock-trivial-handshake scheme implemented in FC v0.20.0. Fixes: kata-containers#2378 Depends-on: kata-containers#2431 Signed-off-by: Penny Zheng <penny.zheng@arm.com>

We need to include changes in PR github.com/kata-containers/agent#706 (kata-containers/agent#706, to use the new vsock-trivial-handshake scheme implemented in FC v0.20.0. Fixes: kata-containers#2378 Signed-off-by: Penny Zheng <penny.zheng@arm.com>

lifupan approved these changes Dec 27, 2019

View reviewed changes

This was referenced Dec 27, 2019

FC: Update Firecracker to v0.20.0 kata-containers/runtime#2378

Closed

FC: Update Firecracker to v0.20.0 kata-containers/runtime#2379

Merged

jodh-intel approved these changes Jan 2, 2020

View reviewed changes

amshinde approved these changes Jan 3, 2020

View reviewed changes

Pennyzct force-pushed the vsock_handshake branch from 806dc6f to b7d6ae5 Compare January 6, 2020 02:34

Pennyzct force-pushed the vsock_handshake branch from b7d6ae5 to 29d5a2b Compare January 8, 2020 09:29

Pennyzct added a commit to Pennyzct/runtime that referenced this pull request Jan 9, 2020

DEBUG: vendor update

b05a97f

Add changes from kata-containers/agent#706. THIS IS ONLY FOR DEBUGGING! Fixes: kata-containers#2378 Signed-off-by: Penny Zheng <penny.zheng@arm.com>

Pennyzct added a commit to Pennyzct/runtime that referenced this pull request Jan 10, 2020

DEBUG: vendor update

112dc00

Add changes from kata-containers/agent#706. THIS IS ONLY FOR DEBUGGING! Signed-off-by: Penny Zheng <penny.zheng@arm.com>

Pennyzct force-pushed the vsock_handshake branch 2 times, most recently from adad4ca to 9b10b2e Compare January 10, 2020 05:19

Pennyzct added a commit to Pennyzct/runtime that referenced this pull request Jan 10, 2020

DEBUG: vendor update

5421cee

Add changes from kata-containers/agent#706. THIS IS ONLY FOR DEBUGGING! Signed-off-by: Penny Zheng <penny.zheng@arm.com>

teawater mentioned this pull request Jan 22, 2020

protocols: Fix client hybrid vsock protocol #717

Closed

sboeuf reviewed Jan 22, 2020

View reviewed changes

jcvenegas pushed a commit to jcvenegas/runtime that referenced this pull request Jan 23, 2020

DEBUG: vendor update Add changes from kata-containers/agent#706. THIS…

a7c243b

… IS ONLY FOR DEBUGGING! Signed-off-by: Penny Zheng <penny.zheng@arm.com>

Pennyzct added 2 commits January 30, 2020 03:18

Pennyzct force-pushed the vsock_handshake branch from 9b10b2e to accab34 Compare January 30, 2020 03:23

Pennyzct added a commit to Pennyzct/runtime that referenced this pull request Jan 30, 2020

Debug: update agent client

8eebb73

Here, We import changes from PR kata-containers/agent#706 to see if it's really working. Fixes: kata-containers#2378 Signed-off-by: Penny Zheng <penny.zheng@arm.com>

teawater merged commit cc95027 into kata-containers:master Jan 30, 2020

likebreath mentioned this pull request Feb 4, 2020

clh: docker kill test hangs randomly kata-containers/tests#2141

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FC: Add new vsock connection handshake #706

FC: Add new vsock connection handshake #706

Pennyzct commented Dec 27, 2019

codecov bot commented Dec 27, 2019 •

edited

Loading

Pennyzct commented Dec 27, 2019

jodh-intel left a comment

jodh-intel Jan 2, 2020

Pennyzct Jan 6, 2020

Pennyzct commented Jan 6, 2020

devimc commented Jan 6, 2020

Pennyzct commented Jan 8, 2020

Pennyzct commented Jan 8, 2020

devimc commented Jan 8, 2020

Pennyzct commented Jan 10, 2020

devimc commented Jan 10, 2020

jodh-intel commented Jan 20, 2020

sboeuf commented Jan 22, 2020

devimc commented Jan 22, 2020

sboeuf commented Jan 22, 2020

sboeuf Jan 22, 2020

Pennyzct Jan 27, 2020

sboeuf Jan 29, 2020

sboeuf Jan 22, 2020

amshinde commented Jan 22, 2020

Pennyzct commented Jan 27, 2020

jcvenegas commented Jan 29, 2020

Pennyzct commented Jan 30, 2020

Pennyzct commented Jan 30, 2020

FC: Add new vsock connection handshake #706

FC: Add new vsock connection handshake #706

Conversation

Pennyzct commented Dec 27, 2019

codecov bot commented Dec 27, 2019 • edited Loading

Codecov Report

Pennyzct commented Dec 27, 2019

jodh-intel left a comment

Choose a reason for hiding this comment

jodh-intel Jan 2, 2020

Choose a reason for hiding this comment

Pennyzct Jan 6, 2020

Choose a reason for hiding this comment

Pennyzct commented Jan 6, 2020

devimc commented Jan 6, 2020

Pennyzct commented Jan 8, 2020

Pennyzct commented Jan 8, 2020

devimc commented Jan 8, 2020

Pennyzct commented Jan 10, 2020

devimc commented Jan 10, 2020

jodh-intel commented Jan 20, 2020

sboeuf commented Jan 22, 2020

devimc commented Jan 22, 2020

sboeuf commented Jan 22, 2020

sboeuf Jan 22, 2020

Choose a reason for hiding this comment

Pennyzct Jan 27, 2020

Choose a reason for hiding this comment

sboeuf Jan 29, 2020

Choose a reason for hiding this comment

sboeuf Jan 22, 2020

Choose a reason for hiding this comment

amshinde commented Jan 22, 2020

Pennyzct commented Jan 27, 2020

jcvenegas commented Jan 29, 2020

Pennyzct commented Jan 30, 2020

Pennyzct commented Jan 30, 2020

codecov bot commented Dec 27, 2019 •

edited

Loading