Fix MAAS storage issues

axw · axw · commit ff98e33283aa · 2017-05-10T09:23:23.000+08:00
- report WWN for block devices, and add to volume info; the MAAS provider now checks the id_path to see if it is a WWN, and matches on that. - set the status of volumes created/attached by the machine provisioner when setting instance info. This ensures that volumes show up as "attaching" or "attached" initially, and not "pending" forever for static volumes. - don't attempt to reattach static volumes. Fixes https://bugs.launchpad.net/juju/+bug/1677001
diff --git a/apiserver/common/storagecommon/blockdevices.go b/apiserver/common/storagecommon/blockdevices.go
@@ -17,6 +17,7 @@ func BlockDeviceFromState(in state.BlockDeviceInfo) storage.BlockDevice {
 		in.Label,
 		in.UUID,
 		in.HardwareId,
+		in.WWN,
 		in.BusAddress,
 		in.Size,
 		in.FilesystemType,
@@ -33,6 +34,12 @@ func MatchingBlockDevice(
 	attachmentInfo state.VolumeAttachmentInfo,
 ) (*state.BlockDeviceInfo, bool) {
 	for _, dev := range blockDevices {
+		if volumeInfo.WWN != "" {
+			if volumeInfo.WWN == dev.WWN {
+				return &dev, true
+			}
+			continue
+		}
 		if volumeInfo.HardwareId != "" {
 			if volumeInfo.HardwareId == dev.HardwareId {
 				return &dev, true
diff --git a/apiserver/common/storagecommon/storage.go b/apiserver/common/storagecommon/storage.go
@@ -229,7 +229,10 @@ func volumeAttachmentDevicePath(
 	volumeAttachmentInfo state.VolumeAttachmentInfo,
 	blockDevice state.BlockDeviceInfo,
 ) (string, error) {
-	if volumeInfo.HardwareId != "" || volumeAttachmentInfo.DeviceName != "" || volumeAttachmentInfo.DeviceLink != "" {
+	if volumeInfo.HardwareId != "" ||
+		volumeInfo.WWN != "" ||
+		volumeAttachmentInfo.DeviceName != "" ||
+		volumeAttachmentInfo.DeviceLink != "" {
 		// Prefer the volume attachment's information over what is
 		// in the published block device information.
 		var deviceLinks []string
@@ -238,6 +241,7 @@ func volumeAttachmentDevicePath(
 		}
 		return storage.BlockDevicePath(storage.BlockDevice{
 			HardwareId:  volumeInfo.HardwareId,
+			WWN:         volumeInfo.WWN,
 			DeviceName:  volumeAttachmentInfo.DeviceName,
 			DeviceLinks: deviceLinks,
 		})
diff --git a/apiserver/common/storagecommon/storage_test.go b/apiserver/common/storagecommon/storage_test.go
@@ -59,6 +59,7 @@ func (s *storageAttachmentInfoSuite) SetUpTest(c *gc.C) {
 		DeviceName:  "sda",
 		DeviceLinks: []string{"/dev/disk/by-id/verbatim"},
 		HardwareId:  "whatever",
+		WWN:         "drbr",
 	}}
 	s.st = &fakeStorage{
 		storageInstance: func(tag names.StorageTag) (state.StorageInstance, error) {
@@ -119,6 +120,17 @@ func (s *storageAttachmentInfoSuite) TestStorageAttachmentInfoPersistentHardware
 	})
 }
 
+func (s *storageAttachmentInfoSuite) TestStorageAttachmentInfoPersistentWWN(c *gc.C) {
+	s.volume.info.WWN = "drbr"
+	info, err := storagecommon.StorageAttachmentInfo(s.st, s.storageAttachment, s.machineTag)
+	c.Assert(err, jc.ErrorIsNil)
+	s.st.CheckCallNames(c, "StorageInstance", "StorageInstanceVolume", "VolumeAttachment", "BlockDevices")
+	c.Assert(info, jc.DeepEquals, &storage.StorageAttachmentInfo{
+		Kind:     storage.StorageKindBlock,
+		Location: filepath.FromSlash("/dev/disk/by-id/wwn-drbr"),
+	})
+}
+
 func (s *storageAttachmentInfoSuite) TestStorageAttachmentInfoMatchingBlockDevice(c *gc.C) {
 	// The bus address alone is not enough to produce a path to the block
 	// device; we need to find a published block device with the matching
diff --git a/apiserver/common/storagecommon/volumes.go b/apiserver/common/storagecommon/volumes.go
@@ -115,6 +115,7 @@ func VolumeToState(v params.Volume) (names.VolumeTag, state.VolumeInfo, error) {
 	}
 	return volumeTag, state.VolumeInfo{
 		v.Info.HardwareId,
+		v.Info.WWN,
 		v.Info.Size,
 		"", // pool is set by state
 		v.Info.VolumeId,
@@ -139,6 +140,7 @@ func VolumeInfoFromState(info state.VolumeInfo) params.VolumeInfo {
 	return params.VolumeInfo{
 		info.VolumeId,
 		info.HardwareId,
+		info.WWN,
 		info.Pool,
 		info.Size,
 		info.Persistent,
diff --git a/apiserver/diskmanager/diskmanager.go b/apiserver/diskmanager/diskmanager.go
@@ -95,6 +95,7 @@ func stateBlockDeviceInfo(devices []storage.BlockDevice) []state.BlockDeviceInfo
 			dev.Label,
 			dev.UUID,
 			dev.HardwareId,
+			dev.WWN,
 			dev.BusAddress,
 			dev.Size,
 			dev.FilesystemType,
diff --git a/apiserver/params/storage.go b/apiserver/params/storage.go
@@ -172,6 +172,7 @@ type Volume struct {
 type VolumeInfo struct {
 	VolumeId   string `json:"volume-id"`
 	HardwareId string `json:"hardware-id,omitempty"`
+	WWN        string `json:"wwn,omitempty"`
 	// Pool is the name of the storage pool used to
 	// allocate the volume. Juju controllers older
 	// than 2.2 do not populate this field, so it may
diff --git a/cmd/juju/storage/volume.go b/cmd/juju/storage/volume.go
@@ -33,6 +33,9 @@ type VolumeInfo struct {
 	// from params.Volume
 	HardwareId string `yaml:"hardware-id,omitempty" json:"hardware-id,omitempty"`
 
+	// from params.Volume
+	WWN string `yaml:"wwn,omitempty" json:"wwn,omitempty"`
+
 	// from params.Volume
 	Size uint64 `yaml:"size" json:"size"`
 
@@ -119,6 +122,7 @@ func createVolumeInfo(details params.VolumeDetails) (names.VolumeTag, VolumeInfo
 	var info VolumeInfo
 	info.ProviderVolumeId = details.Info.VolumeId
 	info.HardwareId = details.Info.HardwareId
+	info.WWN = details.Info.WWN
 	info.Pool = details.Info.Pool
 	info.Size = details.Info.Size
 	info.Persistent = details.Info.Persistent
diff --git a/dependencies.tsv b/dependencies.tsv
@@ -21,7 +21,7 @@ github.com/juju/ansiterm	git	b99631de12cf04a906c1d4e4ec54fb86eae5863d	2016-09-07
 github.com/juju/blobstore	git	06056004b3d7b54bbb7984d830c537bad00fec21	2015-07-29T11:18:58Z
 github.com/juju/bundlechanges	git	7725027b95e0d54635e0fb11efc2debdcdf19f75	2016-12-15T16:06:52Z
 github.com/juju/cmd	git	e47739aefe0adb68a401fcd371c0c92c8f6f8d99	2017-04-13T02:13:06Z
-github.com/juju/description	git	c1f554b8467afc4ec5e9175d162cc9a7217436a5	2017-05-09T00:06:37Z
+github.com/juju/description	git	142fe63614f1a2defb592b6bdf6bd0ee47fdccfd	2017-05-10T01:22:00Z
 github.com/juju/errors	git	1b5e39b83d1835fa480e0c2ddefb040ee82d58b3	2015-09-16T12:56:42Z
 github.com/juju/gnuflag	git	4e76c56581859c14d9d87e1ddbe29e1c0f10195f	2016-08-09T16:52:14Z
 github.com/juju/go-oracle-cloud	git	932a8cea00a1cd5cba3829a672c823955db674ae	2017-04-21T13:45:47Z
diff --git a/provider/maas/volumes.go b/provider/maas/volumes.go
@@ -323,7 +323,7 @@ func (mi *maas2Instance) volumes(
 		// There should be exactly one block device per label.
 		if len(devices) == 0 {
 			continue
-		} else if len(devices) > 0 {
+		} else if len(devices) > 1 {
 			// This should never happen, as we only request one block
 			// device per label. If it does happen, we'll just report
 			// the first block device and log this warning.
@@ -362,8 +362,13 @@ func (mi *maas2Instance) volumes(
 			deviceName := idPath[len(devPrefix):]
 			attachment.DeviceName = deviceName
 		} else if strings.HasPrefix(idPath, devDiskByIdPrefix) {
-			hardwareId := idPath[len(devDiskByIdPrefix):]
-			vol.HardwareId = hardwareId
+			const wwnPrefix = "wwn-"
+			id := idPath[len(devDiskByIdPrefix):]
+			if strings.HasPrefix(id, wwnPrefix) {
+				vol.WWN = id[len(wwnPrefix):]
+			} else {
+				vol.HardwareId = id
+			}
 		} else {
 			// It's neither /dev/<name> nor /dev/disk/by-id/<hardware-id>,
 			// so set it as the device link and hope for
diff --git a/provider/maas/volumes_test.go b/provider/maas/volumes_test.go
@@ -87,7 +87,10 @@ func (s *volumeSuite) TestInstanceVolumesMAAS2(c *gc.C) {
 					&fakeBlockDevice{name: "sde", idPath: "/dev/disk/by-dname/sde", size: 250362438230},
 				},
 				"4": {
-					&fakeBlockDevice{name: "sdf", idPath: "/dev/disk/by-dname/sdf", size: 280362438231},
+					&fakeBlockDevice{name: "sdf", idPath: "/dev/disk/by-id/wwn-drbr", size: 280362438231},
+				},
+				"5": {
+					&fakeBlockDevice{name: "sdg", idPath: "/dev/disk/by-dname/sdg", size: 280362438231},
 				},
 			},
 		},
@@ -97,12 +100,13 @@ func (s *volumeSuite) TestInstanceVolumesMAAS2(c *gc.C) {
 		names.NewVolumeTag("1"),
 		names.NewVolumeTag("2"),
 		names.NewVolumeTag("3"),
+		names.NewVolumeTag("4"),
 	})
 	c.Assert(err, jc.ErrorIsNil)
-	// Expect 3 volumes - root volume is ignored, as are volumes
+	// Expect 4 volumes - root volume is ignored, as are volumes
 	// with tags we did not request.
-	c.Assert(volumes, gc.HasLen, 3)
-	c.Assert(attachments, gc.HasLen, 3)
+	c.Assert(volumes, gc.HasLen, 4)
+	c.Assert(attachments, gc.HasLen, 4)
 	c.Check(volumes, jc.SameContents, []storage.Volume{{
 		names.NewVolumeTag("1"),
 		storage.VolumeInfo{
@@ -122,6 +126,13 @@ func (s *volumeSuite) TestInstanceVolumesMAAS2(c *gc.C) {
 			VolumeId: "volume-3",
 			Size:     238764,
 		},
+	}, {
+		names.NewVolumeTag("4"),
+		storage.VolumeInfo{
+			VolumeId: "volume-4",
+			Size:     267374,
+			WWN:      "drbr",
+		},
 	}})
 	c.Assert(attachments, jc.SameContents, []storage.VolumeAttachment{{
 		names.NewVolumeTag("1"),
@@ -139,6 +150,10 @@ func (s *volumeSuite) TestInstanceVolumesMAAS2(c *gc.C) {
 		storage.VolumeAttachmentInfo{
 			DeviceLink: "/dev/disk/by-dname/sdd",
 		},
+	}, {
+		names.NewVolumeTag("4"),
+		mTag,
+		storage.VolumeAttachmentInfo{},
 	}})
 }
 
diff --git a/state/blockdevices.go b/state/blockdevices.go
@@ -38,6 +38,7 @@ type BlockDeviceInfo struct {
 	Label          string   `bson:"label,omitempty"`
 	UUID           string   `bson:"uuid,omitempty"`
 	HardwareId     string   `bson:"hardwareid,omitempty"`
+	WWN            string   `bson:"wwn,omitempty"`
 	BusAddress     string   `bson:"busaddress,omitempty"`
 	Size           uint64   `bson:"size"`
 	FilesystemType string   `bson:"fstype,omitempty"`
diff --git a/state/machine.go b/state/machine.go
@@ -1209,12 +1209,30 @@ func (m *Machine) SetInstanceInfo(
 	if err := m.SetDevicesAddressesIdempotently(devicesAddrs); err != nil {
 		return errors.Trace(err)
 	}
+
+	// Record volumes and volume attachments, and set the initial
+	// status: attached or attaching.
 	if err := setProvisionedVolumeInfo(m.st, volumes); err != nil {
 		return errors.Trace(err)
 	}
 	if err := setMachineVolumeAttachmentInfo(m.st, m.Id(), volumeAttachments); err != nil {
 		return errors.Trace(err)
 	}
+	volumeStatus := make(map[names.VolumeTag]status.Status)
+	for tag := range volumes {
+		volumeStatus[tag] = status.Attaching
+	}
+	for tag := range volumeAttachments {
+		volumeStatus[tag] = status.Attached
+	}
+	for tag, status := range volumeStatus {
+		if err := m.st.SetVolumeStatus(tag, status, "", nil, nil); err != nil {
+			return errors.Annotatef(
+				err, "setting status of %s", names.ReadableString(tag),
+			)
+		}
+	}
+
 	return m.SetProvisioned(id, nonce, characteristics)
 }
 
diff --git a/state/machine_test.go b/state/machine_test.go
@@ -993,6 +993,9 @@ func (s *MachineSuite) TestMachineSetInstanceInfoSuccess(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 	volumeInfo.Pool = "loop-pool" // taken from params
 	c.Assert(info, gc.Equals, volumeInfo)
+	volumeStatus, err := volume.Status()
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(volumeStatus.Status, gc.Equals, status.Attaching)
 }
 
 func (s *MachineSuite) TestMachineSetProvisionedWhenNotAlive(c *gc.C) {
diff --git a/state/migration_export.go b/state/migration_export.go
@@ -394,6 +394,7 @@ func (e *exporter) newMachine(exParent description.Machine, machine *Machine, in
 			Label:          device.Label,
 			UUID:           device.UUID,
 			HardwareID:     device.HardwareId,
+			WWN:            device.WWN,
 			BusAddress:     device.BusAddress,
 			Size:           device.Size,
 			FilesystemType: device.FilesystemType,
@@ -1534,6 +1535,7 @@ func (e *exporter) addVolume(vol *volume, volAttachments []volumeAttachmentDoc)
 		args.Size = info.Size
 		args.Pool = info.Pool
 		args.HardwareID = info.HardwareId
+		args.WWN = info.WWN
 		args.VolumeID = info.VolumeId
 		args.Persistent = info.Persistent
 	} else {
diff --git a/state/migration_export_test.go b/state/migration_export_test.go
@@ -323,6 +323,7 @@ func (s *MigrationExportSuite) TestMachineDevices(c *gc.C) {
 		Label:          "sda-label",
 		UUID:           "some-uuid",
 		HardwareId:     "magic",
+		WWN:            "drbr",
 		BusAddress:     "bus stop",
 		Size:           16 * 1024 * 1024 * 1024,
 		FilesystemType: "ext4",
@@ -348,6 +349,7 @@ func (s *MigrationExportSuite) TestMachineDevices(c *gc.C) {
 	c.Check(ex1.Label(), gc.Equals, "sda-label")
 	c.Check(ex1.UUID(), gc.Equals, "some-uuid")
 	c.Check(ex1.HardwareID(), gc.Equals, "magic")
+	c.Check(ex1.WWN(), gc.Equals, "drbr")
 	c.Check(ex1.BusAddress(), gc.Equals, "bus stop")
 	c.Check(ex1.Size(), gc.Equals, uint64(16*1024*1024*1024))
 	c.Check(ex1.FilesystemType(), gc.Equals, "ext4")
@@ -818,6 +820,7 @@ func (s *MigrationExportSuite) TestVolumes(c *gc.C) {
 	volTag := names.NewVolumeTag("0/0")
 	err := s.State.SetVolumeInfo(volTag, state.VolumeInfo{
 		HardwareId: "magic",
+		WWN:        "drbr",
 		Size:       1500,
 		VolumeId:   "volume id",
 		Persistent: true,
@@ -843,6 +846,7 @@ func (s *MigrationExportSuite) TestVolumes(c *gc.C) {
 	c.Check(provisioned.Size(), gc.Equals, uint64(1500))
 	c.Check(provisioned.Pool(), gc.Equals, "loop")
 	c.Check(provisioned.HardwareID(), gc.Equals, "magic")
+	c.Check(provisioned.WWN(), gc.Equals, "drbr")
 	c.Check(provisioned.VolumeID(), gc.Equals, "volume id")
 	c.Check(provisioned.Persistent(), jc.IsTrue)
 	attachments := provisioned.Attachments()
diff --git a/state/migration_import.go b/state/migration_import.go
@@ -446,6 +446,7 @@ func (i *importer) importMachineBlockDevices(machine *Machine, m description.Mac
 			Label:          device.Label(),
 			UUID:           device.UUID(),
 			HardwareId:     device.HardwareID(),
+			WWN:            device.WWN(),
 			BusAddress:     device.BusAddress(),
 			Size:           device.Size(),
 			FilesystemType: device.FilesystemType(),
@@ -1535,6 +1536,7 @@ func (i *importer) addVolume(volume description.Volume) error {
 	if volume.Provisioned() {
 		info = &VolumeInfo{
 			HardwareId: volume.HardwareID(),
+			WWN:        volume.WWN(),
 			Size:       volume.Size(),
 			Pool:       volume.Pool(),
 			VolumeId:   volume.VolumeID(),
diff --git a/state/migration_import_test.go b/state/migration_import_test.go
@@ -348,6 +348,7 @@ func (s *MigrationImportSuite) TestMachineDevices(c *gc.C) {
 		Label:          "sda-label",
 		UUID:           "some-uuid",
 		HardwareId:     "magic",
+		WWN:            "drbr",
 		BusAddress:     "bus stop",
 		Size:           16 * 1024 * 1024 * 1024,
 		FilesystemType: "ext4",
@@ -961,6 +962,7 @@ func (s *MigrationImportSuite) TestVolumes(c *gc.C) {
 	volTag := names.NewVolumeTag("0/0")
 	volInfo := state.VolumeInfo{
 		HardwareId: "magic",
+		WWN:        "drbr",
 		Size:       1500,
 		Pool:       "loop",
 		VolumeId:   "volume id",
diff --git a/state/migration_internal_test.go b/state/migration_internal_test.go
@@ -575,6 +575,7 @@ func (s *MigrationSuite) TestBlockDeviceFields(c *gc.C) {
 		"Label",
 		"UUID",
 		"HardwareId",
+		"WWN",
 		"BusAddress",
 		"Size",
 		"FilesystemType",
@@ -690,7 +691,7 @@ func (s *MigrationSuite) TestVolumeDocFields(c *gc.C) {
 	s.AssertExportedFields(c, volumeDoc{}, migrated.Union(ignored))
 	// The info and params fields ar structs.
 	s.AssertExportedFields(c, VolumeInfo{}, set.NewStrings(
-		"HardwareId", "Size", "Pool", "VolumeId", "Persistent"))
+		"HardwareId", "WWN", "Size", "Pool", "VolumeId", "Persistent"))
 	s.AssertExportedFields(c, VolumeParams{}, set.NewStrings(
 		"Size", "Pool"))
 }
diff --git a/state/volume.go b/state/volume.go
@@ -123,6 +123,7 @@ type VolumeParams struct {
 // VolumeInfo describes information about a volume.
 type VolumeInfo struct {
 	HardwareId string `bson:"hardwareid,omitempty"`
+	WWN        string `bson:"wwn,omitempty"`
 	Size       uint64 `bson:"size"`
 	Pool       string `bson:"pool"`
 	VolumeId   string `bson:"volumeid"`
diff --git a/storage/blockdevice.go b/storage/blockdevice.go
@@ -36,6 +36,12 @@ type BlockDevice struct {
 	// name, as the hardware ID is immutable.
 	HardwareId string `yaml:"hardwareid,omitempty"`
 
+	// WWN is the block device's World Wide Name (WWN) unique identifier.
+	// Not all block devices have one, so WWN may be empty. This is used
+	// to identify a block device if it is available, in preference to
+	// UUID or device name, as the WWN is immutable.
+	WWN string `yaml:"wwn,omitempty"`
+
 	// BusAddress is the bus address: where the block device is attached
 	// to the machine. This is currently only populated for disks attached
 	// to the SCSI bus.
diff --git a/storage/path.go b/storage/path.go
@@ -11,6 +11,7 @@ import (
 
 const (
 	diskByID         = "/dev/disk/by-id"
+	diskByWWN        = "/dev/disk/by-id/wwn-"
 	diskByDeviceName = "/dev"
 )
 
@@ -19,6 +20,9 @@ const (
 // the first value in device.DeviceLinks, if non-empty; otherwise the device
 // name.
 func BlockDevicePath(device BlockDevice) (string, error) {
+	if device.WWN != "" {
+		return diskByWWN + device.WWN, nil
+	}
 	if device.HardwareId != "" {
 		return filepath.Join(diskByID, device.HardwareId), nil
 	}
diff --git a/storage/path_test.go b/storage/path_test.go
diff --git a/storage/volume.go b/storage/volume.go
diff --git a/worker/diskmanager/lsblk.go b/worker/diskmanager/lsblk.go
diff --git a/worker/diskmanager/lsblk_test.go b/worker/diskmanager/lsblk_test.go
diff --git a/worker/provisioner/provisioner_task.go b/worker/provisioner/provisioner_task.go
diff --git a/worker/storageprovisioner/storageprovisioner_test.go b/worker/storageprovisioner/storageprovisioner_test.go
diff --git a/worker/storageprovisioner/volume_events.go b/worker/storageprovisioner/volume_events.go
diff --git a/worker/storageprovisioner/volume_ops.go b/worker/storageprovisioner/volume_ops.go
