One proxy per vm by jodh-intel · Pull Request #483 · containers/virtcontainers

jodh-intel · 2017-11-13T17:15:19Z

proxy: Launch one proxy instance per pod

Create a proxy instance for each pod (virtual machine).

Since the proxy is now launched by virtcontainers, it is necessary to specify the path to the proxy binary
in the pod configuration.

Fixes #478.

jodh-intel · 2017-11-13T17:15:36Z

Blocked on clearcontainers/proxy#167.

sboeuf · 2017-11-13T19:22:22Z

 	}

+	// Start the proxy
+	err = p.createProxyProcess()


You should move that into p.startVM(netNsPath) function, so that we don't expand too much CreatePod() and RunPod().

()Funnily enough, it was there originally, but I moved it out for clarity since startVM wasn't just starting the VM so the name was confusing. Also, it matched more closely how we handle the shims (p.startShims()).

On the topic of CreatePod() and RunPod(), I've just raised #486 to remove the duplication so how about we land that first and I can then rebase this PR and there won't be any duplication.

sboeuf · 2017-11-13T19:24:51Z

 		return nil, err
 	}

+	err = p.waitForProxy()


Similar to createProxyProcess(), I think this should be moved to stopVM().

sboeuf · 2017-11-13T19:30:46Z

+	// cannot fail on Linux
+	proc, _ := os.FindProcess(pid)
+
+	_, err := proc.Wait()


We should add a timeout here.

Good call.

Looking at this again, the code actually won't work as we can't guarantee the calling process (now) owns the cc-proxy instance (it's likely to have been reparented to PID 1).

Do we already have BKM code somewhere for a (deep breath) pid or/proc/$pid-sniffing loop? I'm pretty sure we used to a long time ago but can't find it atm.

sboeuf · 2017-11-13T19:37:06Z

+
+// waitForProxy waits for the proxy process to finish
+func (p *Pod) waitForProxy() error {
+	pid := p.proxyProcess.Pid


I don't see any place where you save this info about the proxy PID. When DeletePod() will be called, I think that p.proxyProcess is going to be empty.

Good catch - fixing...

sboeuf · 2017-11-13T19:43:51Z

clearcontainers/proxy#167 has been merged, there is no blocker left for this PR.

devimc · 2017-11-13T21:04:16Z

+
+	// construct the socket path the proxy instance will use
+	socketPath := filepath.Join(runStoragePath, pod.id, "proxy.sock")
+	uri := fmt.Sprintf("unix://%s", socketPath)


I wonder if we have to ensure proxy's socket mode is 600

This is just constructing a path to give to the proxy. The proxy itself creates the socket (with perms 0660) here:

https://github.com/clearcontainers/proxy/blob/master/proxy.go#L674

ohh I see, thanks @jodh-intel for the link

sameo · 2017-11-14T14:42:39Z

 	}

+	// Start the proxy
+	err = p.createProxyProcess()


Nit: Could we call that one startProxy() for consistency with the shim part?

Good call - fixing...

sameo · 2017-11-14T15:59:04Z

+
+	p.proxyProcess = newProcess("", pid)
+
+	p.Logger().WithField("proxy-pid", pid).Debug("proxy started")


I'd add the pod ID as part of the log here.

Actually, Logger() will already add that for us ;)

sameo · 2017-11-14T16:12:18Z

+	socketPath := filepath.Join(runStoragePath, pod.id, "proxy.sock")
+	uri := fmt.Sprintf("unix://%s", socketPath)
+
+	args := []string{config.Path, "-uri", uri}


Maybe we could modify the process comm here by using something like "proxy-%s", pod.ID.

We already have pod.id in the path as the socket is placed into the pod-specific directory. We could also add the pod ID to the socket name, but if we do, we'd be getting very close to the 107 byte socket name limit.

jodh-intel · 2017-11-16T16:40:39Z

Branch updated with some simplifications... and more tests! 😄

jodh-intel · 2017-11-16T18:02:25Z

CI is failing due to the API breakage introduced by this PR (CCProxyConfig has changed).

sboeuf · 2017-11-16T18:05:00Z

@jodh-intel then you have to submit the PR on the runtime and point this PR in your commit message.

Quote the url values in the error for maximum clarity if a url is blank. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Change the log message in startShims() to avoid a confusing "shim-count: 0" log entry. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Change "defaultCCProxyURL" to use a Clear Containers 3.x path, rather than a 2.x one. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Test re-vendor for containers/virtcontainers#483. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Update the runtime based on containers/virtcontainers#483, and accompanying changes for the virtcontainers API changes. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Test re-vendor for containers/virtcontainers#483. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Update the runtime based on containers/virtcontainers#483, and accompanying changes for the virtcontainers API changes. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel · 2017-11-27T15:53:23Z

Hi @sboeuf - please can you take another look?

sboeuf · 2017-11-27T16:09:08Z

@jodh-intel yes sure, let me check that right now

Create a proxy instance for each pod (virtual machine). Since the proxy is now launched by virtcontainers, it is necessary to specify the path to the proxy binary in the pod configuration. Depends-on: github.com/clearcontainers/runtime#833 Fixes containers#478. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Update the runtime based on containers/virtcontainers#483, and accompanying changes for the virtcontainers API changes. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel · 2017-11-27T17:45:25Z

@sboeuf - branch updated again.

sboeuf · 2017-11-27T17:46:13Z

@jodh-intel thx, let me take a look !

sboeuf · 2017-11-27T17:53:18Z

LGTM

sboeuf · 2017-11-27T17:54:02Z

Thanks for the PR @jodh-intel, everything looks good now.
@sameo could you please take a look before I merge this ?

sameo · 2017-11-28T12:06:31Z

LGTM

jodh-intel added in progress do-not-merge labels Nov 13, 2017

jodh-intel mentioned this pull request Nov 13, 2017

Use one proxy per VM clearcontainers/runtime#795

Open

7 tasks

sboeuf mentioned this pull request Nov 13, 2017

One proxy per vm clearcontainers/proxy#167

Merged

sboeuf suggested changes Nov 13, 2017

View reviewed changes

sboeuf requested review from amshinde and sameo November 13, 2017 19:43

devimc reviewed Nov 13, 2017

View reviewed changes

sameo reviewed Nov 14, 2017

View reviewed changes

jodh-intel force-pushed the one-proxy-per-vm branch from c3211e3 to 7f7c404 Compare November 16, 2017 16:38

jodh-intel removed the do-not-merge label Nov 16, 2017

jodh-intel mentioned this pull request Nov 20, 2017

"docker run" never returns #491

Closed

James O. D. Hunt added 3 commits November 27, 2017 11:00

shim: Quote urls in error

c993e45

Quote the url values in the error for maximum clarity if a url is blank. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

shim: Only show shim-count if there are any

d8f2a22

Change the log message in startShims() to avoid a confusing "shim-count: 0" log entry. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

proxy: Use CC3 proxy path

619300a

Change "defaultCCProxyURL" to use a Clear Containers 3.x path, rather than a 2.x one. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel force-pushed the one-proxy-per-vm branch from 7f7c404 to f21bd67 Compare November 27, 2017 11:16

jodh-intel pushed a commit to jodh-intel/runtime that referenced this pull request Nov 27, 2017

vendor: Re-vendor virtcontainers.

f0df8e4

Test re-vendor for containers/virtcontainers#483. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel pushed a commit to jodh-intel/runtime that referenced this pull request Nov 27, 2017

vendor: Re-vendor virtcontainers.

8e2040d

Test re-vendor for containers/virtcontainers#483. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel force-pushed the one-proxy-per-vm branch from f21bd67 to 1d9d48d Compare November 27, 2017 11:59

jodh-intel mentioned this pull request Nov 27, 2017

[Do Not Merge] One proxy per vm vc test revendor clearcontainers/runtime#833

Closed

jodh-intel force-pushed the one-proxy-per-vm branch from 1d9d48d to 65e3d91 Compare November 27, 2017 14:24

jodh-intel force-pushed the one-proxy-per-vm branch from 65e3d91 to ebc46d7 Compare November 27, 2017 15:03

jodh-intel force-pushed the one-proxy-per-vm branch from ebc46d7 to 148b06a Compare November 27, 2017 17:44

sboeuf approved these changes Nov 27, 2017

View reviewed changes

sameo merged commit 9294388 into containers:master Nov 28, 2017

sameo removed the in progress label Nov 28, 2017


		p.proxyProcess = newProcess("", pid)

		p.Logger().WithField("proxy-pid", pid).Debug("proxy started")

Conversation

jodh-intel commented Nov 13, 2017

Uh oh!

jodh-intel commented Nov 13, 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!

sboeuf commented Nov 13, 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!

jodh-intel commented Nov 16, 2017

Uh oh!

jodh-intel commented Nov 16, 2017

Uh oh!

sboeuf commented Nov 16, 2017

Uh oh!

jodh-intel commented Nov 27, 2017

Uh oh!

sboeuf commented Nov 27, 2017

Uh oh!

jodh-intel commented Nov 27, 2017

Uh oh!

sboeuf commented Nov 27, 2017

Uh oh!

sboeuf commented Nov 27, 2017

Uh oh!

sboeuf commented Nov 27, 2017

Uh oh!

sameo commented Nov 28, 2017 • edited by dlespiau Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sameo commented Nov 28, 2017 •

edited by dlespiau

Loading