Implement Get-AzureBatchPoolNodeCounts and Start-AzureBatchComputeNodeServiceLogUpload cmdlets #6075

antoniowmsft · 2018-04-30T23:34:49Z

Description

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate change log has been updated
The PR does not introduce breaking changes
If applicable, the changes made in the PR have proper test coverage
For public API changes to cmdlets:
- the changes have gone through a cmdlet design review
- the cmdlet markdown files were generated using the platyPS module

…d Add-AzureBatchComputeNodeServiceLogs cmdlets

…unts on getting PoolNodeCounts

…shell into preview

matthchr · 2018-05-01T00:16:54Z

@antoniowmsft - You probably should consider collapsing some commits so that the history is a bit cleaner (I suspect the powershell guys will request that too so I'm just beating them to the punch =P)

matthchr · 2018-05-01T00:17:55Z

src/ResourceManager/AzureBatch/AzureRM.Batch.psd1

@@ -111,7 +111,8 @@ CmdletsToExport = 'Remove-AzureRmBatchAccount', 'Get-AzureRmBatchAccount',
               'Enable-AzureBatchTask', 'Set-AzureBatchTask', 'Stop-AzureBatchTask', 
               'Get-AzureBatchComputeNode', 'Get-AzureBatchJobSchedule', 
               'New-AzureBatchJobSchedule', 'Remove-AzureBatchJobSchedule', 
-               'Get-AzureBatchTaskCounts'
+               'Get-AzureBatchTaskCounts', 'Get-AzureBatchPoolNodeCounts',
+               'Add-AzureBatchComputeNodeServiceLogs'


I should've raised this on the cmdlet review but it didn't occurr to me. Should this be Start-AzureBatchComputeNodeServiceLogUpload? It doesn't really feel like an Add to me. Start is more inline with Pool resize and other long running operations we do.

Ideally, the cmdlet would wait for the upload, and allow the user to pass -AsJob to have this occur in the background

Add-AzureBatchComputeNodeServiceLogs renamed to Start-AzureBatchComputeNodeServiceLogUpload. Add exception to Get-AzureBatchPoolNodeCounts.

matthchr · 2018-05-01T00:18:34Z

src/ResourceManager/AzureBatch/ChangeLog.md

@@ -27,6 +27,10 @@
 ## Version 4.0.5
 * Fix issue with Default Resource Group in CloudShell



minor: Combine these two sections as they are duplicated

matthchr · 2018-05-01T19:35:56Z

...er/AzureBatch/Commands.Batch.Test/ComputeNodes/AddBatchComputeNodeServiceLogsCommandTests.cs

+            addComputeNodeServiceLogsCommand.BatchContext = context;
+            addComputeNodeServiceLogsCommand.PoolId = "pool";
+            addComputeNodeServiceLogsCommand.ComputeNodeId = "tvm";
+            addComputeNodeServiceLogsCommand.ContainerUrl = "https://containerUrl?sv==";


minor: If this SAS value is reused in multiple places maybe just make it a const at the class level?

matthchr · 2018-05-01T19:36:13Z

...er/AzureBatch/Commands.Batch.Test/ComputeNodes/AddBatchComputeNodeServiceLogsCommandTests.cs

+            getComputeNodeCommand.AdditionalBehaviors = new List<BatchClientBehavior>() { interceptor1 };
+
+            getComputeNodeCommand.ExecuteCmdlet();
+


minor extra spaces here

matthchr · 2018-05-01T19:37:29Z

src/ResourceManager/AzureBatch/Commands.Batch.Test/Pools/GetBatchPoolNodeCountsCommandTests.cs

+            var poolNodeCounts1 = new ProxyModels.PoolNodeCounts()
+            {
+                PoolId = "Pool1",
+                Dedicated = new ProxyModels.NodeCounts(


Can you use named parameters here (i.e. running: 1) to make it more clear what is being set to what?

Same for all the below

matthchr · 2018-05-01T19:56:10Z

src/ResourceManager/AzureBatch/Commands.Batch/help/Add-AzureBatchComputeNodeServiceLogs.md

+```
+
+### -ComputeNode
+{{Fill ComputeNode Description}}


Need to fill this out

matthchr · 2018-05-01T19:56:35Z

src/ResourceManager/AzureBatch/Commands.Batch/help/Add-AzureBatchComputeNodeServiceLogs.md

+```
+
+### -ContainerUrl
+The container url to Azure Storage.


This should be updated along with the "HelpText" field I flagged in the other comment above.

matthchr · 2018-05-01T19:57:04Z

src/ResourceManager/AzureBatch/Commands.Batch/help/Add-AzureBatchComputeNodeServiceLogs.md

+Accept wildcard characters: False
+```
+
+### -EndTime


Same comment as in Powershell code, don't need optional here and update the text

matthchr · 2018-05-01T19:57:27Z

src/ResourceManager/AzureBatch/Commands.Batch/help/Add-AzureBatchComputeNodeServiceLogs.md

+```
+
+### -StartTime
+The start time of service log to be added.


Update text as per previous comment

matthchr · 2018-05-01T22:16:34Z

src/ResourceManager/AzureBatch/Commands.Batch/help/Get-AzureBatchPoolNodeCounts.md

+```
+
+### -MaxCount
+{{Fill MaxCount Description}}


Fill this description and the others below

cormacpayne · 2018-05-02T16:25:15Z

Cmdlet review: https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/39

cormacpayne · 2018-05-02T16:09:39Z

src/ResourceManager/AzureBatch/Commands.Batch.Test/Commands.Batch.Test.csproj

@@ -97,6 +97,12 @@
      <SpecificVersion>False</SpecificVersion>
      <HintPath>..\..\..\packages\Microsoft.IdentityModel.Clients.ActiveDirectory.2.28.3\lib\net45\Microsoft.IdentityModel.Clients.ActiveDirectory.WindowsForms.dll</HintPath>
    </Reference>
+    <Reference Include="Microsoft.Rest.ClientRuntime, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">


@antoniowmsft you can remove the Microsoft.Rest.* references added in this file since they are already referenced from a common targets file

cormacpayne · 2018-05-02T16:10:17Z

src/ResourceManager/AzureBatch/Commands.Batch.Test/packages.config

@@ -19,6 +19,8 @@
  <package id="Microsoft.Data.Services.Client" version="5.6.4" targetFramework="net45" />
  <package id="Microsoft.IdentityModel.Clients.ActiveDirectory" version="2.28.3" targetFramework="net45" />
  <package id="Microsoft.Net.Http" version="2.2.28" targetFramework="net45" />
+  <package id="Microsoft.Rest.ClientRuntime" version="2.3.11" targetFramework="net452" />


@antoniowmsft same comment about removing these added references

cormacpayne · 2018-05-02T16:11:04Z

src/ResourceManager/AzureBatch/Commands.Batch/Commands.Batch.csproj

@@ -57,10 +57,22 @@
    <Reference Include="Microsoft.Data.Services.Client, Version=5.6.4.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
      <HintPath>..\..\..\packages\Microsoft.Data.Services.Client.5.6.4\lib\net40\Microsoft.Data.Services.Client.dll</HintPath>
    </Reference>
+    <Reference Include="Microsoft.Rest.ClientRuntime, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">


@antoniowmsft you can remove the NuGet references added in this file since they are already referenced in a common targets file

cormacpayne · 2018-05-02T16:12:23Z

src/ResourceManager/AzureBatch/Commands.Batch/packages.config

  <package id="Microsoft.Azure.Management.Batch" version="4.2.0" targetFramework="net452" />
  <package id="Microsoft.Data.Edm" version="5.6.4" targetFramework="net452" />
  <package id="Microsoft.Data.OData" version="5.6.4" targetFramework="net452" />
  <package id="Microsoft.Data.Services.Client" version="5.6.4" targetFramework="net452" />
+  <package id="Microsoft.Rest.ClientRuntime" version="2.3.11" targetFramework="net452" />


@antoniowmsft same comment about removing these added references

cormacpayne · 2018-05-02T16:16:32Z

...esourceManager/AzureBatch/Commands.Batch/help/Start-AzureBatchComputeNodeServiceLogUpload.md

+$sasToken = New-AzureStorageContainerSASToken -Name "contosocontainer" -Context $storageContext
+$containerUrl = "https://contosogeneral.blob.core.windows.net/contosocontainer" + $sasToken
+$batchContext = Get-AzureRmBatchAccountKeys -AccountName "contosobatch"
+Start-AzureBatchComputeNodeServiceLogUpload -BatchContext $batchContext -PoolId "contosopool" -ComputeNodeId "tvm-1612030122_1-20180405t234700z" -ContainerUrl $containerUrl -StartTime "2018-01-01 00:00:00Z"


@antoniowmsft for each of these examples, would you also provide the corresponding output that the user would see?

cormacpayne · 2018-05-02T17:00:08Z

src/ResourceManager/AzureBatch/Commands.Batch/Pools/GetBatchPoolNodeCountsCommand.cs

+
+namespace Microsoft.Azure.Commands.Batch
+{
+    [Cmdlet(VerbsCommon.Get, Constants.AzureBatchPoolNodeCounts, DefaultParameterSetName = Constants.AzureBatchPoolNodeCounts), OutputType(typeof(PSPoolNodeCounts))]


@antoniowmsft PowerShell guidelines discourage the user of plural nouns in the name of cmdlets and parameters; see this snippet for more information.

I think we should change the name of this cmdlet to Get-AzureBatchPoolNodeCount

Get-AzureBatchPoolNodeCounts always return node state counts in multiple, so no. This will use plural nouns.

cormacpayne · 2018-05-02T17:01:15Z

src/ResourceManager/AzureBatch/Commands.Batch/Pools/GetBatchPoolNodeCountsCommand.cs

+        public PSCloudPool Pool { get; set; }
+
+        [Parameter(ParameterSetName = Constants.ODataFilterParameterSet)]
+        public int MaxCount { get; set; } = defaultMaxCount;


@antoniowmsft we should add the ValidateRange attribute to this parameter to limit the values that the user can provide

cormacpayne · 2018-05-02T17:02:30Z

src/ResourceManager/AzureBatch/Commands.Batch/Pools/GetBatchPoolNodeCountsCommand.cs

+
+namespace Microsoft.Azure.Commands.Batch
+{
+    [Cmdlet(VerbsCommon.Get, Constants.AzureBatchPoolNodeCounts, DefaultParameterSetName = Constants.AzureBatchPoolNodeCounts), OutputType(typeof(PSPoolNodeCounts))]


@antoniowmsft nit: I would make the default parameter set one of the sets you defined -- specifically Constants.ParentObjectParameterSet

cormacpayne · 2018-05-02T17:05:34Z

...nager/AzureBatch/Commands.Batch/ComputeNodes/StartBatchComputeNodeServiceLogUploadCommand.cs

+
+namespace Microsoft.Azure.Commands.Batch
+{
+    [Cmdlet(VerbsLifecycle.Start, Constants.AzureBatchComputeNodeServiceLogUpload, DefaultParameterSetName = Constants.AzureBatchComputeNodeServiceLogUpload, SupportsShouldProcess = true),         


@antoniowmsft nit: same comment as Get-AzureBatchNodeCount about making the default parameter set one of the existing parameter sets in this cmdlet

Start-AzureBatchComputeNodeServiceLogUpload is using singular noun.

cormacpayne · 2018-05-02T17:08:48Z

...nager/AzureBatch/Commands.Batch/ComputeNodes/StartBatchComputeNodeServiceLogUploadCommand.cs

+                this.EndTime,
+                this.AdditionalBehaviors);
+
+            WriteObject(BatchClient.StartComputeNodeServiceLogUpload(parameters));


@antoniowmsft is this upload a long-running operation? If so, you should add the AsJob parameter to this cmdlet to allow users to run this in the background of the PowerShell process

This looks like a long running task but in fact it is not. Our backend doesn't allow file upload progress to be queried so it doesn't make sense to make cmdlet run as job.

matthchr · 2018-05-02T23:09:54Z

src/ResourceManager/AzureBatch/Commands.Batch/Pools/GetBatchPoolNodeCountsCommand.cs

+        [ValidateNotNullOrEmpty]
+        public PSCloudPool Pool { get; set; }
+
+        [Parameter(ParameterSetName = Constants.ODataFilterParameterSet), ValidateRange(1, 100)]


Don't we allow up to 1000 here?

Nevermind, as we spoke we do not

defaultMaxCount = 10 matches up with server settings.

antoniowmsft · 2018-05-03T00:09:45Z

@cormacpayne Please squash the changes into 1 commit when you merge the changes. Please use this title: "Implement Get-AzureBatchPoolNodeCounts and Start-AzureBatchComputeNodeServiceLogUpload cmdlets"

cormacpayne

@antoniowmsft a few minor comments, otherwise LGTM

Also, you will need to regenerate the markdown files for the new cmdlets once they have been updated with the requested changes

cormacpayne · 2018-05-08T22:31:14Z

src/ResourceManager/AzureBatch/ChangeLog.md

@@ -22,6 +22,8 @@
 ## Version 4.0.7
 * Set minimum dependency of module to PowerShell 5.0
 * Updated New-AzureBatchPool documentation to remove deprecated example
+* Release new cmdlet Get-AzureBatchPoolNodeCounts


@antoniowmsft please move the snippet that you added to below the ## Current Release header

That's why I asked earlier if I need to edit changelog.md. 👍

cormacpayne · 2018-05-08T22:31:57Z

src/ResourceManager/AzureBatch/Commands.Batch/Commands.Batch.csproj

@@ -61,6 +61,12 @@
      <HintPath>..\..\..\packages\WindowsAzure.Storage.6.2.0\lib\net40\Microsoft.WindowsAzure.Storage.dll</HintPath>
      <Private>True</Private>
    </Reference>
+    <Reference Include="Newtonsoft.Json, Version=6.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed, processorArchitecture=MSIL">


@antoniowmsft please remove the references to Newtonsoft.Json and System.Management.Automation added in this file as they are already referenced in a separate common targets file

cormacpayne · 2018-05-08T22:33:20Z

...nager/AzureBatch/Commands.Batch/ComputeNodes/StartBatchComputeNodeServiceLogUploadCommand.cs

+        [ValidateNotNullOrEmpty]
+        public DateTime StartTime { get; set; }
+
+        [Parameter(Position = 4, Mandatory = false,


@antoniowmsft please remove the Position property from this parameter

cormacpayne · 2018-05-08T22:33:46Z

...nager/AzureBatch/Commands.Batch/ComputeNodes/StartBatchComputeNodeServiceLogUploadCommand.cs

+        [ValidateNotNullOrEmpty]
+        public string ComputeNodeId { get; set; }
+
+        [Parameter(Position = 1, ParameterSetName = Constants.ParentObjectParameterSet, ValueFromPipeline = true)]


@antoniowmsft I believe this should be mandatory in its given parameter set

cormacpayne · 2018-05-08T22:36:02Z

src/ResourceManager/AzureBatch/Commands.Batch/Pools/GetBatchPoolNodeCountsCommand.cs

+        [ValidateNotNullOrEmpty]
+        public string PoolId { get; set; }
+
+        [Parameter(Position = 0, ParameterSetName = Constants.ParentObjectParameterSet, 


@antoniowmsft for the two parameters in this file, please remove the Position property (optional positional parameters are discouraged)

…shell into preview

antoniowmsft · 2018-05-09T21:56:14Z

help files regenerated.

antoniowmsft added 16 commits April 9, 2018 10:41

Implement Get-AzureBatchPoolNodeCounts cmdlet.

ac60d0e

Fix Get-AzureBatchPoolNodeCounts cmdlet parameter sets

321d517

Implement Add-AzureBatchComputeNodeServiceLogsCommand

bd1e28a

Fix help files

5e0aee1

Regenerate help files

047e121

Add online version for Add-AzureBatchComputeNodeServiceLogs.md

da889da

ChangeLog

d131ea8

Update help file for Get-AzureBatchPoolNodeCounts

a7978e0

Fix default command parameter sets on Get-AzureBatchPoolNodeCounts an…

c67c32f

…d Add-AzureBatchComputeNodeServiceLogs cmdlets

csproj fix

3fdad35

Implement PSNodeCounts.FormatObject to support printing node state co…

d7bfbc8

…unts on getting PoolNodeCounts

Print PoolNodeCounts in table

47b1c29

Integrate with Microsoft.Azure.Batch 8.1.1 libraries

6518af8

Merge branch 'preview' into preview

2caee73

Microsoft.Azure.Batch assembly should not be private

e418db1

Merge branch 'preview' of https://github.com/antoniowmsft/azure-power…

942120a

…shell into preview

azuresdkci added the needs-review label May 1, 2018

markcowl added the Post Build label May 1, 2018

Fix build break

13eaad6

markcowl removed the Post Build label May 1, 2018

matthchr reviewed May 1, 2018

View reviewed changes

markcowl assigned cormacpayne May 1, 2018

Address PR comments

e13d79f

antoniowmsft changed the title ~~Implement Get-AzureBatchPoolNodeCounts and Add-AzureBatchComputeNodeServiceLogs cmdlets~~ Implement Get-AzureBatchPoolNodeCounts and Start-AzureBatchComputeNodeServiceLogUpload cmdlets May 2, 2018

Address PR comments

ff7a1ad

cormacpayne suggested changes May 2, 2018

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels May 2, 2018

cormacpayne assigned antoniowmsft and unassigned cormacpayne May 2, 2018

Add PSNodeCount serialization test

8844b5e

maddieclayton added the Merge Conflicts label May 2, 2018

antoniowmsft and others added 7 commits May 2, 2018 11:21

Address PR comments

f5d252f

Address PR Comments

ab841c3

Undo unnecessary changes

b2ee4b0

Address PR comments

f85d51a

Add sample output for Start-AzureBatchComputeNodeServiceLogUpload.md

2a72533

Revert accidential AzureRM.psm1 change

8f93550

Merge branch 'preview' into preview

efef759

matthchr reviewed May 2, 2018

View reviewed changes

ValidateRange should specify max as 10

40add9c

matthchr previously approved these changes May 3, 2018

View reviewed changes

cormacpayne suggested changes May 8, 2018

View reviewed changes

cormacpayne removed the Merge Conflicts label May 8, 2018

antoniowmsft added 2 commits May 8, 2018 16:12

Merge branch 'preview' of https://github.com/antoniowmsft/azure-power…

d911832

…shell into preview

Address PR comments

f75d8e2

antoniowmsft dismissed matthchr’s stale review via f75d8e2 May 9, 2018 00:02

Regenerate help files

754ca52

cormacpayne approved these changes May 10, 2018

View reviewed changes

cormacpayne merged commit 2114877 into Azure:preview May 10, 2018

		@@ -27,6 +27,10 @@
		## Version 4.0.5
		* Fix issue with Default Resource Group in CloudShell

		getComputeNodeCommand.AdditionalBehaviors = new List<BatchClientBehavior>() { interceptor1 };

		getComputeNodeCommand.ExecuteCmdlet();

Implement Get-AzureBatchPoolNodeCounts and Start-AzureBatchComputeNodeServiceLogUpload cmdlets #6075

Implement Get-AzureBatchPoolNodeCounts and Start-AzureBatchComputeNodeServiceLogUpload cmdlets #6075

Uh oh!

Conversation

antoniowmsft commented Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

matthchr commented May 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cormacpayne commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

antoniowmsft commented Apr 30, 2018 •

edited

Loading