etcd icon indicating copy to clipboard operation
etcd copied to clipboard

Published 20 hours ago • verified publisher icon etcd-io

Use errors.Is for error equality checks

Open redwrasse opened this issue 1 year ago • 17 comments

Update err == * expressions to errors.Is(err, *), as a preferred means of error equality checks that can handle wrapping (https://pkg.go.dev/errors#pkg-overview).

This PR updates all cases to be fixed in a number of files, but not all files. The rest of the cases should be addressed in a separate PR.

redwrasse avatar Aug 28 '24 05:08 redwrasse

Hi @redwrasse. 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.

k8s-ci-robot avatar Aug 28 '24 05:08 k8s-ci-robot

/ok-to-test

henrybear327 avatar Aug 28 '24 05:08 henrybear327

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 28.26087% with 33 lines in your changes missing coverage. Please review.

Project coverage is 68.90%. Comparing base (2c53be7) to head (503946f). Report is 18 commits behind head on main.

:exclamation: Current head 503946f differs from pull request most recent head d4df7a9

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

Files with missing lines Patch % Lines
client/v3/kubernetes/client.go 0.00% 27 Missing :warning:
server/etcdserver/api/v3discovery/discovery.go 0.00% 1 Missing and 1 partial :warning:
server/etcdserver/server.go 66.66% 2 Missing :warning:
pkg/proxy/server.go 50.00% 1 Missing :warning:
server/etcdserver/v3_server.go 87.50% 0 Missing and 1 partial :warning:

:exclamation: 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/api/v3compactor/periodic.go 90.24% <100.00%> (ø)
pkg/proxy/server.go 60.95% <50.00%> (ø)
server/etcdserver/v3_server.go 72.90% <87.50%> (ø)
server/etcdserver/api/v3discovery/discovery.go 67.80% <0.00%> (ø)
server/etcdserver/server.go 81.26% <66.66%> (ø)
client/v3/kubernetes/client.go 0.00% <0.00%> (ø)

... and 18 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18510      +/-   ##
==========================================
+ Coverage   68.79%   68.90%   +0.11%     
==========================================
  Files         420      420              
  Lines       35489    35470      -19     
==========================================
+ Hits        24413    24439      +26     
+ Misses       9646     9609      -37     
+ Partials     1430     1422       -8

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 2c53be7...d4df7a9. Read the comment docs.

codecov-commenter avatar Aug 28 '24 06:08 codecov-commenter

Looks like a failed lint test, I'll update the PR.

redwrasse avatar Aug 28 '24 16:08 redwrasse

Hmm, failed integration tests https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/etcd-io_etcd/18510/pull-etcd-integration-1-cpu-amd64/1828914519779315712

  • go.etcd.io/etcd/tests/v3/common: TestLeaseGrantKeepAliveOnce/PeerAutoTLS
  • go.etcd.io/etcd/tests/v3/common: TestLeaseGrantKeepAliveOnce

Guessing this is just test flakiness? I'm not reproducing locally.

redwrasse avatar Aug 28 '24 23:08 redwrasse

/retest

henrybear327 avatar Aug 29 '24 08:08 henrybear327

Hi @redwrasse, thanks for your pull request. I did a grep 'err ==' in your branch, and excluding err == nil, I still see some instances of comparisons using ==. Is there a particular reason why your PR addresses only some of them? (I may be missing something)

ivanvc avatar Aug 30 '24 20:08 ivanvc

@ivanvc, nope, I had hoped to be thorough. Let me try to find the ones I missed.

redwrasse avatar Aug 30 '24 21:08 redwrasse

I found three additional errors.Is conversions. Let me know if there's others still present. I see two additional but those are in a proto generated file rpc.pb.gw.go.

There are a handful of cases in tests of the pattern if err == nil || !strings.Contains(err.Error(), rpctypes.ErrRoleEmpty.Error()) that could be changed to errors.Is, but I haven't changed those here.

redwrasse avatar Aug 30 '24 22:08 redwrasse

@redwrasse, sorry for the back and forth sweat_smile: I should have probably given you the list of files since the beginning. So here are some others that I see after pulling your branch:

  • client/pkg/fileutil/lock_linux.go:67
  • client/internal/v2/client.go:371
  • client/internal/v2/keys.go:462
  • client/v3/experimental/recipes/key.go:158
  • client/v3/retry_interceptor.go:257
  • client/v3/client.go:297
  • client/v3/client.go:630
  • client/v3/client.go:648
  • client/v3/watch.go:398
  • etcdctl/ctlv3/command/auth_command.go:85
  • tests/e2e/ctl_v3_member_test.go:196
  • tests/e2e/ctl_v3_member_test.go:224
  • tests/integration/clientv3/connectivity/network_partition_test.go:38
  • tests/integration/clientv3/connectivity/network_partition_test.go:325
  • tests/integration/clientv3/concurrency/example_mutex_test.go:67
  • tests/integration/clientv3/connectivity/black_hole_test.go:116
  • tests/integration/clientv3/connectivity/black_hole_test.go:126
  • tests/integration/clientv3/connectivity/black_hole_test.go:139
  • tests/integration/clientv3/connectivity/black_hole_test.go:149
  • tests/integration/clientv3/connectivity/black_hole_test.go:210
  • tests/integration/clientv3/lease/leasing_test.go:2051 -> I'm not sure about this one
  • tests/integration/clientv3/connectivity/server_shutdown_test.go:104
  • tests/integration/clientv3/experimental/recipes/v3_lock_test.go:142
  • tests/integration/v3_grpc_test.go:117 -> Not sure either
  • server/etcdserver/api/v3compactor/revision.go:92
  • server/etcdserver/api/rafthttp/pipeline.go:168
  • server/etcdserver/api/rafthttp/snapshot_sender.go:113
  • server/etcdserver/api/rafthttp/stream.go:431
  • server/etcdserver/api/snap/snapshotter_test.go:83
  • server/etcdserver/api/v2discovery/discovery.go:190
  • server/etcdserver/api/v2discovery/discovery.go:230
  • server/etcdserver/api/v3rpc/lease.go:103
  • server/etcdserver/api/v3rpc/lease.go:113
  • server/etcdserver/api/v3rpc/lease.go:136
  • server/etcdserver/api/v3rpc/util.go:98
  • server/etcdserver/api/v3rpc/watch.go:214
  • server/etcdserver/api/v3rpc/watch.go:220
  • server/etcdserver/api/v3rpc/watch.go:244
  • server/etcdserver/apply/uber_applier.go:126
  • server/lease/leasehttp/http.go:78
  • server/proxy/grpcproxy/health.go:55
  • server/proxy/grpcproxy/maintenance.go:53
  • server/proxy/grpcproxy/lease.go:248
  • server/storage/wal/decoder.go:92
  • server/storage/wal/decoder.go:117

And matching err !=:

  • server/storage/wal/wal_test.go:155
  • server/storage/wal/wal_test.go:967
  • server/storage/wal/repair_test.go:195
  • server/storage/mvcc/kvstore_test.go:521
  • server/storage/schema/actions_test.go:138
  • server/storage/mvcc/key_index_test.go:216
  • server/storage/mvcc/index_test.go:133
  • server/storage/mvcc/index_test.go:137
  • server/storage/mvcc/kv_test.go:206
  • server/storage/mvcc/kv_test.go:629
  • server/storage/backend/batch_tx.go:128
  • server/lease/lessor_test.go:459
  • server/lease/lessor_test.go:512
  • server/etcdmain/config_test.go:227
  • server/etcdmain/config_test.go:270
  • server/etcdmain/config_test.go:313
  • server/embed/etcd_test.go:35
  • server/auth/jwt_test.go:142
  • and many in server/auth/store_test.go

And the list is way longer than I expected. I didn't continue checking all of the err !=, as there are many matches (other than the proto buffer generated files, as you mentioned). Because of the size of the list and changes, it would be better to do it in a follow-up pull request(s).

Please let me know if the instances I found are not relevant to this change.

ivanvc avatar Aug 30 '24 23:08 ivanvc

@ivanvc thanks, my IDE search was not pulling these up for some reason.

I'm happy to try to make the remaining changes in a separate follow-up PR, if you're fine with the scope of this current one.

redwrasse avatar Aug 30 '24 23:08 redwrasse

@redwrasse, what do you think if we scope it to the files you have already edited in this pull request? In that case, here's the list of missing ones (basically the err != case):

  • tests/integration/clientv3/lease/lease_test.go:42
  • tests/integration/clientv3/lease/lease_test.go:58
  • tests/integration/clientv3/lease/lease_test.go:94
  • tests/integration/clientv3/lease/lease_test.go:118
  • server/etcdserver/server.go:1984
  • server/etcdserver/v3_server.go:300
  • server/etcdserver/v3_server.go:531
  • tests/integration/clientv3/kv_test.go:54
  • tests/integration/clientv3/kv_test.go:59
  • tests/integration/clientv3/kv_test.go:122
  • tests/integration/clientv3/kv_test.go:161
  • tests/integration/clientv3/kv_test.go:203
  • tests/integration/clientv3/kv_test.go:417
  • tests/integration/clientv3/kv_test.go:422
  • tests/integration/clientv3/kv_test.go:754

Thanks again, Samir!

ivanvc avatar Aug 30 '24 23:08 ivanvc

Sounds good! I'll do that.

redwrasse avatar Aug 30 '24 23:08 redwrasse

Alright, I believe the latest commit with the err != cases covers all files changed in this PR.

Hopefully, can get through the rest of them shortly in a separate PR.

redwrasse avatar Aug 31 '24 07:08 redwrasse

Thanks for addressing, @redwrasse. Can you squash your commits, please? Thanks again.

ivanvc avatar Sep 03 '24 20:09 ivanvc

Perfect, I'll work on the rest of the cases as well as possibly the miscellaneous strings.Contains(err.Error(), substring) instances.

redwrasse avatar Sep 04 '24 00:09 redwrasse

Great, I tried to hopefully get the rest of the cases in https://github.com/etcd-io/etcd/pull/18551.

redwrasse avatar Sep 12 '24 19:09 redwrasse

Link to #18576.

Do we have enough approvals to merge this?

ivanvc avatar Sep 19 '24 05:09 ivanvc

@serathius any comment or concern on this PR?

ahrtr avatar Sep 19 '24 09:09 ahrtr

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, ivanvc, jmhbnz, redwrasse, serathius

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

The pull request process is described here

Needs approval from an approver in each of these files:
  • ~~OWNERS~~ [ahrtr,jmhbnz,serathius]

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 avatar Sep 20 '24 07:09 k8s-ci-robot

No concerns.

Replacing err == with errors.Is seems correct and safe enough that we can do it in bolk like this. Would be interesting if there is any static analysis that would highlight outdated comparison method.

That's a good point. Without a static check, I'm sure we'll soon have newly written code checking errors with ==/!=. I checked golangci-lint and revive, but there's nothing yet for it. We'll need to wait and keep an eye on it.

ivanvc avatar Sep 20 '24 17:09 ivanvc