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

Edge node support for HDInsight #4550

Merged
merged 23 commits into from
Nov 12, 2019

Conversation

dintskirveli
Copy link
Contributor

@dintskirveli dintskirveli commented Oct 7, 2019

Hi,

This PR is based on @mbfrahry's work in this closed PR, which appears to have been blocked waiting on a Go SDK change.

I used his branch as a base, made some tweaks, fixed some bugs, and did some testing -- seems to work fine.

Can we revisit this now?

@mbfrahry
Copy link
Member

mbfrahry commented Oct 8, 2019

Hi @dintskirveli. Thanks for fixing this up a bit but the tests still fail because of a broken pipe. Right after creating the edge node, the pipe breaks and we can't read any of the edge node information unless we go into a completely different run which is why this works manually; however, we don't feel comfortable that there aren't unintended effects with that broken pipe. With that said the fix did just get merged into go-autorest so we'll have to wait for that to release and see if it fixes our issue here.

@dintskirveli
Copy link
Contributor Author

@mbfrahry, thanks for getting back.

I think my changes are valuable in that they allow multiple edge nodes to be created, and that's a requirement for us. Can you elaborate on why this is an issue in tests specifically? I'm still trying to get the tests working on our end, so bear with me.

Also -- I had some issues updating to the latest master, which I think has to do with the new plugin SDK. Do you know how to fix that?

❯ terraform apply

Error: Failed to instantiate provider "azurerm" to obtain schema: Unrecognized remote plugin message:

This usually means that the plugin is either invalid or simply
needs to be recompiled to support the latest protocol.

@mbfrahry
Copy link
Member

mbfrahry commented Oct 8, 2019

I agree that your changes are valuable and I'm happy to take those going forward.

As soon as the edge node finishes creating, we get a broken pipe that causes all future read requests to return as empty until we create a new pipe. This is why you're seeing a success when you create manually because a subsequent Terraform run creates a new pipe but if this is running continually, we'll break users. This is the error I'm seeing when I run the tests:

        Error: Error reading edge node for HDInsight Hadoop Cluster "acctesthdi-191008102508284611" (Resource Group "acctestRG-191008102508284611"): hdinsight.ApplicationsClient#Get: Failure sending request: StatusCode=0 -- Original Error: Get https://management.azure.com/subscriptions/XXXX-XXX-XXXX/resourceGroups/acctestRG-191008102508284611/providers/Microsoft.HDInsight/clusters/acctesthdi-191008102508284611/applications/acctesthdi-191008102508284611?api-version=2018-06-01-preview: stream error: stream ID 5; INTERNAL_ERROR

In regards to your issue, I'm not entirely sure why that would be the case without knowing your environment. I'd suggest opening an issue so we can better track this.

@dintskirveli
Copy link
Contributor Author

dintskirveli commented Oct 14, 2019

@mbfrahry, I fixed the above issue -- turned out be a schema problem caused by the plugin SDK migration and a bad merge. I also updated the branch to the latest in master.

Is this the go SDK update you're referring to? #4609

If it's merged, do you mind testing the tests?

@ghost ghost removed the waiting-response label Oct 14, 2019
@dintskirveli
Copy link
Contributor Author

@mbfrahry @katbyte any updates here?

@katbyte
Copy link
Collaborator

katbyte commented Oct 22, 2019

@dintskirveli,

We need to update go-autorest to give it a try. We are waiting for a new release with some more fixes before we upgrade & merge it through the dep chain. Not this week but hopefully in the upcoming weeks we should be able to come back to this.

@katbyte
Copy link
Collaborator

katbyte commented Oct 28, 2019

waiting on the new version of autorest: Azure/go-autorest#479

@tombuildsstuff
Copy link
Contributor

Waiting on #4775

@katbyte
Copy link
Collaborator

katbyte commented Nov 1, 2019

this should be unblocked now @dintskirveli, @mbfrahry

@dintskirveli
Copy link
Contributor Author

@mbfrahry did you have any tests that you were trying? Didn't see any in your original PR

@mbfrahry
Copy link
Member

mbfrahry commented Nov 4, 2019

Hey @dintskirveli, those tests are in this PR under azurerm/resource_arm_hdinsight_hadoop_cluster_test.go. They're labeled TestAccAzureRMHDInsightHadoopCluster_edgeNodeBasic and TestAccAzureRMHDInsightHadoopCluster_addEdgeNodeBasic

@dintskirveli
Copy link
Contributor Author

@mbfrahry yep, found 'em. Thanks.

Good news everyone!

Both tests work fine against the latest master.

❯ make testacc TESTARGS='-run=TestAccAzureRMHDInsightHadoopCluster_edgeNodeBasic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor'|grep -v 'examples') -v -run=TestAccAzureRMHDInsightHadoopCluster_edgeNodeBasic -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
?       github.com/terraform-providers/terraform-provider-azurerm    [no test files]
=== RUN   TestAccAzureRMHDInsightHadoopCluster_edgeNodeBasic
=== PAUSE TestAccAzureRMHDInsightHadoopCluster_edgeNodeBasic
=== CONT  TestAccAzureRMHDInsightHadoopCluster_edgeNodeBasic
--- PASS: TestAccAzureRMHDInsightHadoopCluster_edgeNodeBasic (1571.83s)
PASS
❯ make testacc TESTARGS='-run=TestAccAzureRMHDInsightHadoopCluster_addEdgeNodeBasic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor'|grep -v 'examples') -v -run=TestAccAzureRMHDInsightHadoopCluster_addEdgeNodeBasic -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
?       github.com/terraform-providers/terraform-provider-azurerm    [no test files]
=== RUN   TestAccAzureRMHDInsightHadoopCluster_addEdgeNodeBasic
=== PAUSE TestAccAzureRMHDInsightHadoopCluster_addEdgeNodeBasic
=== CONT  TestAccAzureRMHDInsightHadoopCluster_addEdgeNodeBasic
--- PASS: TestAccAzureRMHDInsightHadoopCluster_addEdgeNodeBasic (2987.54s)
PASS

@dintskirveli
Copy link
Contributor Author

@mbfrahry @katbyte hi! can we review this?

@laktech

This comment has been minimized.

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @dintskirveli, I also see the tests working now but there was one new feature you added that needs to be tested before we can merge it in

azurerm/resource_arm_hdinsight_hadoop_cluster.go Outdated Show resolved Hide resolved
azurerm/common_hdinsight.go Outdated Show resolved Hide resolved
azurerm/common_hdinsight.go Outdated Show resolved Hide resolved
Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @dintskirveli. Unfortunately, we needed to move some of that logic from the removed CustomizeDiff into the destroy edgenode check logic. In it's current state, Terraform would permanently show a difference if a user tried to update vm_size or install_script_actions since they wouldn't actually get updated

azurerm/resource_arm_hdinsight_hadoop_cluster.go Outdated Show resolved Hide resolved
azurerm/resource_arm_hdinsight_hadoop_cluster.go Outdated Show resolved Hide resolved
azurerm/resource_arm_hdinsight_hadoop_cluster.go Outdated Show resolved Hide resolved
azurerm/common_hdinsight.go Show resolved Hide resolved
@dintskirveli
Copy link
Contributor Author

Hey @dintskirveli. Unfortunately, we needed to move some of that logic from the removed CustomizeDiff into the destroy edgenode check logic. In it's current state, Terraform would permanently show a difference if a user tried to update vm_size or install_script_actions since they wouldn't actually get updated

@mbfrahry not sure what you're referring to here. Yes, they would permanently show a difference and would get resolved by deleting and re-adding the application. The modification of the cluster works.

@mbfrahry
Copy link
Member

You are 100% right. I misread that chunk of code. I believe these changes are in a great spot and ready to merge. Thanks so much for your patience here @dintskirveli.

@mbfrahry mbfrahry merged commit e0a5de8 into hashicorp:master Nov 12, 2019
@katbyte katbyte added this to the v1.37.0 milestone Nov 17, 2019
@ghost
Copy link

ghost commented Nov 26, 2019

This has been released in version 1.37.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.37.0"
}
# ... other configuration ...

@rk-tbrigley
Copy link

@mbfrahry @dintskirveli Any chance this same edgenode support could be added to the other HDI resources as well?

@ghost
Copy link

ghost commented Feb 6, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Feb 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants