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

New Resource azurerm_app_configuration_replica #21689

Closed
wants to merge 5 commits into from

Conversation

teowa
Copy link
Contributor

@teowa teowa commented May 8, 2023

resolves #21634
references:

test:

TF_ACC=1 go test -v ./internal/services/appconfiguration -parallel 20 -test.run=TestAccAppConfigurationReplica -timeout 1440m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAppConfigurationReplica_basic
=== PAUSE TestAccAppConfigurationReplica_basic
=== RUN   TestAccAppConfigurationReplica_requiresImport
=== PAUSE TestAccAppConfigurationReplica_requiresImport
=== CONT  TestAccAppConfigurationReplica_basic
=== CONT  TestAccAppConfigurationReplica_requiresImport
--- PASS: TestAccAppConfigurationReplica_requiresImport (234.21s)
--- PASS: TestAccAppConfigurationReplica_basic (273.35s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/appconfiguration      273.389s

}
}

func (r ReplicaResource) Create() sdk.ResourceFunc {
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than being a separate resource, this likely should be embedded within the App Configuration resource as a replica block, in the same way we've done for CosmosDB, since when making changes to the primary it can be necessary to make changes to the replicas too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The replica resource has its own API endpoint to create/delete, and it only rely on the resource ID of App Configuration resource, so I think it's better we create a new resource for this? If we put this inline, it might have issue when App Configuration is successfully created but the Replica fail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@teowa, it is a 1:1 mapping, so unless there is a reason it needs to be separate, ie permissions / policy need updating with the created resource id before replication, it should be within the parent resource. also, as tom mentioned it simplifies the situations when the primary/secondary needs to be updated before/after a change

Copy link
Contributor Author

@teowa teowa May 22, 2023

Choose a reason for hiding this comment

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

This should be 1:n mapping, the API supports creating 1 replica per azure location, so there may be multiple replicas under one app configuration.
image

Copy link
Contributor Author

@teowa teowa May 22, 2023

Choose a reason for hiding this comment

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

If we make the replica a inline block inside the azurerm_app_configuration, but in create/update if the replica fails to create but its parent resource is successfully created, we still need to write parent resource to state? if we don't write the parent resource into state, there will be tf.ImportAsExistsError error. So how can we write partial state under current design(we call read function to write the state only if the whole create/update successfully)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

in create/update if the replica fails to create but its parent resource is successfully created, we still need to write parent resource to state

we would fail creation then.

this is much like the cosmosdb resource with its replication locations property. As tom said this shouljd be part of the parent resource we should mimic the way cosmos db does it?

@@ -49,73 +49,6 @@ func (c Client) LinkWorkaroundDataPlaneClientWithEndpoint(configurationStoreEndp
return &workaroundClient, nil
}

func (c Client) DataPlaneClient(ctx context.Context, configurationStoreId string) (*appconfiguration.BaseClient, error) {
Copy link
Contributor Author

@teowa teowa May 8, 2023

Choose a reason for hiding this comment

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

This is to remove unused code (superseded by

func (c Client) DataPlaneClientWithEndpoint(configurationStoreEndpoint string) (*appconfiguration.BaseClient, error) {
), related to PR #20082

@djsly
Copy link
Contributor

djsly commented May 17, 2023

Looking forward to have that merged and available in this' week release !

@teowa
Copy link
Contributor Author

teowa commented May 22, 2023

Met some issue when I try to provision 2 Replicas under 1 App Configuration, the error message is obscure:

        Error: creating Replica (Subscription: "xxx"
        Resource Group Name: "acctestRG-appconfig-replica-230522111231672038"
        Configuration Store Name: "testaccappconf230522111231672038"
        Replica Name: "replica1"): polling after Create: internal-error: a polling status of `Failed` should be surfaced as a PollingFailedError
        
          with azurerm_app_configuration_replica.test,
          on terraform_plugin_test.tf line 39, in resource "azurerm_app_configuration_replica" "test":
          39: resource "azurerm_app_configuration_replica" "test" {
        
        creating Replica (Subscription: "xxx"
        Resource Group Name: "acctestRG-appconfig-replica-230522111231672038"
        Configuration Store Name: "testaccappconf230522111231672038"
        Replica Name: "replica1"): polling after Create: internal-error: a polling
        status of `Failed` should be surfaced as a PollingFailedError

Seems the SDK has some issue, still investigate this. The error message is related to #21870.

}

// concurrent creation of replicas under one configuration store will fail
locks.ByName(id.ConfigurationStoreName, r.ResourceType())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The API has limitation that we cannot create two replica under one Configuration Store at the same time, here a lock is added.

@teowa
Copy link
Contributor Author

teowa commented May 24, 2023

image

@teowa teowa force-pushed the app_conf_replica branch from 059f972 to 088f9a1 Compare June 5, 2023 05:01
@bjandhyala
Copy link

looking forward for this feature. thank you for your work on this.

@djsly
Copy link
Contributor

djsly commented Jun 19, 2023

@katbyte anything missing for this to be merged ?

@teowa
Copy link
Contributor Author

teowa commented Jul 11, 2023

I have submitted another PR to support the inline replica, please see #22452

@stephybun
Copy link
Member

Closing in favour of #22452

Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 23, 2024
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.

Support for Azure App Configuration - Geo Replication
6 participants