docs: Update arch doc for one proxy / VM #909

jodh-intel · 2018-01-09T07:48:09Z

Update the architecture document to reflect the new proxy behaviour
where there is a proxy instance per virtual machine rather than a
single host-level proxy instance.

This should strictly have been handled on #835.

Fixes #908.

Signed-off-by: James O. D. Hunt james.o.hunt@intel.com

jodh-intel · 2018-01-09T07:50:25Z

Hi @sameo, @sboeuf, @grahamwhaley - could you take a look?

Note also: clearcontainers/proxy#193.

clearcontainersbot · 2018-01-09T08:10:29Z

kubernetes qa-passed 👍

sboeuf · 2018-01-09T08:20:36Z

LGTM

sameo · 2018-01-09T09:13:03Z

docs/architecture/architecture.md

-  the `cc-agent`.
-
-The `cc-proxy` API is available through a single socket for all `cc-shim` and
+to a single `cc-shim` and `cc-runtime` client. Its main role is to route the


A single cc-shim?

Good catch! Branch updated.

jodh-intel · 2018-01-09T10:11:06Z

Hi @klynnrif - please could you my changes [*]?

[*] - If there are other parts of the doc that need altering, let's do that on a separate PR as this doc is a bit of a monster 😄

grahamwhaley

Thanks for picking this up @jodh-intel Minor nits, but generally:
lgtm

grahamwhaley · 2018-01-09T10:14:33Z

docs/architecture/architecture.md

-6. The `cc-proxy` waits for the agent to signal that it is ready and then returns
+5. Spawn the `cc-proxy` process providing a single argument:
+  `cc-proxy --uri $(uri)`
+   * A UNIX socket URI, which will be used by the `cc-shim` and `cc-agent` processes will use to pass information between them.


I think s/will use// ?

grahamwhaley · 2018-01-09T10:15:47Z

docs/architecture/architecture.md

  a token. This token uniquely identifies a process within a container inside
  the virtual machine.
-7. Spawn the `cc-shim` process providing two arguments:
+8. Spawn the `cc-shim` process providing two arguments:


Was surprised you did not change these all to (1.) items whilst here @jodh-intel:-)

I considered it, but then decided to go for a minimal change "in line" with the current content to minimise the review cycle time ;)

grahamwhaley · 2018-01-09T10:16:37Z

docs/architecture/architecture.md

 4. `cc-runtime` sends an agent `EXECMD` command to start the command in the
-  right container. The command is sent to `cc-proxy` who forwards it to the right
-  agent instance running in the appropriate guest.
+  container. The command is sent to `cc-proxy` who forwards it to the


s/who/which/

However, branch updated 😄

clearcontainersbot · 2018-01-09T10:24:45Z

kubernetes qa-passed 👍

clearcontainersbot · 2018-01-09T11:27:03Z

kubernetes qa-passed 👍

klynnrif

A couple suggested rewrites - @jodh-intel I believe I only commented on your changes this time :) Thanks!

klynnrif · 2018-01-09T17:56:21Z

docs/architecture/architecture.md

-6. The `cc-proxy` waits for the agent to signal that it is ready and then returns
+5. Spawn the `cc-proxy` process providing a single argument:
+  `cc-proxy --uri $(uri)`
+   * A UNIX socket URI, which will be used by the `cc-shim` and `cc-agent` processes will use to pass information between them.


Agree with @grahamwhaley here, to keep this active - suggested rewrite that may need adjusted for meaning: A UNIX socket URI used by the cc-shim and cc-agent processes is used to pass information between them.

klynnrif · 2018-01-09T18:11:11Z

docs/architecture/architecture.md

 4. `cc-runtime` sends an agent `EXECMD` command to start the command in the
-  right container. The command is sent to `cc-proxy` who forwards it to the right
-  agent instance running in the appropriate guest.
+  container. The command is sent to `cc-proxy` who forwards it to the


Suggested rewrite: the command is sent to cc-proxy, which forwards it to the

klynnrif · 2018-01-09T18:13:15Z

docs/architecture/architecture.md

  that the virtual machine that used to host the pod should no longer be used.
-5. `cc-runtime` explicitly shuts the virtual machine down.
-6. The host namespaces are cleaned up and destroyed. In particular, `cc-runtime`
+5. The `cc-proxy` instance will then exit.


The cc-proxy instance exits.

klynnrif · 2018-01-09T18:15:11Z

docs/architecture/architecture.md

-5. `cc-runtime` explicitly shuts the virtual machine down.
-6. The host namespaces are cleaned up and destroyed. In particular, `cc-runtime`
+5. The `cc-proxy` instance will then exit.
+6. `cc-runtime` explicitly shuts the virtual machine down.


cc-runtime shuts the virtual machine down.

klynnrif · 2018-01-09T18:15:51Z

docs/architecture/architecture.md

  offloads the networking namespace cleanup path by calling into the specific
  networking model (CNM or CNI) removal method.
-7. All remaining pod related resources on the host are deleted.
+8. All remaining pod related resources on the host are deleted.


All remaining pod-related resources...

Update the architecture document to reflect the new proxy behaviour where there is a proxy instance per virtual machine rather than a single host-level proxy instance. This should strictly have been handled on clearcontainers#835. Fixes clearcontainers#908. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel · 2018-01-10T08:38:16Z

Hi @klynnrif - thanks for reviewing - branch updated.

clearcontainersbot · 2018-01-10T11:26:32Z

kubernetes qa-passed 👍

…atFs build: introduction of archConvertStatFs function

sboeuf approved these changes Jan 9, 2018

View reviewed changes

sameo reviewed Jan 9, 2018

View reviewed changes

jodh-intel force-pushed the arch-doc-one-proxy-per-vm branch from 0d32593 to 8392219 Compare January 9, 2018 10:03

grahamwhaley approved these changes Jan 9, 2018

View reviewed changes

jodh-intel force-pushed the arch-doc-one-proxy-per-vm branch from 8392219 to 87221dd Compare January 9, 2018 10:52

klynnrif suggested changes Jan 9, 2018

View reviewed changes

jodh-intel force-pushed the arch-doc-one-proxy-per-vm branch from 87221dd to 8062cb9 Compare January 10, 2018 08:37

klynnrif approved these changes Jan 10, 2018

View reviewed changes

jodh-intel merged commit 8ababd7 into clearcontainers:master Jan 10, 2018

jodh-intel mentioned this pull request Mar 20, 2018

Why doesn't cc-proxy build cc-proxy-config anymore? clearcontainers/packaging#290

Closed

mcastelino pushed a commit to mcastelino/runtime that referenced this pull request Dec 6, 2018

Merge pull request clearcontainers#909 from alicefr/add_archConvertSt…

c0551de

…atFs build: introduction of archConvertStatFs function

docs: Update arch doc for one proxy / VM #909

docs: Update arch doc for one proxy / VM #909

Uh oh!

Conversation

jodh-intel commented Jan 9, 2018

Uh oh!

jodh-intel commented Jan 9, 2018

Uh oh!

clearcontainersbot commented Jan 9, 2018

Uh oh!

sboeuf commented Jan 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jodh-intel commented Jan 9, 2018

Uh oh!

grahamwhaley left a comment

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!

clearcontainersbot commented Jan 9, 2018

Uh oh!

clearcontainersbot commented Jan 9, 2018

Uh oh!

klynnrif left a comment

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 Jan 10, 2018

Uh oh!

clearcontainersbot commented Jan 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants