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.

Sign up for GitHub

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

fix tag delete following resource create #3187

Merged
merged 1 commit into from
Feb 28, 2023
Merged

fix tag delete following resource create #3187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions azure/services/tags/tags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,14 @@ func (s *Service) Reconcile(ctx context.Context) error {
return errors.Wrap(err, "cannot update tags")
}
}

// We also need to update the annotation if anything changed.
if err := s.Scope.UpdateAnnotationJSON(tagsSpec.Annotation, newAnnotation); err != nil {
return err
}
log.V(2).Info("successfully updated tags")
}

// We also need to update the annotation even if nothing changed to
// ensure it's set immediately following resource creation.
if err := s.Scope.UpdateAnnotationJSON(tagsSpec.Annotation, newAnnotation); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of updating it every time, would it make sense (and be possible) to only set it the first time we go through here and the annotation doesn't exist? otherwise we're updating the annotation every time even if nothing changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When nothing changes, doesn't this basically turn into a no-op? A patch is still unconditionally applied at the end of the reconciliation loop whether or not anything in the AzureCluster, etc. changes, so I don't think this would generate any additional ongoing API overhead. That being said, it would be simple to wrap this in something like a if tagsSpec.Annotation != newAnnotation block.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair

return err
}
Comment on lines +110 to +115
Copy link
Member

Choose a reason for hiding this comment

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

Will this change lead to a new reconciliation for every tag from the for loop up above (as the resource version gets updated on annotation update) ?
If yes, could such a change create throttling situation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This UpdateAnnotationJSON method doesn't make any API calls, it only sets the annotation on the corresponding CAPZ resource like this:

cluster-api-provider-azure/azure/scope/cluster.go

Lines 980 to 999 in 7c92038

// UpdateAnnotationJSON updates the `annotation` with
// `content`. `content` in this case should be a `map[string]interface{}`
// suitable for turning into JSON. This `content` map will be marshalled into a
// JSON string before being set as the given `annotation`.
func (s *ClusterScope) UpdateAnnotationJSON(annotation string, content map[string]interface{}) error {
b, err := json.Marshal(content)
if err != nil {
return err
}
s.SetAnnotation(annotation, string(b))
return nil
}
// SetAnnotation sets a key value annotation on the AzureCluster.
func (s *ClusterScope) SetAnnotation(key, value string) {
if s.AzureCluster.Annotations == nil {
s.AzureCluster.Annotations = map[string]string{}
}
s.AzureCluster.Annotations[key] = value
}

Then at the very end of the AzureCluster-level Reconcile, that resource is committed to the API with all of the changes made in that reconciliation, so different iterations of this for loop would be coalesced into a single Kubernetes API update. I think this change in particular will only introduce at most one additional reconciliation during the lifetime of the resource which adds the annotation the first time.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for pinning cluster-api-provider-azure/azure/scope/cluster.go lines above, helps to put things in perspective.
Following the code flow from the perspective of generating additional reconciliations, I do not see any Patch/Update/return reconcile:true calls on updating annotations or in function s.Scope.UpdateAnnotationJSON().

So looks good to me!

}
return nil
}
Expand Down
1 change: 1 addition & 0 deletions azure/services/tags/tags_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ func TestReconcileTags(t *testing.T) {
},
}}, nil)
s.AnnotationJSON("my-annotation").Return(map[string]interface{}{"key": "value"}, nil)
s.UpdateAnnotationJSON("my-annotation", map[string]interface{}{"key": "value"})
},
},
}
Expand Down