server: Implement compaction hash checking #14120

serathius · 2022-06-15T15:09:40Z

Signed-off-by: Marek Siarkowicz [email protected]

cc @ptabor @ahrtr @spzala
Implements #14039

serathius · 2022-06-15T15:21:14Z

Remaining questions:

Can we enable this by default? It's a risky change, maybe we should just encourage users to enable it. After corruption notification was sent there was a huge response in all etcd configuration management platforms adding corruption check flag.
For cases where cluster was corrupted before upgrade should we also make initial hash check default? I will make transition better as it will not allow corrupted member to join the cluster instead of rising alarm and stopping writes.
Should we leave an escape hatch to disable this check? A flag that changes period that can be used to disable.

codecov-commenter · 2022-06-15T16:05:03Z

Codecov Report

Merging #14120 (d44bbff) into main (4977877) will decrease coverage by 0.00%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##             main   #14120      +/-   ##
==========================================
- Coverage   75.42%   75.41%   -0.01%     
==========================================
  Files         456      456              
  Lines       36919    37037     +118     
==========================================
+ Hits        27846    27933      +87     
- Misses       7340     7358      +18     
- Partials     1733     1746      +13

Flag	Coverage Δ
all	`75.41% <88.88%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
server/config/config.go	`79.76% <ø> (ø)`
server/embed/config.go	`73.40% <50.00%> (-0.25%)`	⬇️
server/storage/mvcc/testutil/hash.go	`83.56% <61.53%> (-12.19%)`	⬇️
server/embed/etcd.go	`75.33% <80.00%> (+0.19%)`	⬆️
server/etcdserver/server.go	`84.49% <94.11%> (-0.90%)`	⬇️
server/etcdmain/config.go	`86.34% <100.00%> (+0.11%)`	⬆️
server/etcdserver/corrupt.go	`88.81% <100.00%> (+2.75%)`	⬆️
server/storage/mvcc/hash.go	`100.00% <100.00%> (ø)`
client/v3/leasing/cache.go	`87.22% <0.00%> (-4.45%)`	⬇️
client/v3/leasing/txn.go	`88.09% <0.00%> (-3.18%)`	⬇️
... and 24 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

serathius · 2022-07-05T15:49:35Z

cc @ahrtr

serathius · 2022-07-05T15:50:50Z

Based on experience on etcd data inconsistency announcement I think we can launch disabled by default with strong suggestion to enable this feature. I have added 2 flags one for enabling the feature and second that allows changing frequency.

server/embed/config.go

server/embed/etcd.go

server/etcdserver/corrupt.go

ahrtr · 2022-07-07T07:20:58Z

server/etcdserver/corrupt.go

+					zap.Uint32("follower-hash", p.resp.Hash),
+					zap.String("follower-peer-id", types.ID(id).String()),
+				)
+				cm.hasher.TriggerCorruptAlarm(id)


Two comments here:

How do you know that which member's data is corrupted? I think we should depend on quorum. For example, for a cluster with 3 members, if two members' hashes are equal, but the third one is different, then we can say the third one's data is corrupted.

Probably we should add a metrics as well, so that users can leverage their monitoring system (such as prometheus) to generate alarm automatically.

Both improvements would be great, however I'm again little worried to backport too big of a feature. Previous consistency checking didn't identify corrupted member nor it had monitoring. I think those are great ideas for feature graduation, but are not needed to address etcd v3.5 supportability as proposed originally.

I agree that we can make any additional improvement on in the main branch. The point here is that it might not be correct to raise an alarm here (line 276), because we are not sure which member's data is corrupted.

You right, but this is true for all data inconsistency checks. We can only hope that it's not the leader.

server/etcdserver/server.go

ahrtr · 2022-07-07T07:30:36Z

I think probably we should also add an API & command to expose the compact hashes, so that users can check the values themselves, and take action accordingly. For example, for a cluster of 3 members, if two members have the same compact hashes, but the third one has a different value. Then the user may want to replace the third member.

I also suggested to add a metrics for this case, please see #14120 (comment)

server/etcdserver/corrupt.go

serathius · 2022-07-21T14:57:25Z

I think probably we should also add an API & command to expose the compact hashes, so that users can check the values themselves, and take action accordingly. For example, for a cluster of 3 members, if two members have the same compact hashes, but the third one has a different value. Then the user may want to replace the third member.

This implementation was meant to be limited to ensure it doesn't require additional API thus is backportable. We compact hashes are exposed via normal hashKV API, however API doesn't expose which revisions hashes where calculated for. This is ok as we will take a different approach when implementing it for v3.6.

serathius · 2022-07-21T16:29:55Z

PTAL @ahrtr

ahrtr · 2022-07-25T05:28:09Z

Overall looks good to me. The benefit for release-3.5 (after backporting this and one previous related PR) is that data inconsistency/corruption detection is enhanced to leverage the side effect of compaction operation. But users can only know the result by checking the log and alarm, which may not be user-friendly.

I agree that we should try NOT to change the API in a stable release, so it's OK just to backport what we have done to 3.5, and try to do more enhancement in 3.6 (see #14120 (comment) ).

server/embed/config.go

server/etcdmain/config.go

ahrtr · 2022-07-25T05:52:28Z

server/etcdserver/corrupt.go

+			}
+			cm.mux.Unlock()
+			cm.lg.Info("finished compaction hash check", zap.Int("number-of-hashes-checked", i+1))
+			return


Since the hashes entries are ordered in descending order, so you are intentionally return here if the hash checking on the latest revision is successful?

Is it possible that the latest [compactionRev, rev] hashes match, but previous ones do NOT match?

Yes, it could as a corruption can technically self healed. Does it mean etcd is corrupted? In my opinion no.

For example imagine that there PUT requests that was not reflected on all members. It means that hashes between members will not match. However, sometime later comes a DELETE request on the same key and it is reflected. That would restore the hash between members and invalidate previous corruption. If we look through cached hashes, the latest one will match, even though the old ones don't.

I tried to intentionally create such situation in which previous hashes do not match and the latest hashes match, but one member's data is indeed corrupted, but eventually I failed to create such situation. The root cause is etcd scans the whole KEY space starting from 0x00 to the specified compactMainRev; Specifically, the last isn't set to the prevCompactRev.

So in summary, the case that the latest [compactionRev, rev] hashes match, but previous ones do NOT match will never happen. And I think it's more safe to scan the whole KEY space instead of starting from prevCompactRev.

ahrtr · 2022-07-25T05:54:06Z

server/etcdserver/corrupt.go

+		cm.lg.Warn("skipped revision in compaction hash check; was not able to check all peers",
+			zap.Int("number-of-peers-checked", peersChecked),
+			zap.Int("number-of-peers", len(peers)),
+			zap.Int64("revision", hash.Revision),


Does it make sense to print the member IDs which are or aren't checked in this loop?

There is a log above that list all peers that were successfully checked. Writing both set of checked and it's complement is overkill

tests/e2e/etcd_corrupt_test.go

Signed-off-by: Marek Siarkowicz <[email protected]>

serathius · 2022-07-25T12:02:19Z

Fixed member id in #14272 and rebased this PR on it. This is ok as I expect we will want to backport MemberID fix.

Signed-off-by: Marek Siarkowicz <[email protected]>

ahrtr · 2022-07-26T08:04:18Z

server/etcdserver/corrupt.go

+					zap.Uint32("follower-hash", p.resp.Hash),
+					zap.String("follower-peer-id", p.id.String()),
+				)
+				return


I suggest not to return here; instead, we should continue to compare other peers' hash, and generate multiple alrams if needed.

What you are proposing is not consistent with periodic check which only raises one alarm even though there could be multiple members corrupted.

Should we at least finish comparing all peers' hashes and generate a info or error log? I think it's helpful for 3.5, at least we can tell the hashes from log.

This will heavily complicate implementation. There is nothing in those logs that users cannot get themselves.

Imagine a situation for 3.5, when the compact hash checker raises an alarm CORRUPT, then users must figure out manually which member's data is corrupted. If all member's revisions are exactly the same, and the corruption is just caused by some disk I/O bit flip or disk corruption. Then users must get all members' log, and find out all the log entries below, and compare the hashes themselves.

{"level":"info","ts":"2022-07-26T15:26:01.386+0800","caller":"mvcc/kvstore_compaction.go:65","msg":"finished scheduled compaction","compact-revision":9,"took":"40.078µs","hash":1087596250}

If we finish comparing all peers' hashes here and print a info or error log, then the users can easily figure out which member's data is corrupted.

Actually It just a minor code change, and I don't think it complicate the implementation.

For 3.6, we can do some additional enhancement, but for 3.5 the users can only use the log to do analysis themselves.

Of course, we can do it in a separate PR.

ahrtr · 2022-07-26T08:18:35Z

The ID is displayed as hex-decimal, but it's displayed as decimal in the ERRORS. I think we should fix this, of course in a separate PR.

$ etcdctl  endpoint status -w table --cluster  
+------------------------+------------------+---------------+-----------------+---------+----------------+-----------+------------+-----------+------------+--------------------+--------------------------------+
|        ENDPOINT        |        ID        |    VERSION    | STORAGE VERSION | DB SIZE | DB SIZE IN USE | IS LEADER | IS LEARNER | RAFT TERM | RAFT INDEX | RAFT APPLIED INDEX |             ERRORS             |
+------------------------+------------------+---------------+-----------------+---------+----------------+-----------+------------+-----------+------------+--------------------+--------------------------------+
|  http://127.0.0.1:2379 | 8211f1d0f64f3269 | 3.6.0-alpha.0 |           3.6.0 |   25 kB |          25 kB |      true |      false |         4 |         69 |                 69 |  memberID:10501334649042878790 |
|                        |                  |               |                 |         |                |           |            |           |            |                    |                 alarm:CORRUPT  |
| http://127.0.0.1:22379 | 91bc3c398fb3c146 | 3.6.0-alpha.0 |           3.6.0 |   25 kB |          25 kB |     false |      false |         4 |         69 |                 69 |  memberID:10501334649042878790 |
|                        |                  |               |                 |         |                |           |            |           |            |                    |                 alarm:CORRUPT  |
| http://127.0.0.1:32379 | fd422379fda50e48 | 3.6.0-alpha.0 |           3.6.0 |   25 kB |          25 kB |     false |      false |         4 |         69 |                 69 |  memberID:10501334649042878790 |
|                        |                  |               |                 |         |                |           |            |           |            |                    |                 alarm:CORRUPT  |
+------------------------+------------------+---------------+-----------------+---------+----------------+-----------+------------+-----------+------------+--------------------+--------------------------------+

serathius · 2022-07-26T12:03:42Z

ping @ahrtr

ahrtr

LGTM with a minor comment.

serathius changed the title ~~server: Implementat of compaction hash checking~~ server: Implement compaction hash checking Jun 15, 2022

serathius force-pushed the compact-check branch from fc37c2d to 39e165a Compare June 15, 2022 15:11

serathius marked this pull request as draft June 15, 2022 15:18

serathius force-pushed the compact-check branch from 39e165a to b1a0f1e Compare June 15, 2022 15:19

serathius force-pushed the compact-check branch 2 times, most recently from d72aad2 to baf9952 Compare July 5, 2022 14:18

serathius requested review from ahrtr and ptabor July 5, 2022 15:49

serathius marked this pull request as ready for review July 5, 2022 15:49

ahrtr reviewed Jul 7, 2022

View reviewed changes

server/etcdserver/corrupt.go Show resolved Hide resolved

serathius force-pushed the compact-check branch 2 times, most recently from 4cf993b to a871db1 Compare July 21, 2022 14:54

serathius force-pushed the compact-check branch 2 times, most recently from f8a3f9f to 0dc860e Compare July 21, 2022 15:47

ahrtr reviewed Jul 25, 2022

View reviewed changes

server/embed/config.go Show resolved Hide resolved

ahrtr reviewed Jul 25, 2022

View reviewed changes

serathius force-pushed the compact-check branch from 0dc860e to 1363c51 Compare July 25, 2022 10:08

tests: Rename corruptHash to CorruptBBolt

99b982e

Signed-off-by: Marek Siarkowicz <[email protected]>

serathius force-pushed the compact-check branch from 1363c51 to e1c0bb4 Compare July 25, 2022 11:45

serathius added 2 commits July 25, 2022 13:59

tests: Move CorruptBBolt to testutil

2644982

Signed-off-by: Marek Siarkowicz <[email protected]>

server: Refactor compaction checker

c58ec9f

Signed-off-by: Marek Siarkowicz <[email protected]>

tests: Cover periodic check in tests

f0f750f

Signed-off-by: Marek Siarkowicz <[email protected]>

serathius force-pushed the compact-check branch from e1c0bb4 to 9fb2ebf Compare July 25, 2022 12:01

serathius force-pushed the compact-check branch from 9fb2ebf to 93f27c3 Compare July 25, 2022 12:21

serathius added 2 commits July 26, 2022 09:31

server: Implement compaction hash checking

6697fca

Signed-off-by: Marek Siarkowicz <[email protected]>

server: Make corrtuption check optional and period configurable

d44bbff

Signed-off-by: Marek Siarkowicz <[email protected]>

serathius force-pushed the compact-check branch from 93f27c3 to d44bbff Compare July 26, 2022 07:31

ahrtr reviewed Jul 26, 2022

View reviewed changes

ahrtr approved these changes Jul 26, 2022

View reviewed changes

ahrtr mentioned this pull request Jul 26, 2022

memberID equals zero in corruption alarm #14272

Merged

serathius merged commit a3b410c into etcd-io:main Jul 26, 2022

serathius mentioned this pull request Jul 27, 2022

Fix corruption checks v3.5 #14282

Merged

ahrtr mentioned this pull request Aug 11, 2022

Print clusterID, memberID and leaseID in hexdecimal #14208

Merged

ahrtr mentioned this pull request Nov 22, 2022

tests/integration: deflake Corruption cases #14824

Merged

jmhbnz mentioned this pull request Jun 8, 2023

Ensure etcd cli help flags cannot be missed #16034

Closed

serathius deleted the compact-check branch June 15, 2023 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: Implement compaction hash checking #14120

server: Implement compaction hash checking #14120

serathius commented Jun 15, 2022

serathius commented Jun 15, 2022

codecov-commenter commented Jun 15, 2022 •

edited

Loading

serathius commented Jul 5, 2022

serathius commented Jul 5, 2022

ahrtr Jul 7, 2022

serathius Jul 21, 2022 •

edited

Loading

ahrtr Jul 22, 2022

serathius Jul 22, 2022

ahrtr commented Jul 7, 2022

serathius commented Jul 21, 2022

serathius commented Jul 21, 2022

ahrtr commented Jul 25, 2022

ahrtr Jul 25, 2022

serathius Jul 25, 2022 •

edited

Loading

ahrtr Jul 26, 2022

ahrtr Jul 25, 2022 •

edited

Loading

serathius Jul 25, 2022 •

edited

Loading

serathius commented Jul 25, 2022

ahrtr Jul 26, 2022

serathius Jul 26, 2022 •

edited

Loading

ahrtr Jul 26, 2022

serathius Jul 26, 2022

ahrtr Jul 26, 2022 •

edited

Loading

ahrtr Jul 26, 2022

ahrtr commented Jul 26, 2022

serathius commented Jul 26, 2022

ahrtr left a comment

server: Implement compaction hash checking #14120

server: Implement compaction hash checking #14120

Conversation

serathius commented Jun 15, 2022

serathius commented Jun 15, 2022

codecov-commenter commented Jun 15, 2022 • edited Loading

Codecov Report

serathius commented Jul 5, 2022

serathius commented Jul 5, 2022

Choose a reason for hiding this comment

serathius Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Jul 7, 2022

serathius commented Jul 21, 2022

serathius commented Jul 21, 2022

ahrtr commented Jul 25, 2022

Choose a reason for hiding this comment

serathius Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

serathius Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

serathius commented Jul 25, 2022

Choose a reason for hiding this comment

serathius Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Jul 26, 2022

serathius commented Jul 26, 2022

ahrtr left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 15, 2022 •

edited

Loading

serathius Jul 21, 2022 •

edited

Loading

serathius Jul 25, 2022 •

edited

Loading

ahrtr Jul 25, 2022 •

edited

Loading

serathius Jul 25, 2022 •

edited

Loading

serathius Jul 26, 2022 •

edited

Loading

ahrtr Jul 26, 2022 •

edited

Loading