server-side reordering on stateful_external_ip of google_compute_region_instance_group_manager

[edwardmedia]

Affected Resource(s)

• google_compute_region_instance_group_manager

=== CONT  TestAccRegionInstanceGroupManager_stateful
    provider_test.go:315: Step 1/6 error: After applying this test step, the plan was not empty.
        stdout:
        
        
        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place
        
        Terraform will perform the following actions:
        
          # google_compute_region_instance_group_manager.igm-basic will be updated in-place
          ~ resource "google_compute_region_instance_group_manager" "igm-basic" {
                id                               = "projects/ci-test-project-188019/regions/us-central1/instanceGroupManagers/tf-test-rigm-u86rw6989b"
                name                             = "tf-test-rigm-u86rw6989b"
                # (14 unchanged attributes hidden)
        
        
              ~ stateful_external_ip {
                  ~ interface_name = "nic1" -> "nic0"
                    # (1 unchanged attribute hidden)
                }
              ~ stateful_external_ip {
                  ~ interface_name = "nic0" -> "nic1"
                    # (1 unchanged attribute hidden)
                }
        
        
        
                # (4 unchanged blocks hidden)
            }
        
        Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccRegionInstanceGroupManager_stateful (191.75s)
FAIL
FAIL	github.com/hashicorp/terraform-provider-google-beta/google-beta	191.789s
FAIL

b/340204874

[rileykarson]

Depending on the rules of the API we may be able to fix this in a backwards compatible way by following the state setting protocol (#4328) and reordering the values as we're writing them in to state (during Read) to match the old state (and presumably the user's configuration).

If the values are returned in insertion order by the API- indicating this is an ordered list, not an unordered one- we will want to avoid doing so. However, given we're getting a diff, insertion order seems unlikely.

[askubis]

I have a fix for that in my pull request. I am happy to be the owner of this bug

[roaks3]

This one is causing a lot of noise in our PRs, and fails in our nightly tests. Since we have this bug to capture the failure, I think we should skip the test while we work out a solution. It looks to be a little more involved than simply changing the fields from List to Set.

[askubis]

I don't think it's more than that. Set is unordered, list is. based on the output from the failure it's a matter of ordering (nic0 became nic1 and vice-versa).
I also reran the test several times after and before this change.

I don't mind skipping it though.

[edwardmedia]

#13735

[melinath]

@askubis are you still planning to work on this issue?

[askubis]

yes, I the PR is ready, GoogleCloudPlatform/magic-modules#7205
but it's a breaking change ( I want to change a List to a Set), and thus I have to wait for a major release window

[melinath]

@askubis We just shipped the 5.0 major release in September - the next one will be in about a year. As described in #13430 (comment) it may be possible to fix this in a backwards-compatible way as well (without needing to wait for the next major release)

[askubis]

Yes, I'm aware I've missed it this time :(
I'm happy to see someone else fix this if there's a volunteer.
P.S. Do you have an example PR that would show how such server-side reordering can be implemented?

[melinath]

The way to implement this would be in the Read function by adding code to rewrite the field's value before setting it in state - if it's possible, using the field's flattener for this would be preferred. I did a quick survey but wasn't able to find an example PR.

[SarahFrench]

Would it be possible for me to pick up the short-term fix where we manipulate the stateful_external_ip ordering in the Read function? I'm sorry to potentially remove your first contribution @askubis but a customer has escalated this issue, meaning that we'd like a fix put in place as soon as possible.

We could close this issue with that short-term fix, and open a new issue that describes swapping to the correct fix of swapping to a Set, and link your PR to that issue

[askubis]

Sure, I don't mind someone else doing that. please assign to yourself @SarahFrench
And sorry for missing the major release window, that would have fixed it :(

[SarahFrench]

No worries at all- this issue not being flagged for the v5.0.0 major release was an oversight on our part not yours! 😅

This GH issue has been marked as part of the Future Major Release milestone now so it'll be on our radar when we plan for v6.0.0, and we can get your PR merged then.

When I make a PR for the short-term fix I'll make sure it doesn't close this issue prematurely.

[askubis]

Great, thank you very much! :)

[SarahFrench]

My PR above is merged, but the work in there should be pulled out and replaced with changing the stateful_external_ip and stateful_internal_ip fields to Sets in the next major release

[SarahFrench]

I'm going to unassign myself, as this issue now represents changing this field to a Set. The immediate problem was addressed by adding logic to reorder items in the List to address diffs, but that logic should be removed and replaced with Sets.