hypervisor: add hot plugging support for Qemu Q35 #456

devimc · 2017-10-30T15:06:56Z

Qemu Q35 machine type does not support to hot plug devices
directly on pcie.0, hence we have to find a way to allow users
hot plug N devices.
The only way to hot plug devices in Qemu Q35 is through PCI brdiges.
Each PCI bridge is able to support until 32 devices, therefore we have a
limitation in the amount of devices we can hot plug.
This patch adds support for hot plugging devices on PCI bridges in
pc and q35 machine types.

Signed-off-by: Julio Montes julio.montes@intel.com

devimc · 2017-10-30T15:08:27Z

@mcastelino @amshinde can you please review it ?

jodh-intel · 2017-10-30T15:56:19Z

bridge.go

+	pcieBridge            = "pcie"
+)
+
+const pciBridgeMaxCapacity = 30


Where does this limit come from?

Is there any way to avoid hard-coding this?

Where does this limit come from?

Qemu's documentation is not very good, I found this [1], but I'm not able to hot plug 32 devices,
I guess because of SHPC is enabled.

Is there any way to avoid hard-coding this?

I don't think so

[1] https://github.com/qemu/qemu/blob/master/docs/pcie.txt#L76

Having a peek at: https://en.wikipedia.org/wiki/PCI_configuration_space
looks like the per-bus device space is defined by a 5-bit field.
Maybe 0 and 0x1F are reserved - leaving 30 ? Just a guess. We could add a note saying this is part of the PCI spec I guess etc.

jodh-intel · 2017-10-30T16:03:18Z

bridge.go

+	// hotplug drive on this bridge
+	drive.Bus = b.ID
+
+	// hotplug drive on this bridge's addres


typo: "address".

jodh-intel · 2017-10-30T16:04:19Z

bridge.go

+	return bridges
+}
+
+func (b *Bridge) hotplugAddPCIDevice(h hypervisor, drive Drive, devType deviceType) error {


Am I missing something? This function and hotplugRemovePCIDevice() don't seem to be used?

ouch!, sorry I forgot to push some patches

PR updated, thanks

@devimc this function says hotplugAddPCIDevice. But this seems to be a specific implementation for a disk.

sboeuf · 2017-10-30T15:22:58Z

bridge.go

@@ -0,0 +1,122 @@
+//


Please fix errors in your commit message:

hotplug: Implement Bridge structure Bridge represents a PCI or PCIE bridge and can be used to hot plug PCI devices for pc and q35 machine types. Signed-off-by: Julio Montes <julio.montes@intel.com>

fixed, thanks

The title of the commit message is still wrong.

ahh ok s/Implements/Implement/
done, thanks

sboeuf · 2017-10-30T16:01:28Z

bridge.go

+	// hotplug drive on this bridge's addres
+	drive.Addr = fmt.Sprintf("0x%x", addr)
+
+	err := h.hotplugAddDevice(drive, devType)


I am not sure this is the right way to implement it inside virtcontainers. This should be called from the hypervisor implementation since different hypervisors might handle this in a different way.

@devimc you have not taken this comment into account.

sboeuf · 2017-10-30T16:01:34Z

bridge.go

+		return fmt.Errorf("Unable to hot unplug device %s: not present on bridge", drive.ID)
+	}
+
+	err := h.hotplugRemoveDevice(drive, devType)


why Ditto [1]?

[1] https://bulbapedia.bulbagarden.net/wiki/Ditto_(Pok%C3%A9mon) 😄

sboeuf · 2017-10-30T16:12:03Z

pod.go

+
+	// Bridges is the list of pci bridges attached to the pod
+	// pci bridges can be used to hot plug devices
+	Bridges []*Bridge


This should be an hypervisor resource IMO.

I think a bridge is also a resource, same as memory and CPU

@sboeuf what do you mean by hypervisor resource. This is a VM/podlevel resource as @devimc indicates.

devimc · 2017-10-30T16:56:36Z

@sboeuf @jodh-intel changes applied

sboeuf · 2017-10-30T17:40:26Z

device.go

-		if err = h.hotplugAddDevice(drive, blockDev); err != nil {
+		var err error
+		for _, b := range c.pod.config.VMConfig.Bridges {
+			err = b.hotplugAddPCIDevice(h, drive, blockDev)


That's exactly what I am pointing in a previous commit as not the right way to proceed. hotplugAddPCIDevice() should be called from the hypervisor implementation (no need for h as parameter in that case), and we should not modify the call here. Hypervisor implementation should be able to detect if it needs to hotplug a PCI device or not I think.

we need to save PCI bridges in the pod state, because of before trying to hot plug any device, we must know what slots are available, is the hypervisor structure saved in the pod state?

@devimc currently, we don't save any data related to hypervisor in a state file (relates to run-time which is different from config files storing the Hypervisor config). But I think that if you need to store some informations so that you can reuse them across different containers, you should maybe create a new state file for the hypervisor.
Before you do that, I'd like to make sure I understand properly what you need.

@sboeuf do we store the POD state somewhere? also where do we store details about the devices attached to the PODs and containers.

amshinde · 2017-10-31T16:40:40Z

bridge.go

+	}
+
+	// hotplug drive on this bridge
+	drive.Bus = b.ID


You are updating drive here. Wouldn't you require to pass pointer to the drive for the update?

amshinde · 2017-10-31T16:55:33Z

bridge.go

+	}
+
+	// free address to re-use the same slot with other devices
+	delete(b.Address, devAddr)


You are reusing the pci slot here, have you checked if the predicted drive name you get is correct after doing this.
You may have to change the logic to predict names based on the pci slot address, rather than index at which the drive is attached on the main bus that is being done currently. I had avoided it earlier, due to the differences in addresses that are available on pci and pcie bus. But now that we are using bridges with explicit pci addresses, we may need to revisit predicting the drive names.

@amshinde that is a good idea. This will ensure we are fully OS independent.

mcastelino · 2017-10-31T19:47:29Z

bridge.go

+	return bridges
+}
+
+func (b *Bridge) hotplugAddPCIDevice(h hypervisor, drive Drive, devType deviceType) error {


@devimc this function says hotplugAddPCIDevice. But this seems to be a specific implementation for a disk.

mcastelino · 2017-10-31T19:49:44Z

pod.go

+
+	// Bridges is the list of pci bridges attached to the pod
+	// pci bridges can be used to hot plug devices
+	Bridges []*Bridge


@sboeuf what do you mean by hypervisor resource. This is a VM/podlevel resource as @devimc indicates.

mcastelino · 2017-10-31T19:52:58Z

qemu.go

+				Bus:  bus,
+				ID:   b.ID,
+				// Each bridge is required to be assigned a unique chassis id > 0
+				Chassis: (idx + 1),


@devimc does it make sense to store the chassis id in the bridge structure for tracking. If not it becomes implicit.

I think it is not needed, we already have an ID to identify the bridge

mcastelino · 2017-10-31T19:54:00Z

qemu.go

+
+		// Addr is PCI specific, use ExecutePCIDeviceAdd if it's NOT empty
+		if drive.Addr != "" {
+			if err := q.qmpMonitorCh.qmp.ExecutePCIDeviceAdd(q.qmpMonitorCh.ctx, drive.ID, devID, driver, drive.Addr, drive.Bus); err != nil {


Best to make this function specific PCIBlockDeviceAdd or make it truly generic. Right now the function name does not match the implementation

mcastelino · 2017-10-31T19:56:23Z

bridge.go

+	Type bridgeType
+
+	//ID is used to identify the bridge in the hypervisor
+	ID string


Can we also add the chassis information here.

mcastelino · 2017-10-31T20:13:43Z

bridge.go

+	}
+
+	// free address to re-use the same slot with other devices
+	delete(b.Address, devAddr)


@amshinde that is a good idea. This will ensure we are fully OS independent.

mcastelino · 2017-10-31T20:15:20Z

device.go

-		if err = h.hotplugAddDevice(drive, blockDev); err != nil {
+		var err error
+		for _, b := range c.pod.config.VMConfig.Bridges {
+			err = b.hotplugAddPCIDevice(h, drive, blockDev)


@sboeuf do we store the POD state somewhere? also where do we store details about the devices attached to the PODs and containers.

devimc · 2017-11-01T16:55:15Z

@mcastelino @amshinde @sboeuf I did some changes in order to handle Sebastien's comments,
now:

hypervisor is who plug and unplug devices
hypervisor updates the bridges and its addresses
All bridges are part of the hypervisor state
hypervisor state is part of pod state

could you please re-review it? thanks

sboeuf · 2017-11-01T16:58:52Z

@devimc thanks for the update, I am gonna take a look

sboeuf

Still some changes, and I am sorry but I need to think about the best way to get this information stored for the bridges.
BTW, could you rebase your PR on latest master, and merge some of your commits. It would help to understand better. Having too many commits makes it complicated IMO. One commit per feature is usually a good thing.

sboeuf · 2017-11-02T06:13:06Z

bridge.go

+// Bridge is a bridge where devices can be hot plugged
+type Bridge struct {
+	// Address contains information about devices plugged and its address in the bridge
+	Address map[uint32]string


Correct me if I am wrong but no need for a very specific uint32 type here since that's only an index, right ?
uint should be okay. But on the other hand, if you really think about saving space, then you might want to go with uint8 which is gonna be enough for 30 possible entries.

pcie has more capacity, for this reason I'd like to keep uint32

sboeuf · 2017-11-02T06:16:24Z

bridge.go

+		return nil
+	}
+
+	for i := uint32(0); i < n; i++ {


Once again, if we really want to limit the memory, then let's use uint8.

sboeuf · 2017-11-02T06:17:46Z

bridge.go

+// addDevice on success adds the device ID to the bridge and return the address
+// where the device was added, otherwise an error is returned
+func (b *Bridge) addDevice(ID string) (uint32, error) {
+	var addr uint32


It's more a key than an address.

sboeuf · 2017-11-02T06:18:15Z

bridge.go

+// otherwise an error is returned
+func (b *Bridge) removeDevice(ID string) error {
+	// check if the device was hot plugged in the bridge
+	for addr, devID := range b.Address {


It's more a key than an address. Let's call it key.

sboeuf · 2017-11-02T06:23:45Z

hypervisor.go

@@ -161,6 +161,10 @@ type HypervisorConfig struct {
 	// Pod configuration VMConfig.Memory overwrites this.
 	DefaultMemSz uint32

+	// DefaultBridges specifies default number of Bridges for the VM.


s/Bridges/bridges/

sboeuf · 2017-11-02T06:24:00Z

hypervisor.go

@@ -161,6 +161,10 @@ type HypervisorConfig struct {
 	// Pod configuration VMConfig.Memory overwrites this.
 	DefaultMemSz uint32

+	// DefaultBridges specifies default number of Bridges for the VM.
+	// bridges can be used to hot plug devices


s/bridges/Bridges/

sboeuf · 2017-11-02T06:34:00Z

device.go

@@ -232,7 +232,7 @@ func (device *BlockDevice) attach(h hypervisor, c *Container) (err error) {

 		device.DeviceInfo.Hotplugged = false
 	} else {
-		if err = h.hotplugAddDevice(drive, blockDev); err != nil {
+		if err := c.pod.hypervisor.hotplugAddDevice(drive, blockDev); err != nil {


I think you can remove this commit. There is no point in using c.pod.hypervisor instead of the provided hypervisor.

sboeuf · 2017-11-02T06:36:27Z

device.go

 	if device.DeviceInfo.Hotplugged {
 		deviceLogger().WithField("device", device.DeviceInfo.HostPath).Info("Unplugging block device")

 		drive := Drive{
 			ID: makeBlockDevIDForHypervisor(device.DeviceInfo.ID),
 		}

-		if err := h.hotplugRemoveDevice(drive, blockDev); err != nil {
-			deviceLogger().WithError(err).Error("Failed to unplug block device")
+		if err := c.pod.hypervisor.hotplugRemoveDevice(drive, blockDev); err != nil {


Again, I don't see the point with this, hypervisor object is provided, it is better to use this than c.pod.hypervisor.

sboeuf · 2017-11-02T06:39:17Z

hypervisor.go

+type HypervisorState struct {
+	// Bridges is the list of pci bridges attached to the pod
+	// pci bridges can be used to hot plug devices
+	Bridges []*Bridge


You cannot do that unfortunately... The thing here is that you want to store this information through a file on the host filesystem. If you store a list of pointers, then when another instance of cc-runtime will read this file, those pointers will certainly not contain the data you expect.
You need a list of real objects

type HypervisorState struct { Bridges []Bridge }

sboeuf · 2017-11-02T07:04:27Z

pod.go

@@ -72,6 +72,9 @@ type State struct {

 	// Bool to indicate if the drive for a container was hotplugged.
 	HotpluggedDrive bool `json:"hotpluggedDrive"`
+
+	// HypervisorState keeps the state of the hypervisor
+	HypervisorState HypervisorState `json:"hypervisorState"`


This is not the way we have implemented the state files on virtcontainers. Let me think about it, I'll try to come up with a clean solution for this storage purpose (now that I am clear about what we expect).
@amshinde please take a look and give some inputs about this too.

sboeuf · 2017-11-02T18:41:05Z

@devimc I have discussed with @amshinde about the options we have to implement the storage of this list of bridges. I know what we want to store is at a Pod level (because it depends on the VM), but I don't want to store this through the pod state since this is very specific to the hypervisor implementation. Indeed, let's take the exemple of Xen hypervisor, and let's say we want to hotplug a new device. In case Xen does not need a bridge because the hotplug works as expected, we don't want to update the Pod state for nothing. What we need is to let every hypervisor implementation decide if they want to store something into hypervisor.json and what they want to store.

In our case this means several changes, let me try to summarize this:

1. Update Qemu structure

type qemu struct {
        pod *Pod
        ...
        bridges []Bridges
}

2. Update hypervisor interface
Change this:

type hypervisor interface {
        init(config HypervisorConfig) error
        ...
}

to that:

type hypervisor interface {
        init(pod *Pod) error
        ...
}

The explanation is that we need the pod to be stored inside qemu structure so that we can access storage anytime we want from the qemu implementation. Xen implementation would do nothing with this pod actually since it would not need to store anything.
This pod handler is also going to be used to retrieve the HypervisorConfig through pod.config.HypervisorConfig.

Of course, you're gonna have to update the mock implementation, but that's nothing.

3. Modify doFetchPod() sequence
Change this:

func doFetchPod(podConfig PodConfig) (*Pod, error) {
	...
	if err := hypervisor.init(podConfig.HypervisorConfig); err != nil {
		return nil, err
	}
	...
	p := &Pod{
		id:              podConfig.ID,
		hypervisor:      hypervisor,
		...
	}
        ...

to that:

func doFetchPod(podConfig PodConfig) (*Pod, error) {
	...
	p := &Pod{
		id:              podConfig.ID,
		hypervisor:      hypervisor,
		...
	}

        if err := hypervisor.init(pod); err != nil {
		return nil, err
	}
        ...

We need this, otherwise we cannot provide the pod to the hypervisor.init().

4. Create a new hypervisor.json file
For this one, follow what we have inside filesystem.go and duplicate what is needed for creating a hypervisorFileType.

5. Fetch hypervisor info

func (q *qemu) init(pod *Pod) error {
        q.pod = pod
        ...
        // Fetch the hypervisor.json if it exists and assign q.bridges with what we read.
}

Every time the hypervisor is gonna be created from doFetchPod(), the call to hypervisor.init() will fetch the info from the storage in the specific case of Qemu. Also, we save the pod handler to be able to reuse it later for storage purpose.

6. Store hypervisor info
Both createPod(), hotplugAddDevice() and hotplugRemoveDevice() are gonna be responsible for updating the storage since they will modify the list of bridges.

@devimc I am trying to think of everything here, but I might have forgotten some details. @amshinde, @sameo please let me know what you think about this proposal and let me know if something is missing.

devimc · 2017-11-03T19:09:06Z

@sboeuf your proposal is good and feasible

amshinde · 2017-11-03T19:32:01Z

@sboeuf you have captured almost everything that we discussed. Another point to note, we should consider storing the hypervisor.json at the end of a container lifecycle event vs having it done immediately after a device is added.

sboeuf · 2017-11-03T20:02:19Z

@amshinde oh yes you're right, and this should come as a second step (as an improvement).

sboeuf · 2017-11-03T20:02:58Z

@devimc I am glad that's clear :)

devimc · 2017-11-03T21:08:33Z

@sboeuf just one question, if hypervisor.json is hypervisor specific then filesystem fetch method should return []byte and store should receive interface{}, right?
because filesystem doesn't know how to unmarshall the data

sboeuf · 2017-11-03T22:32:28Z

@devimc oh yeah let me clarify this. So basically I think you'll have the fetch method returning an interface{} and the store method receiving an interface{}. And from qemu implementation, you will provide QemuState (make sure the struct and all its fields are public otherwise marshalling won't work) through the interface{} parameter (that's for the store() and it should be straightforward). For the fetch(), you will receive an interface{} that you're going to cast into the expected structure from every hypervisor implementation that needs to retrieve hypervisor.json.
Is that clearer ? Don't hesitate if you still have questions ;)

devimc · 2017-11-08T20:06:42Z

@sboeuf changes applied

sboeuf · 2017-11-08T20:55:11Z

@devimc thanks, I am going to review this !

sboeuf

@devimc few comments but this is getting better :)

sboeuf · 2017-11-08T21:32:19Z

filesystem.go

@@ -32,6 +32,8 @@ import (
 // pods and containers.
 type podResource int

+type HypervisorState []byte


This is not needed. There is no point in creating this type since we know that what comes out of marshalling is always an array of bytes.

sboeuf · 2017-11-08T21:49:01Z

qemu.go

@@ -39,6 +39,10 @@ type qmpChannel struct {
 	qmp          *ciaoQemu.QMP
 }

+type qemuState struct {


This must be an exported structure, otherwise the JSON marshalling won't work.

sboeuf · 2017-11-08T21:53:07Z

filesystem.go

@@ -675,10 +728,31 @@ func (fs *filesystem) fetchPodNetwork(podID string) (NetworkNamespace, error) {
 	return NetworkNamespace{}, fmt.Errorf("Unknown network type")
 }

+func (fs *filesystem) fetchHypervisorState(podID string, state interface{}) error {


No need for this cast here, we can simplify this function like this:

func (fs *filesystem) fetchHypervisorState(podID string, state interface{}) error { data, err := fs.fetchResource(true, podID, "", hypervisorFileType) if err != nil { return err } if err = json.Unmarshal(data, state); err != nil { return err } return nil }

data being a simple []byte type.

I'll cast data to []byte, if not, I get this error

# github.com/containers/virtcontainers ./filesystem.go:735:25: cannot use data (type interface {}) as type []byte in argument to json.Unmarshal: need type assertion Makefile:27: recipe for target 'build' failed make: *** [build] Error 2

func (fs *filesystem) fetchHypervisorState(podID string, state interface{}) error { data, err := fs.fetchResource(true, podID, "", hypervisorFileType) if err != nil { return err } if err = json.Unmarshal(data.([]byte), state); err != nil { return err } return nil }

@devimc you're right but there is an underlying problem here and it's not related to your PR, but to the way the storage (and the fetch specifically) is implemented in virtcontainers.
I have submitted #472 in order to make this more flexible and it should solve your issues. I am waiting for @sameo to review this PR and hopefully this will get merged soon.

#472 and #477 have been merged! You can rebase your PR on top of it, and simplify the storage part :)

sboeuf · 2017-11-08T22:09:55Z

qemu.go

+		return err
+	}
+
+	if err := q.pod.storage.storeHypervisorState(q.pod.id, q.state); err != nil {


This can be improved later (moving this to container.go where we actually add the devices), but that's fine for now.

sboeuf · 2017-11-08T22:10:01Z

qemu.go

+		return err
+	}
+
+	if err := q.pod.storage.storeHypervisorState(q.pod.id, q.state); err != nil {


sboeuf

One comment, but everything else looks fine

sboeuf · 2017-11-13T17:57:57Z

filesystem.go

@@ -598,10 +610,23 @@ func (fs *filesystem) fetchPodNetwork(podID string) (NetworkNamespace, error) {
 	return networkNS, nil
 }

+func (fs *filesystem) fetchHypervisorState(podID string, state interface{}) error {
+	return fs.fetchResource(true, podID, "", hypervisorFileType, state)


This is perfect, but look at commonResourceChecks(), you have to update the list of known file type.

change applied, thanks

sboeuf · 2017-11-13T19:47:10Z

LGTM

Qemu Q35 machine type does not support to hot plug devices directly on pcie.0, hence we have to find a way to allow users hot plug N devices. The only way to hot plug devices in Qemu Q35 is through PCI brdiges. Each PCI bridge is able to support until 32 devices, therefore we have a limitation in the amount of devices we can hot plug. This patch adds support for hot plugging devices on PCI bridges in pc and q35 machine types. Signed-off-by: Julio Montes <julio.montes@intel.com>

devimc added the in progress label Oct 30, 2017

devimc requested review from amshinde and mcastelino October 30, 2017 15:07

jodh-intel reviewed Oct 30, 2017

View reviewed changes

sboeuf suggested changes Oct 30, 2017

View reviewed changes

devimc force-pushed the q35Hotplug branch from 4a67d04 to c5741a6 Compare October 30, 2017 16:54

sboeuf reviewed Oct 30, 2017

View reviewed changes

devimc force-pushed the q35Hotplug branch from c5741a6 to e0768a2 Compare October 30, 2017 20:49

devimc added the do-not-merge label Oct 31, 2017

amshinde reviewed Oct 31, 2017

View reviewed changes

mcastelino reviewed Oct 31, 2017

View reviewed changes

devimc force-pushed the q35Hotplug branch 2 times, most recently from fe2d2b7 to eef1642 Compare November 1, 2017 16:47

sboeuf suggested changes Nov 2, 2017

View reviewed changes

devimc force-pushed the q35Hotplug branch from eef1642 to 28520b5 Compare November 3, 2017 21:56

devimc force-pushed the q35Hotplug branch 3 times, most recently from e48dcbb to 6c938cf Compare November 8, 2017 19:52

devimc force-pushed the q35Hotplug branch from 6c938cf to 9e42be0 Compare November 8, 2017 20:05

devimc changed the title ~~Q35 hotplug~~ hypervisor: add hot plugging support for Qemu Q35 Nov 8, 2017

devimc force-pushed the q35Hotplug branch from 9e42be0 to a410d58 Compare November 8, 2017 20:09

sboeuf suggested changes Nov 8, 2017

View reviewed changes

devimc force-pushed the q35Hotplug branch from a410d58 to 7c00a73 Compare November 9, 2017 14:15

devimc removed the do-not-merge label Nov 9, 2017

devimc force-pushed the q35Hotplug branch 2 times, most recently from 1a1208f to 2826db8 Compare November 13, 2017 17:11

sboeuf suggested changes Nov 13, 2017

View reviewed changes

devimc force-pushed the q35Hotplug branch from 2826db8 to 328661f Compare November 13, 2017 19:12

sboeuf approved these changes Nov 13, 2017

View reviewed changes

devimc force-pushed the q35Hotplug branch 2 times, most recently from 8fe21f4 to d0114d8 Compare November 13, 2017 21:12

devimc force-pushed the q35Hotplug branch from d0114d8 to 42df5a6 Compare November 13, 2017 21:29

devimc mentioned this pull request Nov 13, 2017

hypervisor: switch to Q35 clearcontainers/runtime#707

Closed

sboeuf merged commit e62b2ae into containers:master Nov 14, 2017

sboeuf removed the in progress label Nov 14, 2017

jcvenegas mentioned this pull request Nov 29, 2017

Release 3.0.9 clearcontainers/runtime#838

Merged

devimc deleted the q35Hotplug branch December 21, 2017 15:22

hypervisor: add hot plugging support for Qemu Q35 #456

hypervisor: add hot plugging support for Qemu Q35 #456

Conversation

devimc commented Oct 30, 2017 • edited

devimc commented Oct 30, 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

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

devimc commented Oct 30, 2017

Choose a reason for hiding this comment

devimc Oct 30, 2017 • edited

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

devimc commented Nov 1, 2017

sboeuf commented Nov 1, 2017

sboeuf 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

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 2, 2017

devimc commented Nov 3, 2017

amshinde commented Nov 3, 2017

sboeuf commented Nov 3, 2017

sboeuf commented Nov 3, 2017

devimc commented Nov 3, 2017 • edited

sboeuf commented Nov 3, 2017

devimc commented Nov 8, 2017

sboeuf commented Nov 8, 2017

sboeuf 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

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 left a 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 13, 2017

devimc commented Oct 30, 2017 •

edited

devimc Oct 30, 2017 •

edited

devimc commented Nov 3, 2017 •

edited