Disk: correct volume error - drive is read only #183
Conversation
|
|
|
Closing and Reopening PR to trigger CLA check. |
|
Hi @JoshuaJSwain - It looks like the tests (unit and integration) are failing on this. Are you able to look into these? |
|
@JoshuaJSwain - are there any other methods of detecting if the disk/partition/volume is readonly rather than just retrying the format? Just think it might be worth checking this. |
Codecov Report
@@ Coverage Diff @@
## master #183 +/- ##
======================================
+ Coverage 94% 94% +<1%
======================================
Files 8 8
Lines 873 875 +2
======================================
+ Hits 821 823 +2
Misses 52 52 |
|
I can't seem to find any identifying properties of the partition being not ready that can be tested against in powershell. I'll need help with the unit tests as I'm not sure why those are failing. Import-Module 'C:\Program Files\WindowsPowerShell\Modules\StorageDsc\Modules\StorageDsc\DSCResources\MSFTDSC_Disk\MSFTDSC_Disk.psm1' Set-TargetResource -DriveLetter z -FSLabel test -AllocationUnitSize 65536 -FSFormat NTFS -PartitionStyle GPT -DiskIdType UniqueId -DiskId 6000C2921F16E654D83EE153F15DB810 -Verbose
reproduce outside the module attempting to get any information from the partition to test against for this race condition: Get-Disk -UniqueId 6000C2921F16E654D83EE153F15DB810 | get-Partition | Remove-Partition -Confirm:$false
$partition | format-custom * class CimInstance#ROOT/Microsoft/Windows/Storage/MSFT_Partition DiskId = \?\scsi#disk&ven_vmware&prod_virtual_disk#5&4a84755&0&000800#{53f56307-b6bf-11d0-94f2-00a0c91efb8b} CimInstanceProperties = CimSystemProperties = |
…hile loop" This reverts commit 88fa9c6.
|
@PlagueHO I did some more testing and we can simplify the fix by adding the sleep wait right after the new-partition creation to avoid the race condition. |
|
Cool! Thanks @JoshuaJSwain - I'll complete the review tomorrow! Just got a couple of other reviews to get through tonight. |
There was a problem hiding this comment.
Hello @PlagueHO checking into to see if you've had a chance to review the changes yet
Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @PlagueHO)
|
Hi @JoshuaJSwain - On it now! 😁 sorry about the delay! |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @JoshuaJSwain)
CHANGELOG.md, line 6 at r3 (raw file):
- Disk: - Added a sleep-wait after new-partition - fixes [Issue #85](https://github.com/PowerShell/StorageDsc/issues/85).
The issue doesn't actually refer to the specific error that this seems to be resolving. Could you put some detail into here as to the condition that is occurring? Also, the sleep-wait should be start-sleep - and could you also mention how long?
Modules/StorageDsc/DSCResources/MSFTDSC_Disk/MSFTDSC_Disk.psm1, line 440 at r3 (raw file):
# Create the partition. $partition = $disk | New-Partition @partitionParams | ForEach-Object { Start-Sleep -Seconds 3; $_}
A few queries here:
The foreach-object wrapper doesn't seem necessary here. It seems that this code is functionally the same as:
$partition = $disk | New-Partition @partitionParams
Start-Sleep -Seconds 3I could be wrong though - could you clarify?
Also, can we throw a message in before the wait, letting the user know that the resource is waiting 'x' seconds because "description of the reason the wait is required". This will aid in debugging.
We would also need to add a comment as to why this change would work and what situation it is fixing. It isn't quite clear as to the condition it is avoiding? E.g. does the disk report that .IsReadOnly = $false, but the disk is actually read only? If that is the case, which line actually hits the error?
The other query I have is that because we're not using a deterministic method of determining if the partition is "ready" then 3 seconds might only work sometimes (every disk will behave differently). E.g. what if it takes longer than 3 seconds before becoming "ready"? Is there another way we can validate if the disk is ready?
Following on from this ideally we'd want a way to put test automation over this to validate that it does actually resolve the issue?
Sorry about all the questions 😁 all just part of the review process.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @PlagueHO)
CHANGELOG.md, line 6 at r3 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
The issue doesn't actually refer to the specific error that this seems to be resolving. Could you put some detail into here as to the condition that is occurring? Also, the
sleep-waitshould bestart-sleep- and could you also mention how long?
Updated the changelog to clarify and correct the fix implemented.
Modules/StorageDsc/DSCResources/MSFTDSC_Disk/MSFTDSC_Disk.psm1, line 440 at r3 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
A few queries here:
The foreach-object wrapper doesn't seem necessary here. It seems that this code is functionally the same as:$partition = $disk | New-Partition @partitionParams Start-Sleep -Seconds 3I could be wrong though - could you clarify?
Also, can we throw a message in before the wait, letting the user know that the resource is waiting 'x' seconds because "description of the reason the wait is required". This will aid in debugging.
We would also need to add a comment as to why this change would work and what situation it is fixing. It isn't quite clear as to the condition it is avoiding? E.g. does the disk report that .IsReadOnly = $false, but the disk is actually read only? If that is the case, which line actually hits the error?
The other query I have is that because we're not using a deterministic method of determining if the partition is "ready" then 3 seconds might only work sometimes (every disk will behave differently). E.g. what if it takes longer than 3 seconds before becoming "ready"? Is there another way we can validate if the disk is ready?
Following on from this ideally we'd want a way to put test automation over this to validate that it does actually resolve the issue?
Sorry about all the questions 😁 all just part of the review process.
All great questions, thanks for reviewing. The foreach-object is not needed since we'll be passing one object and $disk is pulled from the function above and I've removed it.
The fix is to avoid the error on format volume that states the disk is readonly and leaves the volume as a raw format. There is no property indicating the partition is readonly. Running $partition = $disk | New-Partition -UseMaximumSize and reviewing all properties of $partition reveal no indication the partition is not ready. This appears to be a fundamental problem with the the New-Partition cmdlet that we're working around by a wait to avoid the race condition.
I really haven't found a wait to determine or test for the partition not ready as noted in #183 (comment) you can reproduce by looping on the code below and when the issue is encountered inspect the $partition properties - but it's not marked as readonly. I'd welcome any input that I might be missing to get a better test on the partition readiness so we can use that instead.
Get-Disk -UniqueId 6000C2921F16E654D83EE153F15DB810 | get-Partition | Remove-Partition -Confirm:$false
$disk = Get-Disk -UniqueId 6000C2921F16E654D83EE153F15DB810
$partition = $disk | New-Partition -UseMaximumSize
$partition = $partition | Get-Partition
$volume = $partition | Get-Volume
$volume = $partition | Format-Volume -FileSystem NTFS -NewFileSystemLabel test -AllocationUnitSize 65536 -Confirm:$false
Format-Volume : Cannot perform the requested operation when the drive is read only
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @PlagueHO)
|
Hi @JoshuaJSwain - it looks like there are some merge conflicts here - can you resolve and I'll try and get the review sorted this weekend (a public holiday so I have extra time for reviews 😁) |
There was a problem hiding this comment.
Fixed, sorry - missed that in this review interface.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @PlagueHO)
There was a problem hiding this comment.
Reviewed 1 of 75 files at r6, 1 of 2 files at r7.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @JoshuaJSwain)
CHANGELOG.md, line 17 at r7 (raw file):
- Disk: - Added a Start-Sleep -Seconds 3 after new-partition - fixes [Issue #85](https://github.com/PowerShell/StorageDsc/issues/85). - The problem occurs when the partition is created and the format-volume
To indicate this detail is all related to the above issue, can you do something like:
- Added a Start-Sleep -Seconds 3 after new-partition.
The problem occurs when the partition is created and the format-volume
is attempted before the volume has completed.
There appears to be no property to determine if the partition is
sufficiently ready to format and it will often format as a raw volume when
the error occurs - fixes [Issue #85](https://github.com/PowerShell/StorageDsc/issues/85).DSCResources/MSFTDSC_Disk/MSFTDSC_Disk.psm1, line 444 at r7 (raw file):
Write-Verbose -Message ( @( "$($MyInvocation.MyCommand): " $('waiting for 3 seconds for partition creation to complete')
Can you move this string over to the localization file? e.g. See other Write-Verbose strings.
Modules/StorageDsc/DSCResources/MSFTDSC_Disk/MSFTDSC_Disk.psm1, line 440 at r3 (raw file):
Previously, JoshuaJSwain (Josh Swain) wrote…
All great questions, thanks for reviewing. The foreach-object is not needed since we'll be passing one object and $disk is pulled from the function above and I've removed it.
The fix is to avoid the error on format volume that states the disk is readonly and leaves the volume as a raw format. There is no property indicating the partition is readonly. Running $partition = $disk | New-Partition -UseMaximumSize and reviewing all properties of $partition reveal no indication the partition is not ready. This appears to be a fundamental problem with the the New-Partition cmdlet that we're working around by a wait to avoid the race condition.
I really haven't found a wait to determine or test for the partition not ready as noted in #183 (comment) you can reproduce by looping on the code below and when the issue is encountered inspect the $partition properties - but it's not marked as readonly. I'd welcome any input that I might be missing to get a better test on the partition readiness so we can use that instead.
Get-Disk -UniqueId 6000C2921F16E654D83EE153F15DB810 | get-Partition | Remove-Partition -Confirm:$false
$disk = Get-Disk -UniqueId 6000C2921F16E654D83EE153F15DB810
$partition = $disk | New-Partition -UseMaximumSize
$partition = $partition | Get-Partition
$volume = $partition | Get-Volume
$volume = $partition | Format-Volume -FileSystem NTFS -NewFileSystemLabel test -AllocationUnitSize 65536 -Confirm:$false
Format-Volume : Cannot perform the requested operation when the drive is read only
Great detail and response! Thank you - and sorry for taking so long to review. Day job has been taking up too much time.
I'm wondering if it is possible if we can actually do the same thing as what you're doing by putting a "minimum sleep time" into the while block below instead?
E.g.
<#
After creating the partition it can take a few seconds for it to become writeable.
Wait for up to 30 seconds for the partition to become writeable.
ADD YOUR COMMENTS HERE - LIKE IN CHANGELOG TO HELP FUTURE CONTRIBUTORS UNDERSTAND WHY THIS IS
#>
$timeAtStart = Get-Date
$minimumTimeToWait = $timeAtStart + (New-Timespan -Second 3)
$maximumTimeToWait = $timeAtStart + (New-Timespan -Second 30)
while ($partition.IsReadOnly -and (Get-Date) -lt $maximumTimeToWait -and (Get-Date) -lt $minimumTimeToWait)
{
Write-Verbose -Message ( @(
"$($MyInvocation.MyCommand): "
($localizedData.NewPartitionIsReadOnlyMessage `
-f $DiskIdType, $DiskId, $partition.PartitionNumber)
) -join '' )
Start-Sleep -Seconds 1
# Pull the partition details again to check if it is readonly
$partition = $partition | Get-Partition
}What do you think? It then combines all the "wait" logic into one block rather than two.
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 4 unresolved discussions (waiting on @JoshuaJSwain and @PlagueHO)
CHANGELOG.md, line 17 at r7 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
To indicate this detail is all related to the above issue, can you do something like:
- Added a Start-Sleep -Seconds 3 after new-partition. The problem occurs when the partition is created and the format-volume is attempted before the volume has completed. There appears to be no property to determine if the partition is sufficiently ready to format and it will often format as a raw volume when the error occurs - fixes [Issue #85](https://github.com/PowerShell/StorageDsc/issues/85).
Done.
DSCResources/MSFTDSC_Disk/MSFTDSC_Disk.psm1, line 445 at r8 (raw file):
"$($MyInvocation.MyCommand): " $('waiting for 3 seconds for partition creation to complete') ) -join '' )
I'll test the minimumTimeToWait approach and we can drop this entire block, if we need it I'll move the message to MSFTDSC_Disk.strings
Modules/StorageDsc/DSCResources/MSFTDSC_Disk/MSFTDSC_Disk.psm1, line 440 at r3 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Great detail and response! Thank you - and sorry for taking so long to review. Day job has been taking up too much time.
I'm wondering if it is possible if we can actually do the same thing as what you're doing by putting a "minimum sleep time" into the while block below instead?
E.g.
<# After creating the partition it can take a few seconds for it to become writeable. Wait for up to 30 seconds for the partition to become writeable. ADD YOUR COMMENTS HERE - LIKE IN CHANGELOG TO HELP FUTURE CONTRIBUTORS UNDERSTAND WHY THIS IS #> $timeAtStart = Get-Date $minimumTimeToWait = $timeAtStart + (New-Timespan -Second 3) $maximumTimeToWait = $timeAtStart + (New-Timespan -Second 30) while ($partition.IsReadOnly -and (Get-Date) -lt $maximumTimeToWait -and (Get-Date) -lt $minimumTimeToWait) { Write-Verbose -Message ( @( "$($MyInvocation.MyCommand): " ($localizedData.NewPartitionIsReadOnlyMessage ` -f $DiskIdType, $DiskId, $partition.PartitionNumber) ) -join '' ) Start-Sleep -Seconds 1 # Pull the partition details again to check if it is readonly $partition = $partition | Get-Partition }What do you think? It then combines all the "wait" logic into one block rather than two.
I like this approach, testing it out.
…g on get-partition read only
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @PlagueHO)
DSCResources/MSFTDSC_Disk/MSFTDSC_Disk.psm1, line 444 at r7 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you move this string over to the localization file? e.g. See other Write-Verbose strings.
Done.
Modules/StorageDsc/DSCResources/MSFTDSC_Disk/MSFTDSC_Disk.psm1, line 440 at r3 (raw file):
Previously, JoshuaJSwain (Josh Swain) wrote…
I like this approach, testing it out.
With a minor change in the if statement to use -or and group the isreadony with the maximumtimetowait it works as expected.
run a bunch of loops with 100% success
Get-Disk -UniqueId 6000C293B173D112578A8085B2543E3F | get-Partition | Remove-Partition -Confirm:$false
Set-TargetResource -DriveLetter z -FSLabel test -AllocationUnitSize 65536 -FSFormat NTFS -PartitionStyle GPT -DiskIdType UniqueId -DiskId 6000C293B173D112578A8085B2543E3F -Verbose
VERBOSE: Set-TargetResource: Setting disk with UniqueId '6000C293B173D112578A8085B2543E3F' status for drive letter 'z'.
VERBOSE: Set-TargetResource: Checking disk with UniqueId '6000C293B173D112578A8085B2543E3F' partition style.
VERBOSE: Set-TargetResource: Disk with UniqueId '6000C293B173D112578A8085B2543E3F' is already initialized with 'GPT'.
VERBOSE: Set-TargetResource: Disk with UniqueId '6000C293B173D112578A8085B2543E3F' does not contain a partition assigned to drive letter 'z'.
VERBOSE: Set-TargetResource: Creating partition on disk with UniqueId '6000C293B173D112578A8085B2543E3F' with drive letter 'z' using all free space.
VERBOSE: Set-TargetResource: New partition '1' on disk with UniqueId '6000C293B173D112578A8085B2543E3F' is readonly. Waiting for it to become writable.
VERBOSE: Set-TargetResource: New partition '1' on disk with UniqueId '6000C293B173D112578A8085B2543E3F' is readonly. Waiting for it to become writable.
VERBOSE: Set-TargetResource: New partition '1' on disk with UniqueId '6000C293B173D112578A8085B2543E3F' is readonly. Waiting for it to become writable.
VERBOSE: Set-TargetResource: Formatting the volume as 'NTFS'.
There was a problem hiding this comment.
I think it's all sorted now and ready for review.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @PlagueHO)
|
Cool. I'll get onto this today @JoshuaJSwain |
|
Hi @PlagueHO checking for feedback on this one. |
|
On it tonight- sorry about the delay again! |
There was a problem hiding this comment.
Awesome stuff @JoshuaJSwain
Reviewed 3 of 3 files at r9.
Reviewable status:complete! all files reviewed, all discussions resolved
|
@JoshuaJSwain - I messed up and didn't notice you'd raised your PR against the Master branch. So I've got to revert it. Can you re-raise this PR against the Dev branch again for me and I'll get it through quickly. |
Pull Request (PR) description
This Pull Request (PR) fixes the following issues
Task list
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
This change is