config: Add VM-based container configuration section #949

sameo · 2018-02-12T14:59:05Z

This adds a section to describe VM based container configurations to be
used by OCI runtimes using hardware virtualization to provide another
layer of isolation.

As part of this section we define 3 entries:

A virtual machine root image opbject. This is the guest image that
contains the virtual machine root filesystem. The container image will
be mounted on top of that filesystem.
A virtual machine kernel object. This is the kernel that will boot
inside the virtual machine. The object describes the host kernel image
path, additional parameters and an optional guest initrd for the
kernel to use.
A virtual machine hypervisor object. This is the hypervisor that will
manage the container virtual machine from the host. The object
describe a hypervisor binary path and some additional parameters.

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

Important note: This is a refresh of the very long standing PR #405.

sameo · 2018-02-12T15:02:41Z

cc @vbatts @mrunalp @runcom @stevvooe @jodh-intel

sameo · 2018-02-12T15:06:10Z

cc @gnawux @bergwolf @laijs

Could you please provide some feedback here?

wking · 2018-02-12T17:29:28Z

config-vm.md

+Used by virtual-machine-based runtimes only.
+
+* **`path`** (string, required) specifies the host path to the hypervisor used to manage the container virtual machine.
+* **`parameters`** (string, optional) specifies a space-separated list of parameters to pass to the hypervisor.


If this is going to be an array, can it be a JSON array (“array of strings”) instead of a single JSON string with internal space delimiters?

Given that this is supposed to be a parameter list that could be passed as is to the command, and would obviously be quite specific to that binary, what would be the advantage of using a JSON array?

Given that this is supposed to be a parameter list that could be passed as is to the command, and would obviously be quite specific to that binary...

So is execv's argv, and we use an array there.

... what would be the advantage of using a JSON array?

No need to quote/escape spaces within arguments and a simpler structure for removing/replacing arguments while composing the array.

wking · 2018-02-12T17:32:08Z

config-vm.md

@@ -0,0 +1,49 @@
+# <a name="VirtualMachineSpecificContainerConfiguration" /> Virtual-machine-specific Container Configuration
+
+Virtual-machine-based containers require additional configuration to that specified in the [default spec configuration](config.md).


config.md isn't the “default” configuration, it's the (mostly) platform-agnostic configuration. It's not entirely platform-agnostic (e.g. hooks is POSIX-only), but I think we can do better than “default”. Maybe “base”? “main”?

wking · 2018-02-12T17:33:55Z

config-vm.md

+
+This **optional** configuration is specified in a "VM" object:
+
+* **`imagePath`** (string, required) host path to file that represents the root filesystem for the container virtual machine.


“required” → “REQUIRED” (and similarly below, also “optional” → “OPTIONAL”. The casing needs to match the declarations here.

wking · 2018-02-13T00:20:35Z

config-vm.md

+
+* **`imagePath`** (string, REQUIRED) host path to file that represents the root filesystem for the container virtual machine.
+* **`kernel`** (object, REQUIRED) specifies details of the kernel to boot the container virtual machine with.
+* **`hypervisor`** (object, REQUIRED) specifies details of the hypervisor that manages the container virtual machine.


Do all VM-based runtimes support multiple hypervisors? I'd expect this to be OPTIONAL, with an implementation-defined default.

laijs · 2018-02-13T03:28:46Z

config-vm.md

+
+This **optional** configuration is specified in a "VM" object:
+
+* **`imagePath`** (string, REQUIRED) host path to file that represents the root filesystem for the container virtual machine.


It (imagePath) should be optional.

imagePath and initrd are both booting media. We need to mark them both optional and document that at least one of them should be specified.

Moreover, since imagePath and initrd are both booting media, they are recommended to be put together. So we can move imagePath to the kernel section around the initrd field. After we do it, we may also need to rename kernel to boot and rename kernel.path to boot.kernel. Another reason why boot is better than kernel here is that bios or cbfs can be added to the boot section if any team strongly require them.

Finally, I like the name imagePath, but I didn't expect that it has ambiguous with the path field in the Root Configuration section. So I suggest to rename it to initImage, since it and initrd are almost the same functionality.

I'm pretty sure you're familiar with that already, but an initrd is typically linked to a specific kernel build whereas the imagePath here is pointing at a kernel agnostic artifact. So although I agree it makes sense to make imagePath optional, I don't think it should be under the kernel section. As a matter of fact, I think the imagePath should be part of an image section where further down the road we could add an image type field.
I like the boot suggestion, especially because it could include a firmware path (this could very well be needed, I agree). But I'd prefer to have an optional firmware section instead.
I like the boot suggestion and

sameo · 2018-02-14T12:43:27Z

@wking @laijs Updated with changes for your suggestions/comments.

laijs · 2018-02-15T14:47:05Z

LGTM

wking · 2018-02-15T17:36:15Z

config-vm.md

+
+Virtual-machine-based containers require additional configuration to that specified in the [base spec configuration](config.md).
+
+This **optional** configuration is specified in a "VM" object:


This line doesn't seem to have a precedent in the other platform-specific sections. Can we drop it? If we follow the Linux pattern, we'd have something like:

This section describes the schema for the [virtual-machine-specific section](config.md#platform-specific-configuration) of the [container configuration](config.md). The virtual-machine container specification provides additional configuration for the hypervisor, kernel, and image.

wking · 2018-02-15T17:38:28Z

config-vm.md

+
+* **`hypervisor`** (object, OPTIONAL) specifies details of the hypervisor that manages the container virtual machine.
+* **`kernel`** (object, REQUIRED) specifies details of the kernel to boot the container virtual machine with.
+* **`image`** (object, OPTIONAL) specifies details of the image that contains the root filesystem for the container virtual machine.


Other platform-specific sections don't take this table-of-contents approach, and instead define the properties (hypervisor, etc.) in the section that describes their structure. For example, devices (linux.devices) is defined here, immediately after the Devices section header.

wking · 2018-02-15T17:40:04Z

config-vm.md

+
+## <a name="HypervisorObject" /> Hypervisor Object
+
+Used by virtual-machine-based runtimes only.


I don't see a precedent for this line (which you also use in the following sections). I think we can drop the lines, because the semantics are implied by the presence of this section within the Virtual-machine-specific Container Configuration section.

wking · 2018-02-15T17:50:04Z

config-vm.md

+
+Used by virtual-machine-based runtimes only.
+
+* **`path`** (string, REQUIRED) specifies the host path to the hypervisor used to manage the container virtual machine.


MUST this path be absolute? If so, precedent suggests wording like:

This value MUST be an absolute path in the [runtime mount namespace](glossary.md#runtime-namespace).

or:

This value MUST be an absolute path.

If, on the other hand, relative paths are allowed, precedent suggests:

This value is either absolute or relative to the bundle.

I prefer “runtime mount namespace”, for which we have a glossary entry, over your current “host path”. The glossary entry is currently Linux-specific, but the idea is generic enough that we can generalize the entry. For this particular property, it's hard to imagine someone expecting a container mount namespace path, but since we've already defined terminology for this concept, I think we should use it.

wking · 2018-02-15T17:51:24Z

config-vm.md

+
+Used by virtual-machine-based runtimes only.
+
+* **`path`** (string, REQUIRED) specifies the host path to the kernel used to boot the container virtual machine. This is an absolute path on the host.


To restrict to absolute paths, I think you want a MUST in there somewhere (otherwise validation tooling can't complain). See my earlier comment for examples of existing spec wording for this restriction.

wking · 2018-02-15T17:52:46Z

config-vm.md

+
+* **`path`** (string, REQUIRED) specifies the host path to the kernel used to boot the container virtual machine. This is an absolute path on the host.
+* **`parameters`** (array of strings, OPTIONAL) specifies an array of parameters to pass to the kernel.
+* **`initrd`** (string, OPTIONAL) specifies the host path to an initial ramdisk to be used by the container virtual machine.


Same path-wording issues as above. Are relative paths allowed? Do you want to use existing terminology to bind to the runtime mount namespace?

wking · 2018-02-15T17:53:52Z

config-vm.md

+
+Used by virtual-machine-based runtimes only.
+
+* **`path`** (string, REQUIRED) specifies the absolute host path to the container virtual machine root image. This image contains the root filesystem that the virtual machine **`kernel`** will boot into, not to be confused with the container root filesystem itself. The latter, as specified by **`path`** from the [Root Configuration](config.md#Root-Configuration) section, will be mounted inside the virtual machine at a location chosen by the virtual-machine-based runtime.


Same path-wording issues as above.

wking · 2018-02-15T18:00:30Z

config-vm.md

+* **`path`** (string, REQUIRED) specifies the absolute host path to the container virtual machine root image. This image contains the root filesystem that the virtual machine **`kernel`** will boot into, not to be confused with the container root filesystem itself. The latter, as specified by **`path`** from the [Root Configuration](config.md#Root-Configuration) section, will be mounted inside the virtual machine at a location chosen by the virtual-machine-based runtime.
+
+
+## <a name="FullyPopulatedVMExample" /> Example of a fully-populated `VM` object


There's no precedent for “fully-populated”. I'd rather have per-vm-child-property examples like the other platform-specific sections. I have no preference between rooted examples (like the Windows section uses) or relative example (like the Solaris and Linux sections use). A fully-populated example could go in here. And if you decide to keep a full vm example here, I'd just title it “Example” and rely on section nesting to define the example scope.

wking · 2018-02-15T18:06:07Z

config.md

@@ -462,6 +462,12 @@ Instead they MUST ignore unknown properties.
 Runtimes that are reading or processing this configuration file MUST generate an error when invalid or unsupported values are encountered.
 Unless support for a valid value is explicitly required, runtimes MAY choose which subset of the valid values it will support.

+## <a name="VirtualMachine" />VM


The VM-specific section isn't quite platform-specific, but I think this needs to go into the platform-specific section because not all runtimes will be able to support these properties, and platforms are linked to the compliance language. This is one of the reasons I prefer “protocol” to “platform”.

You should also define the vm property wherever the Virtual Machine entry ends up in config.md.

sameo · 2018-02-16T11:09:25Z

@wking I think I addressed all your comments, thanks a lot for the thorough review.

wking · 2018-02-16T19:27:33Z

schema/config-schema.json

@@ -199,6 +199,9 @@
        },
        "windows": {
            "$ref": "config-windows.json#/windows"
+        },
+	"vm": {


This looks like a non-standard indent. You can fix it with:

schema $ make fmt

wking · 2018-02-16T19:30:39Z

config.md

@@ -349,6 +349,8 @@ For Windows based systems the user structure has the following fields:
    This MUST be set if the target platform of this spec is `windows`.
 * **`solaris`** (object, OPTIONAL) [Solaris-specific configuration](config-solaris.md).
    This MAY be set if the target platform of this spec is `solaris`.
+* **`vm`** (object, OPTIONAL) [Virtual-machine-specific configuration](config-vm.md).


You'll also want to add virtual-machine entries here and here.

wking · 2018-02-16T19:32:31Z

specs-go/config.go

+// VM contains information for virtual-machine-based containers.
+type VM struct {
+	// Hypervisor specifies hypervisor-related configuration for virtual-machine-based containers.
+	Hypervisor VMHypervisor `json:"hypervisor"`


hypervisor is optional, so you'll want:

Hypervisor *VMHypervisor `json:"hypervisor,omitempty"`

And similarly for Image below.

wking · 2018-02-16T19:35:22Z

schema/config-vm.json

+{
+    "vm": {
+        "description": "configuration for virtual-machine-based containers",
+        "id": "https://opencontainers.org/schema/bundle/vm",


You should drop these id entries (#945).

wking · 2018-02-16T19:37:55Z

schema/config-vm.json

+        "id": "https://opencontainers.org/schema/bundle/vm",
+        "type": "object",
+        "required" : [
+            "kernel",


This trailing comma is invalid JSON.

sameo · 2018-02-18T22:26:18Z

@wking All comments addressed, thanks.

wking · 2018-02-19T18:42:39Z

3d8cce4 looks good to me. It doesn't add any new runtime requirements though, so there would be no way to check for runtime compliance. That's true for other portions of the spec as well though (e.g. most of the Linux cgroups config, #746), so it may not be an issue for maintainers. If it is an issue, we'll want additional language like:

Runtimes MUST execute the `hypervisor` with [IEEE Std 1003.1-2008 `execvp`'s *argv*][ieee-1003.1-2008-functions-exec] set to the union of `path` and `parameters`.

for hypervisor, and similarly for the other properties.

Actually, for hypervisor, I'd encourage reuse of the existing process or hooks schema, which would make life easier for implementations (see here where I called for those to be unified). If you use the hooks schema, you get very similar semantics to your existing hypervisor schema, except that you have args instead of parameters, and you gain env and timeout. You also get to punt on runtime requirements as “not a problem for this PR, because we're leaning on current spec language”. That seems like a win to me.

vbatts

I like how simple this this

vbatts · 2018-03-07T16:23:10Z

specs-go/config.go

+// VMImage contains information about the virtual machine root image.
+type VMImage struct {
+	// Path is the host path to the root image that the VM kernel would boot into.
+	Path string `json:"path"`


Is this leaving the type of that path completely up to the runtime? (qcow, raw, directory, etc)

IIUC, this image is a small disk and is used for booting only. Making it raw is enough.

By the way, I hope rootfs and volume support media types. But it is not the purpose of this PR.

vbatts · 2018-03-07T16:25:02Z

config.md

@@ -349,6 +349,8 @@ For Windows based systems the user structure has the following fields:
    This MUST be set if the target platform of this spec is `windows`.
 * **`solaris`** (object, OPTIONAL) [Solaris-specific configuration](config-solaris.md).
    This MAY be set if the target platform of this spec is `solaris`.
+* **`vm`** (object, OPTIONAL) [Virtual-machine-specific configuration](config-vm.md).


mrunalp · 2018-03-07T21:48:56Z

LGTM

mrunalp · 2018-03-07T21:49:08Z

@crosbymichael PTAL

crosbymichael · 2018-03-09T16:11:15Z

config-vm.md

+**`image`** (object, OPTIONAL) specifies details of the image that contains the root filesystem for the container virtual machine.
+* **`path`** (string, REQUIRED) path to the container virtual machine root image.
+    This value MUST be an absolute path in the [runtime mount namespace](glossary.md#runtime-namespace).
+* **`format`** (string, REQUIRED) format of the container virtual machine root image. The following root image formats are supported:


Supported by what? Who? Support for formats seem like an implementation detail to me for various runtimes, not something that should be dictated by the spec.

@crosbymichael Supported by the current major hypervisors. So you'd prefer to see a free form string that would only be a hint to the hypervisor?

I guess the only worry I have is that people would read the spec and the certification committee will say that these are MUSTs that all runtimes have to support if we have it in the spec.

Could we change the wording to something like "commonly supported formats"?

That would be better indeed, thanks. Let me change that.

Cool, LGTM after this change

This adds a section to describe VM based container configurations to be used by OCI runtimes using hardware virtualization to provide another layer of isolation. As part of this section we define 3 entries: - A virtual machine root image opbject. This is the guest image that contains the virtual machine root filesystem. The container image will be mounted on top of that filesystem. - A virtual machine kernel object. This is the kernel that will boot inside the virtual machine. The object describes the host kernel image path, additional parameters and an optional guest initrd for the kernel to use. - A virtual machine hypervisor object. This is the hypervisor that will manage the container virtual machine from the host. The object describe a hypervisor binary path and some additional parameters. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

crosbymichael · 2018-03-09T22:24:38Z

LGTM

crosbymichael · 2018-03-09T22:24:42Z

Thanks!

wking · 2018-03-09T22:51:46Z

Now that two separate maintainers have (at various points) LGTMed this PR, I'm getting jumpy about it landing without sameo#1 getting a look over. I've just rebased that on top of is PR's current 74b670e, but I'd really like to avoid a third process-launching schema in hypervisor (wking/opencontainer-runtime-spec@edd501a6 shrinks the repo by 19 lines), so if there's anything I can do to make the sub-PR more appealing, let me know.

sameo · 2018-03-12T12:54:13Z

@wking If maintainers want to re-use existing process schema, I'd much prefer to go with wking/opencontainer-runtime-spec@edd501a . The Process description is fairly container specific but we've contemplated containerizing the hypervisor in the past anyway. The only concern I have about it is User being a mandatory field. In most cases the hypervisor/VMM will be called from a runtime implementation (kata-runtime binary or a CRI implementation) and will likely want to run under the same user as the caller, under the same user namespace. Forcing caller to specify this is a little redundant and cumbersome, or is there a way to specify that implicitly?

wking · 2018-03-12T13:27:27Z

If maintainers want to re-use existing process schema, I'd much prefer to go with wking/opencontainer-runtime-spec@edd501a .

I agree. The commit after that was because @crosbymichael specifically requested a copy/paste implementation and not edd501a's reference. I'm hoping I misunderstood, or that he changes his mind after seeing both approaches.

The only concern I have about it is User being a mandatory field.

I'm in favor of making it optional, with unset user properties meaning "don't change this". Previous discussion in #417, #489, #618. It's been required since 8af3eb6. A possible compromise would be removing it from hypervisor, as I'm already doing for the terminal properties in edd501a.

sameo · 2018-03-12T13:41:24Z

A possible compromise would be removing it from hypervisor

You mean by specifying that it MUST NOT be configured ?

wking · 2018-03-12T13:56:15Z

A possible compromise would be removing it from hypervisor

You mean by specifying that it MUST NOT be configured?

Yeah, at least as long as it's required for process.

crosbymichael · 2018-03-15T13:42:11Z

ping @mrunalp can you review again?

sameo · 2018-03-16T09:10:07Z

@crosbymichael @mrunalp @stevvooe Could you also please let us know if you want to use the process schema @wking is proposing or not?

crosbymichael · 2018-03-16T14:28:56Z

@sameo I think it's fine as is. The majority of the fields on Process do not apply here.

wking · 2018-03-21T13:59:15Z

I think it's fine as is. The majority of the fields on Process do not apply here.

So MUST more of them to be unset? Or recycle the hook structure instead? I'd still like to have this process-launching structure avoid being a third, incompatible way to set argv.

We already have two ways to specify a process to launch (for the container process and for hooks). This commit recycles the hook-process schema for launcing the hypervisor. I prefer using the more-fundamental container process schema, but Michael felt it had too many properties that wouldn't apply to the hypervisor-launching case [1]. [1]: opencontainers#949 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

wking · 2018-03-21T16:26:27Z

Or recycle the hook structure instead?

I've filed sameo#2 in case folks want to see what this looks like. With this approach, runtimes can use their existing hook-launching code (although they won't be writing the container state to stdin for hypervisor). They also gain access to env and timeout, and we gain the POSIX references for the path and args definitions while dropping the repository size by 18 lines.

crosbymichael · 2018-03-30T14:59:29Z

ping @opencontainers/runtime-spec-maintainers

bergwolf · 2018-03-30T16:04:08Z

specs-go/config.go

+// VM contains information for virtual-machine-based containers.
+type VM struct {
+	// Hypervisor specifies hypervisor-related configuration for virtual-machine-based containers.
+	Hypervisor VMHypervisor `json:"hypervisor,omitempty"`


If Hypervisor is ever set in a container spec, it can only be run by the same hypervisor type and the same deployment. Is it acceptable from OCI point of view?

Ahh, good point. Shouldn't the hypervisor be exec'd with the correct path and args anyways from a runtime?

This has been spun off into #964.

hqhq · 2018-03-31T01:32:26Z

LGTM

We already have two ways to specify a process to launch (for the container process and for hooks). This commit recycles the hook-process schema for launching the hypervisor. I prefer using the more-fundamental container process schema, but Michael felt it had too many properties that wouldn't apply to the hypervisor-launching case [1]. [1]: opencontainers#949 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

sameo force-pushed the vm-section branch from 92005bf to fcc4888 Compare February 12, 2018 15:44

wking reviewed Feb 12, 2018

View reviewed changes

sameo force-pushed the vm-section branch from fcc4888 to beb7adb Compare February 12, 2018 23:18

wking reviewed Feb 13, 2018

View reviewed changes

laijs reviewed Feb 13, 2018

View reviewed changes

sameo force-pushed the vm-section branch 3 times, most recently from 0741390 to 5e63443 Compare February 14, 2018 12:42

sameo force-pushed the vm-section branch from 5e63443 to 04533be Compare February 14, 2018 12:44

wking reviewed Feb 15, 2018

View reviewed changes

sameo force-pushed the vm-section branch 3 times, most recently from 9ae489f to fb96e51 Compare February 16, 2018 11:08

sameo force-pushed the vm-section branch from fb96e51 to 31c0ec7 Compare February 16, 2018 11:11

wking reviewed Feb 16, 2018

View reviewed changes

sameo force-pushed the vm-section branch from 31c0ec7 to 3d8cce4 Compare February 18, 2018 22:25

vbatts approved these changes Mar 7, 2018

View reviewed changes

sameo force-pushed the vm-section branch 2 times, most recently from 8eb8773 to 3e939f0 Compare March 7, 2018 23:55

sameo force-pushed the vm-section branch from 3445f13 to 2ab053e Compare March 9, 2018 11:09

crosbymichael reviewed Mar 9, 2018

View reviewed changes

sameo force-pushed the vm-section branch from 2ab053e to 74b670e Compare March 9, 2018 22:06

wking mentioned this pull request Mar 21, 2018

config-vm: Recycle the hook-process schema sameo/runtime-spec#2

Closed

bergwolf reviewed Mar 30, 2018

View reviewed changes

hqhq merged commit d362ed3 into opencontainers:master Mar 31, 2018

wking mentioned this pull request Apr 2, 2018

config-vm: Recycle the hook-process schema #962

Closed

vbatts mentioned this pull request Apr 4, 2018

Add VM-based runtime configuration details. #405

Closed

crosbymichael mentioned this pull request Apr 4, 2018

vm based args in spec?? #964

Open

This was referenced Sep 20, 2018

support config options through annotations kata-containers/runtime#753

Closed

support get kernel, image, hypervisor from oci spec kata-containers/runtime#782

Closed

vbatts mentioned this pull request Jan 9, 2020

Release v1.0.2 #1026

Closed

		@@ -0,0 +1,49 @@
		# <a name="VirtualMachineSpecificContainerConfiguration" /> Virtual-machine-specific Container Configuration

		Virtual-machine-based containers require additional configuration to that specified in the [default spec configuration](config.md).


		This optional configuration is specified in a "VM" object:

		* `imagePath` (string, required) host path to file that represents the root filesystem for the container virtual machine.


		## <a name="HypervisorObject" /> Hypervisor Object

		Used by virtual-machine-based runtimes only.


		Used by virtual-machine-based runtimes only.

		* `path` (string, REQUIRED) specifies the host path to the hypervisor used to manage the container virtual machine.

		* `path` (string, REQUIRED) specifies the absolute host path to the container virtual machine root image. This image contains the root filesystem that the virtual machine `kernel` will boot into, not to be confused with the container root filesystem itself. The latter, as specified by `path` from the [Root Configuration](config.md#Root-Configuration) section, will be mounted inside the virtual machine at a location chosen by the virtual-machine-based runtime.


		## <a name="FullyPopulatedVMExample" /> Example of a fully-populated `VM` object

config: Add VM-based container configuration section #949

config: Add VM-based container configuration section #949

Conversation

sameo commented Feb 12, 2018 • edited Loading

sameo commented Feb 12, 2018 • edited Loading

sameo commented Feb 12, 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

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 Feb 14, 2018

laijs commented Feb 15, 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

Choose a reason for hiding this comment

sameo commented Feb 16, 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

sameo commented Feb 18, 2018

wking commented Feb 19, 2018

vbatts left a 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

mrunalp commented Mar 7, 2018 • edited by caniszczyk Loading

mrunalp commented Mar 7, 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

crosbymichael commented Mar 9, 2018 • edited by caniszczyk Loading

crosbymichael commented Mar 9, 2018

wking commented Mar 9, 2018 • edited Loading

sameo commented Mar 12, 2018

wking commented Mar 12, 2018 • edited Loading

sameo commented Mar 12, 2018

wking commented Mar 12, 2018

crosbymichael commented Mar 15, 2018

sameo commented Mar 16, 2018

crosbymichael commented Mar 16, 2018

wking commented Mar 21, 2018 • edited Loading

wking commented Mar 21, 2018

crosbymichael commented Mar 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hqhq commented Mar 31, 2018 • edited by caniszczyk Loading

sameo commented Feb 12, 2018 •

edited

Loading

sameo commented Feb 12, 2018 •

edited

Loading

sameo commented Feb 12, 2018 •

edited

Loading

mrunalp commented Mar 7, 2018 •

edited by caniszczyk

Loading

crosbymichael commented Mar 9, 2018 •

edited by caniszczyk

Loading

wking commented Mar 9, 2018 •

edited

Loading

wking commented Mar 12, 2018 •

edited

Loading

wking commented Mar 21, 2018 •

edited

Loading

hqhq commented Mar 31, 2018 •

edited by caniszczyk

Loading