Add support for scsi by amshinde · Pull Request #205 · clearcontainers/agent

amshinde · 2018-01-25T23:06:00Z

Add support for SCSI disks to be passed for container rootfs and volumes.
If SCSI address is provided, use that to get the SCSI disk name.
This eliminates the need to predict the drive name of the block
device on the host side. We do need to incur the cost of scanning SCSI bus.

jodh-intel

Hi @amshinde - thanks for raising. I've added a few comments.

Please could you add some tests for these changes as well?

jodh-intel · 2018-01-26T08:59:13Z

syscall.go

+	for _, file := range files {
+		host := file.Name()
+		scanPath := filepath.Join(scsiHostPath, host, "scan")
+		if err := ioutil.WriteFile(scanPath, scanData, 0666); err != nil {


This should be 0200 I think.

Yes, it should be 0200. Fixed.

jodh-intel · 2018-01-26T09:01:17Z

syscall.go

+// but we do need to rescan SCSI bus for this.
+func findSCSIDisk(scsiAddr string) (string, error) {
+	// Here in "0:0", the first number is the SCSI host number, while the second number is always 0.
+	scsiPath := fmt.Sprintf("/sys/class/scsi_disk/0:0:%s/device/block", scsiAddr)


This path prefix is also used in waitForBlockDevice(). Can you make it a file-global var as that will avoid the duplication and hard-coding, but also allow the value to be changed for testing.

Done. Have added tests as well.

jodh-intel · 2018-01-26T09:04:37Z

agent.go

 		}

+		if fsmap.SCSIAddr != "" {
+			source, err = getSCSIDisk(fsmap.SCSIAddr)


This is resetting source so is it worth adding an extra check to see if source != "" here? Or is it guaranteed that if SCSIAddr is set that Source is irrelevant?

Could you maybe add some comments to the Fsmap type to explain this?

Does this check need to come before the AbsolutePath check above as currently that code is being executed for no reason if we're using a SCSI device?

I have added in comments.

jodh-intel · 2018-01-26T09:06:35Z

syscall.go

-	if _, err := os.Stat(devicePath); err == nil {
+	if isSCSIAddr {
+		// Here in "0:0", the first number is the SCSI host number, while the second number is always 0.
+		scsiPrefix := "/sys/class/scsi_disk/0:0"


Can you make this path prefix a file-global var to allow it to be re-used (and changed for testing)?

jodh-intel · 2018-01-26T09:06:49Z

syscall.go

+		// Here in "0:0", the first number is the SCSI host number, while the second number is always 0.
+		scsiPrefix := "/sys/class/scsi_disk/0:0"
+
+		path := scsiPrefix + deviceName + "/device/block"


Same comment as above.

jodh-intel · 2018-01-26T09:10:01Z

syscall.go

+		if _, err := os.Stat(path); err == nil {
+			return nil
+		}
+	} else if _, err := os.Stat(devicePath); err == nil {


I think it would be clearer if you moved the devicePath variable down to here and removed the else if:

if isSCSIAddr { : } devicePath := filepath.Join(devPath, deviceName) if _, err := os.Stat(devicePath); err == nil { return nil }

jodh-intel · 2018-01-26T09:11:00Z

syscall.go

-				if d.Action() == "add" && filepath.Base(d.Devpath()) == deviceName {
+
+				if isSCSIAddr && d.Action() == "add" && strings.Contains(d.Devpath(), deviceName) {
+					fieldLogger.Info("Hotplug event received")


Maybe "SCSI hotplug event received"?

jodh-intel · 2018-01-26T09:11:38Z

syscall.go

+	return nil
+}
+
+// findSCSIDisk finds the SCSI disk name associated with the given SCSI address


Nit: Missing period at end of sentence.

jodh-intel · 2018-01-26T09:16:13Z

syscall.go

+		return "", err
+	}
+
+	if len(files) > 1 {


There is an implicit assumption that a single file was read, so how about the following:

l := len(files) if l == 0 || l > 1 { ... }

sboeuf · 2018-02-06T15:50:02Z

agent.go

 	}

-	absoluteRootFs, err := mountContainerRootFs(payload.ID, payload.Image, payload.RootFs, payload.FsType)
+	absoluteRootFs, err := mountContainerRootFs(payload.ID, payload.Image, payload.RootFs, payload.FsType, payload.SCSIAddr)


We should pass the payload to this function, instead of extending its number of parameters, all being attributes of the same structure.

I had done this to avoid introducing hyperstart specific structures in syscall.go.

There is no harm doing so. syscall.go is part of the agent, which makes it tied to hyperstart. I don't think this is an issue to pass an hyperstart structure to this function, and this would simplify things as I mentioned.

I was just trying to avoid introducing any hyperstart structures here, as this is more like an utils package and is a good contender for moving into its own util package, but that is for the future.
I can simplify this for now.

I have pushed changes for this.

sboeuf · 2018-02-06T15:55:39Z

api/commands.go

 	SystemMountsInfo SystemMountsInfo `json:"systemMountsInfo"`
+
+	// SCSI address in the format SCSI-Id:LUN
+	SCSIAddr string `json:"scsiAddr,omitempty"`


I am not sure this is the right place for this field. This is very specific to a type of rootfs. Maybe we should define a structure Rootfs so that we can actually include this type of information. WDYT ?

Yes, this is specific to rootfs, but we have other fields here like Rootfs, FsMap and FsType that are related to rootfs that Iwould be candidates for a combined structure. I would like to do that in a separate PR, as this involves a lot of components.

Fair enough, but please open an issue to track this down ;)

Sure will do.

sboeuf · 2018-02-06T15:57:05Z

syscall.go

 }

-func waitForBlockDevice(deviceName string) error {
+func waitForBlockDevice(deviceName string, isSCSIAddr bool) error {


You should rename this function into waitForDevice() and pass a more global structure. Once again, this looks very specific to SCSI here.

Any comment on this ?

I would prefer keeping this as is as we are waiting for a Block device here, the difference being if the device is passed with virtio-scsi or virtio-block, with the flag indicating this.

Yes I agree this is still a block device, forget this comment.

jodh-intel · 2018-02-22T08:34:01Z

syscall.go

+	var path string
+
+	if isSCSIAddr {
+		path = scsiDiskPrefix + deviceName + "/device/block"


This would be more consistent (and maybe safer) as:

path = filepath.Join(scsiDiskPrefix + deviceName, "/device/block")

You could also create a variable for the /device/block which is used multiple times.

jodh-intel · 2018-02-22T08:39:29Z

syscall.go

-				if d.Action() == "add" && filepath.Base(d.Devpath()) == deviceName {
-					fieldLogger.Info("Hotplug event received")
+
+				//if isSCSIAddr && d.Action() == "add" && strings.Contains(d.Devpath(), deviceName) {


Commented out line.

jodh-intel · 2018-02-22T08:42:03Z

syscall.go

-					fieldLogger.Info("Hotplug event received")
+
+				//if isSCSIAddr && d.Action() == "add" && strings.Contains(d.Devpath(), deviceName) {
+				if isSCSIAddr && d.Action() == "add" && strings.Contains(d.Devpath(), "/0:0:"+deviceName+"/block") {


Can you create a global private variable for the 0:0 (and ideally change scsiDiskPrefix to use it too)?

jodh-intel · 2018-02-22T08:43:51Z

syscall.go

+
+// scanSCSIBus scans SCSI bus for the given SCSI address(SCSI-Id and LUN)
+func scanSCSIBus(scsiAddr string) error {
+	if _, err := os.Stat(scsiHostPath); err != nil {


This call isn't needed as the ReadDir() below will detect ENOENT itself. Same comment for the Stat() in findSCSIDisk().

jodh-intel · 2018-02-22T08:45:41Z

syscall.go

+		// If device node is not found, wait for udev event for the scsi disk,
+		// with the format "0:SCSIId:LUN/block"
+
+		//if err := waitForBlockDevice("0:"+scsiAddr+"/block", true); err != nil {


Can you remove this line please.

amshinde · 2018-02-22T16:10:15Z

@jodh-intel I have pushed changes for your comments. Please have a look.

Add support for SCSI disks to be passed for container rootfs. If SCSI address is provided, use that to get the SCSI disk name. This eliminates the need to predict the drive name of the block device on the host side. We do need to incur the cost of scanning SCSI bus. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

Similar to rootfs, add support for SCSI disk to be passed for a volume mount. Fixes clearcontainers#209 Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

sboeuf · 2018-02-22T21:37:37Z

LGTM
@jodh-intel @egernst please make a new review !

amshinde requested review from egernst and sboeuf January 25, 2018 23:06

jodh-intel suggested changes Jan 26, 2018

View reviewed changes

jodh-intel mentioned this pull request Jan 26, 2018

Add support for scsi for hotplugging block devices containers/virtcontainers#584

Merged

sboeuf suggested changes Feb 6, 2018

View reviewed changes

amshinde force-pushed the add-support-for-scsi branch 2 times, most recently from 1ac4d30 to 66f12ab Compare February 22, 2018 00:39

jodh-intel reviewed Feb 22, 2018

View reviewed changes

amshinde force-pushed the add-support-for-scsi branch 2 times, most recently from f946e73 to be3f50e Compare February 22, 2018 16:08

amshinde added 2 commits February 22, 2018 10:56

scsi: Allow SCSI drives to be passed for volumes

3eacdd5

Similar to rootfs, add support for SCSI disk to be passed for a volume mount. Fixes clearcontainers#209 Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

amshinde force-pushed the add-support-for-scsi branch from be3f50e to 3eacdd5 Compare February 22, 2018 18:56

sboeuf approved these changes Feb 22, 2018

View reviewed changes

jodh-intel approved these changes Feb 23, 2018

View reviewed changes

jodh-intel merged commit 6f6e9ec into clearcontainers:master Feb 23, 2018

Conversation

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

Uh oh!

Uh oh!

jodh-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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

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!

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!

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

amshinde commented Jan 25, 2018 •

edited

Loading

amshinde Feb 22, 2018 •

edited

Loading

amshinde Feb 22, 2018 •

edited

Loading