Remove forceNew for `zone` and `standby_availability_zone` of azurerm_postgresql_flexible_server #13507

neil-yechenwei · 2021-09-26T01:19:10Z

Symptom:
Currently, TF would recreate this resource when zone and standby_availability_zone is changed.

Expected behavior:
After confirmed with service team, TF shouldn't recreate this resource when zone and standby_availability_zone is changed, which means failover is triggered once zone and standby_availability_zone is changed and failover between zone and standby_availability_zone is only supported when their values are exchanged.

Failover is implemented by separate API not update API.

--- PASS: TestAccPostgresqlflexibleServer_basic (564.91s)
--- PASS: TestAccPostgresqlflexibleServer_requiresImport (598.20s)
--- PASS: TestAccPostgresqlflexibleServer_updateMaintenanceWindow (1090.05s)
--- PASS: TestAccPostgresqlflexibleServer_complete (1170.18s)
--- PASS: TestAccPostgresqlflexibleServer_updateSku (1490.18s)
--- PASS: TestAccPostgresqlflexibleServer_failover (1582.92s)
--- PASS: TestAccPostgresqlflexibleServer_pitr (1954.23s)
--- PASS: TestAccPostgresqlflexibleServer_completeUpdate (2148.29s)

internal/services/postgres/postgresql_flexible_server_resource.go

tombuildsstuff

hey @neil-yechenwei

Thanks for this PR.

I've taken a look through and left a few comments inline - if we can fix those up then we should be able to take another look.

Thanks!

tombuildsstuff · 2021-09-28T15:54:31Z

internal/services/postgres/postgresql_flexible_server_resource.go

+			"zone": {
+				Type:     pluginsdk.TypeString,
+				Optional: true,
+				Computed: true,


This shouldn't be Computed, since we should highlight the change when this occurs/isn't specified:

Suggested change

Computed: true,

Similarly, is there a reason this isn't Required? In other clouds it's the convention to have to specify a zone when creating resources

TF would cause diff when it isn't specified. Because service backend would set it with random zone when it isn't specified in request body. So suggest to keep "Computed: true".

Service team confirmed generally user doesn't care where zone is. Service backend would optimize according to the actual situation and the success rate of the creation would be higher. So it isn't required property.

@neil-yechenwei as above this needs to become un-computed here

@tombuildsstuff , as service team confirmed user doesn't care where zone is, so I think we shouldn't highlight the change when it isn't specified. Does it make sense?

@neil-yechenwei this is a core tenant of Terraform that we'll show you the drift - so this has to be uncomputed - I've opened #13843

tombuildsstuff · 2021-09-28T15:54:35Z

internal/services/postgres/postgresql_flexible_server_resource.go

+						"standby_availability_zone": {
+							Type:     pluginsdk.TypeString,
+							Optional: true,
+							Computed: true,


(same here)

TF would cause diff when it isn't specified. Because service backend would set it with random zone when it isn't specified in request body. So suggest to keep "Computed: true".

Service team confirmed generally user doesn't care where standby_availability_zone is. Service backend would optimize according to the actual situation and the success rate of the creation would be higher. So it isn't required property.

@neil-yechenwei as above this needs to become un-computed here

as service team confirmed user doesn't care where zone is, so I think we shouldn't highlight the change when it isn't specified. Does it make sense?

tombuildsstuff · 2021-10-12T17:57:13Z

internal/services/postgres/postgresql_flexible_server_resource.go

@@ -626,8 +692,11 @@ func expandFlexibleServerHighAvailability(inputs []interface{}) *postgresqlflexi
 		Mode: postgresqlflexibleservers.HighAvailabilityMode(input["mode"].(string)),
 	}

-	if v, ok := input["standby_availability_zone"]; ok && v.(string) != "" {
-		result.StandbyAvailabilityZone = utils.String(v.(string))
+	// service team confirmed it doesn't support to update `standby_availability_zone` after the PostgreSQL Flexible Server resource is created


so then how do users fail-back?

Service team confirmed we shouldn't use update operation to fail-back. fail back should be implemented by restart with failover. For instance, zone is set to 1 and standby_availability_zone is set to 2 in tf config. Then we can failover zone to 2 and standby_availability_zone to 1 using restart with failover api after creation.

@neil-yechenwei so in that scenario, how do users fail-back? e.g.

Provision with zone = 1 and standby_zone = 2

Failover so that zone = 2 and standby_zone = 1

Failback so that zone = 1 and standby_zone = 2

(we should also probably rename this field to standby_zone)

Users need to keep zone=1 and standby_availability_zone=2 in TF config and run tf apply to failback after automatically failover.

(standby_availability_zone stands for healthy zone not unhealthy zone. So suggest to keep this name.)

tombuildsstuff · 2021-10-12T17:59:03Z

website/docs/r/postgresql_flexible_server.html.markdown

@@ -91,7 +91,7 @@ The following arguments are supported:

 * `administrator_password` - (Optional) The Password associated with the `administrator_login` for the PostgreSQL Flexible Server. Required when `create_mode` is `Default`.

-* `zone` - (Optional) The availability zone of the PostgreSQL Flexible Server. Possible values are `1`, `2` and `3`. Changing this forces a new PostgreSQL Flexible Server to be created.
+* `zone` - (Optional) The availability zone of the PostgreSQL Flexible Server. Possible values are `1`, `2` and `3`.


The Availability Zones available change per-region - some support No Zones, some support 1 & 2, others support 1, 2 and 3. We should add a callout to explain this

tombuildsstuff · 2021-10-12T18:00:26Z

internal/services/postgres/postgresql_flexible_server_resource_test.go

+				check.That(data.ResourceName).ExistsInAzure(r),
+			),
+		},
+		data.ImportStep("administrator_password", "create_mode"),


can we also add a step to fail-back here too?

neil-yechenwei · 2021-10-13T07:00:11Z

Thanks @tombuildsstuff , I've updated code. Please have another look. Thanks in advance.

github-actions · 2021-10-21T23:05:28Z

This functionality has been released in v2.82.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@neil-yechenwei

…are no longer computed (#13843) es this comment #13507 (files) cc @neil-yechenwei

github-actions · 2021-11-22T02:06:32Z

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.

neil-yechenwei added 7 commits September 25, 2021 11:48

update code

ed8d646

update code

2b0775c

update code

0c6814a

update code

7197737

update code

6d6126b

update code

3779bda

update code

a3670b6

github-actions bot added documentation size/M labels Sep 26, 2021

neil-yechenwei added 3 commits September 26, 2021 10:11

Merge remote-tracking branch 'upstream/main' into fixpostgresqlfsha2

b47c5c6

update code

62f5bcf

update code

5efba83

mybayern1974 reviewed Sep 26, 2021

View reviewed changes

internal/services/postgres/postgresql_flexible_server_resource.go Outdated Show resolved Hide resolved

mybayern1974 reviewed Sep 26, 2021

View reviewed changes

internal/services/postgres/postgresql_flexible_server_resource.go Outdated Show resolved Hide resolved

mybayern1974 reviewed Sep 26, 2021

View reviewed changes

internal/services/postgres/postgresql_flexible_server_resource.go Outdated Show resolved Hide resolved

update code

351c327

neil-yechenwei mentioned this pull request Sep 27, 2021

Update the document for the usage of failover of azurerm_postgresql_flexible_server #13512

Closed

tombuildsstuff self-assigned this Sep 28, 2021

tombuildsstuff added the service/postgresql label Sep 28, 2021

tombuildsstuff suggested changes Oct 12, 2021

View reviewed changes

update code

bc1a669

github-actions bot added size/L and removed size/M labels Oct 13, 2021

neil-yechenwei requested a review from tombuildsstuff October 13, 2021 07:42

katbyte added this to the v2.82.0 milestone Oct 21, 2021

katbyte merged commit 6a3ec6c into hashicorp:main Oct 21, 2021

katbyte added a commit that referenced this pull request Oct 21, 2021

CHANGELOG.md for #13507

4a7b057

katbyte pushed a commit that referenced this pull request Nov 18, 2021

r/postgresql_flexible_server: zone and standby_availability_zone …

f2ec1c8

…are no longer computed (#13843) es this comment #13507 (files) cc @neil-yechenwei

github-actions bot locked as resolved and limited conversation to collaborators Nov 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove forceNew for `zone` and `standby_availability_zone` of azurerm_postgresql_flexible_server #13507

Remove forceNew for `zone` and `standby_availability_zone` of azurerm_postgresql_flexible_server #13507

neil-yechenwei commented Sep 26, 2021 •

edited

Loading

tombuildsstuff left a comment

tombuildsstuff Sep 28, 2021

neil-yechenwei Oct 13, 2021 •

edited

Loading

tombuildsstuff Oct 21, 2021

neil-yechenwei Oct 21, 2021 •

edited

Loading

tombuildsstuff Oct 22, 2021

tombuildsstuff Sep 28, 2021

neil-yechenwei Oct 13, 2021 •

edited

Loading

tombuildsstuff Oct 21, 2021

neil-yechenwei Oct 21, 2021 •

edited

Loading

tombuildsstuff Oct 12, 2021

neil-yechenwei Oct 13, 2021 •

edited

Loading

tombuildsstuff Oct 21, 2021

neil-yechenwei Oct 21, 2021 •

edited

Loading

tombuildsstuff Oct 12, 2021

neil-yechenwei Oct 13, 2021

tombuildsstuff Oct 12, 2021

neil-yechenwei Oct 13, 2021

neil-yechenwei commented Oct 13, 2021 •

edited

Loading

github-actions bot commented Oct 21, 2021

github-actions bot commented Nov 22, 2021

Remove forceNew for zone and standby_availability_zone of azurerm_postgresql_flexible_server #13507

Remove forceNew for zone and standby_availability_zone of azurerm_postgresql_flexible_server #13507

Conversation

neil-yechenwei commented Sep 26, 2021 • edited Loading

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei commented Oct 13, 2021 • edited Loading

github-actions bot commented Oct 21, 2021

github-actions bot commented Nov 22, 2021

Remove forceNew for `zone` and `standby_availability_zone` of azurerm_postgresql_flexible_server #13507

Remove forceNew for `zone` and `standby_availability_zone` of azurerm_postgresql_flexible_server #13507

neil-yechenwei commented Sep 26, 2021 •

edited

Loading

neil-yechenwei Oct 13, 2021 •

edited

Loading

neil-yechenwei Oct 21, 2021 •

edited

Loading

neil-yechenwei Oct 13, 2021 •

edited

Loading

neil-yechenwei Oct 21, 2021 •

edited

Loading

neil-yechenwei Oct 13, 2021 •

edited

Loading

neil-yechenwei Oct 21, 2021 •

edited

Loading

neil-yechenwei commented Oct 13, 2021 •

edited

Loading