Adding support for creating HDInsight clusters with Datalake storage as default filesystem #3224

srsiva · 2016-11-24T10:28:47Z

Description

Adding support for creating HDInsight clusters with Datalake storage as default filesystem

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

Pull request includes test coverage for the included changes.
PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThrough parameter.

Cmdlet Parameter Guidelines

Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

azurecla · 2016-11-24T10:29:01Z

Hi @srsiva, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

It looks like you're working at Microsoft (sisankar). If you're full-time, we DON'T require a contribution license agreement.

If you are a vendor, DO please sign the electronic contribution license agreement. It will take 2 minutes and there's no faxing! https://cla.azure.com.

TTYL, AZPRBOT;

azuresdkci · 2016-11-24T10:30:22Z

Can one of the admins verify this patch?

shahabhijeet · 2016-11-28T19:48:14Z

@srsiva add to whitelist

…tial scan

shahabhijeet · 2016-11-30T22:39:22Z

...esourceManager/HDInsight/Commands.HDInsight.Test/ScenarioTests/DataLakeStoreScenarioTests.cs

@@ -10,11 +10,18 @@ public DataLakeStoreScenarioTests(Xunit.Abstractions.ITestOutputHelper output)
            ServiceManagemenet.Common.Models.XunitTracingInterceptor.AddToContext(new ServiceManagemenet.Common.Models.XunitTracingInterceptor(output));
        }

-        [Fact]
+        [Fact (Skip = "Test requires fixing GraphAPI by upgrading to latest test framework")]


@srsiva could clarify why this test is being skipped. Is it test framework issue or GraphApi issue.
Also make sure you put comment on the test itself as to why is this being skipped and what is the plan to enable it back again.
But at this time, it is not clear why is this being skipped.

@srsiva This is not a sufficient reason to skip this test. Please enable and re-record as necessary.

@markcowl , @shahabhijeet .

I had merged from Azure:dev.
The test framework has been upgraded to Microsoft.Azure.Test.Framework.1.0.6052.28118-prerelease and Graph SDK has been updated to Microsoft.Azure.Graph.RBAC.3.2.0-preview.

However, I am having trouble with getting these upgrades to work with the code.
When I remove the "skip", the NewAzureRmHDInsightCluster cmdlet calls GetApplicationId, which in turn eventually calls MockClientFactory.CreateCustomArmClient.
This function tries to find a management client that it can use (TClient client = ManagementClients.FirstOrDefault(o => o is TClient) as TClient;), fails and then tries to create a real-client by reflecting and calling the constructor.
The problem is that it is not able to create a GraphRbacManagementClient because GraphRbacManagementClient does not have the expected constructor.

I then tried to setup the management client such that MockClientFactory.CreateCustomArmClient will find a client object that it can use directly (and avoid trying to create a real-client via reflection).

I added the code below to HDInsightScenarioTestsBase.cs and expected this to work.
However TestBase.GetGraphServiceClient is throwing a null reference exception...I am not sure why. I am not able to find the code for the TestBase class.
protected void SetupManagementClients()
{
var hdinsightManagementClient = GetHdInsightManagementClient();
var graphRbacManagementClient = TestBase.GetGraphServiceClient(this.csmTestFactory);

helper.SetupSomeOfManagementClients(hdinsightManagementClient, graphRbacManagementClient); }

Can you help me out with getting this to work ?

Thanks
Siva

The solution is to update the version of graph rbac manageemnt client you are using. You cna find examples of constructiung the new client here: http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-demand/1313/

You should also be calling SetupManagementClients instead of SetupSomeOfManagementClients, this will make the test fail fast before runtime if client conbstruction fails.
Please ping us, or att

@markcowl I had upgraded the graph rbac management client to 3.2.0-preview. It still didn't work.
I dug around a bit more and I figured that the new Graph Rbac mgmt client is auto-rest based...and HDInsight was trying to instantiate it using TestBase class from Microsoft.Azure.Test as opposed to Microsoft.Rest.ClientRuntime.Azure.TestFramework.

I tried to make the changes to instantiate HDInsightManagementClient by using Microsoft.Azure.Test ; and GraphRbacManagementClient by Microsoft.Rest.ClientRuntime.Azure.TestFramework....but I ran into some issues.

The call create a graphrbacmgmtclient -- context.GetGraphServiceClient(environment); requires an argument of type TestEnvironment.
The call to create an instance of TestEnvironment -- TestEnvironmentFactory.GetTestEnvironment(); -- fails with an exception "System.InvalidOperationException -- The current SynchronizationContext may not be used as a TaskScheduler".

I am not yet sure why I am running into this issue.

On second thought, I think I can just remove the tests that are being skipped currently.

The New-AzureRmHDInsightCluster cmdlet follows the same code path for WASB and ADLS. The only difference between the tests is that the ADL-tests are converting ObjectId into ApplicationId by using GraphAPI.

After converting ObjectId into ApplicationId, the value is included in ClusterCreateParameters and then the SDK takes over by using the clusterCreateParameters to create the cluster.
The functionality that uses ClusterCreateParameters to create the cluster, is already being tested in SDK.

@markcowl, @shahabhijeet. I have removed the datalake scenario tests, based on the comments above.

shahabhijeet · 2016-11-30T22:40:32Z

...esourceManager/HDInsight/Commands.HDInsight.Test/ScenarioTests/DataLakeStoreScenarioTests.cs

        [Trait(Category.AcceptanceType, Category.CheckIn)]
        public void TestDataLakeStoreClusterCreate()
        {
            NewInstance.RunPsTest("Test-DataLakeStoreClusterCreate");
        }
+
+        [Fact (Skip = "Test requires fixing GraphAPI by upgrading to latest test framework")]


@srsiva please be specific the reasons to skip any tests.

Same comment. Please fix the test and submit

I have removed the datalake scenario tests

shefaliv · 2016-12-02T07:04:22Z

src/ResourceManager/HDInsight/Commands.HDInsight.Test/Commands.HDInsight.Test.csproj

-    <Reference Include="Microsoft.Azure.Management.HDInsight, Version=1.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
-      <SpecificVersion>False</SpecificVersion>
-      <HintPath>..\..\..\packages\Microsoft.Azure.Management.HDInsight.1.3.0\lib\net40\Microsoft.Azure.Management.HDInsight.dll</HintPath>
+    <Reference Include="Microsoft.Azure.Management.HDInsight">


how come the public key token and all are gone?

shahabhijeet · 2016-12-02T07:11:19Z

@shefaliv seems you have approved changes. Could you confirm if you are fine with skipping tests? If yes, I am still not clear the reasons for skipping tests.

markcowl · 2016-12-02T07:52:20Z

@srsiva @shefaliv If these tests are not useful, you should remove them. If they are useful, you should fix them. You should not skip them.

…adlcontainers

markcowl · 2016-12-07T22:46:59Z

@shefaliv Are you ok with this test coverage being removed?

markcowl · 2016-12-07T22:52:29Z

on demand run here: http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-demand/1334/

shefaliv · 2016-12-08T08:58:56Z

Approving the test removal as it is only testing the GraphAPI, which has its own tests by the owning team. Thanks!

srsiva added 6 commits November 18, 2016 15:55

Added support for creating ADL-defaultFS clusters

e7e4880

Merged from Azure:dev

d7788b1

Fix obsoleted field references in tests

a87abac

Minor fixes

c9bbf51

Minor bugfix in New-clusterConfig command

86fd604

Minor bugfix in SetAzureHDInsightDefaultStorageCommand

f32fba7

azurecla added the cla-not-required label Nov 24, 2016

Removed test.pfx that was causing build failures on account of creden…

5fd0bc3

…tial scan

shahabhijeet reviewed Nov 30, 2016

View reviewed changes

shefaliv approved these changes Dec 2, 2016

View reviewed changes

cormacpayne added the Changes Requested label Dec 5, 2016

srsiva added 3 commits December 6, 2016 16:03

Merge branch 'dev' of https://github.com/Azure/azure-powershell into …

f5701f6

…adlcontainers

Removed Datalake scenario tests

c8fba58

Fixed the reference to Microsoft.Azure.Management.HDInsight

b600d27

cormacpayne added Sign Off Required and removed Changes Requested labels Dec 7, 2016

markcowl added Waiting for On Demand Changes Requested and removed Sign Off Required labels Dec 7, 2016

cormacpayne removed the Waiting for On Demand label Dec 8, 2016

cormacpayne merged commit fc5f98c into Azure:dev Dec 8, 2016

Adding support for creating HDInsight clusters with Datalake storage as default filesystem #3224

Adding support for creating HDInsight clusters with Datalake storage as default filesystem #3224

Uh oh!

Conversation

srsiva commented Nov 24, 2016

Description

Uh oh!

azurecla commented Nov 24, 2016

Uh oh!

azuresdkci commented Nov 24, 2016

Uh oh!

shahabhijeet commented Nov 28, 2016

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!

shahabhijeet commented Dec 2, 2016

Uh oh!

markcowl commented Dec 2, 2016

Uh oh!

markcowl commented Dec 7, 2016

Uh oh!

markcowl commented Dec 7, 2016

Uh oh!

shefaliv commented Dec 8, 2016

Uh oh!

Uh oh!