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.

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

Update topology mapping Refs on all proxy instance deletions #9589

Merged
merged 8 commits into from
Jan 20, 2021

Conversation

freddygv
Copy link
Contributor

@freddygv freddygv commented Jan 19, 2021

To keep track of upstream/downstream pairings for the mesh topology visualization we keep the mapping stored in memdb. This pairing mapping contains a map with a reference to every proxy ID that contains this pair in its service definition.

As proxy instances are deregistered, this map needs to be updated to remove that reference for the pairing.

Currently that map is not being updated on deletions when the proxy ID being registered is not the last instance for that proxy name.

This PR ensures we persist the new reference map when proxy IDs are deregistered, and fixes the broken assertion in the relevant test.

Fixes: #9566

@freddygv freddygv requested a review from dnephin January 19, 2021 18:39
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines +3281 to +3282
if _, ok := copy.Refs[uid]; !ok {
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this could be done on the entry so that we skip the deep copy in this case? Probably a minor optimization.

@banks banks added backport/1.9 type/bug Feature does not function as expected labels Jan 20, 2021
.changelog/9589.txt Outdated Show resolved Hide resolved
@banks banks merged commit e50019b into master Jan 20, 2021
@banks banks deleted the topology-cleanup branch January 20, 2021 15:17
@hashicorp-ci
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/312876.

@hashicorp-ci
Copy link
Contributor

🍒✅ Cherry pick of commit e50019b onto release/1.9.x succeeded!

hashicorp-ci pushed a commit that referenced this pull request Jan 20, 2021
* Insert new upstream/downstream mapping to persist new Refs

* Avoid upserting mapping copy if it's a no-op

* Add test with panic repro

* Avoid deleting up/downstreams from inside memdb iterator

* Avoid deleting gateway mappings from inside memdb iterator

* Add CHANGELOG entry

* Tweak changelog entry

Co-authored-by: Paul Banks <banks@banksco.de>
@hashicorp-ci
Copy link
Contributor

🍒❌ Cherry pick of commit e50019b onto release/1.8.x failed! Build Log

@banks banks added this to the 1.9.x milestone Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Feature does not function as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

invalid memory address or nil pointer dereference
5 participants