Azure monitor support for azurerm_hdinsight_*_cluster

[kosinsky]

Adds support for Azure Monitor for HDInsight clusters:

• azurerm_hdinsight_hadoop_cluster
• azurerm_hdinsight_hbase_cluster
• azurerm_hdinsight_interactive_query_cluster
• azurerm_hdinsight_kafla_cluster
• azurerm_hdinsight_spark_cluster
• azurerm_hdinsight_storm_cluster

resource "azurerm_hdinsight_hadoop_cluster" "example" {
  name                = "example-hdicluster"
  resource_group_name = "${azurerm_resource_group.example.name}"
  location            = "${azurerm_resource_group.example.location}"
  cluster_version     = "3.6"
  tier                = "Standard"

  monitor {
    log_analytics_workspace_id = data.azurerm_log_analytics_workspace.example.workspace_id
    primary_key                = data.azurerm_log_analytics_workspace.example.primary_shared_key
  }

}

Fixes #6780

[ross-p-smith]

Hi @tombuildsstuff - is this likely to get in the next release?

[ross-p-smith]

@jackofallops - any chance we can get this one into 2.17.0 with the other HDInisght (external metastore) as it means I can have one testing cycle in my project? We have a lengthy approval process to take new provider versions. Would help me greatly. Ross

[jackofallops]

Hi @kosinsky
Thanks for the work done in this PR.

During the review below it became apparent that the monitor configuration doesn't fit the usual approach to the cluster API interaction in this service. Almost everything about this service requires a cluster rebuild to change, but the monitor config (as I understand it) it entirely configurable out of band of this? Each resource uses exactly the same code irrespective of cluster type, so this feels like it should actually be a separate resource that can be used against any cluster and managed independently of the cluster itself? WDYT?

I've left review comments as far as I got intact below, in particular it was the ForceNew on the monitor schema items that triggered my thinking above since the monitor API calls can only be made against an existing cluster resource.

[kosinsky]

During the review below it became apparent that the monitor configuration doesn't fit the usual approach to the cluster API interaction in this service. Almost everything about this service requires a cluster rebuild to change, but the monitor config (as I understand it) it entirely configurable out of band of this? Each resource uses exactly the same code irrespective of cluster type, so this feels like it should actually be a separate resource that can be used against any cluster and managed independently of the cluster itself? WDYT?

I'm thinking amount of code that is the same for all 8 resources for a while now. For example, most of the code is the same for accounts, metastores, gateway etc. Especially, it's possible that soon new resource(s) will be added as HDInsight adds cluster(s) based on Apache distribution of Hadoop instead of HDP. https://azure.microsoft.com/en-au/blog/azure-hdinsight-and-azure-database-for-postgresql-news/

For HDInsight to Azure monitor mapping it may make sense to use separate resource and the provider doing it for azurerm_monitor_diagnostic_setting in generic way and it works with HDI too. On other hand, some resources have additional Azure Monitor/Log Analytics option. I was looking on AKS and container group resources which work with Monitor as a part of their definition.

Also, Monitor isn't only one option that can be updated after cluster is ready. For example, now hdinsight resources support resizing and API supports configuring autoscaling without recreating cluster. It was planning to work on that after WIP is done. #6470. Code for autoscaling may look pretty close for the monitor.

I don't have strong preference for the property vs. new resource. My light preference is to keep monitor as a property, establish a pattern for updating a cluster, and be more aggressive in refactoring of the hdinsight_* resources to minimize code duplication.

Please, let me know WDYT. Meanwhile, I'll work on other feedback.

[jackofallops]

I don't have strong preference for the property vs. new resource. My light preference is to keep monitor as a property, establish a pattern for updating a cluster, and be more aggressive in refactoring of the hdinsight_* resources to minimize code duplication.

I think this will work either way, I tend to lean towards separation where possible for clarity, but that doesn't necessarily make it the right way to go. I'll review again in light of this.

Also, a quick note on refactoring these resources. We need to be mindful of state change, and of the size of change per PR so any significant rework will likely need to be rolled through one cluster type at a time.

I may have said it before, but thanks for your continued effort on this service, it's very much appreciated 😄

[jackofallops]

Hi @kosinsky
Thanks for this, a few additional minor changes but looks good otherwise.

Thanks!

[kosinsky]

working through the conflicts after merging 6969...

Ooh, I started to work on merge too. @jackofallops, I do the merge if it helps.

[jackofallops]

working through the conflicts after merging 6969...

Ooh, I started to work on merge too. @jackofallops, I do the merge if it helps.

I just finished, made a start as I was expecting the history to have been broken due to the fixes we had to do Monday. Turns out, not a problem for this one!

I'll let travis do its thing and then trigger the tests on CI.

Thanks @kosinsky 😃

Edit: Travis sorted and passed, running CI suite for this service now...

[jackofallops]

Test passing (transient failures aside...)

[ghost]

This has been released in version 2.18.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 = "~> 2.18.0"
}
# ... other configuration ...

[ghost]

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 👉 hashibot-feedback@hashicorp.com. Thanks!