Handle read-only write txn failures gracefully

[shyamjvs]

Following up from #18667 (comment).

The idea is simple:

1. We have two kinds of txns - read-only and write
2. Read-only txns do not have any write operations inside their success/failure operation blocks - these are already bypassing apply layer and served directly via linearizable read loop (xref)
3. Write txns have at least 1 write operation inside their success/failure operation blocks - however during txn execution (apply time), the actual operation paths chosen based on "compare" outputs could effectively render them as read-only txns

This change improves the behavior for 3 by avoiding an unnecessary panic when such read-only txns see an error - instead return the error back to server handler and move on with applying subsequent entries. Panic + server restart seems unnecessary overhead in such cases. This also paves a way to bring back context-based timeouts for such txns. One case where that could help is during high write contention scenarios where CAS operations often end up going to "failure" block and triggers a (potentially slow) range request.

/cc @serathius @ahrtr

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: shyamjvs
Once this PR has been reviewed and has the lgtm label, please assign spzala for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Hi @shyamjvs. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.70%. Comparing base (7cded36) to head (55cbea3).

❗ Current head 55cbea3 differs from pull request most recent head e3d9b31

Please upload reports for the commit e3d9b31 to get more accurate results.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Files with missing lines	Coverage Δ
server/etcdserver/txn/txn.go	88.62% <100.00%> (-4.53%)	⬇️

... and 33 files with indirect coverage changes

@@           Coverage Diff           @@
##             main   #18803   +/-   ##
=======================================
  Coverage   68.70%   68.70%           
=======================================
  Files         420      420           
  Lines       35512    35517    +5     
=======================================
+ Hits        24399    24403    +4     
- Misses       9677     9678    +1     
  Partials     1436     1436           

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7cded36...e3d9b31. Read the comment docs.

[serathius]

Don't like the change and don't think it is correct. My high level understanding of the motivation for isWrite is whether TXn is executed via apply or locally. Even though this is not expressed in code directly, it's reflected in:

• Type of transaction we use (shared vs concurrent). We want to used shared only in apply.
• Closing and reopening transaction. This cannot I mean really cannot be done outside of apply loop.
• Decision whether we panic or return error (we need to panic in apply)

I don't see any benefits of what you are proposing, and I'm against about making isWrite inconsistent between those checks.

What I can recommend is replacing isWrite check with something more explicitly showing differences between how TXN should be executed in Apply vs locally.

[ahrtr]

I understand the motivation of this PR to remove unnecessary panic when the execution branch/path is readonly, but obviously it complicates the TXN applying workflow and without big benefit. So I suggest that we don't change it unless there is a strong justification.

My high level understanding of the motivation for isWrite is whether TXn is executed via apply or locally.

That's the original only use case. The #14149 also removed the unnecessary panic in applying workflow; which I think is also valid use case, although it introduced a potential issues as @shyamjvs pointed out and fixed in #18749

[shyamjvs]

Thanks for those comments @serathius @ahrtr. Few clarifications:

• This change does not modify the behavior of read-only txns being executed locally. Similarly, write txns (whether they end up actually doing a write or not) still go through apply layer. In fact, they have to go through apply layer (otherwise compare operations cannot be serializably evaluated)
• Reg mvcc txn handling, there are two cases here:
  • read-only txn that's handled outside of apply loop - this does not close + reopen the txn and the same txnRead we create for check phase is continued in use for actual txn execution but upgraded as mvcc.NewReadOnlyTxnWrite (this behavior is unchanged)
  • write txn that's handled inside the apply loop which actually performs write - we end the txnRead used for check phase and open a txnWrite for execution phase, which is safe because all write txns are serialized in apply so another write won't interfere (this behavior is also unchanged)
  • now for write txn inside apply loop that does not actually perform writes - this PR is changing the behavior to continue using the txnRead from check phase also for execution phase. IMHO this is actually safer than using a write txn for an operation that we know should be read-only. Because if the txn accidentally tries to perform any writes (despite check phase determining it to be read-only), server will now explicitly panic thanks to using mvcc.ReadOnlyTxnWrite
• Reg "should we panic on read-only write txn error" - this is orthogonal decision from above and we can choose to go either way for both. I'm not too confident about the benefit of panic here though - as it's going to be costly recovery operation, request fails anyway but we lose opportunity to surface the error back to client with panic. FWIW this rationale already lives here today
• The change is relatively small (mainly changing checkTxn part), but I admit it is additional complexity. If we think it isn't worth the tradeoff, I'm happy to close the PR

[ahrtr]

• The change is relatively small (mainly changing checkTxn part), but I admit it is additional complexity. If we think it isn't worth the tradeoff, I'm happy to close the PR

Suggest to close it. Readability & understandability is more important in the critical applying workflow, so let's keep it as simple as possible.

[shyamjvs]

Ack - will let @serathius also chime in with any final thoughts before closing.

[serathius]

Suggest to close it. Readability & understandability is more important in the critical applying workflow, so let's keep it as simple as possible.

+1 to that.

The change is relatively small (mainly changing checkTxn part), but I admit it is additional complexity. If we think it isn't worth the tradeoff, I'm happy to close the PR

We are especially careful about small changes. I don't think there is a tradeoff as we are just sacrificing readability to solve a problem that no one reported. I would like to see some evidence of this being a problem.

[ahrtr]

Closing...

@shyamjvs Please feel free to reach out if you still have any concern or comment. Thank you!

[shyamjvs]

Sounds good, thanks for discussing!