Create connectors in their requested initial state (KIP-980)#12974
Create connectors in their requested initial state (KIP-980)#12974KasparMetsa wants to merge 3 commits intostrimzi:mainstrimzi/strimzi-kafka-operator:mainfrom KasparMetsa:connector-create-in-initial-stateKasparMetsa/strimzi-kafka-operator:connector-create-in-initial-stateCopy head branch name to clipboard
Conversation
Create stopped/paused connectors directly via POST /connectors with initial_state instead of creating them running and stopping them after. Fixes strimzi#11754 Signed-off-by: Kaspar Metsa <kasparme@gmail.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12974 +/- ##
============================================
+ Coverage 79.98% 80.06% +0.08%
- Complexity 6410 6448 +38
============================================
Files 337 341 +4
Lines 22489 22588 +99
Branches 3069 3082 +13
============================================
+ Hits 17987 18086 +99
Misses 3282 3282
Partials 1220 1220
🚀 New features to boost your workflow:
|
Signed-off-by: Kaspar Metsa <kasparme@gmail.com>
c1b159c to
675d92c
Compare
katheris
left a comment
There was a problem hiding this comment.
Thanks for the PR @KasparMetsa, I'm generally happy with the changes. I've added a few comments about some code comments that I don't think are needed and some questions on some of the test changes.
I also want to see what others think on the handling of the 409 response code
| // 409 means the Connect cluster is rebalancing, or a connector with this name already exists. | ||
| // Retrying rides out a rebalance. An "already exists" 409 will not clear on retry, so it uses up | ||
| // the back off and fails this reconcile; the connector is then reconciled on the next one. |
There was a problem hiding this comment.
Again I think this comment is overkill. However, I wonder whether we should be parsing the error and checking it isn't a failure because the connector already exists. Would be interested to see what other maintainers think
There was a problem hiding this comment.
I removed the unnecessary comment.
I used this 409 handling because the existing PUT method (createOrUpdatePutRequest) already
handles 409 the same way, so I followed that pattern.
I think it makes sense here, because a 409 can happen in two cases:
- If the Connect cluster is rebalancing. This is temporary. If we wait and retry, it will try again many times and the create will succeed eventually, so retrying makes sense.
- A connector with that name already exists - retrying won't help, because the name stays taken, so every retry gets
409again. After ~10 tries this reconcile fails.
So the question is really about case 2: should we detect "already exists" and stop retrying instead
of failing?
I don't think it's worth the extra code, because case 2 already fixes itself on the next reconcile:
- Every reconcile starts by asking Connect whether the connector exists (a
GETon its config). (AbstractConnectOperator:412) - If it now exists, the operator takes the update path (
PUT) (AbstractConnectOperator:427) instead of the create path (POST) (AbstractConnectOperator:436), so no409happens. - So the create path (and its
409) only runs while the connector is missing; once it exists, the
next reconcile just reconciles it normally.
It's also a rare case: the operator has a lock per connector, so it can't create a race condition itself - the name would
have to be taken by something outside the operator (for example a connector created directly via the
REST API, or a second operator on the same cluster).
Happy to add explicit "already exists" handling if you'd prefer, but I'd suggest keeping the simple retry.
Signed-off-by: Kaspar Metsa <kasparme@gmail.com>
|
/gha run pipeline=regression |
|
⏳ System test verification started: link The following 6 job(s) will be executed:
Tests will start after successful build completion. |
|
🎉 System test verification passed: link |
Create stopped/paused connectors directly via POST /connectors with initial_state instead of creating them running and stopping them after.
Fixes #11754
Type of change
Description
A
KafkaConnector(or MirrorMaker2 connector) withspec.state: stoppedorpausedwas first created running and then stopped or paused with a second call. This briefly started a connector that should never have run (for example, a MirrorHeartbeat connector created aheartbeatstopic before being stopped).This change uses the Kafka Connect
POST /connectorsendpoint with theinitial_statefield (KIP-980, Kafka 3.7+) to create the connector directly in the requested state, in a single step.Main changes:
KafkaConnectApi.createConnector(...), which sendsPOST /connectorswithinitial_state, plus its implementation.AbstractConnectOperator: the create path (the 404 "connector does not exist" branch) now callscreateConnectorwithspec.state. The oldcreateOrUpdateConnectoris renamed toupdateConnectorand is now only used to update an existing connector (stillPUT /connectors/{name}/config).All Kafka versions currently supported by Strimzi are well past 3.7, so no version guard is needed for
initial_state.No documentation change is needed: the
spec.statefield, its values, and its contract are unchanged. Astoppedconnector still ends up stopped; this only removes the unwanted transient start.AI assistance (Claude Code) was used throughout this change to analyze the issue, write the code and tests, and run the local Kubernetes verification. I reviewed all changes before submitting.
Checklist