Skip to content

azuredevops_team - Fix create/update never reaching Synched when … - #1599

Open
Maikel Dollé (itmagix) wants to merge 1 commit into
microsoft:mainfrom
itmagix:fix/team-create-wait-loop
Open

azuredevops_team - Fix create/update never reaching Synched when …#1599
Maikel Dollé (itmagix) wants to merge 1 commit into
microsoft:mainfrom
itmagix:fix/team-create-wait-loop

Conversation

@itmagix

Copy link
Copy Markdown

All Submissions:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My code follows the code style of this project.
  • Have you checked to ensure there aren't other open PRs for the same update/change?

Description

While migrating a few hundred teams to a new organization I ran into #1582: team creation hangs for 30 minutes and then fails with timeout while waiting for state to become 'Synched' (last state: 'Waiting'), even though the team and its members were created fine in Azure DevOps. Nothing lands in state, so the next run or an import picks the teams up immediately.

The convergence loop in waitForTeamStateChange() has five conditions that all have to hold, and several of them can never become true under normal service behavior:

  • name and description are compared with exact string equality. The service trims surrounding whitespace when it stores them, so the comparison can never pass. A missing value in the response also dereferenced a nil pointer.
  • members and administrators are compared by count only. The service can seed its own ACEs on the team identity token, report extra or duplicate entries, or silently drop a membership, so the counts never match even when everything the configuration asked for is there.
  • the dashboard check reads if dashboards == nil && len(*dashboards) == 0, which dereferences a nil pointer. It could only ever crash or pass.
  • the 30 minute wait timeout was hardcoded and ignored the resource timeouts configuration.

Because the loop does not log which condition is stuck, every one of these quirks produces the same silent 30 minute timeout. The same symptom came up in #1464, where the actual cause turned out to be an unrelated regression in GetAccessControlList() (fixed by the revert in #1466).

This PR changes the loop to:

  • compare trimmed name and description values, and accept a missing value in the response instead of panicking or waiting forever
  • require that all configured members and administrators are present (set intersection, the same semantics the standalone azuredevops_team_members resource already uses) and log a warning when the counts differ
  • treat a nil or empty dashboard list as a warning instead of panicking
  • take the wait timeout from the resource timeouts configuration instead of the hardcoded 30 minutes
  • skip identities without a subject descriptor in getSubjectDescriptors()

No documentation changes are needed. There is no schema change, the documented timeouts block simply works now.

Does this introduce a breaking change?

  • Yes
  • No

Config compatible. Two behavior changes worth calling out: the default create/update wait is now the documented 10 minute resource default instead of a hardcoded 30 minutes, and the loop converges on set intersection instead of exact counts, logging a warning when the team state differs from the configuration.

Test Result

--- PASS: TestTeam_WaitForStateChange_Synced (15.01s)
--- PASS: TestTeam_WaitForStateChange_Converges_WhenServiceNormalizesNameAndDescription (15.01s)
--- PASS: TestTeam_WaitForStateChange_Converges_WhenResponseOmitsDescription (15.01s)
--- PASS: TestTeam_WaitForStateChange_Converges_WhenACLContainsExtraAdministratorACE (15.00s)
--- PASS: TestTeam_WaitForStateChange_Converges_WhenTeamHasExtraMembers (15.01s)
--- PASS: TestTeam_WaitForStateChange_DoesNotPanic_WhenDashboardsListIsNil (15.01s)
--- PASS: TestTeam_WaitForStateChange_TimesOut_WhenConfiguredMemberNeverAppears (12.01s)
ok  	github.com/microsoft/terraform-provider-azuredevops/azuredevops/internal/service/core	102.071s

go build, go vet and the full core package test suite pass. The patched provider has also been verified in a production pipeline while migrating teams between organizations: teams and members are created and the loop converges in seconds instead of hanging for 30 minutes.

Related Issue(s)

Addresses #1582. The same symptom came up in #1464, caused by an unrelated regression and fixed by the revert in PR #1466.

Other information

The triggers for the original hang are all realistic service side behaviors: normalized name and description (confirmed in the #1582 thread), extra ACEs on the team identity namespace, and memberships the identity service does not persist. Each one used to produce the same 30 minute timeout with no indication of which condition was stuck. The loop now logs the drifted condition, which should make reports like #1582 much easier to diagnose.

…the service normalizes values

Addresses microsoft#1582

The convergence loop in waitForTeamStateChange() could never report
`Synched` when Azure DevOps normalizes team name/description on storage
(e.g. trims surrounding whitespace), when the identity security namespace
of a new team contains additional ACEs, or when team membership contains
members not present in the configuration - the checks used exact string
equality and count-only comparisons. A nil dashboard list dereferenced a
nil pointer, and the 30 minute wait timeout was hardcoded, ignoring the
resource `timeouts` configuration.

- compare trimmed name/description values and tolerate omitted response
  fields instead of hanging or panicking
- converge on "all configured members/administrators are present" (set
  intersection, matching the standalone azuredevops_team_members
  resource) and log a warning when counts drift
- nil-guard the dashboard check (previously a potential provider panic)
- honor the resource `timeouts` create/update configuration instead of
  the hardcoded 30 minutes
- skip identities without a subject descriptor when converting members

Adds unit tests covering the convergence loop against normalized values,
omitted response fields, extra ACL ACEs, extra members, a nil dashboard
list, and a configured member that never appears.
@itmagix

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant