Getting rid of nsenter and socat #691

abhi · 2018-03-22T18:39:28Z

This commit removes the usage of nsenter and uses netns
to perform socat operation.
Fixes #144

Random-Liu · 2018-03-22T18:49:07Z

@abhi Can we remove socat as well? I think we just need a goroutine to a tcp connection to the port and forward traffic.

abhi · 2018-03-22T18:49:54Z

@Random-Liu yes. Just raising the PR for e2e.

Random-Liu · 2018-03-22T23:54:01Z

@abhi I see. Will review it when it is ready. :)

Random-Liu · 2018-03-24T09:28:24Z

pkg/server/sandbox_portforward.go

-	nsenter, err := exec.LookPath("nsenter")
-	if err != nil {
-		return errors.Wrap(err, "failed to find nsenter")
+	if s.NetNS == nil {


|| s.NetNS.Closed()

thats already checked by netns.Do

Our Closed is different https://github.com/containerd/cri/blob/master/pkg/store/sandbox/netns.go#L108.

We have the restore support.

Random-Liu · 2018-03-24T09:45:44Z

pkg/server/sandbox_portforward.go

+			wg.Done()
+		}()
+		wg.Wait()
+		logrus.Infof("Finish copy port forward input for %q port %d: %v", id, port)


Add a log for both input copy and output copy, and move them into goroutine?

Random-Liu · 2018-03-24T09:50:19Z

pkg/server/sandbox_portforward.go

+		go func() {
+			if _, err := io.Copy(client, stream); err != nil {
+				logrus.WithError(err).Errorf("Failed to copy port forward input from %q port %d", id, port)
+			}


client.Close()?

I have added a defer on top

Random-Liu · 2018-03-24T09:50:27Z

pkg/server/sandbox_portforward.go

+				logrus.WithError(err).Errorf("Failed to copy port forward output for %q port %d", id, port)
+			}
+			wg.Done()
+		}()


stream.Close()?

Are these 2 Close() are necessary?
If dst is closed, src is open, but there is no new input, will io.Copy stop?

If it does, we don't need the 2 Close().

If it doesn't, I think we'll probably need them.

Random-Liu · 2018-03-24T09:56:30Z

pkg/server/sandbox_portforward.go

@@ -17,24 +17,22 @@ limitations under the License.
 package server


Update dependencies in README.md and travis.

abhi · 2018-03-24T17:29:05Z

@Random-Liu done.

Random-Liu · 2018-03-26T06:36:52Z

@abhi We need to start 2 goroutines to copy stream. However, the 2 goroutines may not be in the same network namespace. We are not sure whether it will work or not. The test result shows that it works, but we need to double check with the golang team.

I don't think we have time to figure that out before the coming rc.0 release. @abhi Let's only remove nsenter and still use socat in this PR. We can remove socat in rc.1 release.

abhi · 2018-03-26T13:34:40Z

@Random-Liu yes I am of the same opinion as well. I will remove the socat commits.

Random-Liu · 2018-03-26T17:28:36Z

@abhi also pointed me to another potential issue containernetworking/plugins#98.

Given so, let's move this issue to next release, so that we have more time to investigate this.

It is safe to move this PR to next release, because there is no actual functionality change, we just get rid of 2 binary dependencies nsenter and socat in this PR.

Random-Liu · 2018-04-03T18:34:32Z

We plan to cut an early rc.1 release today to carry several fix and improvement. Move this to rc.2.

Random-Liu · 2018-04-05T07:01:00Z

@abhi Here is a piece of code:

package main

import (
	"fmt"
	"io"
	"os"
	"sync"
	"time"
)

func main() {
	f, err := os.OpenFile("io", os.O_RDWR|os.O_CREATE, 0666)
	if err != nil {
		panic(err)
	}
	var wg sync.WaitGroup
	wg.Add(2)
	go func() {
		io.Copy(os.Stdout, f)
		fmt.Println("file -> stdout done")
		wg.Done()
	}()
	go func() {
		io.Copy(f, os.Stdin)
		fmt.Println("stdin -> file done")
		wg.Done()
	}()
	time.Sleep(5 * time.Second)
	f.Close()
	wg.Wait()
	fmt.Println("all done")
}

You will see, after 5 seconds, the program won't exit even though f is closed. Because there is no input from os.Stdin to drive the io.Copy to write once, and io.Copy won't know f has been closed. Once you input something into stdin, the problem will exit.

So basically io.Copy will stop immediately when input is closed, but when output is closed, it won't stop until there is something from input.

To fix this, we should explicitly close input of the other io.Copy when one io.Copy is done. This is what I suggested in #691 (comment).

This commit removes the usage of nsenter and uses netns to perform socat operation. Signed-off-by: abhi <[email protected]>

Random-Liu · 2018-04-09T18:41:53Z

pkg/server/sandbox_portforward.go

-	nsenter, err := exec.LookPath("nsenter")
-	if err != nil {
-		return errors.Wrap(err, "failed to find nsenter")
+	if s.NetNS == nil {


Our Closed is different https://github.com/containerd/cri/blob/master/pkg/store/sandbox/netns.go#L108.

We have the restore support.

Random-Liu · 2018-04-09T18:42:13Z

pkg/server/sandbox_portforward.go

-	if err != nil {
-		return errors.Wrap(err, "failed to find nsenter")
+	if s.NetNS == nil {
+		return errors.Errorf("failed to find network namespace fo sandbox %q in store", id)


network namespace for sandbox %q is closed

Random-Liu · 2018-04-09T18:44:53Z

pkg/server/sandbox_portforward.go

+		var wg sync.WaitGroup
+		client, err := net.Dial("tcp4", fmt.Sprintf("localhost:%d", port))
+		if err != nil {
+			return errors.Wrap(err, "failed to dial")


nit: "failed to dial %q", port

Random-Liu · 2018-04-09T18:46:09Z

pkg/server/sandbox_portforward.go

+		go func() {
+			defer client.Close()
+			if _, err := io.Copy(client, stream); err != nil {
+				logrus.WithError(err).Errorf("Failed to copy port forward input from %q port %d", id, port)


s/from/for?

Random-Liu · 2018-04-09T18:46:31Z

pkg/server/sandbox_portforward.go

+			if _, err := io.Copy(client, stream); err != nil {
+				logrus.WithError(err).Errorf("Failed to copy port forward input from %q port %d", id, port)
+			}
+			logrus.Infof("Finish copy port forward input for %q port %d: %v", id, port)


Random-Liu · 2018-04-09T18:51:00Z

pkg/server/sandbox_portforward.go

+			if _, err := io.Copy(stream, client); err != nil {
+				logrus.WithError(err).Errorf("Failed to copy port forward output for %q port %d", id, port)
+			}
+			logrus.Infof("Finish copy port forward output for %q port %d: %v", id, port)


Signed-off-by: abhi <[email protected]>

Random-Liu · 2018-04-09T21:49:52Z

/lgtm

k8s-ci-robot added the size/M label Mar 22, 2018

Random-Liu added this to the v1.0.0-rc.0 milestone Mar 22, 2018

Random-Liu self-assigned this Mar 22, 2018

abhi force-pushed the socat branch from a115cad to fbcb034 Compare March 24, 2018 05:37

k8s-ci-robot added size/L and removed size/M labels Mar 24, 2018

abhi force-pushed the socat branch from fbcb034 to 3049a7f Compare March 24, 2018 05:38

k8s-ci-robot added size/M and removed size/L labels Mar 24, 2018

abhi force-pushed the socat branch from 3049a7f to 65709e2 Compare March 24, 2018 05:40

k8s-ci-robot added size/L and removed size/M labels Mar 24, 2018

abhi changed the title ~~[WIP] Using netns to perform socat~~ Getting rid of nsenter and socat Mar 24, 2018

Random-Liu reviewed Mar 24, 2018

View reviewed changes

abhi force-pushed the socat branch 3 times, most recently from 19eb7a6 to 57790c7 Compare March 24, 2018 15:57

Random-Liu modified the milestones: v1.0.0-rc.0, v1.0.0-rc.1, v1.0.0-rc.2 Mar 26, 2018

Random-Liu modified the milestones: v1.0.0-rc.1, v1.0.0-rc.2 Apr 3, 2018

Using netns to perform socat

aeef99a

This commit removes the usage of nsenter and uses netns to perform socat operation. Signed-off-by: abhi <[email protected]>

abhi force-pushed the socat branch from 57790c7 to dfbfb56 Compare April 6, 2018 17:23

Random-Liu reviewed Apr 9, 2018

View reviewed changes

abhi and others added 3 commits April 9, 2018 14:31

Getting rid of socat

02b952e

Signed-off-by: abhi <[email protected]>

Updating travis.yml and Readme to remove nsenter and socal

1317854

Signed-off-by: abhi <[email protected]>

Updating ansible installer

c200cb4

Signed-off-by: abhi <[email protected]>

abhi force-pushed the socat branch from dfbfb56 to c200cb4 Compare April 9, 2018 21:32

k8s-ci-robot added the lgtm label Apr 9, 2018

Random-Liu merged commit 3d0706c into containerd:master Apr 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Getting rid of nsenter and socat #691

Getting rid of nsenter and socat #691

abhi commented Mar 22, 2018 •

edited

Loading

Random-Liu commented Mar 22, 2018 •

edited

Loading

abhi commented Mar 22, 2018

Random-Liu commented Mar 22, 2018

Random-Liu Mar 24, 2018

abhi Mar 24, 2018

Random-Liu Apr 9, 2018

Random-Liu Mar 24, 2018

abhi Mar 24, 2018

Random-Liu Mar 24, 2018

abhi Mar 24, 2018

Random-Liu Mar 24, 2018

Random-Liu Mar 24, 2018 •

edited

Loading

abhi commented Mar 24, 2018

Random-Liu commented Mar 26, 2018 •

edited

Loading

abhi commented Mar 26, 2018

Random-Liu commented Mar 26, 2018

Random-Liu commented Apr 3, 2018

Random-Liu commented Apr 5, 2018 •

edited

Loading

Random-Liu Apr 9, 2018

Random-Liu Apr 9, 2018

Random-Liu Apr 9, 2018

Random-Liu Apr 9, 2018

Random-Liu Apr 9, 2018

Random-Liu Apr 9, 2018

Random-Liu commented Apr 9, 2018

		@@ -17,24 +17,22 @@ limitations under the License.
		package server

Getting rid of nsenter and socat #691

Getting rid of nsenter and socat #691

Conversation

abhi commented Mar 22, 2018 • edited Loading

Random-Liu commented Mar 22, 2018 • edited Loading

abhi commented Mar 22, 2018

Random-Liu commented Mar 22, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu Mar 24, 2018 • edited Loading

Choose a reason for hiding this comment

abhi commented Mar 24, 2018

Random-Liu commented Mar 26, 2018 • edited Loading

abhi commented Mar 26, 2018

Random-Liu commented Mar 26, 2018

Random-Liu commented Apr 3, 2018

Random-Liu commented Apr 5, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Apr 9, 2018

abhi commented Mar 22, 2018 •

edited

Loading

Random-Liu commented Mar 22, 2018 •

edited

Loading

Random-Liu Mar 24, 2018 •

edited

Loading

Random-Liu commented Mar 26, 2018 •

edited

Loading

Random-Liu commented Apr 5, 2018 •

edited

Loading