-
Notifications
You must be signed in to change notification settings - Fork 916
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
ARTEMIS-4305 Zero persistence does not work in kubernetes #4899
Conversation
94ac5ea
to
7f8833c
Compare
I think you could simplify this quite a bit. Here's what I suggest...
You also needs tests to verify the fix and mitigate regressions in the future. |
Initially I attempted what you suggest about lazy initializing the node id like that, precicely because I wanted to keep the code changes to a minimum. However, that ended up being much more complicated(rather than simplified), because of the way IMO from a functional standpoint, adding the
|
7f8833c
to
5eabef6
Compare
@@ -408,10 +416,15 @@ public void endOfBatch(Object connectionID) { | |||
} | |||
|
|||
private void doBufferReceived(final Packet packet) { | |||
if (isHealthy && !isCorrectPing(packet)) { | |||
isHealthy = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting this line out will effectively disable the fix. This will cause the new test ZeroPersistenceSymmetricalClusterTest
to fail.
If this is an issue in Core, it will be an issue in AMQP as well. we should make sure AMQP also takes care of this? WDYT @jbertram @gemmellr @gtully @tabish121 ? |
I believe the use-case here only involves cluster nodes and the core connections between them. Therefore, I don't think AMQP is in view. |
5eabef6
to
4c9dabe
Compare
Fixed the checkstyle issues. |
…rams` map to acceptors and connectors
* include original node id in `TransportConfiguration` decoding * match ping packets' nodeUUID against the connection's transport configuration target nodeUUID; if any side is missing this data, the match succeeds * destroy a remoting connection if it ever becomes unhealthy(ping nodeUUID is different that the target)
4c9dabe
to
cd49b15
Compare
I pulled down your PR branch and executed
Also, I put together a proof-of-concept fix on https://github.com/jbertram/activemq-artemis/tree/ARTEMIS-4305-alt which seems like would address the problem you're seeing but is much simpler than the fix in this PR. Can you test this fix? |
cd49b15
to
5838513
Compare
I had to update the test to work with JUnit 5(I didn't notice on my last rebase that Artemis upgraded from an earlier JUnit).
I am having trouble running the multicast discovery tests on my mac. I setup my routes as described in https://issues.apache.org/jira/browse/ARTEMIS-2341 (like last time and that was sufficient to make multicast discovery work). So as it stands I am unable to verify if your fix works. I was only able to look at the code, and as far as I can tell, my concerns remain same as I mentioned earlier. However, if your fix works reliably, I have no objections. Could you please pull the latest version of test |
I added your test to the branch with my fix, and I can see my fix detecting a problem and closing the connection, but the test still fails, and I still see messages like this:
Is this the kind of message you see in your K8s cluster when this problem occurs and is that what you were referring to in the Jira when you said this?
I reproduced this with a very simple manual test with 2 clustered nodes with persistence disabled. When I
I then cherry-picked your |
No, this.removeMemberMethod = Topology
.class
.getDeclaredMethod("removeMember", long.class, String.class, boolean.class);
this.removeMemberMethod.setAccessible(true);
...
removeMemberMethod.invoke(topology, topologyMember.getUniqueEventID() + 1000, topologyMemberId);
I assume you mean you reproduced the WARN message. This is not what we are trying to fix.
We were already running that earlier. That resulted in other bugs with the topology. Can't remember exact details anymore(was a couple of years ago), but sometimes when a peer came back after the reconnect attempts stopped, it would no longer be re-admitted in the cluster.
Do you mean it is a false negative(test failed but shouldn't have)? How do you verify that the topology recovers correctly? |
As far as I can tell the
If that's the case then I think that is the bug we should be attempting to fix. For your particular use-case I think you should definitely be using
Your current configuration with As it stands, the recommended configuration fixes the problem with the test-case in this PR so this fix is not valid and won't be merged. |
I am not sure I understand why should All these are supposed to be valid configurations, right? In all cases we are talking about a bug being fixed. We are perfeclty happy to use the current configuration(prefer it infact, as it has been fairliy stable for some time, apart for this one bug). Given that we've already spent so much resources on fixing this bug, I do not see a reason to use another configuration which we have to fix all over again. Even if our current configuration is not the "best" one by some criteria, why should this bug remain in the code? |
It's better because it actually fits your use-case. Since persistence is disabled then every time a broker restarts it will have a new identity. This is true even if you weren't on K8s. Since the broker's identity will change that means the cluster-connection will also need to be torn down and recreated so there's no reason to attempt to reconnect. Reconnecting a cluster connection only makes sense if the same exact server is coming back. With
In your case I would say the configuration is not valid. To be clear, not all configurations are valid for all use-cases. For example, if your use-case required messages to survive a broker restart then setting
It's not that your current configuration is not the "best." It's that your current configuration is directly leading to the problem with the stale When evaluating which changes should be merged into the code-base one must weigh the risk of the change (e.g. additional complexity, possibility for unintended consequences, performance impact, etc.) against the benefit. In this case I do not believe the benefits outweigh the risks given the fact that you're not using a valid configuration for your use-case. Ultimately I'm not sure I'd actually consider the current behavior a bug given that a valid configuration solves the issue. That said, there may, in fact, be another bug that you hit with the valid configuration. I expect this bug would impact other users and would be worth investigating and resolving. I empathize with you about the lost work/time regarding this PR. I know the feeling because I've lost lots of work myself when my own PRs were rejected. Ideally this is all part of the process to make the software better for everyone in the end. Of course, you are free to maintain a fork of ActiveMQ Artemis and apply your own patches (e.g. this one) before deploying in your environment. That's one of the many great benefits of open source. |
Yes, however that (may) be a recovarable situation - to just fix our configuration. In this case however, there is no recovering(unless we resort to hacks), no matter which configuration we use. Running inside k8s is not the only use case we want to support. We also deploy in other modes, where nodes do keep identities between restarts. There can be all kinds of variations to the deployment. We need a single configuration that works in all cases, not multiple different ones, each of which is prone to various bugs(as history has proven). We would never get anything resolved that way. |
I spoke at length with Ivan on Slack yesterday about this issue, and I wanted to summarize my thoughts here for posterity's sake. The real issue that this PR is attempting to address is related to a quirk (for lack of a better word) in how TCP can sometimes work in a containerized environment. In short, it's possible for a TCP connection to be closed on one side without the other side receiving the appropriate The use-case here involves a cluster of embedded brokers each running on separate K8s pods without persistence. If one of those pods is restarted by K8s using the same IP address then the "same" broker rejoins the cluster with a new identity (since the node ID is not persisted and reused) and when this TCP "quirk" happens the other clustered brokers don't clean up their cluster connection. Their TCP connection to the old broker apparently just becomes a connection to the new broker. This, of course, breaks things in weird ways with the internal state (e.g. That said, the test on this PR is not testing this use-case. Instead it is testing a similar but technically different use-case where one node in a cluster is restarted with a new identity (i.e. node ID) on the same IP address. If the other nodes have Ultimately, I do not believe the test on this PR is valid for the real problem that needs to be solved (i.e. the missing TCP At this point I'm closing this PR as I don't see a future for it here. |
In a cluster deployed in kubernetes, when a node is destroyed it terminates the process and shuts down the network before the process has a chance to close connections. Then a new node might be brought up, reusing the old node’s ip. If this happens before the connection ttl, from artemis’ point of view, it looks like as if the connection came back. Yet it is actually not the same, the peer has a new node id, etc. This messes things up with the cluster, the old message flow record is invalid.
This also solves another similar issue - if a node goes down and a new one comes in with a new nodeUUID and the same IP before the cluster connections in the others timeout, it would cause them to get stuck and list both the old and the new nodes in their topologies.
The changes are grouped in tightly related incremental commits to make it easier to understand what is changed:
Ping
packets includenodeUUID
TransportConfiguration
RemotingConnectionImpl#doBufferReceived
tracks for ping nodeUUID mismatch with the target to flag it asunhealthy
;ClientSessionFactoryImpl
destroys unhealthy connections(in addition to not receiving any data on time)