wait for ASO tags to converge before updating #4149

nojnhuh · 2023-10-17T16:39:05Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

From https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/4069/files#r1344633228:

I had noticed a race condition while running the tags e2e test where if the tags have not converged, then CAPZ could lose track of some tags that have been deleted from spec.additionalTags and treat them like tags that existed outside of spec.additionalTags and keep them around without ever deleting them. Waiting for the tags to converge on the ASO resource fixes that issue AFAICT.

Overall, this makes tags reconciliation work more similarly to the more synchronous method used before when calling the tags APIs directly.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

cherry-pick candidate

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Fixed a bug causing some tags on ASO resources not to be deleted

nojnhuh · 2023-10-17T16:48:37Z

/cherry-pick release-1.11

k8s-infra-cherrypick-robot · 2023-10-17T16:48:39Z

@nojnhuh: once the present PR merges, I will cherry-pick it on top of release-1.11 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.11

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov · 2023-10-17T16:51:41Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (5c6f04d) 57.82% compared to head (5b35816) 57.85%.
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4149      +/-   ##
==========================================
+ Coverage   57.82%   57.85%   +0.03%     
==========================================
  Files         187      187              
  Lines       19195    19205      +10     
==========================================
+ Hits        11099    11111      +12     
+ Misses       7468     7466       -2     
  Partials      628      628

Files	Coverage Δ
azure/services/aso/tags.go	`93.18% <100.00%> (+2.00%)`	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nawazkh

Some questions inline.

nawazkh · 2023-10-17T16:53:35Z

azure/services/aso/aso_test.go

@@ -710,7 +710,7 @@ func TestCreateOrUpdateResource(t *testing.T) {

 		specMock.MockTagsGetterSetter.EXPECT().GetActualTags(gomock.Any()).Return(nil)
 		specMock.MockTagsGetterSetter.EXPECT().GetAdditionalTags().Return(nil)
-		specMock.MockTagsGetterSetter.EXPECT().GetDesiredTags(gomock.Any()).Return(nil)
+		specMock.MockTagsGetterSetter.EXPECT().GetDesiredTags(gomock.Any()).Return(nil).Times(2)


specMock.MockTagsGetterSetter.EXPECT().GetDesiredTags(gomock.Any()).Return(nil).Times(2) Does this mean that the test will try to check the chain twice?

Trying to understand, why are we checking two times specifically ?

That means this will be called twice or else the test should fail. gomock complains if no count is specified this way and the function is called more than once. Whether we specify .Times(2) or .AnyTimes() doesn't matter to me since the "2" isn't significant other than to get the test to pass.

Okay, I see the reason why we are testing for .Times(2) in this chain.
That is because the func .GetDesiredTags() gets called twice in tags.go. Good granular check. Thanks for adding.

nawazkh · 2023-10-17T16:54:51Z

azure/services/aso/tags.go

@@ -47,6 +50,14 @@ func reconcileTags[T genruntime.MetaObject](t TagsGetterSetter[T], existing T, r
 		}

 		existingTags = t.GetActualTags(existing)
+		if !reflect.DeepEqual(t.GetDesiredTags(existing), existingTags) &&


Asking out of curiosity since I have not touched this piece of code; How do the tags get merged?
Does this transient error being returned here reflect that CAPZ is waiting for some external agent to "merge" the tags for us?

The tags get merged based on the CAPZ resource and ASO spec and status as defined further down in this function. The tests might help clarify.

A transient error gets returned when we're assuming ASO is actively doing work to reconcile spec.tags and then update status.tags.

Makes sense after going over the code below this change.
It would be good to have a short comment summarizing the reason for early return however.

nawazkh · 2023-10-17T20:44:43Z

/lgtm

k8s-ci-robot · 2023-10-17T20:44:51Z

LGTM label has been added.

Git tree hash: 2b01d27b24620fd9713d7cc3415e1befb736ea27

Jont828

/lgtm

mboersma

/lgtm
/approve

k8s-ci-robot · 2023-10-18T11:44:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mboersma

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mboersma]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-infra-cherrypick-robot · 2023-10-18T13:23:13Z

@nojnhuh: #4149 failed to apply on top of branch "release-1.11":

Applying: wait for ASO tags to converge before updating
Using index info to reconstruct a base tree...
M	azure/services/aso/aso_test.go
M	azure/services/aso/tags.go
M	azure/services/aso/tags_test.go
Falling back to patching base and 3-way merge...
Auto-merging azure/services/aso/tags_test.go
CONFLICT (content): Merge conflict in azure/services/aso/tags_test.go
Auto-merging azure/services/aso/tags.go
CONFLICT (content): Merge conflict in azure/services/aso/tags.go
Auto-merging azure/services/aso/aso_test.go
CONFLICT (content): Merge conflict in azure/services/aso/aso_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 wait for ASO tags to converge before updating
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.11

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 17, 2023

k8s-ci-robot requested review from Jont828 and sonasingh46 October 17, 2023 16:39

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 17, 2023

nojnhuh mentioned this pull request Oct 17, 2023

convert managedclusters and agentpools to ASO #4069

Merged

4 tasks

nawazkh reviewed Oct 17, 2023

View reviewed changes

wait for ASO tags to converge before updating

5b35816

nojnhuh force-pushed the aso-tags branch from 880baf5 to 5b35816 Compare October 17, 2023 20:35

k8s-ci-robot assigned nawazkh Oct 17, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2023

Jont828 approved these changes Oct 18, 2023

View reviewed changes

k8s-ci-robot assigned Jont828 Oct 18, 2023

mboersma approved these changes Oct 18, 2023

View reviewed changes

k8s-ci-robot assigned mboersma Oct 18, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2023

k8s-ci-robot merged commit 925e7ad into kubernetes-sigs:main Oct 18, 2023
10 checks passed

k8s-ci-robot added this to the v1.12 milestone Oct 18, 2023

nojnhuh mentioned this pull request Oct 18, 2023

[release-1.11] wait for ASO tags to converge before updating #4154

Merged

4 tasks

This was referenced Dec 12, 2023

ASO: Return readyErr over not done err when tags fail #4371

Merged

[release-1.11] ASO: Return readyErr over not done err when tags fail #4380

Merged

nojnhuh mentioned this pull request Feb 2, 2024

only consider spec in existing when reconciling ASO tags #4531

Merged

4 tasks

This was referenced Feb 6, 2024

[release-1.12] only consider spec in existing when reconciling ASO tags #4540

Merged

[release-1.13] only consider spec in existing when reconciling ASO tags #4541

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wait for ASO tags to converge before updating #4149

wait for ASO tags to converge before updating #4149

nojnhuh commented Oct 17, 2023

nojnhuh commented Oct 17, 2023

k8s-infra-cherrypick-robot commented Oct 17, 2023

codecov bot commented Oct 17, 2023 •

edited

Loading

nawazkh left a comment

nawazkh Oct 17, 2023

nawazkh Oct 17, 2023

nojnhuh Oct 17, 2023

nawazkh Oct 17, 2023 •

edited

Loading

nawazkh Oct 17, 2023

nojnhuh Oct 17, 2023

nawazkh Oct 17, 2023

nawazkh commented Oct 17, 2023

k8s-ci-robot commented Oct 17, 2023

Jont828 left a comment

mboersma left a comment

k8s-ci-robot commented Oct 18, 2023

k8s-infra-cherrypick-robot commented Oct 18, 2023

wait for ASO tags to converge before updating #4149

wait for ASO tags to converge before updating #4149

Conversation

nojnhuh commented Oct 17, 2023

nojnhuh commented Oct 17, 2023

k8s-infra-cherrypick-robot commented Oct 17, 2023

codecov bot commented Oct 17, 2023 • edited Loading

Codecov Report

nawazkh left a comment

Choose a reason for hiding this comment

nawazkh Oct 17, 2023

Choose a reason for hiding this comment

nawazkh Oct 17, 2023

Choose a reason for hiding this comment

nojnhuh Oct 17, 2023

Choose a reason for hiding this comment

nawazkh Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

nawazkh Oct 17, 2023

Choose a reason for hiding this comment

nojnhuh Oct 17, 2023

Choose a reason for hiding this comment

nawazkh Oct 17, 2023

Choose a reason for hiding this comment

nawazkh commented Oct 17, 2023

k8s-ci-robot commented Oct 17, 2023

Jont828 left a comment

Choose a reason for hiding this comment

mboersma left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 18, 2023

k8s-infra-cherrypick-robot commented Oct 18, 2023

codecov bot commented Oct 17, 2023 •

edited

Loading

nawazkh Oct 17, 2023 •

edited

Loading