Add support for scsi for hotplugging block devices by amshinde · Pull Request #584 · containers/virtcontainers

amshinde · 2018-01-26T01:29:18Z

With this PR, block devices are hotplugged using either virtio-scsi or virtio-block based on a configuration.

jodh-intel · 2018-01-26T09:20:08Z

device.go

 	DeviceInfo DeviceInfo

-	// Path at which the device appears inside the VM, outside of the container mount namespace.
+	// SCSI Address of the block device, in case the device is attached using SCSI driver


Could you add details of the format here as you did in clearcontainers/agent#205?

I have added details here.

jodh-intel · 2018-01-26T09:22:47Z

mount.go

+// qemu code suggests that scsi-id can take values from 0 to 255 inclusive, while lun can
+// take values from 0 to 16383 inclusive. But lun values over 255 do not seem to follow
+// consistent SCSI addressing. Hence we limit to 255.
+func getSCSIIdLun(index int) (int, int, error) {


Could you rename to scsiIDLun or similar (https://golang.org/doc/effective_go.html#Getters)?

@jodh-intel This is not technically a getter for a struct field, but a function that derives the SCSI id and LUN based on an index.

Ah - fair point.

jodh-intel · 2018-01-26T09:22:59Z

mount.go

+	return index / 256, index % 256, nil
+}
+
+func getSCSIAddress(index int) (string, error) {


Same comment - we don't need the get prefix.

jodh-intel · 2018-01-26T09:23:14Z

mount_test.go

+		{258, 1, 2},
+		{512, 2, 0},
+		{513, 2, 1},
+	}


jodh-intel · 2018-01-26T09:24:57Z

hypervisor.go

 	DisableBlockDeviceUse bool

+	// BlockDeviceDriver specifies the driver to be used for block device
+	// either "virtio-scsi" or "virtio-blk" with the default driver being "virtio-scsi"


For maintenance, it's going to be safer if you refer to VirtioBlock and VirtioSCSI. But It could also be useful to define a DefaultVirtio which could also be mentioned here.

amshinde · 2018-02-21T22:55:37Z

@sboeuf Can you take a look at this as well. CI is failing, likely due to lack of SCSI support on the agent side that needs to be merged first.

sboeuf · 2018-02-22T00:33:10Z

@amshinde yes I'll review both PR (virtcontainers and agent) tomorrow !

sboeuf

PR looks pretty good, I have a few comments.
As a global comment by looking at all this code related to devices and rootfs, I realize we have some kind of duplicates between what device.go does for block devices and what hyperstart_agent.go does for rootfs when it is a block device. I think there is room for factorization here, WDYT @amshinde ? (of course this would be done in a separate PR)

sboeuf · 2018-02-22T19:17:07Z

mount.go

+		return -1, -1, fmt.Errorf("Index cannot be negative")
+	}
+
+	if index > 65535 {


Please use a constant for this.

sboeuf · 2018-02-22T19:30:37Z

qemu.go

+				return err
+			}

+			if err = q.qmpMonitorCh.qmp.ExecuteSCSIDeviceAdd(q.qmpMonitorCh.ctx, drive.ID, devID, driver, bus, scsiID, lun); err != nil {


The code looks good but I have noticed that you're not handling the case where we want to remove a SCSI device. Am I missing something about the way SCSI works, or have you forgotten to implement this ?

The way to remove SCSI device is the same as virtio-block device.

Makes sense, thanks !

sboeuf · 2018-02-22T19:32:50Z

hyperstart_agent.go

+			}
+			container.Image = driveName
+		} else {
+			scsiAddr, err := getSCSIAddress(c.state.BlockIndex)


Very nice that we don't need to predict here ;)

sboeuf · 2018-02-22T19:34:29Z

pkg/hyperstart/types.go

 	Fstype           string              `json:"fstype,omitempty"`
 	Image            string              `json:"image"`
-	Addr             string              `json:"addr,omitempty"`
+	SCSIAddr         string              `json:"scsiAddr,omitempty"`


Why do you want to change the name ? Addr is generic enough to include SCSI case , right ?

I had this as Addr in the agent, but changed this to SCSIAddr following your review comment which I agree as SCSIAddr makes it clearer. Hence the change here.

I had forgotten...

sboeuf · 2018-02-22T19:37:29Z

device.go

-	// Path at which the device appears inside the VM, outside of the container mount namespace.
+	// SCSI Address of the block device, in case the device is attached using SCSI driver
+	// SCSI address is in the format SCSI-Id:LUN
+	SCSIAddr string


Don't you think this should be part of the DeviceInfo ?

No the DeviceInfo includes fields that are common to all devices. This is specific to Block device.

Makes sense

sboeuf · 2018-02-22T19:41:52Z

container.go


-	agentCaps := c.pod.agent.capabilities()
-	hypervisorCaps := c.pod.hypervisor.capabilities()
+	if !c.pod.config.HypervisorConfig.DisableBlockDeviceUse {


Please rebase, as this has been merged in a separate PR.

Rebased. Moved some code from qemu.go to qemu_arch_base.go introduced with arm support.

sboeuf · 2018-02-22T23:06:39Z

qemu_arch_base.go

 	return q.appendBlockDevice(devices, drive), nil
 }

+func (q *qemu) appendSCSIController(devices []govmmQemu.Device, podConfig PodConfig) []govmmQemu.Device {


need to be a function of qemuArchBase

Introduce a configuration option for using either virtio-scsi or virtio-block for hotplugging drives with virtio-scsi being the default driver. Hotplug block devices based on this confuration. In order to hotplug SCSI drives, we need to coldplug a SCSI controller while starting up qemu. With virtio-scsi, use the index at which the drive is attached to come up with a SCSI id and LUN. This will be used by the agent to detect the drive name. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

Since virtio-scsi is used as default driver for block devices, modify tests to include this in the default config. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

In case of SCSI driver, pass the SCSI address in the format "SCSI-id:LUN" to hyperstart agent. This will be used by the agent to find the device within the VM. This avoids predicting the drive name on the host side in case of SCSI. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

Volumes can now be passed using SCSI. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

Test for SCSI and block drivers while attaching block device. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

sboeuf · 2018-02-22T23:26:18Z

LGTM

amshinde · 2018-02-23T16:43:31Z

@jodh-intel Can you take another look at this?

sboeuf · 2018-02-23T16:46:13Z

@amshinde please update the agent commit sha1 in the runtime, this way, after you get your PR on the runtime merged, this PR should be re-triggered and pass.

jodh-intel · 2018-02-23T17:37:33Z

lgtm

amshinde · 2018-02-26T18:35:12Z

@sboeuf I reran the CI after agent SHA update. The CI has passed now. Can we merge this?
This does introduce SCSI as the default storage driver, which means we need to add support for SCSI for kata agent as well. We can change this to virtio-block in a follow up PR in the interim for kata.

sboeuf · 2018-02-26T19:16:02Z

@amshinde yes perfect !

This reverts commit 494b46c, reversing changes made to e475a8f.

amshinde force-pushed the switch-to-scsi branch 2 times, most recently from 8bd1c5b to 7b72bf1 Compare January 26, 2018 02:40

jodh-intel reviewed Jan 26, 2018

View reviewed changes

egernst mentioned this pull request Feb 15, 2018

qemu: Add qemu-cc clearcontainers/packaging#34

Closed

amshinde force-pushed the switch-to-scsi branch 2 times, most recently from 9c09c7f to 1f6287f Compare February 21, 2018 22:53

sboeuf reviewed Feb 22, 2018

View reviewed changes

amshinde force-pushed the switch-to-scsi branch from 1f6287f to e3bcd95 Compare February 22, 2018 21:51

sboeuf reviewed Feb 22, 2018

View reviewed changes

amshinde added 5 commits February 22, 2018 15:18

tests: Fix tests to take into account default driver for block devices

80bdc50

Since virtio-scsi is used as default driver for block devices, modify tests to include this in the default config. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

scsi: Add SCSI support for volumes as well

a1a569a

Volumes can now be passed using SCSI. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

test: Modify block device test to test for SCSI as well

07c2c88

Test for SCSI and block drivers while attaching block device. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

amshinde force-pushed the switch-to-scsi branch from e3bcd95 to 07c2c88 Compare February 22, 2018 23:18

sboeuf approved these changes Feb 22, 2018

View reviewed changes

sboeuf merged commit 494b46c into containers:master Feb 26, 2018

sboeuf pushed a commit that referenced this pull request Mar 1, 2018

Revert "Merge pull request #584 from amshinde/switch-to-scsi"

5ecd7e0

This reverts commit 494b46c, reversing changes made to e475a8f.

sboeuf pushed a commit that referenced this pull request Mar 1, 2018

Revert "Merge pull request #584 from amshinde/switch-to-scsi"

c0c15ed

This reverts commit 494b46c, reversing changes made to e475a8f.

sboeuf pushed a commit that referenced this pull request Mar 1, 2018

Revert "Merge pull request #584 from amshinde/switch-to-scsi"

575cf36

This reverts commit 494b46c, reversing changes made to e475a8f.

Conversation

amshinde commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

amshinde commented Feb 21, 2018

Uh oh!

sboeuf commented Feb 22, 2018

Uh oh!

sboeuf 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!

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 Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sboeuf commented Feb 22, 2018

Uh oh!

amshinde commented Feb 23, 2018

Uh oh!

sboeuf commented Feb 23, 2018

Uh oh!

jodh-intel commented Feb 23, 2018 • edited by dlespiau Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amshinde commented Feb 26, 2018

Uh oh!

sboeuf commented Feb 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

amshinde commented Jan 26, 2018 •

edited

Loading

sboeuf Feb 22, 2018 •

edited

Loading

jodh-intel commented Feb 23, 2018 •

edited by dlespiau

Loading