[Compute] Add cmdlets for CRP Log Analytics. #5665

hyonholee · 2018-03-02T23:32:52Z

Design review accepted : https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/13

This PR also contains the following bug fixes.

#5189

#5101

#3735

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

…rshell into loganalytics

maddieclayton

Will check this again when test coverage is added.

maddieclayton · 2018-03-03T00:35:58Z

src/ResourceManager/Compute/Commands.Compute.Test/ScenarioTests/LogAnalyticTests.ps1

Both of these tests should have a positive test case (can you create a new blob and container and put the logs there?)

I cannot check in SAS Uri to GitHub because of credscan. If I add a positive case with real SAS Uri and then remove secrets, then anyone who runs this test with record mode will fail. Wayne already tested this manually and I also can see that this Powershell command sends a correct REST call from recorded json file.

If you create a new storage account in test, do the manipulations there, and remove the storage account, you should not have a security issue. I believe that this code is working now, but we need positive tests to ensure it continues working in the future.

@maddieclayton I added positive test case. Note that storage cmdlets are not available in compute test, so I had to manually run those cmdlets. So I commented out any manual steps in the tests.

I think that you are able to access the cmdlets, as it is being done here:

azure-powershell/src/ResourceManager/Compute/Commands.Compute.Test/ScenarioTests/DiagnosticsExtensionTests.ps1

Line 42 in bbe5ee9

New-AzureRmStorageAccount -ResourceGroupName $rgname -Name $storagename -Location $loc -Type $storagetype

. Am I not understanding this code correctly? Network also uses dataplane Storage cmdlets and has no additional information in the test csproj:

azure-powershell/src/ResourceManager/Network/Commands.Network.Test/ScenarioTests/NetworkWatcherAPITests.ps1

Line 481 in cd0497d

New-AzureStorageContainer -Name $containerName -Context $context

@maddieclayton I update the test

…nto loganalytics # Conflicts: # src/ResourceManager/Compute/ChangeLog.md

maddieclayton · 2018-03-06T19:10:31Z

src/ResourceManager/Compute/Commands.Compute.Test/ScenarioTests/LogAnalyticTests.ps1

+#>
+function Test-ExportLogAnalyticThrottledRequests
+{
+    $loc = "West Central US";


Should use Get-Location everywhere a location is set: https://github.com/Azure/azure-powershell/blob/preview/src/ResourceManager/Common/Commands.ScenarioTests.ResourceManager.Common/Common.ps1#L583

Please ensure you specify all three parameter values.

@maddieclayton this API is only available in this location. Get-Location gives a different location, so I need to give a specific location for this API.

maddieclayton · 2018-03-06T19:12:52Z

src/ResourceManager/Compute/Commands.Compute.Test/ScenarioTests/LogAnalyticTests.ps1

I think that you are able to access the cmdlets, as it is being done here:

azure-powershell/src/ResourceManager/Compute/Commands.Compute.Test/ScenarioTests/DiagnosticsExtensionTests.ps1

Line 42 in bbe5ee9

New-AzureRmStorageAccount -ResourceGroupName $rgname -Name $storagename -Location $loc -Type $storagetype

. Am I not understanding this code correctly? Network also uses dataplane Storage cmdlets and has no additional information in the test csproj:

azure-powershell/src/ResourceManager/Network/Commands.Network.Test/ScenarioTests/NetworkWatcherAPITests.ps1

Line 481 in cd0497d

New-AzureStorageContainer -Name $containerName -Context $context

maddieclayton · 2018-03-06T19:18:09Z

src/ResourceManager/Compute/Commands.Compute.Test/ScenarioTests/LogAnalyticTests.ps1

+#>
+function Test-ExportLogAnalytics
+{
+    $rgname = "hyleelog";


For all assets, use getassetname: https://github.com/Azure/azure-powershell/blob/preview/src/ResourceManager/Common/Commands.ScenarioTests.ResourceManager.Common/Common.ps1#L371

I manually give this value, because I created the resource group manually for setting up my test. (because I cannot run storage cmdlets during the test.) So, I should not use getassetname because it gives a random name.

maddieclayton · 2018-03-06T19:19:50Z

src/ResourceManager/Compute/Commands.Compute.Test/ScenarioTests/LogAnalyticTests.ps1

+        $result = Export-AzureRmLogAnalyticThrottledRequests -Location $loc -FromTime $from -ToTime $to -BlobContainerSasUri $sasuri -GroupByThrottlePolicy -GroupByOperationName -GroupByResourceName;
+        Assert-AreEqual "Succeeded" $result.Status;
+        $output = $result | Out-String;
+        Assert-True { $output.Contains(".csv"); }


Is there a way to check this file to ensure it has the right format?

I don't think checking the content of the file is a scope of Powershell cmdlets test. It should be checked during the API test.

Could we check the blob to ensure the file is there?

maddieclayton · 2018-03-06T19:21:03Z

src/ResourceManager/Compute/Commands.Compute.Test/ScenarioTests/LogAnalyticTests.ps1

+.SYNOPSIS
+Test Export Log Analytic Request Rate By Interval
+#>
+function Test-ExportLogAnalyticRequestRateByInterval


Change test names to reflect that this is a test for failure.

maddieclayton · 2018-03-06T19:35:18Z

src/ResourceManager/Compute/Commands.Compute/Generated/Snapshot/SnapshotGrantAccessMethod.cs

            Position = 3,
-            Mandatory = false)]
-        [AllowNull]
+            Mandatory = true)]


Same comment about breaking changes.

resolved offline.

maddieclayton · 2018-03-06T19:37:42Z

...ourceManager/Compute/Commands.Compute/help/Export-AzureRmLogAnalyticRequestRateByInterval.md

+## SYNTAX
+
+```
+Export-AzureRmLogAnalyticRequestRateByInterval [-FromTime] <DateTime> [-GroupByOperationName]


Regenerate this once you reorder the parameters in cs file so that this syntax makes sense.

I think the help generator has a bug. The parameters have positional values except switch parameters, but the help generator does not generate Syntax correctly according to the positional values.

We don't own the help generator, so if you would like to file a bug you can do so here: https://github.com/PowerShell/platyPS. For now, the best way to solve this issue is order the parameters in the cs file in the order you want them shown in the syntax.

maddieclayton · 2018-03-06T19:38:14Z

...ourceManager/Compute/Commands.Compute/help/Export-AzureRmLogAnalyticRequestRateByInterval.md

+```
+
+### -GroupByOperationName
+Group query result by  by Operation Name.


you say by twice

maddieclayton · 2018-03-06T19:38:52Z

src/ResourceManager/Compute/Commands.Compute/help/Export-AzureRmLogAnalyticThrottledRequests.md

+```
+
+### -GroupByOperationName
+Group query result by  by Operation Name.


Same comment here

maddieclayton · 2018-03-06T19:39:49Z

tools/StaticAnalysis/Exceptions/BreakingChangeIssues.csv

 "Microsoft.Azure.Commands.Compute.dll","Microsoft.Azure.Commands.Compute.Automation.UpdateAzureRmVmss","Update-AzureRmVmss","0","2020","The cmdlet 'Update-AzureRmVmss' no longer supports the type 'System.Nullable`1[System.Int32]' for parameter 'MaxUnhealthyInstancePercent'.","Change the type for parameter 'MaxUnhealthyInstancePercent' back to 'System.Nullable`1[System.Int32]'."
 "Microsoft.Azure.Commands.Compute.dll","Microsoft.Azure.Commands.Compute.Automation.UpdateAzureRmVmss","Update-AzureRmVmss","0","2020","The cmdlet 'Update-AzureRmVmss' no longer supports the type 'System.Nullable`1[System.Int32]' for parameter 'SkuCapacity'.","Change the type for parameter 'SkuCapacity' back to 'System.Nullable`1[System.Int32]'."
 "Microsoft.Azure.Commands.Compute.dll","Microsoft.Azure.Commands.Compute.Automation.UpdateAzureRmVmss","Update-AzureRmVmss","0","2020","The cmdlet 'Update-AzureRmVmss' no longer supports the type 'System.Nullable`1[Microsoft.Azure.Management.Compute.Models.CachingTypes]' for parameter 'OsDiskCaching'.","Change the type for parameter 'OsDiskCaching' back to 'System.Nullable`1[Microsoft.Azure.Management.Compute.Models.CachingTypes]'."
+"Microsoft.Azure.Commands.Compute.dll","Microsoft.Azure.Commands.Compute.Automation.GrantAzureRmDiskAccess","Grant-AzureRmDiskAccess","0","2030","The parameter 'Access' is no longer optional for the parameter set 'DefaultParameter' for cmdlet 'Grant-AzureRmDiskAccess'.","Make 'Access' optional for the parameter set 'DefaultParameter'."


Remove these two lines, we are not accepting breaking changes in non preview modules until May.

This is actually a bug fix. Access should be a mandatory in CRP API. not giving this value in Powershell results in a bad request.

Approved offline.

…nto loganalytics # Conflicts: # src/ResourceManager/Compute/ChangeLog.md

maddieclayton · 2018-03-07T22:35:30Z

src/ResourceManager/Compute/Commands.Compute.Test/ScenarioTests/LogAnalyticTests.ps1

If you set the "preferredLocation" parameter in Get-Location, it will be set to that location if it is available in the service.

maddieclayton · 2018-03-07T22:36:41Z

src/ResourceManager/Compute/Commands.Compute.Test/ScenarioTests/LogAnalyticTests.ps1

please use getassetname for all resources.

@maddieclayton Get-ComputeTestResourceName uses getassetname.

maddieclayton · 2018-03-07T22:37:22Z

src/ResourceManager/Compute/Commands.Compute.Test/ScenarioTests/LogAnalyticTests.ps1

I thought we determined that you are able to run the storage cmdlets here. Why are these lines still commented out?

@maddieclayton the test framework does not work with data plane cmdlets in playback mode. If I uncomment these lines, this test only works in record mode test.

maddieclayton · 2018-03-07T22:38:22Z

src/ResourceManager/Compute/Commands.Compute.Test/ScenarioTests/LogAnalyticTests.ps1

+        $result = Export-AzureRmLogAnalyticThrottledRequests -Location $loc -FromTime $from -ToTime $to -BlobContainerSasUri $sasuri -GroupByThrottlePolicy -GroupByOperationName -GroupByResourceName;
+        Assert-AreEqual "Succeeded" $result.Status;
+        $output = $result | Out-String;
+        Assert-True { $output.Contains(".csv"); }


Could we check the blob to ensure the file is there?

maddieclayton · 2018-03-07T22:39:58Z

src/ResourceManager/Compute/Commands.Compute/Generated/Disk/DiskGrantAccessMethod.cs

            Position = 3,
-            Mandatory = false)]
-        [AllowNull]
+            Mandatory = true)]


resolved offline - cmdlet fails without the parameter specified.

maddieclayton · 2018-03-07T22:42:06Z

...mpute/Commands.Compute/Generated/LogAnalytic/LogAnalyticExportRequestRateByIntervalMethod.cs

+            Mandatory = true,
+            ValueFromPipelineByPropertyName = true,
+            ValueFromPipeline = false)]
+        [AllowNull]


maddieclayton · 2018-03-07T22:42:16Z

...r/Compute/Commands.Compute/Generated/LogAnalytic/LogAnalyticExportThrottledRequestsMethod.cs

+            });
+        }
+
+


maddieclayton · 2018-03-07T22:42:39Z

src/ResourceManager/Compute/Commands.Compute/Generated/Snapshot/SnapshotGrantAccessMethod.cs

            Position = 3,
-            Mandatory = false)]
-        [AllowNull]
+            Mandatory = true)]


resolved offline.

maddieclayton · 2018-03-07T22:44:40Z

...ourceManager/Compute/Commands.Compute/help/Export-AzureRmLogAnalyticRequestRateByInterval.md

+## SYNTAX
+
+```
+Export-AzureRmLogAnalyticRequestRateByInterval [-FromTime] <DateTime> [-GroupByOperationName]


We don't own the help generator, so if you would like to file a bug you can do so here: https://github.com/PowerShell/platyPS. For now, the best way to solve this issue is order the parameters in the cs file in the order you want them shown in the syntax.

maddieclayton · 2018-03-07T22:45:22Z

tools/StaticAnalysis/Exceptions/BreakingChangeIssues.csv

 "Microsoft.Azure.Commands.Compute.dll","Microsoft.Azure.Commands.Compute.Automation.UpdateAzureRmVmss","Update-AzureRmVmss","0","2020","The cmdlet 'Update-AzureRmVmss' no longer supports the type 'System.Nullable`1[System.Int32]' for parameter 'MaxUnhealthyInstancePercent'.","Change the type for parameter 'MaxUnhealthyInstancePercent' back to 'System.Nullable`1[System.Int32]'."
 "Microsoft.Azure.Commands.Compute.dll","Microsoft.Azure.Commands.Compute.Automation.UpdateAzureRmVmss","Update-AzureRmVmss","0","2020","The cmdlet 'Update-AzureRmVmss' no longer supports the type 'System.Nullable`1[System.Int32]' for parameter 'SkuCapacity'.","Change the type for parameter 'SkuCapacity' back to 'System.Nullable`1[System.Int32]'."
 "Microsoft.Azure.Commands.Compute.dll","Microsoft.Azure.Commands.Compute.Automation.UpdateAzureRmVmss","Update-AzureRmVmss","0","2020","The cmdlet 'Update-AzureRmVmss' no longer supports the type 'System.Nullable`1[Microsoft.Azure.Management.Compute.Models.CachingTypes]' for parameter 'OsDiskCaching'.","Change the type for parameter 'OsDiskCaching' back to 'System.Nullable`1[Microsoft.Azure.Management.Compute.Models.CachingTypes]'."
+"Microsoft.Azure.Commands.Compute.dll","Microsoft.Azure.Commands.Compute.Automation.GrantAzureRmDiskAccess","Grant-AzureRmDiskAccess","0","2030","The parameter 'Access' is no longer optional for the parameter set 'DefaultParameter' for cmdlet 'Grant-AzureRmDiskAccess'.","Make 'Access' optional for the parameter set 'DefaultParameter'."


Approved offline.

maddieclayton · 2018-03-08T22:13:17Z

on demand here: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-demand/578/ (and 577)

maddieclayton · 2018-03-08T23:26:30Z

failing on demand test is now skipped, okay to merge.

hyonholee added 2 commits March 1, 2018 18:15

Merge branch 'accelerator' of https://github.com/hyonholee/azure-powe…

a11a9b3

…rshell into loganalytics

Merge branch 'accelerator' of https://github.com/hyonholee/azure-powe…

db43c15

…rshell into loganalytics

hyonholee force-pushed the loganalytics branch from fcaacbe to 5be64f9 Compare March 2, 2018 23:54

azuresdkci added the needs-review label Mar 3, 2018

maddieclayton self-assigned this Mar 3, 2018

maddieclayton suggested changes Mar 3, 2018

View reviewed changes

maddieclayton assigned hyonholee and unassigned maddieclayton Mar 3, 2018

maddieclayton added needs-revision and removed needs-review labels Mar 3, 2018

hyonholee added 2 commits March 2, 2018 17:47

Add log analytic cmdlets and fix some minor bugs.

b0a0d51

Merge branch 'preview' of https://github.com/Azure/azure-powershell i…

17ed73f

…nto loganalytics # Conflicts: # src/ResourceManager/Compute/ChangeLog.md

hyonholee force-pushed the loganalytics branch from 5be64f9 to 17ed73f Compare March 3, 2018 01:53

maddieclayton assigned maddieclayton and unassigned hyonholee Mar 5, 2018

maddieclayton added needs-review and removed needs-revision labels Mar 5, 2018

maddieclayton suggested changes Mar 6, 2018

View reviewed changes

maddieclayton assigned hyonholee and unassigned maddieclayton Mar 6, 2018

maddieclayton added needs-revision and removed needs-review labels Mar 6, 2018

Merge branch 'preview' of https://github.com/Azure/azure-powershell i…

32fae96

…nto loganalytics # Conflicts: # src/ResourceManager/Compute/ChangeLog.md

hyonholee force-pushed the loganalytics branch 2 times, most recently from 99608dc to 90b5c52 Compare March 7, 2018 04:02

maddieclayton assigned maddieclayton and unassigned hyonholee Mar 7, 2018

maddieclayton added needs-review and removed needs-revision labels Mar 7, 2018

maddieclayton suggested changes Mar 7, 2018

View reviewed changes

maddieclayton assigned hyonholee and unassigned maddieclayton Mar 7, 2018

maddieclayton added needs-revision and removed needs-review labels Mar 7, 2018

hyonholee force-pushed the loganalytics branch 3 times, most recently from 3890923 to 7b500c8 Compare March 8, 2018 21:50

Update LogAnalyticTests

a247ecc

hyonholee force-pushed the loganalytics branch from 7b500c8 to a247ecc Compare March 8, 2018 22:01

maddieclayton added needs-review Waiting for On Demand and removed needs-revision needs-review labels Mar 8, 2018

maddieclayton assigned maddieclayton and unassigned hyonholee Mar 8, 2018

maddieclayton approved these changes Mar 8, 2018

View reviewed changes

maddieclayton merged commit afcbd6f into Azure:preview Mar 9, 2018

hyonholee deleted the loganalytics branch March 9, 2018 19:18

[Compute] Add cmdlets for CRP Log Analytics. #5665

[Compute] Add cmdlets for CRP Log Analytics. #5665

Uh oh!

Conversation

hyonholee commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

maddieclayton left a comment

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!

maddieclayton Mar 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

hyonholee commented Mar 2, 2018 •

edited

Loading

maddieclayton Mar 3, 2018 •

edited

Loading