initial shim implementation #1

bergwolf · 2017-11-21T12:56:00Z

This is based on runv's current Hyperstart interface and can be migrated to the new gRPC APIs once they are settled.

Currently the agent address parameter must be an unix socket. To support vsock endpoints, we need a new gRPC dialer for vsock connections, which should be added in runtime gRPC API helpers instead.

TODOs:

sameo · 2017-11-21T18:36:16Z

main.go

+		proxyWinsize  bool
+	)
+
+	flag.BoolVar(&debug, "debug", false, "enable debug logging")


Using logrus would allow us to pass a string to --log instead of a boolean.

Let's stick with glog for now and migrate to logrus once the decision is made.

Let's discuss tomorrow about that.

sameo · 2017-11-21T18:40:16Z

main.go

+
+	flag.BoolVar(&debug, "debug", false, "enable debug logging")
+	flag.StringVar(&logDir, "logdir", "", "the directory for the logging")
+	flag.StringVar(&root, "root", "", "root directory for storage of container state")


I guess we want to store some state here to handle the shim crashes? Ideally we should be able to restart a shim and get the container state from the agent and have a stateless shim.

What states do you have in mind? IMO the shim should be stateless. The way to restart is to start it again with the same parameters. But I can add states file if necessary.

@bergwolf Yes, the shim should be stateless. But passing a -root option for root directory for storage of container state seems to indicate the opposite. Since this option is not used in the code, I'm not sure what the intention here is and I want to make sure the shim is indeed stateless.

@sameo I agree. I'll drop it. It was needed by runv shim but not anymore here.

sameo · 2017-11-21T18:41:13Z

main.go

+	}
+
+	// wait to be waken up if starting a new pod
+	if process == "init" {


You want to define this "init string as a constant in the code, to be more explicit.

So could you describe who's supposed to wake us up here, and why do we need to wait?

The runtime. With it, we can launch shim and create guest in parallel.

sameo · 2017-11-21T18:54:55Z

main.go

+	flag.BoolVar(&proxyExitCode, "proxy-exit-code", true, "proxy exit code of the process")
+	flag.BoolVar(&proxyStdio, "proxy-stdio", true, "proxy stdio of the process")
+	flag.BoolVar(&proxySignal, "proxy-signal", true, "proxy signals to the process")
+	flag.BoolVar(&proxyWinsize, "proxy-winsize", true, "proxy tty size to the process")


Are there any use cases where we wouldn't want to do any of these 3: stdio, signal and tty size?

Not one I can think of. These are just in case.

This list of parameters seems pretty complex. We should make this simpler. We really need to know about the container/process IDs, and the socket URL. But the rest has to be done in every case, we don't need to enable/disable such things with parameters.

@bergwolf I propose we remove the options that are not used for now and simplify the shim CLI. It's easier to add new options when needed than to remove one and potentiall break backward compatibility.

I think it won't hurt anything if we already have them configurable.

Moreover, I am thinking about split proxy-stdio to in and output, because sometimes we don't want to input anything, for example, logs only.

Having options just in case makes both the code base and the CLI/UI more complex. Simplicity should be an important goal for us.

Fair enough. I'll remove unnecessary options. We can add them back when needed in future, but not in the first versions.

sameo · 2017-11-21T19:01:59Z

shim.go

+	}()
+}
+
+func (s *Shim) forwardAllSignals() chan os.Signal {


How's SIGKILL handled?

bergwolf · 2017-11-22T02:36:57Z

shim.go

+func NewShim(container, process, addr string) (*Shim, error) {
+	if agent, err := NewShimAgent(addr); err != nil {
+		return nil, err
+	} else {


agent is defined in if statement.

sameo · 2017-11-21T23:49:32Z

agent.go

+func NewHyperAgent(addr string) (shimAgent, error) {
+	if h, err := libhyperstart.NewGrpcBasedHyperstart(addr); err != nil {
+		return nil, err
+	} else {


nit: No need for the else here.

It is needed as h variable is defined in the if statement.

@bergwolf Well not really, you can simplify like this:

h, err := libhyperstart.NewGrpcBasedHyperstart(addr); if err != nil { return nil, err } return h, nil

@sboeuf It's just another version. I wouldn't call it simpler though :)

sameo · 2017-11-21T23:57:21Z

agent.go

+
+// A new shim agent implementation should have the shimAgent interface
+// Currently it is a subset of libhyperstart.Hyperstart interface but it
+// allows for new agent and protocols.


I'm not a huge fan of this requirement. I believe the only requirement for an agent should be the gRPC interface. Is that what you meant by This is based on runv's current Hyperstart interface and can be migrated to the new gRPC APIs once they are settled. ?

This is more like a work around as we have not had an working agent (or mock) for the new gRPC APIs. But we need the shim could be tested.

@sameo The shimAgent interface will also be removed once we can import the new gRPC APIs. The idea is not to import runv in shim.go for now. In future, agent.go will not import runv either.

Who's responsible for moving gRPC API to kata ? I think we should not merge anything as long as we rely on some runv based packages (even if the content is basically the same.)

@sboeuf kata-containers/runtime#2 is working on this goal.

bergwolf · 2017-11-22T02:33:47Z

agent.go

+
+// A new shim agent implementation should have the shimAgent interface
+// Currently it is a subset of libhyperstart.Hyperstart interface but it
+// allows for new agent and protocols.


@sameo The shimAgent interface will also be removed once we can import the new gRPC APIs. The idea is not to import runv in shim.go for now. In future, agent.go will not import runv either.

bergwolf · 2017-11-22T02:34:00Z

agent.go

+func NewHyperAgent(addr string) (shimAgent, error) {
+	if h, err := libhyperstart.NewGrpcBasedHyperstart(addr); err != nil {
+		return nil, err
+	} else {


It is needed as h variable is defined in the if statement.

bergwolf · 2017-11-22T02:34:28Z

main.go

+		proxyWinsize  bool
+	)
+
+	flag.BoolVar(&debug, "debug", false, "enable debug logging")


Let's stick with glog for now and migrate to logrus once the decision is made.

bergwolf · 2017-11-22T02:34:52Z

main.go

+
+	flag.BoolVar(&debug, "debug", false, "enable debug logging")
+	flag.StringVar(&logDir, "logdir", "", "the directory for the logging")
+	flag.StringVar(&root, "root", "", "root directory for storage of container state")


What states do you have in mind? IMO the shim should be stateless. The way to restart is to start it again with the same parameters. But I can add states file if necessary.

bergwolf · 2017-11-22T02:35:49Z

main.go

+	flag.BoolVar(&proxyExitCode, "proxy-exit-code", true, "proxy exit code of the process")
+	flag.BoolVar(&proxyStdio, "proxy-stdio", true, "proxy stdio of the process")
+	flag.BoolVar(&proxySignal, "proxy-signal", true, "proxy signals to the process")
+	flag.BoolVar(&proxyWinsize, "proxy-winsize", true, "proxy tty size to the process")


Not one I can think of. These are just in case.

bergwolf · 2017-11-22T02:36:26Z

main.go

+	}
+
+	// wait to be waken up if starting a new pod
+	if process == "init" {


The runtime. With it, we can launch shim and create guest in parallel.

bergwolf · 2017-11-22T02:36:57Z

shim.go

+func NewShim(container, process, addr string) (*Shim, error) {
+	if agent, err := NewShimAgent(addr); err != nil {
+		return nil, err
+	} else {


agent is defined in if statement.

bergwolf · 2017-11-22T02:37:37Z

shim.go

+	}()
+}
+
+func (s *Shim) forwardAllSignals() chan os.Signal {


sboeuf · 2017-11-22T09:34:01Z

glog vs logrus is one thing we want to clarify, but there is also govendor vs dep we need to discuss about.

gnawux · 2017-11-22T09:42:37Z

@sboeuf we won't mind to change to golang official dep tool whenever it graduates from experimental stage.

sameo · 2017-11-22T10:03:14Z

@gnawux dep is no longer experimental. From https://github.com/golang/dep:

dep is safe for production use. That means two things:

* Any valid metadata file (Gopkg.toml and Gopkg.lock) will be readable and considered valid by any future version of dep.
* The CLI UI is mostly stable. dep init and dep ensure are mostly set; dep status is likely to change a fair bit, and dep prune is going to be absorbed into dep ensure.

gnawux · 2017-11-22T10:22:31Z

@sameo I agree to change to dep.

I notice the message in README of golang/dep as well. I think it is better to use the future official tool, though the readme said is still in experimental.

bergwolf · 2017-11-23T07:32:03Z

updated with following changes:

remove unnecessary arguments
constify "init" process name
replace govendor with golang official dep

laijs · 2017-11-23T12:20:43Z

Since the agent-api pr was merged, it is better to switch to it.

... and I'm shocked that the dependence is so large for such a small program.

jodh-intel · 2017-11-27T15:40:57Z

shim.go

+
+type Shim struct {
+	container string
+	process   string


Presumably container is the containerID? If so, I think it would be clearer to name it with the ID suffix.

Is process the PID? If so, again, why not specify it as pid int?

processes' numeric pid are not seen on the host, we don't want to expose them, right?
{containerID, processID} is used to refer the init/exec process in the container.
Both containerID and processID are strings.

(Linux processes spawned by init/exec process which are supposed to be invisible in the host can't not be referenced.)

jodh-intel · 2017-11-27T15:42:17Z

shim.go

+}
+
+func (s *Shim) forwardAllSignals() chan os.Signal {
+	sigc := make(chan os.Signal, 2048)


Why 2048? Could this be a top-level const maybe for clarity?

I learnt the number from docker. The same code is still at
https://github.com/containerd/containerd/blob/master/cmd/containerd/main.go#L80

jodh-intel · 2017-11-27T15:43:17Z

shim.go

+	return sigc
+}
+
+func (s *Shim) resizeTty() {


I appreciate that it's longer, but resizeTerminal() looks a little "easier on the eye" to me? (Same comment to the other areas of the code that mention Tty). That would also be in line with the naming of term.RestoreTerminal().

sorry but the name tty is in sync with agent gRPC definition TtyWinResize

jodh-intel · 2017-11-27T15:47:48Z

main.go

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


We might want to think about switching to logrus for all components.

jodh-intel · 2017-11-27T15:49:07Z

main.go

+	"github.com/moby/moby/pkg/term"
+)
+
+const INIT_PROCESS = "init"


This is a public symbol so maybe initProcess instead?

No need to make this global. Also use of underscores is discouraged in golang.

jodh-intel · 2017-11-27T15:51:25Z

pipe.go

+
+type inPipe struct {
+	s    shimAgent
+	c, p string


I think it would be clearer if all these members had longer more meaningful names.

amshinde · 2017-11-27T20:54:14Z

shim.go

+
+	go func() {
+		for sig := range sigc {
+			if sig == syscall.SIGCHLD || sig == syscall.SIGPIPE || sig == syscall.SIGWINCH {


We need to consider a few more signals here like SIGBUS, SIGSEGV and SIGABRT. These should be ignored as well as they may indicate runtime exceptions with the shim itself.

amshinde · 2017-11-27T20:57:06Z

main.go

@@ -20,27 +20,19 @@ func main() {
 	var (


Typo in the commit message. Also can this be squashed with the previous commit?

bergwolf · 2017-12-01T02:53:54Z

updated to address @jodh-intel's and @amshinde's comments, change to use logrus and connect to agent gRPC server with vsock supported.

sameo · 2017-12-04T17:19:36Z

pipe.go

+	}
+	return 0, err
+}
+


I would factorise the *out code and define:

type outPipe struct { ctx context.Context agent *shimAgent containerId string pid uint32 bool errPipe }

And then:

func (p *outPipe) Read(data []byte) (n int, err error) { var readFn func(ctx context.Context, in *ReadStreamRequest, opts ...grpc1.CallOption) (*ReadStreamResponse, error) if p.errPipe { readFn = p.agent.ReadStderr } else { readFn = p.agent.ReadStdout } resp, err := readFn(p.ctx, &pb.ReadStreamRequest{ ContainerId: p.containerId, PID: p.pid, Len: uint32(len(data))}) if err == nil { copy(data, resp.Data) return len(resp.Data), nil } // check if it is &grpc.rpcError{code:0xb, desc:"EOF"} and return io.EOF instead if grpc.Code(err) == codes.OutOfRange && grpc.ErrorDesc(err) == "EOF" { return 0, io.EOF } return 0, err }

@sameo Good point! I'm keeping the pipe struct in sync but using a helper function for both outPipe and errPipe Read() functions. Please see if the new code looks good to you.

sameo · 2017-12-04T17:22:23Z

main.go

+		os.Exit(exitFailure)
+	}
+
+	// wait to be waken up if starting a new sandbox


Sorry to insist on that one, but could you please describe why we need to wait from the shim when being a sandbox shim. And also document who's supposed to send us SIGUSR1, and at which point.

I dropped it because with current StartContainer() and ExecProcess() APIs, runtime can no longer start shim and create sandbox in parallel.

sameo · 2017-12-04T17:24:32Z

This looks good. I only have a couple of minor comments, besides the missing points from the TODO list (most important one being unit tests, imho).

bergwolf · 2017-12-05T02:45:58Z

UT success depends on agent pr kata-containers/agent#36. Will trigger travis once that one is merged.

sameo · 2017-12-05T07:41:02Z

LGTM

sameo · 2017-12-05T07:46:31Z

Gopkg.toml

+
+
+[[constraint]]
+  branch = "master"


@bergwolf You need to revendor to have the tests pass.
Also, could you please lock that one to a specific version (The one that include the dialer fixes)?

jodh-intel · 2017-12-05T11:02:09Z

Makefile

@@ -0,0 +1,8 @@
+all:
+	go build -o shim


Can we name the binary kata-shim (to match main.go:shimName)? A binary called shim is too generic imho. This will require an update to .gitignore too.

I've raised a PR to do the same for the agent: kata-containers/agent#39.

jodh-intel · 2017-12-05T11:05:43Z

main.go

+	// winsize
+	s, err := term.SetRawTerminal(os.Stdin.Fd())
+	if err != nil {
+		shimLog.Errorf("failed to set raw terminal: %v", err)


This would be better as:

shimLog.WithError(err).Error("failed to set raw terminal")

Same comment for other .Errorf() calls.

jodh-intel · 2017-12-05T11:07:33Z

main.go

+	// wait until exit
+	exitcode, err := shim.wait()
+	if err != nil {
+		shimLog.Errorf("failed waiting for process %d: %s", pid, err)


It would be good to start using structured logging from the outset, as we've done for the kata agent, so this could become:

shimLog.WithError(err).WithField("pid", pid).Error("failed to wait for shim process")

Same comment for the other logging calls which could use the logrus .WithField() or .WithFields(logrus.Fields{ ... }) calls.

jodh-intel · 2017-12-05T11:12:58Z

shim.go

+		_, err2 := s.agent.CloseStdin(s.ctx, &pb.CloseStdinRequest{
+			ContainerId: s.containerId,
+			PID:         s.pid})
+		shimLog.Infof("copy stdin err1 %s err2 %s", err1, err2)


Do we really want to log these errors every time? It would be more conventional to do something like:

if err1 != nil { shimLog.WithError(err1).Warn("failed to copy stdin to pipe")

(and the same sort of thing for err2).

Same comment applies to the log calls below.

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

bergwolf · 2017-12-05T13:57:36Z

updated to address @jodh-intel's comments.

jodh-intel · 2017-12-05T14:11:56Z

.gitignore

@@ -0,0 +1 @@
+shim


Nit: this should be kata-shim now.

ah, fixed. Thanks!

jodh-intel · 2017-12-05T14:12:50Z

Aside from the nit...

lgtm.

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

laijs · 2017-12-05T14:54:59Z

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>

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>

…r-enabling HACK HACK HACK: get firecracker working

sameo requested review from amshinde and sboeuf November 21, 2017 18:44

sameo reviewed Nov 21, 2017

View reviewed changes

bergwolf commented Nov 22, 2017

View reviewed changes

bergwolf force-pushed the shim branch from 99b570d to 0c42377 Compare November 23, 2017 07:30

bergwolf force-pushed the shim branch from 0c42377 to fbd1871 Compare November 27, 2017 10:15

jodh-intel reviewed Nov 27, 2017

View reviewed changes

amshinde reviewed Nov 27, 2017

View reviewed changes

bergwolf force-pushed the shim branch 3 times, most recently from 8b9619c to cc8b402 Compare December 1, 2017 02:53

bergwolf force-pushed the shim branch from cc8b402 to 420f0cc Compare December 1, 2017 02:55

sameo reviewed Dec 4, 2017

View reviewed changes

bergwolf force-pushed the shim branch from 420f0cc to 7c16520 Compare December 5, 2017 02:40

sameo reviewed Dec 5, 2017

View reviewed changes

bergwolf force-pushed the shim branch from 7c16520 to ce370bc Compare December 5, 2017 07:54

jodh-intel reviewed Dec 5, 2017

View reviewed changes

initial shim implementation

592a590

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

bergwolf added 3 commits December 5, 2017 21:55

add unit tests

0eacb67

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

add vendor

eee5f0c

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

add travis ci

5c15fe1

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

bergwolf force-pushed the shim branch from ce370bc to 4cafc2b Compare December 5, 2017 13:56

jodh-intel reviewed Dec 5, 2017

View reviewed changes

bergwolf added 2 commits December 5, 2017 22:41

add gitignore

51a1143

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

add readme

4e52208

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

bergwolf force-pushed the shim branch from 4cafc2b to 4e52208 Compare December 5, 2017 14:41

laijs merged commit 58ac96f into kata-containers:master Dec 5, 2017

jcvenegas mentioned this pull request Apr 25, 2018

ci: test go 1.10 #77

Closed

sboeuf pushed a commit to sboeuf/shim that referenced this pull request Dec 8, 2018

Merge pull request kata-containers#1 from mcastelino/topic/firecracke…

9532400

…r-enabling HACK HACK HACK: get firecracker working

initial shim implementation #1

initial shim implementation #1

Conversation

bergwolf commented Nov 21, 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

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

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

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

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

Choose a reason for hiding this comment

sboeuf commented Nov 22, 2017

gnawux commented Nov 22, 2017

sameo commented Nov 22, 2017

gnawux commented Nov 22, 2017

bergwolf commented Nov 23, 2017

laijs commented Nov 23, 2017

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

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

bergwolf commented Dec 1, 2017

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

sameo commented Dec 4, 2017

bergwolf commented Dec 5, 2017

sameo commented Dec 5, 2017 • edited by amshinde 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

bergwolf commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel commented Dec 5, 2017 • edited by amshinde Loading

laijs commented Dec 5, 2017 • edited by amshinde Loading

bergwolf commented Nov 21, 2017 •

edited

Loading

sameo commented Dec 5, 2017 •

edited by amshinde

Loading

jodh-intel commented Dec 5, 2017 •

edited by amshinde

Loading

laijs commented Dec 5, 2017 •

edited by amshinde

Loading