search

dsccommunity / StorageDsc

DSC resource module is used to manage storage on Windows Servers.
https://dsccommunity.org
MIT License
71 stars 52 forks source link

xWaitForDisk | xDisk: Disk number changed between reboots #81

Closed johlju closed 7 years ago

johlju commented 7 years ago

Disk number seem to be unreliable to select the right disk. Or might be that I'm I doing something wrong? :) /cc @PlagueHO

I configured a cluster successfully with the disks. 

VERBOSE: [SQLTEST4]: LCM:  [ Start  Resource ]  [[xWaitForDisk]Disk1_DriveE]
VERBOSE: [SQLTEST4]: LCM:  [ Start  Test     ]  [[xWaitForDisk]Disk1_DriveE]
VERBOSE: [SQLTEST4]:                            [[xWaitForDisk]Disk1_DriveE] Test-TargetResource:
VERBOSE: [SQLTEST4]:                            [[xWaitForDisk]Disk1_DriveE] Test-TargetResource: Found disk '1' named 'MSFT Virtual HD'.
VERBOSE: [SQLTEST4]: LCM:  [ End    Test     ]  [[xWaitForDisk]Disk1_DriveE]  in 1.0310 seconds.
VERBOSE: [SQLTEST4]: LCM:  [ Skip   Set      ]  [[xWaitForDisk]Disk1_DriveE]
VERBOSE: [SQLTEST4]: LCM:  [ End    Resource ]  [[xWaitForDisk]Disk1_DriveE]
VERBOSE: [SQLTEST4]: LCM:  [ Start  Resource ]  [[xDisk]Disk1_DriveE]
VERBOSE: [SQLTEST4]: LCM:  [ Start  Test     ]  [[xDisk]Disk1_DriveE]
VERBOSE: [SQLTEST4]:                            [[xDisk]Disk1_DriveE] Test-TargetResource: Testing Disk '1' status for drive letter 'E'.
VERBOSE: [SQLTEST4]:                            [[xDisk]Disk1_DriveE] Test-TargetResource: Checking if disk number '1' is initialized.
VERBOSE: [SQLTEST4]:                            [[xDisk]Disk1_DriveE] Perform operation 'Query CimInstances' with following parameters, ''queryExpression' = SELECT BlockSize from Win32_Volume WHERE DriveLetter = 'E:','queryDialect' = WQL,'namespaceName' = ro
ot\cimv2'.
VERBOSE: [SQLTEST4]:                            [[xDisk]Disk1_DriveE] Operation 'Query CimInstances' complete.
VERBOSE: [SQLTEST4]: LCM:  [ End    Test     ]  [[xDisk]Disk1_DriveE]  in 1.6140 seconds.
VERBOSE: [SQLTEST4]: LCM:  [ Skip   Set      ]  [[xDisk]Disk1_DriveE]
VERBOSE: [SQLTEST4]: LCM:  [ End    Resource ]  [[xDisk]Disk1_DriveE]

After reboot of the server I tried to apply the configuration again. And it cannot find the disks any more.

VERBOSE: [SQLTEST4]: LCM:  [ Start  Resource ]  [[xWaitForDisk]Disk1_DriveE]
VERBOSE: [SQLTEST4]: LCM:  [ Start  Test     ]  [[xWaitForDisk]Disk1_DriveE]
VERBOSE: [SQLTEST4]:                            [[xWaitForDisk]Disk1_DriveE] Test-TargetResource:
VERBOSE: [SQLTEST4]:                            [[xWaitForDisk]Disk1_DriveE] Test-TargetResource: Disk '1' not found.
VERBOSE: [SQLTEST4]: LCM:  [ End    Test     ]  [[xWaitForDisk]Disk1_DriveE]  in 7.9280 seconds.
VERBOSE: [SQLTEST4]: LCM:  [ Start  Set      ]  [[xWaitForDisk]Disk1_DriveE]
VERBOSE: [SQLTEST4]:                            [[xWaitForDisk]Disk1_DriveE] Set-TargetResource:
VERBOSE: [SQLTEST4]:                            [[xWaitForDisk]Disk1_DriveE] Set-TargetResource: Disk '1' not found.
VERBOSE: [SQLTEST4]:                            [[xWaitForDisk]Disk1_DriveE] Set-TargetResource: Disk '1' not found.
VERBOSE: [SQLTEST4]:                            [[xWaitForDisk]Disk1_DriveE] Set-TargetResource: Disk '1' not found.
VERBOSE: [SQLTEST4]:                            [[xWaitForDisk]Disk1_DriveE] Set-TargetResource: Disk '1' not found.
...

Running diskpart shows that the disks no longer has the same disk numbers.

PS C:\Windows\system32> diskpart

Microsoft DiskPart version 10.0.14393.0

Copyright (C) 1999-2013 Microsoft Corporation.
On computer: SQLTEST4

DISKPART> list disk

  Disk ###  Status         Size     Free     Dyn  Gpt
  --------  -------------  -------  -------  ---  ---
  Disk 0    Online           80 GB      0 B        *
  Disk 6    Reserved       1024 MB      0 B        *
  Disk 7    Reserved       1024 MB      0 B        *
  Disk 8    Reserved       1024 MB      0 B        *
  Disk 9    Reserved       1024 MB      0 B        *
  Disk 10   Reserved       1024 MB      0 B        *

DISKPART>

The cluster is happy at least.

PS C:\Windows\system32> Get-ClusterGroup -Name 'Available Storage' | Get-ClusterResource

Name            State  OwnerGroup        ResourceType
----            -----  ----------        ------------
SQL2016-BACKUP  Online Available Storage Physical Disk
SQL2016-DATA    Online Available Storage Physical Disk
SQL2016-LOG     Online Available Storage Physical Disk
SQL2016-SYSDATA Online Available Storage Physical Disk
SQL2016-TEMPDB  Online Available Storage Physical Disk

image

Disk number 6 should be disk number 1. 

PS C:\Windows\system32> Get-ClusterGroup -Name 'Available Storage' | Get-ClusterResource -Name SQL2016-DATA | fl *

Characteristics         : Quorum, BroadcastDelete, MonitorReattach
Cluster                 : TESTCLU01
DeadlockTimeout         : 300000
Description             :
Id                      : 1f6b8d9a-1ee0-4173-ac7d-99d825815614
IsCoreResource          : False
EmbeddedFailureAction   : 2
IsAlivePollInterval     : 4294967295
IsNetworkClassResource  : False
IsStorageClassResource  : True
LastOperationStatusCode : 0
LooksAlivePollInterval  : 4294967295
MaintenanceMode         : False
MonitorProcessId        : 2420
Name                    : SQL2016-DATA
OwnerGroup              : Available Storage
OwnerNode               : SQLTEST4
PendingTimeout          : 180000
PersistentState         : 1
ResourceSpecificData1   : 0
ResourceSpecificData2   : 0
ResourceSpecificStatus  :
ResourceType            : Physical Disk
RestartAction           : 2
RestartDelay            : 500
RestartPeriod           : 900000
RestartThreshold        : 1
RetryPeriodOnFailure    : 3600000
SeparateMonitor         : False
State                   : Online
StatusInformation       : 0

image

johlju commented 7 years ago

Disks is shared thru iSCSI. Here is the relevant config.

        Service 'iSCSIService'
        {
            Name = 'MSiSCSI'
            StartupType = 'Automatic'
            State = 'Running'
        }

        iSCSIInitiator 'iSCSIInitiator'
        {
            Ensure = 'Present'
            NodeAddress = 'iqn.1991-05.com.microsoft:san01-cluster01-target'
            TargetPortalAddress = '192.168.0.80'
            TargetPortalPortNumber = 3260
            InitiatorPortalAddress = '192.168.0.98'
            IsPersistent = $true
            iSNSServer = 'san01.company.local'

            DependsOn = "[Service]iSCSIService"
        }

        xWaitforDisk Disk1_DriveE
        {
                DiskNumber = 1
                RetryIntervalSec = 60
                RetryCount = 60

            DependsOn = "[iSCSIInitiator]iSCSIInitiator"
        }

        xDisk Disk1_DriveE
        {
                DiskNumber = 1
                DriveLetter = 'E'
                FSLabel = 'Data'
                AllocationUnitSize = 64KB

                DependsOn = '[xWaitforDisk]Disk1_DriveE'
        }

        xClusterDisk ClusterDisk_DriveE
        {
            Number = 1
            Ensure = 'Present'
            Label = 'SQL2016-DATA'

            DependsOn = '[xDisk]Disk1_DriveE'
        }
johlju commented 7 years ago

This is being worked on in xFailOverCluster. see PowerShell/xFailOverCluster#26. Maybe the same logic could be brought to xDisk and xWaitForDisk?

PlagueHO commented 7 years ago

Hi @johlju - sorry for not looking at this earlier! That does look like the Disk Number isn't static. I'll check out what is being done in xFailOverCluster and see if we can port it over here. May need to still support Disk Number to prevent breaking legacy code, but also possibly support an alternate disk identifier?

johlju commented 7 years ago

Absolutely agree disk number should be kept also. If another identifier of some sort is used instead, that could "override" the disk number.

Also read that this could occur when using iSCSI, but then only with multiple paths (which I do not have, only one NIC). Which led me to believe that when using for example FC storage, disk number is (more) static.

After doing another reboot a few days later the disk jumped back to disk number 1-5.

Another thing is that the bug could be me, maybe it is me that has not configured Windows/iSCSI correctly for using iSCSI disks? :smile:

Zuldan commented 7 years ago

@PlagueHO and @johlju I've now hit this issue. The disk numbers are changing and causing havoc with this resource.

Below are all the properties for a disk. Maybe a unique identifier that could be used instead is SerialNumber/UniqueId or even GUID (used in the pull request for xFailOverCluster @johlju mentioned).

PartitionStyle        : GPT
ProvisioningType      : Fixed
OperationalStatus     : Online
HealthStatus          : Healthy
BusType               : SAS
UniqueIdFormat        : FCPH Name
OfflineReason         :
AllocatedSize         : 4294967296
BootFromDisk          : False
FirmwareVersion       : 1.0
FriendlyName          : VMware Virtual disk SCSI Disk Device
Guid                  : {455c60cf-03e3-417d-8afd-9d12edf3bae7}
IsBoot                : False
IsClustered           : False
IsOffline             : False
IsReadOnly            : False
IsSystem              : False
LargestFreeExtent     : 0
Location              : PCIROOT(0)#PCI(1700)#PCI(0000)#SAS(P00T00L00)
LogicalSectorSize     : 512
Manufacturer          : VMware
Model                 : Virtual disk
Number                : 10
NumberOfPartitions    : 2
ObjectId              : \\?\scsi#disk&ven_vmware&prod_virtual_disk#5&25e285ad&0&000000#{53f56307-b6bf-11d0-94f2-00a0c91
                        efb8b}
Path                  : \\?\scsi#disk&ven_vmware&prod_virtual_disk#5&25e285ad&0&000000#{53f56307-b6bf-11d0-94f2-00a0c91
                        efb8b}
PhysicalSectorSize    : 512
SerialNumber          : 6000c29a39d00c9d162fe873670718a7
Signature             :
Size                  : 4294967296
UniqueId              : 6000C29A39D00C9D162FE873670718A7
PSComputerName        :
CimClass              : ROOT/Microsoft/Windows/Storage:MSFT_Disk
CimInstanceProperties : {AllocatedSize, BootFromDisk, BusType, FirmwareVersion...}
CimSystemProperties   : Microsoft.Management.Infrastructure.CimSystemProperties
PlagueHO commented 7 years ago

Hi @zuldan. Im going to try and get the work done on this over the easter break/this weekend. I'm going to start by seeing if I can find a way to effectively test for the problem so I can be sure im fixing it :grin: TDD FTW!

Zuldan commented 7 years ago

@PlagueHO your timing couldn't be better. I've got the config for all our SQL AOAG clusters in the lab working perfectly (except for this disk issue) and was going to start pushing out clusters into the Dev environment next week but was forced to delay that because of this problem.

I only noticed the problem when I added multiple controllers and multiple disks to those controllers on the VM. Usually if you have a single controller and a couple of disks, the order you put the disks in is the order they appear within the VM, however when in the scenario described above, the disk order is scrambled.

I definitely think disk numbers need to be kept, but for me it's not it's a legacy the thing, it's because 90% of our servers only have a single controller so the disk number identifier works really well and I wouldn't want to have to record down the serial/GUID for every disk on every VM and then inject that back into their DSC config. So having the ability to switch over to another identifier for our clustered servers would be great.

If you want to go through some ideas around over the weekend or need some testing done I'm more than happy to help.

FYI.

Zuldan commented 7 years ago

To diagnose the issue I slapped together some dirty code to provide all the stats from 'Disk' to 'Access Path.

$ServerList = @('LABSERVER01','LABSERVER02')

foreach ($Server in $ServerList) {
    Invoke-Command -ComputerName $Server -ScriptBlock {
        $CIMDiskList = Get-CimInstance -ClassName Win32_DiskDrive
        $DiskList = Get-Disk
        $DriveList = @()

        foreach ($Disk in $DiskList) {
            $PartitionList = $Disk | Get-Partition
            foreach ($Partition in $PartitionList) {
                $VolumeList = $Partition | Get-Volume
                foreach ($Volume in $VolumeList) {
                    $SCSIData = $CIMDiskList | Where-Object -FilterScript { $PSItem.Index -eq $Disk.Number }
                    $DriveList += [pscustomobject]@{
                        DiskNumber = $Disk.Number
                        PartitionStyle = $Disk.PartitionStyle
                        AccessPath = $Partition.AccessPaths[0]
                        FileSystemLabel = $Volume.FileSystemLabel
                        Size = [math]::Round(($Volume.Size / 1GB),0)
                        FileSystem = $Volume.FileSystem
                        SCSIPort = $SCSIData.SCSIPort
                        SCSITargetId = $SCSIData.SCSITargetId
                        DiskGUID = $Disk.Guid
                        DiskUniqueId = $Disk.UniqueId
                        DiskType = $Disk.FriendlyName
                    }        
                }
            }
        }

        $DriveList | Sort-Object -Property DiskNumber
    } | OGV 
}

It appears that there isn't always a GUID for every disk so it would be better to use UniqueId?

DiskNumber PartitionStyle AccessPath                                        FileSystemLabel Size FileSystem SCSIPort SCSITargetId DiskGUID                               DiskUniqueId                    
---------- -------------- ----------                                        --------------- ---- ---------- -------- ------------ --------                               ------------                    
         0 MBR            C:\                                               SYSTEM            24 NTFS              2            0                                        6000C29D7E1780F6544C2131A1E4369B
         0 MBR            \\?\Volume{67881813-e0ea-11e6-80b7-806e6f6e6963}\                    0                   2            0                                        6000C29D7E1780F6544C2131A1E4369B
         0 MBR            \\?\Volume{67881811-e0ea-11e6-80b7-806e6f6e6963}\ System Reserved    0 NTFS              2            0                                        6000C29D7E1780F6544C2131A1E4369B
         1 GPT            E:\                                               PAGEFILE          10 NTFS              2            1 {4e1913e8-ee25-472f-8293-bb4037a4a20b} 6000C292E103C02996E7B5D40EABA62F
         2 GPT            C:\MountPoint\SQLLOG01\                           SQLLOG01           4 NTFS              3            0 {74c45082-ce10-4c44-9b4e-7ce47f2abb15} 6000C292C934C0657163D01A716C90AD
         3 GPT            C:\MountPoint\SQLLOG02\                           SQLLOG02           4 NTFS              3            1 {329d9ad1-87e7-473d-9c19-6363aad07428} 6000C299967F904E8031F14ED654099A
         4 GPT            C:\MountPoint\SQLTEMP01\                          SQLTEMP01          4 NTFS              3            2 {238934f7-81ba-409b-bd18-c013495ad8f8} 6000C29B203307E19302F667FE663966
         5 GPT            C:\MountPoint\SQLTEMP02\                          SQLTEMP02          4 NTFS              3            3 {c8bea9a8-bbee-4792-993c-07b885b134ad} 6000C29FD0846DD3CA4545BD03981B33
         6 GPT            C:\MountPoint\SQLTEMP03\                          SQLTEMP03          4 NTFS              3            4 {5864e41a-e856-47ac-b93c-49692ef26d84} 6000C2983154FC9000353CBAA87F75BB
         7 GPT            C:\MountPoint\SQLTEMP04\                          SQLTEMP04          4 NTFS              3            5 {dc40aa51-5063-47b9-824e-501cffc7ee20} 6000C29E63F8239406BA5639EBA8D3FB
         8 GPT            C:\MountPoint\SQLDATA01\                          SQLDATA01          4 NTFS              3            6 {397e1165-03ce-41c5-82b7-04f6c22bf715} 6000C29DDD85C1E920F935E6838EC9A4
         9 GPT            C:\MountPoint\SQLDATA02\                          SQLDATA02          4 NTFS              3            8 {eb912800-707e-4eca-8a29-b75fc452370b} 6000C292F83FA2D9B029BE0B3D2B831F
        10 GPT            C:\MountPoint\SQLDATA03\                          SQLDATA03          4 NTFS              4            0 {455c60cf-03e3-417d-8afd-9d12edf3bae7} 6000C29A39D00C9D162FE873670718A7
        11 GPT            C:\MountPoint\SQLDATA04\                          SQLDATA04          4 NTFS              4            2 {95be409a-f851-496f-bad2-6035902fd5f3} 6000C2927DB2BC760F03F7F5C2F7CE92
        12 GPT            C:\MountPoint\SQLDATA05\                          SQLDATA05          2 NTFS              5            0 {0508b2a1-146b-48a3-a517-8e588323665e} 6000C2919523F876874EEDA1A2B1F67B
        13 GPT            C:\MountPoint\SQLDATA06\                          SQLDATA06          2 NTFS              5            1 {5d9dc46c-26bc-4f06-b208-14a1ecd83d63} 6000C29A996C6E07EB9AE6FC50A373C1
        14 GPT            C:\MountPoint\SQLDATA07\                          SQLDATA07          2 NTFS              5            2 {776574fa-c4d3-4448-bf8c-b4fad2d1ad3d} 6000C29D063CBC1F47BB61914A413AB7
        15 GPT            C:\MountPoint\SQLDATA08\                          SQLDATA08          2 NTFS              5            3 {53725931-9e65-4d8e-a4e2-c8d32697c509} 6000C292FCE53FE46CBBA150A616C108

I came up with a couple of ways to implement it. See MOFs below. Would love a discussion and/or more ideas.

Idea 1 (most obvious one) Pro: The resources support both DiskNumber and UniqueId properties. No breaking change. Con: Will get a bit ugly with primary key.

[ClassVersion("1.0.0.0"), FriendlyName("xDisk")]
class MSFT_xDisk : OMI_BaseResource
{
    [Key, Description("Specifies the identifier for which disk to modify.")] String DriveLetter;
    [Required, Description("Specifies the disk number for which disk to modify.")] Uint32 DiskNumber;
    [Write, Description("Specifies the uniqueid for which disk to modify.")] String UniqueId;
    [Write, Description("Specifies the size of new volume.")] Uint64 Size;
    [Write, Description("Define volume label if required.")] String FSLabel;
    [Write, Description("Specifies the allocation unit size to use when formatting the volume.")] uint32 AllocationUnitSize;
    [Write, Description("Specifies the file system format of the new volume."), ValueMap{"NTFS","ReFS"}, Values{"NTFS","ReFS"}] String FSFormat;
};

[ClassVersion("1.0.0.0"), FriendlyName("xDiskAccessPath")]
class MSFT_xDiskAccessPath : OMI_BaseResource
{
    [Key, Description("Specifies the access path folder to the assign the disk volume to.")] String AccessPath;
    [Required, Description("Specifies the disk number for which disk to modify.")] Uint32 DiskNumber;
    [Write, Description("Specifies the uniqueid for which disk to modify.")] String UniqueId;
    [Write, Description("Specifies the size of new volume.")] Uint64 Size;
    [Write, Description("Define volume label if required.")] String FSLabel;
    [Write, Description("Specifies the allocation unit size to use when formatting the volume.")] uint32 AllocationUnitSize;
    [Write, Description("Specifies the file system format of the new volume."), ValueMap{"NTFS","ReFS"}, Values{"NTFS","ReFS"}] String FSFormat;
};

Idea 2 Pro: A bit cleaner, validation set telling the resource in a nicer way which type of identifier is going to be used. Also, adding support for additional identifier types will be a bit easier moving forward. Con: A breaking change, however we still get to use DiskNumber and UniqueId. If people are worried about breaking their DSC configs then they can just go back to the old version of the resources.

[ClassVersion("1.0.0.0"), FriendlyName("xDisk")]
class MSFT_xDisk : OMI_BaseResource
{
    [Key, Description("Specifies the identifier for which disk to modify.")] String DriveLetter;
    [Required, Description("Specifies the identifier for which disk to modify.")] string Identifier;
    [Write, Description("Specifies the identifier type"), ValueMap{"DiskNumber","UniqueId"}, Values{"DiskNumber","UniqueId"}] String IdentifierType;
    [Write, Description("Specifies the size of new volume.")] Uint64 Size;
    [Write, Description("Define volume label if required.")] String FSLabel;
    [Write, Description("Specifies the allocation unit size to use when formatting the volume.")] uint32 AllocationUnitSize;
    [Write, Description("Specifies the file system format of the new volume."), ValueMap{"NTFS","ReFS"}, Values{"NTFS","ReFS"}] String FSFormat;
};

[ClassVersion("1.0.0.0"), FriendlyName("xDiskAccessPath")]
class MSFT_xDiskAccessPath : OMI_BaseResource
{
    [Key, Description("Specifies the access path folder to the assign the disk volume to.")] String AccessPath;
    [Required, Description("Specifies the identifier for which disk to modify.")] string Identifier;
    [Write, Description("Specifies the identifier type"), ValueMap{"DiskNumber","UniqueId"}, Values{"DiskNumber","UniqueId"}] String IdentifierType;
    [Write, Description("Specifies the size of new volume.")] Uint64 Size;
    [Write, Description("Define volume label if required.")] String FSLabel;
    [Write, Description("Specifies the allocation unit size to use when formatting the volume.")] uint32 AllocationUnitSize;
    [Write, Description("Specifies the file system format of the new volume."), ValueMap{"NTFS","ReFS"}, Values{"NTFS","ReFS"}] String FSFormat;
};

Idea 3 (the idea here is that generally if you have a small amount of disks you assign drive letters and if you have a large amount of disks you use DiskAccess paths [letter limit of 25 excluding 'C']) Pro: xDisk - No breaking change. Remains the same as before. xDiskAccessPath - Only supports UniqueId as an identifier. Con: xDisk - Will not support an identifier other than Disk Number, not really an issue if you are only use 1 controller and a small amount of disks. xDiskAccessPath - Breaking change. No disk number as an identifier

[ClassVersion("1.0.0.0"), FriendlyName("xDisk")]
class MSFT_xDisk : OMI_BaseResource
{
    [Key, Description("Specifies the identifier for which disk to modify.")] String DriveLetter;
    [Required, Description("Specifies the disk number for which disk to modify.")] Uint32 DiskNumber;
    [Write, Description("Specifies the size of new volume.")] Uint64 Size;
    [Write, Description("Define volume label if required.")] String FSLabel;
    [Write, Description("Specifies the allocation unit size to use when formatting the volume.")] uint32 AllocationUnitSize;
    [Write, Description("Specifies the file system format of the new volume."), ValueMap{"NTFS","ReFS"}, Values{"NTFS","ReFS"}] String FSFormat;
};

[ClassVersion("1.0.0.0"), FriendlyName("xDiskAccessPath")]
class MSFT_xDiskAccessPath : OMI_BaseResource
{
    [Key, Description("Specifies the access path folder to the assign the disk volume to.")] String AccessPath;
    [Required, Description("Specifies the uniqueid for which disk to modify.")] String UniqueId;
    [Write, Description("Specifies the size of new volume.")] Uint64 Size;
    [Write, Description("Define volume label if required.")] String FSLabel;
    [Write, Description("Specifies the allocation unit size to use when formatting the volume.")] uint32 AllocationUnitSize;
    [Write, Description("Specifies the file system format of the new volume."), ValueMap{"NTFS","ReFS"}, Values{"NTFS","ReFS"}] String FSFormat;
};
PlagueHO commented 7 years ago

Cool! Great info and fantastic suggestions.

I personally like Idea 1 because of not being a breaking change. The primary key thing is a pain, but I think it is acceptable - I've used similar patterns in xNetworking and xCertificate - so I don't think it is a terrible idea.

We'd need to also change the DiskNumber from a Required to a Write parameter.

I'd also suggest using DiskUniqueId to make it clear this is a parameter of the disk.

I'd also add a parameter validation function in that would validate the parameter combination is valid - if the combo was invalid (none or both of DiskNumber and DiskUniqueId were set) then an exception would be logged.

So:

[ClassVersion("1.0.0.0"), FriendlyName("xDisk")]
class MSFT_xDisk : OMI_BaseResource
{
    [Key, Description("Specifies the identifier for which disk to modify.")] String DriveLetter;
    [Write, Description("Specifies the disk number for which disk to modify.")] Uint32 DiskNumber;
    [Write, Description("Specifies the uniqueid for which disk to modify.")] String DiskUniqueId;
    [Write, Description("Specifies the size of new volume.")] Uint64 Size;
    [Write, Description("Define volume label if required.")] String FSLabel;
    [Write, Description("Specifies the allocation unit size to use when formatting the volume.")] uint32 AllocationUnitSize;
    [Write, Description("Specifies the file system format of the new volume."), ValueMap{"NTFS","ReFS"}, Values{"NTFS","ReFS"}] String FSFormat;
};

[ClassVersion("1.0.0.0"), FriendlyName("xDiskAccessPath")]
class MSFT_xDiskAccessPath : OMI_BaseResource
{
    [Key, Description("Specifies the access path folder to the assign the disk volume to.")] String AccessPath;
    [Write, Description("Specifies the disk number for which disk to modify.")] Uint32 DiskNumber;
    [Write, Description("Specifies the uniqueid for which disk to modify.")] String DiskUniqueId;
    [Write, Description("Specifies the size of new volume.")] Uint64 Size;
    [Write, Description("Define volume label if required.")] String FSLabel;
    [Write, Description("Specifies the allocation unit size to use when formatting the volume.")] uint32 AllocationUnitSize;
    [Write, Description("Specifies the file system format of the new volume."), ValueMap{"NTFS","ReFS"}, Values{"NTFS","ReFS"}] String FSFormat;
};

@TravisEz13 , @johlju - do you guys have any thoughts on this as a solution?

johlju commented 7 years ago

It looks good to me! Maybe just change DriveLetter to DrivePath (or something) if mountpoints are used?

johlju commented 7 years ago

My bad, xDiskAccessPath is for "mount points" and xDisk is for "drive letters". I didn't realize there was two almost identical resources. So idea 1 with @PlagueHO's additions is all good to me.

PlagueHO commented 7 years ago

Cooleo! I'll get this out over the weekend.

PlagueHO commented 7 years ago

@johlju , @Zuldan - I've nearly finished the changes to xDisk (which will map to xDiskAccessPath fairly easily).

However, with regards xWaitForDisk, Idea 1 won't actually work - because DiskNumber is the key to the resource (so we can't make it optional). One solution here would be to implement Idea 2 on xWaitForDisk, but this has the following problems:

  1. Would be a breaking change
  2. Would have the resources behave in an inconsistent fashion (DiskNumber/DiskUniqueId or Identifer/IdentifierType) which I think will result in confusion.

So if we decided Idea 2 is acceptable for xWaitForDisk then perhaps we should change all resources to Idea 2?

One other alternative would be to add a new resource xWaitForDiskUniqueId (or some name like that) which would be the same as xWaitForDisk but use the DiskUniqueId as the key instead of DiskNumber.

Would love your guys thoughts before I proceed too far down one track or another...

Zuldan commented 7 years ago

@PlagueHO I'm happy with idea 2 for all resources. If we change one then they all should be aligned.

As for the breaking change, there's nothing stopping people from downloading previous versions of the resources to keep compatibility. DSC is all about continuous deployment, make changes and move forward ;-)

PlagueHO commented 7 years ago

@Zuldan - I'm in agreement. I think Idea 2 is the way to go if we're going to do a breaking change because I think consistency is very important. But I'd like some feedback from @johlju and @TravisEz13 because of being a breaking change.

@TravisEz13 - a summary of the problem is that DiskNumber in some scenarios can not be used to uniquely identify a disk. Our proposal is to replace the old 'DiskNumber' key with a 'DiskId' key that can be used as either Disk Number (the default) or as a Disk Unique Id. A new 'DiskIdType' parameter would be added that would allow the DiskId to either be 'Number' (default) or 'UniqueId'.

The Resources would be changed like this:

xDisk:

[ClassVersion("1.0.0.0"), FriendlyName("xDisk")]
class MSFT_xDisk : OMI_BaseResource
{
    [Key, Description("Specifies the drive letter to assign to the volume.")] String DriveLetter;
    [Write, Description("Specifies the disk identifier for the disk to modify.")] String DiskId;
    [Write, Description("Specifies the identifier type that is used to identify the disk."), ValueMap{"Number","UniqueId"}, Values{"Number","UniqueId"}] String DiskIdType;
    [Write, Description("Specifies the size of new volume.")] Uint64 Size;
    [Write, Description("Define volume label if required.")] String FSLabel;
    [Write, Description("Specifies the allocation unit size to use when formatting the volume.")] uint32 AllocationUnitSize;
    [Write, Description("Specifies the file system format of the new volume."), ValueMap{"NTFS","ReFS"}, Values{"NTFS","ReFS"}] String FSFormat;
};

xDiskAccessPath:

[ClassVersion("1.0.0.0"), FriendlyName("xDiskAccessPath")]
class MSFT_xDiskAccessPath : OMI_BaseResource
{
    [Key, Description("Specifies the access path folder to the assign the disk volume to.")] String AccessPath;
    [Write, Description("Specifies the disk identifier for the disk to modify.")] String DiskId;
    [Write, Description("Specifies the identifier type that is used to identify the disk."), ValueMap{"Number","UniqueId"}, Values{"Number","UniqueId"}] String DiskIdType;
    [Write, Description("Specifies the size of new volume.")] Uint64 Size;
    [Write, Description("Define volume label if required.")] String FSLabel;
    [Write, Description("Specifies the allocation unit size to use when formatting the volume.")] uint32 AllocationUnitSize;
    [Write, Description("Specifies the file system format of the new volume."), ValueMap{"NTFS","ReFS"}, Values{"NTFS","ReFS"}] String FSFormat;
};
[ClassVersion("1.0.0.0"), FriendlyName("xWaitForDisk")]
class MSFT_xWaitForDisk : OMI_BaseResource
{
    [Key, Description("Specifies the disk identifier for the disk to wt for.")] String DiskId;
    [Write, Description("Specifies the identifier type that is used to identify the disk."), ValueMap{"Number","UniqueId"}, Values{"Number","UniqueId"}] String DiskIdType;
    [Write, Description("Specifies the number of seconds to wait for the disk to become available.")] Uint32 RetryIntervalSec;
    [Write, Description("The number of times to loop the retry interval while waiting for the disk.")] Uint32 RetryCount;
};

@Zuldan - I know you're waiting on this change, but can you wait a little bit longer so I can get confirmation about the proposal?

Zuldan commented 7 years ago

@PlagueHO happy to wait mate. Better to have everyone agree to the changes.

PlagueHO commented 7 years ago

Cheers @Zuldan - it shouldn't take long to make the change as I've already done a reasonable amount on this. So as soon as I get the go-ahead I'll get it done!

johlju commented 7 years ago

@PlagueHO Awesome work as always!

Idea 1 If we would keep going in the idea 1 track, can't xWaitForDisk still have DiskNumber as Key, and you add DiskUniqueId as an optional parameter. IfDiskUniqueId is assigned a value then that will override DiskNumber, would that be a solution? Then it will not be a breaking change, but must be clearly documented that so is the case, both in README.md and verbose outputs.

Idea 2 If we still call the parameter DiskNumber instead of DiskId and DiskIdType has default value of 'Number' then it would not be a breaking change. The name DiskNumber might be confusing setting it to a GUID (UniqueId). So not the best solution. With that said, I do not have a problem making a breaking change if that solves a problem and improves the resource. In this case it will do both.

Summary If my suggestion for idea 1 does not feel good enough, then I all for idea 2 making this a breaking change.

PlagueHO commented 7 years ago

@Zuldan , @johlju I've made the change to xDisk here (including full unit and integration test coverage etc): https://github.com/PlagueHO/xStorage/tree/Issue-81/Support-DiskUniqueId

I'm going to repeat to xDiskAccessPath and xWaitForDisk tomorrow and the submit the PR.

Once that PR is through I'll start work on the new xDiskEx, xWaitForDiskEx, xPartition and xVolume.

johlju commented 7 years ago

I will help you review this one :)

PlagueHO commented 7 years ago

@johlju - I was hoping you would :grin: Your reviews are the as good as they get. Right - onto the next changes.

PlagueHO commented 7 years ago

Hi @Zuldan, @johlju - I've submitted a PR with these changes. I've also converted xDisk to use the xDiskAccessPath pattern which is a more robust pattern (and has less bugs).

Once this PR is through I'll look into creating the new resources as I still feel the current resources won't work in many scenarios.

johlju commented 7 years ago

@PlagueHO Once you fixed the comments on the PR I will use the code in my lab to verify the function as well. 😄

PlagueHO commented 7 years ago

Awesome! Thanks @johlju. I'm also going to update https://github.com/PlagueHO/LabBuilder to use the new version as well and use that to run some tests. @Zuldan - do you have any test environments you can run this version through?

Zuldan commented 7 years ago

@PlagueHO fantastic work! @johlju thank you for reviewing.

@PlagueHO I have a ton of xDiskAccessPath and xWaitForDisk to configs I can test with. I'll have to create some with xDisk. I'll begin updating my configs and start the testing then get back to you.

PlagueHO commented 7 years ago

@johlju, @Zuldan - I'm just going to fix this issue: https://github.com/PowerShell/xStorage/issues/86 in the PR as well.

Zuldan commented 7 years ago

@PlagueHO, I can report back the new code is working well. I can scramble the disks around on multiple VM's and it's picking up the correct disks every time when using DiskUniqueId. My old configurations using Disk Number (with updated config) are also still working. Very happy!

PlagueHO commented 7 years ago

@Zuldan - awesome! Very glad to hear! Woop :grin: I'm just making some final tweaks as per @johlju 's review comments and then I'll get it merged!