Purge "rumor heat" information for departed members #5744

christophermaier · 2018-10-15T19:40:40Z

This should allow us to do a degree of cleanup of old rumor
information. Absent this, large and long-running Habitat networks can
accumulate significant amounts of wasted memory on what is effectively
useless historical information.

The RumorHeat data structure effectively maps a rumor to a mapping
of members to the number of times the member has seen the rumor. This
allows us to know who's seen a rumor already, and who hasn't.

Now, when a member departs the ring, we'll remove any of the
"member-to-number-of-times-they've-seen-the-rumor" mappings for that
member.

Additionally, we'll remove all such mappings for any rumors about
services hosted on that member. After all, the member is going away,
so all of its services will too.

We aren't removing the membership rumors, though, because that's how
we pass the word that the member is departed.

Future work may add more cleanups and rearrangements.

Signed-off-by: Christopher Maier cmaier@chef.io

This should allow us to do a degree of cleanup of old rumor information. Absent this, large and long-running Habitat networks can accumulate significant amounts of wasted memory on what is effectively useless historical information. The `RumorHeat` data structure effectively maps a rumor to a mapping of members to the number of times the member has seen the rumor. This allows us to know who's seen a rumor already, and who hasn't. Now, when a member departs the ring, we'll remove any of the "member-to-number-of-times-they've-seen-the-rumor" mappings for that member. Additionally, we'll remove _all_ such mappings for any rumors about _services_ hosted on that member. After all, the member is going away, so all of its services will too. We aren't removing the membership rumors, though, because that's how we pass the word that the member is departed. Future work may add more cleanups and rearrangements. Signed-off-by: Christopher Maier <cmaier@chef.io>

thesentinels · 2018-10-15T19:40:42Z

Thanks for the pull request! Here is what will happen next:

Your PR will be reviewed by the maintainers
If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

christophermaier · 2018-10-15T19:41:18Z

I added the "do not merge" label while I run this through some further local testing. Pushing it out for early feedback, though.

adamhjk

What happens when a departed member returns? Instinctually, the system exepcts to find a key in the rumor_heat for everyone? Or does it get recreated?

Wroth adding a test for, at least.

christophermaier · 2018-10-15T21:25:37Z

@adamhjk The rumor heat structure would get repopulated again for that member. In that regard, it'd basically behave the same as if a totally new Supervisor joined up.

baumanj

Looks generally good to me. Make some suggestions, but nothing I feel too strongly about.

baumanj · 2018-10-15T20:56:42Z

components/butterfly/src/rumor/heat.rs

+    /// memory this consumes. If that member should ever come back
+    /// again, all rumors would be considered "hot" for them, so they
+    /// will get a bit more network traffic initially.
+    pub fn purge(&self, id: &str) {


Some debug!s info about how many things were removed might be nice

baumanj · 2018-10-15T20:58:23Z

components/butterfly/src/server/expire.rs

@@ -57,6 +57,8 @@ impl Expire {
                .members_expired_to_departed(self.timing.departure_timeout_duration());

            for id in newly_departed_members {
+                // Purge "heat" information for a member that's gone


Between the good naming of newly_departed_members and rumor_heat.purge(&id), I honestly think this comment is superfluous.

baumanj · 2018-10-15T21:16:13Z

components/butterfly/src/server/mod.rs

                trace_incarnation,
                trace_health
            );
+
+            // Purge "heat" information for a member that's gone


Again, I'm not sure this comment says anything the code doesn't convey quite clearly (ditto to the several other times this same comment is used).

What is confusing to me though is why we still start_hot_rumor even if we're purging. I'm not saying this is wrong (still trying to grok this code), but it seems strange enough that a comment about that might be warranted.

baumanj · 2018-10-15T21:53:12Z

components/butterfly/src/server/mod.rs

                trace_incarnation,
                trace_health
            );
+
+            // Purge "heat" information for a member that's gone
+            if member_id != self.member_id() && health == Health::Departed {


This is almost identical to the code in insert_member. I wonder if it's possible to factor out the commonality as this sort of duplication seems prone to bugs. I don't just mean for the added code, but for the two functions more generally.

Good point... I'm not clear on if there's actually a meaningful distinction between these functions; the names don't give it away, at least. This would probably be good to tackle in a follow-on PR.

baumanj · 2018-10-15T21:56:19Z

components/butterfly/src/server/mod.rs

@@ -744,6 +757,9 @@ impl Server {
                {
                    // TODO (CM): Why are we inferring departure from
                    // a service rumor?
+
+                    // Purge "heat" information for a member that's gone
+                    self.rumor_heat.purge(&service_rumor.member_id);


I wonder if this and the duplication on L804 could be avoided by putting this logic inside insert_health_by_id.

I'd initially wanted to do that, too, but the MemberList knows nothing of the RumorHeat data structure 😦

raskchanky

I'm still coming to grips with this part of the codebase, so take my approval with the appropriately sized grain of salt, but it all looks good to me.

jeremymv2 · 2018-10-16T00:39:31Z

components/butterfly/src/rumor/heat.rs

+        // all types of rumors.
+        for heat in heat_map.values_mut() {
+            heat.remove(id);
+        }


Perhaps either quantifying or summarizing the bytes purged might be of some use here?

+use std::mem; ... for heat in heat_map.values_mut() { - heat.remove(id); + if let Some(r) = heat.remove(id) { + debug!("Purging rumor heat of size {} bytes", mem::size_of_val(&r)); + } else { + debug!("Member {} unexpectedly had no rumor heat to purge.", id); + } }

size_of_val doesn't always give you what you think you want, since it's the size of the value, which may not be the same as the "total size" of the value, including any heap allocations. For example, you'll always get 24 if your value is a String, since a String is a 3-byte "fat pointer", regardless of how many characters are in the actual character string pointed to on the heap.

However, in this case, it's always going to be 64 bytes; 56 for the member ID (24 for the "fat pointer" and 32 for the ID string itself) and 8 for the usize counter.

Signed-off-by: Christopher Maier <cmaier@chef.io>

Obvious fix; these changes are the result of automation not creative thinking.

christophermaier added T-DO-NOT-MERGE labels Oct 15, 2018

christophermaier self-assigned this Oct 15, 2018

christophermaier requested review from baumanj, fnichol, raskchanky and reset as code owners October 15, 2018 19:40

adamhjk reviewed Oct 15, 2018

View reviewed changes

baumanj approved these changes Oct 15, 2018

View reviewed changes

raskchanky approved these changes Oct 15, 2018

View reviewed changes

jeremymv2 reviewed Oct 16, 2018

View reviewed changes

Address PR review comments

11caecb

Signed-off-by: Christopher Maier <cmaier@chef.io>

christophermaier removed the T-DO-NOT-MERGE label Oct 16, 2018

christophermaier merged commit aac5876 into master Oct 16, 2018

christophermaier deleted the cm/prune-rumors branch October 16, 2018 12:51

chef-ci added a commit that referenced this pull request Oct 16, 2018

Update CHANGELOG.md with details from pull request #5744

db99def

Obvious fix; these changes are the result of automation not creative thinking.

christophermaier mentioned this pull request Oct 18, 2018

Garbage Collection of old Rumors #5761

Open

christophermaier added Type:BugFixes PRs that fix an existing bug and removed X-fix labels Jul 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Purge "rumor heat" information for departed members #5744

Purge "rumor heat" information for departed members #5744

christophermaier commented Oct 15, 2018

thesentinels commented Oct 15, 2018

christophermaier commented Oct 15, 2018

adamhjk left a comment

christophermaier commented Oct 15, 2018

baumanj left a comment

baumanj Oct 15, 2018

baumanj Oct 15, 2018

baumanj Oct 15, 2018

baumanj Oct 15, 2018

christophermaier Oct 16, 2018

baumanj Oct 15, 2018

christophermaier Oct 16, 2018

raskchanky left a comment

jeremymv2 Oct 16, 2018

christophermaier Oct 16, 2018

Navigation Menu

Purge "rumor heat" information for departed members #5744

Purge "rumor heat" information for departed members #5744

Conversation

christophermaier commented Oct 15, 2018

thesentinels commented Oct 15, 2018

christophermaier commented Oct 15, 2018

adamhjk left a comment

Choose a reason for hiding this comment

christophermaier commented Oct 15, 2018

baumanj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raskchanky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment