Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xDisk - Feature Request - MountPoints #47

Closed
Zuldan opened this issue Aug 15, 2016 · 45 comments
Closed

xDisk - Feature Request - MountPoints #47

Zuldan opened this issue Aug 15, 2016 · 45 comments
Labels
bug The issue is a bug. in progress The issue is being actively worked on by someone.

Comments

@Zuldan
Copy link

Zuldan commented Aug 15, 2016

https://powershell.org/forums/topic/dsc-resource-for-mount-points/

Please kindly add support for mount points. Many server admins use Mount Points instead of Driver Letters. Example SQL Server which requires database files to be split over many LUNs to deal with queue depth issues.

So instead of...

xDisk CreateSVolumeForSQL 
{
    DiskNumber          = 2
    DriveLetter         = 'S'
    AllocationUnitSize  = 64kb
}

...you could do...

xDisk CreateMointPointForSQL
{
    DiskNumber          = 2
    MountPath           = 'C:\MyMounts\SQLData01'
    AllocationUnitSize  = 64kb
}
@Zuldan
Copy link
Author

Zuldan commented Aug 16, 2016

Some example code...

$Disk = Get-Disk 2
$Disk | Initialize-Disk -PartitionStyle GPT
$disk | New-Partition -UseMaximumSize -MbrType IFS
$Partition = Get-Partition -DiskNumber $Disk.Number
$Partition | Format-Volume -FileSystem NTFS -Confirm:$false
$Partition | Add-PartitionAccessPath -AccessPath "C:\MyMounts\SQLData01"

@PlagueHO
Copy link
Member

Hi @Zuldan - I'll add it to my backlog 😄 Will see if I can take a look this weekend. Sounds like a pretty useful feature to me though.

The only thing would be I'd need to eliminate the tech debt on this resource at the same time - which might stretch this change out a bit. But I'll give it a shot.

@Zuldan
Copy link
Author

Zuldan commented Aug 16, 2016

@PlagueHO you rock mate! I'm surprised the mount point option hasn't already been added.

I'm trying to finish off this standard SQL build and having this feature would make it 10x better. If I can help in anyway please let me know.

@kwirkykat kwirkykat added bug The issue is a bug. help wanted The issue is up for grabs for anyone in the community. labels Aug 19, 2016
@Zuldan
Copy link
Author

Zuldan commented Aug 20, 2016

Just about finished writing the SQL configuration. Had to create a dirty script resource for configuring trace flags (dsccommunity/SqlServerDsc#110). Only thing remaining is mount points for the iSCSi LUNs (if you can't fit it in this weekend, no worries). Btw, your ISCSi resource works perfectly!

@PlagueHO
Copy link
Member

It's on my list for tomorrow :) Will do my best. This resource module has a lot of tech debt that needs to be addressed but I'm planning on having a go at it tomorrow 😄

@Zuldan
Copy link
Author

Zuldan commented Aug 20, 2016

Understood. Very grateful.

@PlagueHO
Copy link
Member

Hi @Zuldan - unfortunately this resource module is in worse shape than I thought. My main concern is that the existing unit tests are very limited in scope and there are no integration tests at all (it should be possible to create them). I raised an issue 6 months ago to get this resolved but no one has picked it up.

So I'll need to get it done. I just did it with xCertificate so I'm getting good at it now 😄

However:
DriveLetter is a Key in the existing xDisk MOF, so changing it to be optional really isn't possible.

However, adding a new Resource (xMountParition) to mount a partition disk (that has a drive letter) to a path should be possible. E.g.

xDisk CreateSVolumeForSQL 
{
    DiskNumber          = 2
    DriveLetter         = 'S'
    AllocationUnitSize  = 64kb
}
xMountParition MountFolderForSQL 
{
    DiskNumber          = 2
    PartitionNumber   = 1
    MountPath            = 'C:\MyMounts\SQLData01'
}

or

xDisk CreateSVolumeForSQL 
{
    DiskNumber          = 2
    DriveLetter         = 'S'
    AllocationUnitSize  = 64kb
}
xMountParition MountFolderForSQL 
{
    DriveLetter          = 'S'
    MountPath            = 'C:\MyMounts\SQLData01'
}

The MOF might look like this:

[ClassVersion("0.1.0.0"), FriendlyName("xMountParition")] 
class MSFT_xMountParition : OMI_BaseResource
{
    [ley] string MountDisk;
    [write] uint32 DiskNumber;
    [write] uint32 PartitionNumber;
    [write] string DriveLetter;
    [Write, Description("Determines whether the partition mount point should be added or removed"), ValueMap{"Present","Absent"}, Values{"Present","Absent"}] String Ensure;
};

I'll get started on reducing the tech debt for now.

But if I could get feedback from the DSC team on this proposal it would be appreciated (@kwirkykat, @TravisEz13).

@Zuldan
Copy link
Author

Zuldan commented Aug 21, 2016

@PlagueHO, thank you so much for looking into it. I think a seperate resource makes sense (there is no such thing as a drive label for mount points, for example). Here are my thoughts...

I would make xMountPartition (or xMountPoint?) completely seperate to xDisk. So basically xMountPartition would not reference a drive letter (like in your second example). I cannot think of a situation where you would want to redirect a drive letter to a mount, instead you would only want to mount a LUN (partition on a volume). As an example, with SQL you could easily have well over 20 mount points (a database split by the number of CPUs and each put on a seperate LUN, same goes for temp do and logs etc for high performance configurations). When you start using drive letters you start running out of options quickly. So your first example is perfect except you would want to specify the Allocation Unit Size, because xMountPartition should be handling the partition format as well. I guess xMountPartition would be identical to xDisk in functionality except it's creating mount points instead of drive letters.

Since a new resource might need to be created, would you consider making it class resource (so much cleaner!) instead of a cmdlet resource? or maybe Microsoft is only accepting cmdlet resources.

@PlagueHO
Copy link
Member

Hi @Zuldan -

Good point about the Drive Letter - I'd rather not include the option if it wasn't required.

So the MOF would look like:

[ClassVersion("0.1.0.0"), FriendlyName("xMountParition")] 
class MSFT_xMountParition : OMI_BaseResource
{
    [key] string MountPath;
    [write] uint32 DiskNumber;
    [write] uint32 PartitionNumber;
    [write] uint64 Size;
    [write] string FSLabel;
    [write] uint32 AllocationUnitSize;
    [Write, Description("Determines whether the partition mount point should be added or removed"), ValueMap{"Present","Absent"}, Values{"Present","Absent"}] String Ensure;
};

However, in this case I'd recommend against using class based resources because it prevents use on WMF4 nodes (a large amount of enterprises still haven't upgraded to WMF5 - and may lag behind still for sometime). In general all DSC Resource Kit resources are implemented as MOF based for this reason. The enterprise I work for is still WMF4 mostly - so this would prevent our teams from using it internally too - so I'm being selfish 😄

Anyway, I'll leave this discussion open for a day or so before I do any work on this. Besides, just bringing this resource to HQRM so that I can start making changes is going to take a few evenings work.

@Zuldan
Copy link
Author

Zuldan commented Aug 21, 2016

@PlagueHO, MOF looks good except I would remove FSLabel as it's only for drive letters. Also typo with xMountParition should be xMountPartition. I'm also not exactly sure PartitionNumber is needed either. xDisk always creates just 1 partition. You can't have multiple partitions on a volume and each with their own drive letter using xDisk. Maybe it was never implemented to keep things simple and probably because most people these days don't create multiple partitions on a volume. Also what would you do in situations when someone wanted to create partition 2 and partition 1 didn't exist. So maybe the mount point should always mount partition 1 (like xDisk does with drive letters).

WMF4...Ok that makes perfect sense to only have cmdlet based resources. Can't believe I didn't think about that.

@PlagueHO
Copy link
Member

@Zuldan - good thinking. Makes perfect sense about the partitions. So:

[ClassVersion("0.1.0.0"), FriendlyName("xMountPoint")] 
class MSFT_xMountPoint : OMI_BaseResource
{
    [key] string MountPath;
    [write] uint32 DiskNumber;
    [write] uint64 Size;
    [write] uint32 AllocationUnitSize;
    [Write, Description("Determines whether the partition mount point should be added or removed"), ValueMap{"Present","Absent"}, Values{"Present","Absent"}] String Ensure;
};

I changed name to xMointPoint like suggested as well.

I've completed about 33% of the HQRM changes but it will take another couple of nights to completely finish (as I have to write unit tests and some integration tests which takes a while because I didn't write the original code). I've also got to finish the conversion of iSCSI over to HQRM tomorrow night....so things are taking longer than I had hoped.

@Zuldan
Copy link
Author

Zuldan commented Aug 21, 2016

No stress over getting this one done. If it's last in the queue that's fine with me. Go go iSCSI to MSFT! Super exciting stuff.

With regards to xMountPoint. How about we go with your idea from #54 and include a FileSystem property. With NTFS and ReFS as options but NTFS as default.

[ClassVersion("0.1.0.0"), FriendlyName("xMountPoint")]
class MSFT_xMountPoint : OMI_BaseResource
{
[key] string MountPath;
[write] uint32 DiskNumber;
[write] uint64 Size;
[write] uint32 AllocationUnitSize;
[write] string FileSystem
[Write, Description("Determines whether the mount point should be added or removed"), ValueMap{"Present","Absent"}, Values{"Present","Absent"}] String Ensure;
};

@PlagueHO
Copy link
Member

I'm about halfway done on #53, but in doing it I've found numerous issues that need to be resolved. So I've raised them as issues I'll resolve these at the same time. So I'll need some feed back from the DSC community on the changes before I can finish.

@Zuldan
Copy link
Author

Zuldan commented Aug 23, 2016

No worries mate. Thanks for the heads up.

If you're ever on Gitter give me a shout. I just have a couple of questions about version control software.

@PlagueHO
Copy link
Member

I've done as much as I can do on #53. Just waiting on #38 to be merged and a plan for #56 so I can submit the final HQRM version.

@kwirkykat kwirkykat added in progress The issue is being actively worked on by someone. and removed help wanted The issue is up for grabs for anyone in the community. labels Aug 29, 2016
@Zuldan
Copy link
Author

Zuldan commented Sep 13, 2016

Hi @PlagueHO, any chance at looking at xMountPoint since #38 is merged and #56 is stale due a bug? Sorry I know your really busy on many other resources. I don't want to come across as pushy. Tell me to bugger off if need be.

@PlagueHO
Copy link
Member

Hi @Zuldan - no worries! Completely understand.

I've added this item to my task list in #53.

I do have a bit of a concern though with allowing this resource to partition and format disks. This could lead to conflicts such as the same disk being provisioned twice using two different methods (xDisk and xMountPoint) - which could be done in two different ways (resulting in a config that would never come into sync).

I'd much rather allow xDisk to be able to format disks without attaching a Drive letter and have MountPoint only mount existing disks (never perform any destructive work on the disk). The name xMountPoint implies you're just assigning a disk to a mount point - not performing any destructive work on the disk.

Also having two different resources be able to perform the same tasks on the same object feels wrong to me.

Anyway, I'll continue working on all this tonight/tomorrow and see what I can come up with. I think I'll have to do away with the multiple mount point support in xMountImage because the cmdlets simply don't support it (unless @TravisEz13 pops up with a work around/suggestion). But I imagine the MSFTies are going to be a bit snowed under with the lead up to Ignite and WS2016 release - so I'd expect things to be a bit slow for a while.

I'll keep you in the loop with progress on this!

@Zuldan
Copy link
Author

Zuldan commented Sep 13, 2016

@PlagueHO, happy to have the mount point feature as part of xDisk. I'm guessing removing key from DriveLetter will be a breaking changing though? Hopefully MS will be ok with it.

If you need me to do any testing, I'm more than happy to help.

Sounds like @kwirkykat and her colleagues are under the pump at the moment from management for the xPSDSC 21/09 deadline which maybe has something to do with Ignite. Speaking of which, I really hope WMF 5.1 is released for 2012 R2 during Ignite. It fixes some big issues I have with DSC (generating large amount of IO on the disk which cause a large amount of data replication between AU and NZ offices. It also finally has proper node reporting when using ConfigIDs and debugging class resources is actually do-able now, not to mention all the nice speed increases.

@PlagueHO
Copy link
Member

@Zuldan- working hard on this one today so should have something for you by the end of play today (as part of #53). But I expect it won't be reviewed by the team for a while - so you'd need to use the Dev branch version for a bit 😄

@Zuldan
Copy link
Author

Zuldan commented Sep 16, 2016

@PlagueHO thank you so much! Really excited.

@PlagueHO
Copy link
Member

Haha @Zuldan ! Anyway, I've gone with adding the new xMountPoint resource (that includes formatting and partitioning etc) because the amount of change required to allow xDisk to support mount points.

@Zuldan
Copy link
Author

Zuldan commented Sep 17, 2016

No worries mate. A resource from scratch with HQRM in mind. It's going to look beautiful!

@PlagueHO
Copy link
Member

@Zuldan - it's turning out to mainly just be a clone/modification of HQRM version of xDisk.

I've also gone with the name xDiskAccessPath (to relate it to xDisk as it can be used in place of it as well as relate it to the cmdlet name that actually assigns the path Add-PartitionAccessPath

Hope this is OK?

@Zuldan
Copy link
Author

Zuldan commented Sep 17, 2016

@PlagueHO that name is fine. Sorry about all the work this is causing. Could you post the MOF?

@PlagueHO
Copy link
Member

Oh it isn't all that hard at all TBH - the hard work was bringing xStorage up to HQRM (lots of bugs, lots of violations, almost no tests etc).

Here is the MOF:


[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 preferred letter to assign to the disk volume.")] 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;
};

And here is the Example usage I'm including:

# This configuration will wait for disk 2 to become available, and then make the disk available as
# two new formatted volumes mounted to folders c:\SQLData and c:\SQLLog, with c:\SQLLog using all
# available space after c:\SQLData has been created.
Configuration Sample_DataDiskwithAccessPath
{

    Import-DSCResource -ModuleName xStorage

    Node localhost
    {
        xWaitforDisk Disk2
        {
             DiskNumber = 2
             RetryIntervalSec = 60
             Count = 60
        }

        xDiskAccessPath DataVolume
        {
             DiskNumber = 2
             AccessPath = 'c:\SQLData'
             Size = 10GB
        }

        xDiskAccessPath LogVolume
        {
             DiskNumber = 2
             AccessPath = 'c:\SQLLog'
             DependsOn = '[xDisk]DataVolume'
        }
    }
}

DataDisk -outputpath C:\Sample_DataDiskwithAccessPath
Start-DscConfiguration -Path C:\Sample_DataDiskwithAccessPath -Wait -Force -Verbose

@Zuldan
Copy link
Author

Zuldan commented Sep 17, 2016

@PlagueHO, thanks for that.

FSLabel can probably be removed as you only set a label on a drive letter.

Everything else looks good so far. Yay for ReFS support.

@PlagueHO
Copy link
Member

@Zuldan - true. I'll get rid of it (just to save time implementing the tests for it).

@PlagueHO
Copy link
Member

Ok @Zuldan!

The new resource is code complete and can be found in this branch of my Repo:
https://github.com/PlagueHO/xStorage/tree/Issue-53/Upgrade-to-HQRM-Standards

What is done:

  • New Resource code including Localization and MOF to HQRM standards
  • Readme.md
  • Example
  • Integration Tests
  • Unit Tests

So as you can see I haven't finished the unit tests, but I should have them done by later tonight.

I also haven't actually tested the scenario where multiple partitions are added to the same disk using this resource. I also have no idea if mixing and matching xDisk and xDiskAssignPath on the same disk will work - it probably would but I'm not certain. Using xDisk and xDiskAssignPath on different disks on the same node will be no-problemo.

TBH I don't actually like the design pattern used for xDisk and xDiskAssignPath - I think it doesn't fit well as there is no way of dismounting a disk or mount path and supporting multiple partitions on the same disk is "kludgy". But I'll leave that discussion for another day.

Let me know if you run into any bugs/problems. I'll probably submit this PR to xStorage later on tonight.

@Zuldan
Copy link
Author

Zuldan commented Sep 18, 2016

@PlagueHO if I wasn't 3 hrs away I'd drive over and give you a crate of beer. Thank you so much for making the resource. Did a quick test in my home lab, looks good so far. Ive got some SQL test servers at work so I'll test it on those tomorrow.

@PlagueHO
Copy link
Member

Haha @Zuldan just none of that Castlemaine XXXX or VB eh? 😁 It's a pleasure getting this done for the community - might even be able to get our operations teams using it as well.

Anyway, just working on the unit tests tonight - should have the PR submitted by tomorrow.

@Zuldan
Copy link
Author

Zuldan commented Sep 23, 2016

@PlagueHO, I had the new resource running on a couple of SQL servers this week and it's been working great. I can't thank you enough. Would love to catch up for a coffee (on me) when I'm in NZ next for work.

@PlagueHO
Copy link
Member

@Zuldan! That is awesome news - glad it seems to be bug free so far (and hopefully completely).

Definitely! Look me up next time you're over here- I'm always up for a coffee (fuels my DSC obsession after all 😀 )

@SQLHorizons
Copy link

@PlagueHO Ref: xDiskAccessPath, could I request you keep FSLabel, as this makes the function of the disks clearer from Disk Management GUI. I had been working on this code myself for the past 2 days, until I hit a problem with the disk refresh and stumbled on this chain in my research. See attached code
xSQLStorage.zip

As you seem to be ahead of me on this one, I'll chalk it up to a good learning exercise and test against your code.

@Zuldan
Copy link
Author

Zuldan commented Sep 27, 2016

@pauldmax I'm clearly missing something here. Are you saying you can assign labels to volumes without drive letters?

@PlagueHO
Copy link
Member

PlagueHO commented Sep 28, 2016

@PauldMax - I understand I think. The Volume still has a label attached (even though it isn't visible when mounted as drive letter). It would be easy enough to allow this to be set (in fact I actually originally did set it but removed the code as I didn't think it was helpful).

I'm sure I can re-expose it. I'll try and get it done this weekend and submit it as part of the existing PR. Otherwise I'll do it as part of a subsequent PR.

@SQLHorizons
Copy link

@Zuldan, yes you can assign labels to mounted drives. This makes function clearer for those that still need to use Disk Management
diskmgr

@SQLHorizons
Copy link

@PlagueHO, run a few test with your resource and seeing the same issue I was facing with my own. If I have a parent child disk config, where we mount the parent (say T:) and then configuring a path for a child mount (say T:\Data_X\data_01), then attempt to mount the child disk, the resource is failing saying path doesn't exist. It's almost as if the LCM is not aware of the parent disk yet, and add a wait for volume made no difference.
See attached example mof and errors below.

Error Detail is The SendConfigurationApply function did not succeed.. Resource Id is [xDiskAccessPath]DiskNumber::[MountSQLDisk]Data_01 and Source Info is ::39::5::xDiskAccessPath. Error Message is PowerShell DSC resource MSFT_xDiskAccessPath failed to execute Test-TargetResource functionality with error message: Access Path 'T:\Data_X\data_01' is not found. .

Error Message: PowerShell DSC resource MSFT_xDiskAccessPath failed to execute Test-TargetResource functionality with error message: Access Path 'T:\Data_X\data_01' is not found.

s1.mof.txt

Good news is that because this is on a pull server, when it re-runs 15 minutes later the LCM can see the T: drive that was created on the last pass.

@PlagueHO
Copy link
Member

@pauldmax that is some good info there. I'll do some digging this weekend and create some integration tests for the scenario. Assuming they replicate the issue I'll to my best to fix it. We have encountered some other issues with delays before.

@PlagueHO
Copy link
Member

PlagueHO commented Oct 1, 2016

@pauldmax - I've implemented the changes you've asked for (FSLabel support). However, I haven't taken a look at the issue you've reported yet. I'll try and get onto this as soon as I can, but the conversion to HQRM takes up a lot of time - so haven't really had a chance to look at this in detail. I may revisit it after the PR is merged simply because I've had this PR open for a while now and I'd like to get it closed.

@ArieHein
Copy link

ArieHein commented Oct 8, 2016

This is the same, in idea, as DSC resources being copied and used in the same script...cant do that as the session didnt have the resources when iniitialized. When the xStorage loads up it will gather all drives for that session. Until you close the session and open a new one, you wont see the new drives.

You could potentially in the code, invoke a rescan of the drives and create a new object from the returned results and act on that ?

@PlagueHO
Copy link
Member

PlagueHO commented Oct 8, 2016

@ArieHein - I think you're right. I also think this issue is the same.

It is easy enough to fix for a drive (just call Get-PSDrive and a rescan is triggered), but for a mount point I'm not sure. But I'm sure there is a way, I just need to put aside some time for testing things out.

I'm thinking at the moment that I'd like to get this PR merged and then raise this particular issue as a separate issue. I'd like to fix the problem before the next release, and do intend to do that, but it would be good to get this PR merged as I seem to have a lot of them open at the moment in various stages of the process.

@kwirkykat - do you want me to fix this issue in this PR or raise this as a separate issue and fix it in another PR?

@kwirkykat
Copy link
Contributor

@PlagueHO Unless you've already added this, yes please submit a separate PR

@PlagueHO
Copy link
Member

@kwirkykat - I haven't added this. But I will work on this once this PR has gone through.

Having so many PR's up in the air is making me dizzy 😄 so I'm just trying to get some closed before working on any new ones.

@SQLHorizons
Copy link

@PlagueHO, sorry it's been awhile, but I'm back using dsc with another client and see that the issue with parent child disk config I highlighted on the Sep 29 2016 is still a problem. Is there anyway I can work around this?

@PlagueHO
Copy link
Member

PlagueHO commented Sep 1, 2017

Hi @SQLHorizons - Sorry, I really dropped the ball on this one! I think because it got mixed in with all the other stuff in the issue.

Could I be a total pain and get you to raise this in a separate issue so I can track it and resolve it correctly? I'll do my best to get this looked at by the end of next week (I'm completely flat out at my day job till Thursday next week). Hope this is OK?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue is a bug. in progress The issue is being actively worked on by someone.
Projects
None yet
Development

No branches or pull requests

5 participants