[jira] [Commented] (CASSANDRA-13700) Heartbeats can cause gossip information to go permanently missing on certain nodes
[
https://issues.apache.org/jira/browse/CASSANDRA-13700?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16145523#comment-16145523
]
Jason Brown commented on CASSANDRA-13700:
-
pushed the minor fix as sha {{6b927783ba0777d3dd7c2c3311b246a8dbce5b59}} to
2.1+.
> Heartbeats can cause gossip information to go permanently missing on certain
> nodes
> --
>
> Key: CASSANDRA-13700
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13700
> Project: Cassandra
> Issue Type: Bug
> Components: Distributed Metadata
>Reporter: Joel Knighton
>Assignee: Joel Knighton
>Priority: Critical
> Fix For: 2.2.11, 3.0.15, 3.11.1, 4.0, 2.1.19
>
>
> In {{Gossiper.getStateForVersionBiggerThan}}, we add the {{HeartBeatState}}
> from the corresponding {{EndpointState}} to the {{EndpointState}} to send.
> When we're getting state for ourselves, this means that we add a reference to
> the local {{HeartBeatState}}. Then, once we've built a message (in either the
> Syn or Ack handler), we send it through the {{MessagingService}}. In the case
> that the {{MessagingService}} is sufficiently slow, the {{GossipTask}} may
> run before serialization of the Syn or Ack. This means that when the
> {{GossipTask}} acquires the gossip {{taskLock}}, it may increment the
> {{HeartBeatState}} version of the local node as stored in the endpoint state
> map. Then, when we finally serialize the Syn or Ack, we'll follow the
> reference to the {{HeartBeatState}} and serialize it with a higher version
> than we saw when constructing the Ack or Ack2.
> Consider the case where we see {{HeartBeatState}} with version 4 when
> constructing an Ack and send it through the {{MessagingService}}. Then, we
> add some piece of state with version 5 to our local {{EndpointState}}. If
> {{GossipTask}} runs and increases the {{HeartBeatState}} version to 6 before
> the {{MessageOut}} containing the Ack is serialized, the node receiving the
> Ack will believe it is current to version 6, despite the fact that it has
> never received a message containing the {{ApplicationState}} tagged with
> version 5.
> I've reproduced in this in several versions; so far, I believe this is
> possible in all versions.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13700) Heartbeats can cause gossip information to go permanently missing on certain nodes
[
https://issues.apache.org/jira/browse/CASSANDRA-13700?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16108999#comment-16108999
]
Joel Knighton commented on CASSANDRA-13700:
---
I'm not sure of any places in the current codebase where the distinction
matters in practice, but the change is cleaner and makes the code more tolerant
of changes elsewhere, so +1.
> Heartbeats can cause gossip information to go permanently missing on certain
> nodes
> --
>
> Key: CASSANDRA-13700
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13700
> Project: Cassandra
> Issue Type: Bug
> Components: Distributed Metadata
>Reporter: Joel Knighton
>Assignee: Joel Knighton
>Priority: Critical
> Fix For: 2.2.11, 3.0.15, 3.11.1, 4.0, 2.1.19
>
>
> In {{Gossiper.getStateForVersionBiggerThan}}, we add the {{HeartBeatState}}
> from the corresponding {{EndpointState}} to the {{EndpointState}} to send.
> When we're getting state for ourselves, this means that we add a reference to
> the local {{HeartBeatState}}. Then, once we've built a message (in either the
> Syn or Ack handler), we send it through the {{MessagingService}}. In the case
> that the {{MessagingService}} is sufficiently slow, the {{GossipTask}} may
> run before serialization of the Syn or Ack. This means that when the
> {{GossipTask}} acquires the gossip {{taskLock}}, it may increment the
> {{HeartBeatState}} version of the local node as stored in the endpoint state
> map. Then, when we finally serialize the Syn or Ack, we'll follow the
> reference to the {{HeartBeatState}} and serialize it with a higher version
> than we saw when constructing the Ack or Ack2.
> Consider the case where we see {{HeartBeatState}} with version 4 when
> constructing an Ack and send it through the {{MessagingService}}. Then, we
> add some piece of state with version 5 to our local {{EndpointState}}. If
> {{GossipTask}} runs and increases the {{HeartBeatState}} version to 6 before
> the {{MessageOut}} containing the Ack is serialized, the node receiving the
> Ack will believe it is current to version 6, despite the fact that it has
> never received a message containing the {{ApplicationState}} tagged with
> version 5.
> I've reproduced in this in several versions; so far, I believe this is
> possible in all versions.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13700) Heartbeats can cause gossip information to go permanently missing on certain nodes
[
https://issues.apache.org/jira/browse/CASSANDRA-13700?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16103667#comment-16103667
]
Jason Brown commented on CASSANDRA-13700:
-
[~jkni] In our internal review of this change, we discovered that that patch
can be made incrementally safer. In
{{Gossiper#getStateForVersionBiggerThan()}}, we [get the
generation|https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/gms/Gossiper.java#L899],
but it's possible the {{epState.getHeartBeatState()}} could have been swapped
out before the next line executes (where we get the version).
Are you ok if I make the following small change to
{{Gossiper#getStateForVersionBiggerThan()}}:
{code}
-int localHbGeneration =
epState.getHeartBeatState().getGeneration();
-int localHbVersion =
epState.getHeartBeatState().getHeartBeatVersion();
+HeartBeatState heartBeatState = epState.getHeartBeatState();
+int localHbGeneration = heartBeatState.getGeneration();
+int localHbVersion = heartBeatState.getHeartBeatVersion();
{code}
Basically, just grab a reference the the same `HeartBeatState` that we'll use
for both the generation and version. Of course, this does not protect against
that `HeartBeatState` instance being mutated, but at least we can be smarter
about what we reference from the `epState`.
> Heartbeats can cause gossip information to go permanently missing on certain
> nodes
> --
>
> Key: CASSANDRA-13700
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13700
> Project: Cassandra
> Issue Type: Bug
> Components: Distributed Metadata
>Reporter: Joel Knighton
>Assignee: Joel Knighton
>Priority: Critical
> Fix For: 2.2.11, 3.0.15, 3.11.1, 4.0, 2.1.19
>
>
> In {{Gossiper.getStateForVersionBiggerThan}}, we add the {{HeartBeatState}}
> from the corresponding {{EndpointState}} to the {{EndpointState}} to send.
> When we're getting state for ourselves, this means that we add a reference to
> the local {{HeartBeatState}}. Then, once we've built a message (in either the
> Syn or Ack handler), we send it through the {{MessagingService}}. In the case
> that the {{MessagingService}} is sufficiently slow, the {{GossipTask}} may
> run before serialization of the Syn or Ack. This means that when the
> {{GossipTask}} acquires the gossip {{taskLock}}, it may increment the
> {{HeartBeatState}} version of the local node as stored in the endpoint state
> map. Then, when we finally serialize the Syn or Ack, we'll follow the
> reference to the {{HeartBeatState}} and serialize it with a higher version
> than we saw when constructing the Ack or Ack2.
> Consider the case where we see {{HeartBeatState}} with version 4 when
> constructing an Ack and send it through the {{MessagingService}}. Then, we
> add some piece of state with version 5 to our local {{EndpointState}}. If
> {{GossipTask}} runs and increases the {{HeartBeatState}} version to 6 before
> the {{MessageOut}} containing the Ack is serialized, the node receiving the
> Ack will believe it is current to version 6, despite the fact that it has
> never received a message containing the {{ApplicationState}} tagged with
> version 5.
> I've reproduced in this in several versions; so far, I believe this is
> possible in all versions.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13700) Heartbeats can cause gossip information to go permanently missing on certain nodes
[
https://issues.apache.org/jira/browse/CASSANDRA-13700?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16095090#comment-16095090
]
Jason Brown commented on CASSANDRA-13700:
-
re: CASSANDRA-11825. Yes, shared mutable state strikes again, and thanks for
addressing them separately ;)
> Heartbeats can cause gossip information to go permanently missing on certain
> nodes
> --
>
> Key: CASSANDRA-13700
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13700
> Project: Cassandra
> Issue Type: Bug
> Components: Distributed Metadata
>Reporter: Joel Knighton
>Assignee: Joel Knighton
>Priority: Critical
>
> In {{Gossiper.getStateForVersionBiggerThan}}, we add the {{HeartBeatState}}
> from the corresponding {{EndpointState}} to the {{EndpointState}} to send.
> When we're getting state for ourselves, this means that we add a reference to
> the local {{HeartBeatState}}. Then, once we've built a message (in either the
> Syn or Ack handler), we send it through the {{MessagingService}}. In the case
> that the {{MessagingService}} is sufficiently slow, the {{GossipTask}} may
> run before serialization of the Syn or Ack. This means that when the
> {{GossipTask}} acquires the gossip {{taskLock}}, it may increment the
> {{HeartBeatState}} version of the local node as stored in the endpoint state
> map. Then, when we finally serialize the Syn or Ack, we'll follow the
> reference to the {{HeartBeatState}} and serialize it with a higher version
> than we saw when constructing the Ack or Ack2.
> Consider the case where we see {{HeartBeatState}} with version 4 when
> constructing an Ack and send it through the {{MessagingService}}. Then, we
> add some piece of state with version 5 to our local {{EndpointState}}. If
> {{GossipTask}} runs and increases the {{HeartBeatState}} version to 6 before
> the {{MessageOut}} containing the Ack is serialized, the node receiving the
> Ack will believe it is current to version 6, despite the fact that it has
> never received a message containing the {{ApplicationState}} tagged with
> version 5.
> I've reproduced in this in several versions; so far, I believe this is
> possible in all versions.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13700) Heartbeats can cause gossip information to go permanently missing on certain nodes
[
https://issues.apache.org/jira/browse/CASSANDRA-13700?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16095087#comment-16095087
]
Jason Brown commented on CASSANDRA-13700:
-
+1
> Heartbeats can cause gossip information to go permanently missing on certain
> nodes
> --
>
> Key: CASSANDRA-13700
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13700
> Project: Cassandra
> Issue Type: Bug
> Components: Distributed Metadata
>Reporter: Joel Knighton
>Assignee: Joel Knighton
>Priority: Critical
>
> In {{Gossiper.getStateForVersionBiggerThan}}, we add the {{HeartBeatState}}
> from the corresponding {{EndpointState}} to the {{EndpointState}} to send.
> When we're getting state for ourselves, this means that we add a reference to
> the local {{HeartBeatState}}. Then, once we've built a message (in either the
> Syn or Ack handler), we send it through the {{MessagingService}}. In the case
> that the {{MessagingService}} is sufficiently slow, the {{GossipTask}} may
> run before serialization of the Syn or Ack. This means that when the
> {{GossipTask}} acquires the gossip {{taskLock}}, it may increment the
> {{HeartBeatState}} version of the local node as stored in the endpoint state
> map. Then, when we finally serialize the Syn or Ack, we'll follow the
> reference to the {{HeartBeatState}} and serialize it with a higher version
> than we saw when constructing the Ack or Ack2.
> Consider the case where we see {{HeartBeatState}} with version 4 when
> constructing an Ack and send it through the {{MessagingService}}. Then, we
> add some piece of state with version 5 to our local {{EndpointState}}. If
> {{GossipTask}} runs and increases the {{HeartBeatState}} version to 6 before
> the {{MessageOut}} containing the Ack is serialized, the node receiving the
> Ack will believe it is current to version 6, despite the fact that it has
> never received a message containing the {{ApplicationState}} tagged with
> version 5.
> I've reproduced in this in several versions; so far, I believe this is
> possible in all versions.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13700) Heartbeats can cause gossip information to go permanently missing on certain nodes
[
https://issues.apache.org/jira/browse/CASSANDRA-13700?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16094911#comment-16094911
]
Joel Knighton commented on CASSANDRA-13700:
---
Thanks, Jason! In this case, I agree the first option is safer for this issue.
Something like the second likely makes sense eventually, at least as part of a
larger audit of correctness issues in gossip. I believe your volatile
suggestion is correct.
I don't have a lot of helpful information to reproduce this; it reproduces in
larger clusters, particularly with higher latency levels. We can see the
effects locally with a few well-timed sleeps in MessagingService, but that
isn't terribly representative.
Branches pushed here:
||branch||
|[13700-2.1|https://github.com/jkni/cassandra/tree/13700-2.1]||
|[13700-2.2|https://github.com/jkni/cassandra/tree/13700-2.2]||
|[13700-3.0|https://github.com/jkni/cassandra/tree/13700-3.0]||
|[13700-3.11|https://github.com/jkni/cassandra/tree/13700-3.11]||
|[13700-trunk|https://github.com/jkni/cassandra/tree/13700-trunk]||
There's a somewhat conceptually similar issue when we bump the gossip
generation in the middle of constructing a reply - I believe that's the cause
in [CASSANDRA-11825], which presents similar problems. I'm choosing to address
them separately because they're indeed distinct problems and 11825 requires an
additional trigger (enabling and disabling gossip during runtime).
> Heartbeats can cause gossip information to go permanently missing on certain
> nodes
> --
>
> Key: CASSANDRA-13700
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13700
> Project: Cassandra
> Issue Type: Bug
> Components: Distributed Metadata
>Reporter: Joel Knighton
>Assignee: Joel Knighton
>Priority: Critical
>
> In {{Gossiper.getStateForVersionBiggerThan}}, we add the {{HeartBeatState}}
> from the corresponding {{EndpointState}} to the {{EndpointState}} to send.
> When we're getting state for ourselves, this means that we add a reference to
> the local {{HeartBeatState}}. Then, once we've built a message (in either the
> Syn or Ack handler), we send it through the {{MessagingService}}. In the case
> that the {{MessagingService}} is sufficiently slow, the {{GossipTask}} may
> run before serialization of the Syn or Ack. This means that when the
> {{GossipTask}} acquires the gossip {{taskLock}}, it may increment the
> {{HeartBeatState}} version of the local node as stored in the endpoint state
> map. Then, when we finally serialize the Syn or Ack, we'll follow the
> reference to the {{HeartBeatState}} and serialize it with a higher version
> than we saw when constructing the Ack or Ack2.
> Consider the case where we see {{HeartBeatState}} with version 4 when
> constructing an Ack and send it through the {{MessagingService}}. Then, we
> add some piece of state with version 5 to our local {{EndpointState}}. If
> {{GossipTask}} runs and increases the {{HeartBeatState}} version to 6 before
> the {{MessageOut}} containing the Ack is serialized, the node receiving the
> Ack will believe it is current to version 6, despite the fact that it has
> never received a message containing the {{ApplicationState}} tagged with
> version 5.
> I've reproduced in this in several versions; so far, I believe this is
> possible in all versions.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13700) Heartbeats can cause gossip information to go permanently missing on certain nodes
[
https://issues.apache.org/jira/browse/CASSANDRA-13700?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16094748#comment-16094748
]
Jason Brown commented on CASSANDRA-13700:
-
We also might need to make {{HeartBeatState.version}} volatile, but I'm still
thinking about it (just adding it here for discussion)
> Heartbeats can cause gossip information to go permanently missing on certain
> nodes
> --
>
> Key: CASSANDRA-13700
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13700
> Project: Cassandra
> Issue Type: Bug
> Components: Distributed Metadata
>Reporter: Joel Knighton
>Assignee: Joel Knighton
>Priority: Critical
>
> In {{Gossiper.getStateForVersionBiggerThan}}, we add the {{HeartBeatState}}
> from the corresponding {{EndpointState}} to the {{EndpointState}} to send.
> When we're getting state for ourselves, this means that we add a reference to
> the local {{HeartBeatState}}. Then, once we've built a message (in either the
> Syn or Ack handler), we send it through the {{MessagingService}}. In the case
> that the {{MessagingService}} is sufficiently slow, the {{GossipTask}} may
> run before serialization of the Syn or Ack. This means that when the
> {{GossipTask}} acquires the gossip {{taskLock}}, it may increment the
> {{HeartBeatState}} version of the local node as stored in the endpoint state
> map. Then, when we finally serialize the Syn or Ack, we'll follow the
> reference to the {{HeartBeatState}} and serialize it with a higher version
> than we saw when constructing the Ack or Ack2.
> Consider the case where we see {{HeartBeatState}} with version 4 when
> constructing an Ack and send it through the {{MessagingService}}. Then, we
> add some piece of state with version 5 to our local {{EndpointState}}. If
> {{GossipTask}} runs and increases the {{HeartBeatState}} version to 6 before
> the {{MessageOut}} containing the Ack is serialized, the node receiving the
> Ack will believe it is current to version 6, despite the fact that it has
> never received a message containing the {{ApplicationState}} tagged with
> version 5.
> I've reproduced in this in several versions; so far, I believe this is
> possible in all versions.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-13700) Heartbeats can cause gossip information to go permanently missing on certain nodes
[
https://issues.apache.org/jira/browse/CASSANDRA-13700?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16094744#comment-16094744
]
Jason Brown commented on CASSANDRA-13700:
-
[~jkni] Fantastic debugging here, Joel. We have seen this problem, as well,
with missing STATUS and TOKENS entries.
I followed this through, and I believe you are correct. Just to point out
(because I had to dig and reason through it), the key problem is (as Joel
points out) the shared mutable state of {{HeartBeatState}}. In
{{Gossiper.getStateForVersionBiggerThan}}, when the local node is building up
the {{Map states}} about itself, if any
states are added after the function returns *and* the heartbeat is incremented
before serialization, the peer will get the updated heartbeat value but not the
updated states (as we the set of states for the local node that we're sending
over was already constructed a priori the serialization).
Off the top of my head, I think there are at least two possible ways to fix
this:
- clone the {{HeartBeatState}} when constructing the {{EndpointState}} to
return from {{Gossiper.getStateForVersionBiggerThan}}. That way it's not
referencing mutable heartbeat state.
- execute the {{GossipTask}} on the same thread the we receive the gossip
syn/ack/ack2 messages (on the {{Stage.GOSSIP}} thread). That way we force
(almost) all references to gossip's stated mutable state into one thread.
The first option is simpler, smaller in scope, and certainly safer.
The second option is has performance implications, especially if the
{{GossipTask}} takes a while to execute, then we could start backing up the
tasks on the stage. This option, though, has the "possibility" of eliminating
more of the state race bugs that we seems to continually uncover as time goes
on. (Side note: there are still some updates to local Gossip state from the
main thread (via {{StorageService}}) at startup, and the response to the
{{EchoMessage}} is on the wrong thread, as well.)
Joel, can you share the method of how you are able to reproduce this?
> Heartbeats can cause gossip information to go permanently missing on certain
> nodes
> --
>
> Key: CASSANDRA-13700
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13700
> Project: Cassandra
> Issue Type: Bug
> Components: Distributed Metadata
>Reporter: Joel Knighton
>Assignee: Joel Knighton
>Priority: Critical
>
> In {{Gossiper.getStateForVersionBiggerThan}}, we add the {{HeartBeatState}}
> from the corresponding {{EndpointState}} to the {{EndpointState}} to send.
> When we're getting state for ourselves, this means that we add a reference to
> the local {{HeartBeatState}}. Then, once we've built a message (in either the
> Syn or Ack handler), we send it through the {{MessagingService}}. In the case
> that the {{MessagingService}} is sufficiently slow, the {{GossipTask}} may
> run before serialization of the Syn or Ack. This means that when the
> {{GossipTask}} acquires the gossip {{taskLock}}, it may increment the
> {{HeartBeatState}} version of the local node as stored in the endpoint state
> map. Then, when we finally serialize the Syn or Ack, we'll follow the
> reference to the {{HeartBeatState}} and serialize it with a higher version
> than we saw when constructing the Ack or Ack2.
> Consider the case where we see {{HeartBeatState}} with version 4 when
> constructing an Ack and send it through the {{MessagingService}}. Then, we
> add some piece of state with version 5 to our local {{EndpointState}}. If
> {{GossipTask}} runs and increases the {{HeartBeatState}} version to 6 before
> the {{MessageOut}} containing the Ack is serialized, the node receiving the
> Ack will believe it is current to version 6, despite the fact that it has
> never received a message containing the {{ApplicationState}} tagged with
> version 5.
> I've reproduced in this in several versions; so far, I believe this is
> possible in all versions.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
