You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/07/07 18:50:09 UTC
[GitHub] [kafka] rondagostino opened a new pull request #10991: MINOR: system test fix for 3 co-located KRaft controllers
rondagostino opened a new pull request #10991:
URL: https://github.com/apache/kafka/pull/10991
This patch adds a sanity-check bounce system test for the case where we have 3 co-located KRaft controllers and fixes the system test code so that this case will pass. We were not testing this case previously, and it would not pass because the first Kafka node would never be considered started. We start the Kafka nodes serially, and we decide that a node has successfully started when it logs a particular message. This message is not logged until the broker has identified the controller (i.e. the leader of the KRaft quorum). There cannot be a leader in the KRaft case until a majority of the KRaft quorum has started, so with 3 co-located controllers the first node could never be considered "started" by the system test. This patch skips that log message check when a majority of the controllers have not yet started. It also sends SIGKILL to any running nodes when a majority of co-located controllers have been stopped -- otherwise they do not shutdown, and we spin for the 60 seco
nd timeout.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe edited a comment on pull request #10991: MINOR: system test fix for 3 co-located KRaft controllers
Posted by GitBox <gi...@apache.org>.
cmccabe edited a comment on pull request #10991:
URL: https://github.com/apache/kafka/pull/10991#issuecomment-876023353
The new test works for me with : `./tests/docker/ducker-ak test tests/kafkatest/sanity_checks/test_bounce.py`
I had to make a small fix to our Dockerfile due to python's usual version compatibility mess. PR here: https://github.com/apache/kafka/pull/10995/files
Incidentally, even if you want to use the branch builder instead of Jenkins, you can just choose to run a single test through the web UI. That will be a lot quicker than a full test run.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #10991: MINOR: system test fix for 3 co-located KRaft controllers
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #10991:
URL: https://github.com/apache/kafka/pull/10991#issuecomment-876023353
LGTM.
The new test works for me with : `./tests/docker/ducker-ak test tests/kafkatest/sanity_checks/test_bounce.py`
I had to make a small fix to our Dockerfile due to python's usual version compatibility mess. PR here: https://github.com/apache/kafka/pull/10995/files
Incidentally, even if you want to use the branch builder instead of Jenkins, you can just choose to run a single test through the web UI. That will be a lot quicker than a full test run.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #10991: MINOR: system test fix for 3 co-located KRaft controllers
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10991:
URL: https://github.com/apache/kafka/pull/10991#issuecomment-879886127
https://confluent-kafka-branch-builder-system-test-results.s3-us-west-2.amazonaws.com/2021-07-14--001.system-test-kafka-branch-builder--1626231208--rondagostino--minor_systests_colocated_kraft_startup_fix--27f60380f/report.html
```
================================================================================
SESSION REPORT (ALL TESTS)
ducktape version: 0.8.1
session_id: 2021-07-14--001
run time: 169 minutes 1.441 seconds
tests run: 878
passed: 669
failed: 12
ignored: 197
================================================================================
```
We would expect this to be a bit faster since we start brokers in parallel instead of serially, and recent run with the same number of workers (https://github.com/apache/kafka/pull/10982#issuecomment-875181066) took 177 minutes, so this is about 5% faster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #10991: MINOR: system test fix for 3 co-located KRaft controllers
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #10991:
URL: https://github.com/apache/kafka/pull/10991#issuecomment-876822385
> That's a good idea. I think the diff above has a problem in that we would emit the message too early for the case where process.roles=broker, but I think the idea in general is sound.
How about something like this?
```
diff --git a/core/src/main/scala/kafka/server/KafkaRaftServer.scala b/core/src/main/scala/kafka/server/KafkaRaftServer.scala
index 8e77357383..310aed8253 100644
--- a/core/src/main/scala/kafka/server/KafkaRaftServer.scala
+++ b/core/src/main/scala/kafka/server/KafkaRaftServer.scala
@@ -106,10 +106,15 @@ class KafkaRaftServer(
override def startup(): Unit = {
Mx4jLoader.maybeLoad()
raftManager.startup()
- controller.foreach(_.startup())
- broker.foreach(_.startup())
+ controller.foreach { controller =>
+ controller.startup()
+ info(KafkaBroker.STARTED_MESSAGE + " controller")
+ }
+ broker.foreach { broker =>
+ broker.startup()
+ info(KafkaBroker.STARTED_MESSAGE + " broker")
+ }
AppInfoParser.registerAppInfo(Server.MetricsPrefix, config.brokerId.toString, metrics, time.milliseconds())
- info(KafkaBroker.STARTED_MESSAGE)
}
override def shutdown(): Unit = {
```
> Maybe we emit a different message when there is a controller role and that starts, and we emit the same message we emit now after we start the broker (if any). So it might look like this (Im typing this free-form, so ignore any typos -- it's just to get the point across) [...] And then in the system test code, where we check for the log message, we check for the CONTROLLER_STARTED_MESSAGE when the roles include the controller role -- otherwise we check for the standard message.
Hmm, what's the advantage of checking for two different strings?
It seems like if we have the broker emit "Kafka Server started broker" and have the controller emit "Kafka Server started controller" we can check for different messages if we want later, but still have simple code now.
> Just realized that one potential problem with the log message approach is that the last broker in a 3-broker cluster with 3 co-located controllers dos not shut down cleanly. The current approach deals with this by sending SIGKILL instead of SIGTERM. The log message approach doesn't deal with it, so the shutdown in the system test would end up timing out after 60 seconds and then sending SIGKILL later as part of the cleanup (the test would still pass). Not sure if this adds credence to the current approach?
I think you do need to special case this and send SIGKILL to the last n / 2 (round down) combined nodes. But at least we can avoid special-casing startup, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #10991: MINOR: system test fix for 3 co-located KRaft controllers
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10991:
URL: https://github.com/apache/kafka/pull/10991#issuecomment-876040323
Just realized that one potential problem with the log message approach is that the last broker in a 3-broker cluster with 3 co-located controllers dos not shut down cleanly. The current approach deals with this by sending SIGKILL instead of SIGTERM. The log message approach doesn't deal with it, so the shutdown in the system test would end up timing out after 60 seconds and then sending SIGKILL later as part of the cleanup (the test would still pass). Not sure if this adds credence to the current approach?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #10991: MINOR: system test fix for 3 co-located KRaft controllers
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10991:
URL: https://github.com/apache/kafka/pull/10991#issuecomment-876031699
> The idea is that if the controller portion of the combined node is up, it's reasonable to proceed
That's a good idea. I think the diff above has a problem in that we would emit the message too early for the case where `process.roles=broker`, but I think the idea in general is sound.
Maybe we emit a different message when there is a controller role and that starts, and we emit the same message we emit now after we start the broker (if any). So it might look like this (Im typing this free-form, so ignore any typos -- it's just to get the point across):
``` Mx4jLoader.maybeLoad()
raftManager.startup()
controller.foreach { c =>
c.startup()
info(KafkaBroker.CONTROLLER_STARTED_MESSAGE) // something that doesn't match the regex in the system test
}
broker.foreach(_.startup())
AppInfoParser.registerAppInfo(Server.MetricsPrefix, config.brokerId.toString, metrics, time.milliseconds())
info(KafkaBroker.STARTED_MESSAGE)
```
And then in the system test code, where we check for the log message, we check for the CONTROLLER_STARTED_MESSAGE when the roles include the controller role -- otherwise we check for the standard message.
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #10991: MINOR: system test fix for 3 co-located KRaft controllers
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #10991:
URL: https://github.com/apache/kafka/pull/10991#issuecomment-881768683
Thanks, @rondagostino ! I like the concurrent-start solution a lot better than any of the previous ones we were discussing. LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #10991: MINOR: system test fix for 3 co-located KRaft controllers
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10991:
URL: https://github.com/apache/kafka/pull/10991#issuecomment-877430917
TL;DR: my preference is to skip the regex check rather than adding a new message and carefully/appropriately grepping for it.
> I think you do need to special case this and send SIGKILL to the last n / 2 (round down) combined nodes
Yeah, we have to keep track of the number of co-located controllers that are running in order to send `SIGKILL` to brokers when there are less than a majority of controllers available. For completeness, I will note here that co-located controller nodes will always be the lowest-numbered nodes in the cluster given the way the system test code is written (https://github.com/apache/kafka/blob/trunk/tests/kafkatest/services/kafka/kafka.py#L724-L729), and that means those nodes will be started and stopped first given the way ducktape is written. So upon shutdown of a co-located cluster, once we don't have a majority of controllers, the rest of the nodes will have to be killed. For example, if we were to have 5 brokers and only 3 co-located nodes, the first 2 nodes will be controllers and can be shutdown normally, but then the last 3 will have to be killed due to lack of quorum (I discovered a minor bug in the current PR, which I am fixing with a small commit, while testing locally f
or the shutdown of a cluster with 3 co-located controllers out of 5 total nodes).
For starting up nodes, the question becomes whether we wait for a log message or not when starting a broker with less than a majority of co-located controllers running. It gets a bit tricky because we have to wait for both the controller and the broker to start when starting a node with `process.roles=broker,controller` would result in a majority, but if we wouldn't have a majority with this node started then we would have to grep for just the controller log message. Furthermore, any controller-related message must not match the grep that identifies both the broker and controller being started.
Finally, it generally would not be the case that we would start a node with `process.roles=broker` in the system tests with less than a majority of co-located controllers because of the startup order described above -- typically all co-located controllers are started before anything without a co-located controller starts. But if we wanted to deal with it accurately in case it were to happen, then we would have to know to skip the regex altogether here.
Assume for a moment that we skip the regex check if the node to be started wouldn't get us to a co-located quorum. With 3 co-located controllers this means we skip the regex check until we start the second controller node, and at that point and thereafter we wait for the broker to be fully started with the current regex check. And if there were any problems previously that we didn't catch due to not explicitly checking for a controller-specific startup message (e.g. the first node didn't start properly) then this next controller would not start and the regex would not be satisfied -- and the test would fail. This isn't ideal -- we would like to have known about the failure when the previous controller did not start properly -- but it seems like a reasonable tradeoff to be able to avoid the messiness of expanding our `grep`-based paradigm.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #10991: MINOR: system test fix for 3 co-located KRaft controllers
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10991:
URL: https://github.com/apache/kafka/pull/10991#issuecomment-876658421
In case we want to stick with the current solution:
```
================================================================================
SESSION REPORT (ALL TESTS)
ducktape version: 0.8.1
session_id: 2021-07-07--001
run time: 177 minutes 18.294 seconds
tests run: 878
passed: 669
failed: 12
ignored: 197
================================================================================
```
Failures are the familiar culprits and unrelated to this change.
https://confluent-kafka-branch-builder-system-test-results.s3-us-west-2.amazonaws.com/2021-07-07--001.1625698454--rondagostino--minor_systests_colocated_kraft_startup_fix--21f01e440/report.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe merged pull request #10991: MINOR: system test fix for 3 co-located KRaft controllers
Posted by GitBox <gi...@apache.org>.
cmccabe merged pull request #10991:
URL: https://github.com/apache/kafka/pull/10991
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org