Issue: intel/cc-oci-runtime#622
Fix (without unit test): intel/cc-oci-runtime#631

We don't use fd passing anymore so client.NewClient should take the more general net.Conn interface.

After updating to the latest proxy commit, I started having failures on local and on semaphore [1]:

cloud@ubuntu-dev:~/go/src/github.com/clearcontainers/tests$ sudo docker run -ti ubuntu bash
connect_to_proxy:866:Error while connecting to proxy with address /var/run/cc-oci-runtime/proxy.sock: No such file or directory

docker: Error response from daemon: exit status 1: "open /var/lib/virtcontainers/pods/aa5e79927786e80ce7c25d54733c2657c12a4a53e86708556ae8e29ea741110a/config.json: no such file or directory\n".

cc-proxy is running:

cloud@ubuntu-dev:~/go/src/github.com/clearcontainers/tests$ systemctl status cc-proxy.socket 
● cc-proxy.socket - Clear Containers Proxy Socket
   Loaded: loaded (/lib/systemd/system/cc-proxy.socket; disabled; vendor preset: enabled)
   Active: active (running) since Fri 2017-06-30 12:26:48 UTC; 5min ago
     Docs: https://github.com/clearcontainers/proxy
   Listen: /var/run/clear-containers/proxy.sock (Stream)

cloud@ubuntu-dev:~/go/src/github.com/clearcontainers/tests$ sudo ls -l /var/run/clear-containers/proxy.sock
srw-rw---- 1 root root 0 Jun 30 12:26 /var/run/clear-containers/proxy.sock

[1] https://semaphoreci.com/clearcontainers/tests/branches/master/builds/41

From @dlespiau on December 8, 2016 12:26

It would nice to be able to daemonize the proxy from the command line. The key here would to fork to the daemon process once the proxy is able to handle connections on its socket.

• maybe use https://github.com/sevlyar/go-daemon
• "daemonize" once the socket has been created
• original process exits, daemon process continues and will accept socket connections

The runtime could use this feature to auto-start the proxy when needed.

Copied from original issue: intel/cc-oci-runtime#520

The fewer things we have to maintain the better. While I wrote custom scripts to make sure we don't regress on goreport, it may be a good idea to replace them by gometalinter. Careful to not overdo it and introduce awkward checkers that make us bend backwards to eliminate unhelpful warnings.

Since the port to logrus, we now have two '\n' in log entries from the qemu console socket.

Since there are multiple versions of the proxy in existence and since they accept different options, it would be a good idea to add a --version to all variants that shows a version number and commit ID as a debug aide.

See clearcontainers/shim#32.

We'd like clients (runtime/shim) to be able to send debugging logs to the proxy so it can become the central place to look at when diagnosing/debugging a problem.

For that, a Log payload could be introduced. The exact log format is up for debate, but we should strongly considerate doing structured logging. Given we already send JSON there, seems only natural do have that payload be a json object with multiple key: value.

With logrus, this would come with the library, eg.

{"animal":"walrus","level":"info","msg":"A group of walrus emerges from the
ocean","size":10,"time":"2014-03-10 19:57:38.562264131 -0400 EDT"}

It'd be useful to have the hypervisor pid in the aggregated logs. One good option to know about qemu's pid is for the runtime/virtcontainers to tell us in RegisterVM.

James commented in #61 that it'd be useful to log the pid of the actors involved.

These objects are starting to become more complicated with the 3.0 protocol, splitting and documenting them separately is starting to become needed.

We have a few data structures to garbage collect if runtime & shim don't behave:

• If a token is never claimed by a shim, it's never garbage collected at the moment
• sessions created when a shim connects aren't freed until DisconnectShim. We should guard against shims closing the connection and never calling DisconnectShim

An idea would be to have a goroutine in charge of looking at old tokens and sessions and garbage collect them after some time.

Define a simple, TCP based protocol for communicating with container shims

The protocol would allow shims to send over a single TCP socket the following commands:

• Connection/Disconnection: For a shim to explicitly connect and pass an authentication token to the proxy.
• I/O: Send stdin streams to a given container.
• Signals: All signals, including SIGWINCH.
• Logs: Shim logs could be aggregated by the proxy for easier debugging.

And to receive the following commands:

• I/O: Receive stdout/stderr streams from a given container

Currently the proxy really only knows about containers, because of its root in docker. AttachVM takes an argument called containerID, but that's really a podID.

We may or may not want to clear up the confusion for the cases when clear containers is used with k8s.

From @sameo on December 2, 2016 17:36

If cc-proxy crashes:

1. all cc-shim instances terminate.
2. cc-proxy will not be able to restore its internal state after restarting.

We need to work on:

1. Have cc-shim retry connecting to the proxy when the socket is closing/disappearing
2. Have cc-proxy re-build all its states when restarting, based on the stored information

Copied from original issue: intel/cc-oci-runtime#505

megacheck found a bit of dead code, we should fix this and re-enable megacheck.

At the moment we have the main package with the core objects (proxy, vm, ioSession, ..) and the main function. The problem with those objects being part of the main package is that golint doesn't pickup some "issues" (eg. undocumented exported symbols).

Plan is to split the main package in two so golint can do its work and we have a clean separation between the main function and the core objects.

While we have a number of tests on the proxy itself, we should make sure the runtime still works and passes all test before merging proxy changes.

This should be as easy as possible: clone the repo where the tests are (could be the runtime repo), execute a script.

We still need to think about our test pipeline so the above could change.

We have a few messages here and there, but it's probably a good idea to add a few more to fully capture the life cycle of a vm, session and client objects.

Need to port PR intel/cc-oci-runtime#996 to 3.0.

We should make our logging better and adopt structured logging.

As virtcontainers recently moved to logrus, we can just follow the trend.

Once a client has done a ConnectShim, we should restrict what it can do to the shim only frames (for instance no hyper Command).

In order to be less hyperstart specific, reduce the current runtime protocol to the following commands:

• Hello: For registering a new VM. This returns a token to be used by a shim to connect to the proxy through the shim protocol.
• Bye: For unregistering a VM.
• Attach: Attach to an already running VM. This returns a token for the shim.
• Command: Send a raw CTL command

It's possible that we've been asked for shim tokens but never managed to fully start the container for some reason. When the runtime tries to clean up in the error path by calling UnregisterVM, we should make sure we clean up all objects associated with the VM in the proxy struct.

And test it.

Up until now, we never needed the content of the message back from hyperstart. With the new ps payload though, we need it.

The code discarding the response starts at https://github.com/clearcontainers/proxy/blob/master/vm.go#L428

https://help.github.com/articles/adding-a-code-of-conduct-to-your-project/

The proxy API has notifications. Right now only the shim written in C needs those notifications (ProcessExited). If we ever need notifications from a client written in go, we'll need to improve the client API and add a routing facility to handle async ProcessExited events like we've done in virtcontainers (containers/virtcontainers#86).

This way a client can subscribe to notifications and receive them through a channel.

Thinking about it, runtime and shim should be able to always give us the container they are logging for.

It would be useful to create a UML sequence diagram showing the message flows between the following:

• proxy
• shim
• runtime
• agent

When newcontainer/execcmd hyperstart commands are issued, they start a process that send data on stdout/err. The proxy needs to do something with that data, namely send it to the corresponding shim.

Now that the shim connection is explicit, it theoretically possible a runtime asks us to forward a newcontainer command without a shim associated with the I/O token. We have two ways to solve that:

• Queue the stdout/err data in the proxy until the shim associates
• Require that a shim fully setup before we accept a newcontainer command

The second option is the one that requires the least amount of work and is also quite logical after all: don't start the process before the I/O path is fully up. Flow should go:

• create
  1. runtime asks for an I/O token with RegisterVM/AttachVM
  2. runtime creates shim process with Token
  3. shim issues a ConnectShim command with the Token
• start
  1. runtime issues newcontainer command

Things are a bit more fuzzy for execcmd as the runtime needs to know when the shim is associated. This could be fixed by the shim telling the runtime it's ready and the runtime would wait for that before doing an execcmd.

From @jodh-intel on October 26, 2016 8:0

The proxy code is sprinkled with magic numbers. These should be converted to well-documented consts.

See review comments on PR #306

Copied from original issue: intel/cc-oci-runtime#351

Can we document which tools we use for golang vendoring please.
From the presence of https://github.com/clearcontainers/proxy/blob/master/Gopkg.lock I would guess we probably use https://github.com/golang/dep. Can we confirm that?

From @jodh-intel on October 26, 2016 7:57

Related to #348 and original comments added to PR #306.

Copied from original issue: intel/cc-oci-runtime#350

~# /usr/libexec/clear-containers/cc-proxy --version
Version: 3.0.0-alpha.1+
~#
~#
~# apt-get install cc-proxy
Reading package lists... Done
Building dependency tree
Reading state information... Done
cc-proxy is already the newest version (3.0.0alpha.3+git.b73e4a3-0+3.1).
cc-proxy set to manually installed.
0 upgraded, 0 newly installed, 0 to remove and 5 not upgraded.

Currenly, the shim can send data as soon as its connected. The problem is that the process inside the VM may not be fully started and ready to process that input.

Just like the runtime waits for the shim to connect before issuing the exec (to make sure there's a shim ready to receive stdout), we can make shims wait for the exec to return before forwarding stdin data.

We need to integrate the CI under clearcontainers/tests and enable semaphoreCI

Add @grahamwhaley as a project approver.

Add support for https://github.com/clearcontainers/tests/tree/master/cmd/checkcommits, to match the runtimes.

gometalinter takes a long time to get and compile. We should be able to cache the source and binary and always do a gometalinter --install --update

Archana wanted to be able to write a frame from separate goroutines and that does sound like a good idea (because the protocol has some notifications and that's handy to send them from any goroutine we want). The easy way to do that is that have a single Write() but, currently, it means w need to copy the payload to write into a bigger header+payload buffer.

We should be able to do better, my suggestion from PR #11 copied below:

mean the api package, not the client<->proxy on-wire protocol. If we want to avoid a copy we need to change the API exposed by the api package (yes, too many APIs). For instance, the following doesn't work anymore as payload is given by the caller which means we would need to copy it into the big header+payload buffer:

func WriteCommand(w io.Writer, op Command, payload []byte) error {
 	return WriteFrame(w, NewFrame(TypeCommand, int(op), payload))
 }

So, instead, we need to ask Frame for the payload slice which would point to the underlying big buffer. Now the problem is that solution requires to know the size of the payload before asking the Frame for the payload slice. It's not possible to known the slice length beforehand in all cases. When using json.Marshal() for instance, we're given the encoded byte buffer back and only then we know the size of the JSON serialization.

So, to solve all of the above, an interface that works is getting an io.Writer from the Frame struct. That io.Writer would be on top of a byte buffer that grows on demand and we can give the io.Writer to json.Encode to have the JSON serialization happen in the big (header+payload) buffer.

That would look like:

func NewFrame(t FrameType, op int) *Frame
func NewFrameWithPayloadSize(t FrameType, op int, payloadSize int) *Frame

func (f *Frame) getPayloadWriter() io.Writer

NewFrameWithPayloadSize() is the usual optimization of sizing the buffer correctly when knowing the size beforehand (say for stream frames) and avoid multiples reallocations.

In the Frame struct, the frame data could a bytes.Buffer with the constructor writing the header.

All in all, a bit of complexity, but may be worth it.

The proxy should define these limits so that clients do not end up allocating a huge amount of memory. This also ensures that the client is not talking to a misbehaving proxy. The proxy and clients can then use these limits to send data in chunks.

There are some cases when we don't care about the container workload (or a new process) I/O. Such a case is when we start the pause container in an empty pod.

We should allow starting a container/process without an I/O token for such cases.

It may be nice to control the amount of logging remotely (maybe only if the proxy has been started with a debug option to avoid clients abusing of that facility and spam logs). Controls could have filters to further select the container we're interested in.

eg.

{"level": "debug", "containerID": "XXXXXXXX"}

would only output debug messages for messages tagged with the provided containerID

Implement store/restore of cc-proxy's state to/from disk.
This is needed to support high availability feature #4.

Because we have agreed that a state transition from created to stopped is possible for virtcontainers and our runtime, we can end up with situation where the proxy waits for the shim process to start so that it can forward a KILL or TERM signal. Instead, we want the proxy to be smarter in this type of case, so that it kills the shim by himself. Indeed, there is no process/container running inside the VM, meaning that the corresponding shim will never receive the exit code from the VM. The proxy has to detect those cases and kill the shim. That's the only way to avoid waiting for something which will never happen.

Remove user dlespiau as a project approver as he is no longer a core maintainer.

It's probably a good idea to add pullapprove for the shim, for instance to check the signed-off-by tag. We may want to follow the clearcontainers/runtime rule for some time and only require 1 approver.

In case the shim receive a signal and call into the proxy to handle this signal, the proxy should not signal the session.vm.containerID. Instead, there are two options:

• The shim can give the container ID
  OR
• Every session holds a container ID

This is needed for the case where the podID is different from the the containerID.