[3.5] etcdserver: backport check scheduledCompactKeyName and finishedCompac… #16068

CaojiamingAlan · 2023-06-12T22:12:55Z

…tKeyName before writing hash to release-3.5.

Follow up of #15985

Fix #15919.
Check ScheduledCompactKeyName and FinishedCompactKeyName before writing hash to hashstore.
If they do not match, then it means this compaction has once been interrupted and its hash value is invalid. In such cases, we won't write the hash values to the hashstore, and avoids the incorrect corruption alarm.

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

chaochn47 · 2023-06-13T18:06:05Z

server/mvcc/kvstore.go

+// checkPrevCompactionCompleted checks whether the previous scheduled compaction is completed.
+func (s *store) checkPrevCompactionCompleted() bool {
+	tx := s.b.ReadTx()
+	tx.Lock()


Just curious why not use tx.RLock() but understand the PR is a backport of #15985

Lock() for ReadTx() should be by default a Rlock().

For my understanding, do you mean the following RWMutex Lock is equal to RLock to protect the read buffer?

etcd/server/storage/backend/read_tx.go

Lines 127 to 130 in 5e40a8b

func (rt *readTx) Lock() { rt.mu.Lock() }

func (rt *readTx) Unlock() { rt.mu.Unlock() }

func (rt *readTx) RLock() { rt.mu.RLock() }

func (rt *readTx) RUnlock() { rt.mu.RUnlock() }

After investigating a little bit, I think you are right, we should use RLock() instead. Lock() should only be used when the read buffer is updated by the writes committed by the BatchTxs. However, there are several positions where use read-only operations but Lock() is used. I think it is good to open a separate PR to correct them all.

Please correct me if I'm wrong.

Yep. That aligns with my understanding.

I also don't think it blocks this PR to be merged. This can be corrected separately.

Old implementation uses stricter lock, which is sub-optimal, but is not a bug.

stricter lock is true, it's also wrong lock. It's a bug to me.

It should also have impact on performance for Non-K8s use case when auth is enabled. [Of course, it will be better if we have some performance comparison, but I will NOT force contributor to do it given it's a straightforward change]

To ensure release stability, we need to assume that the list you proposed can have a mistake.

That's why we need careful review. I wouldn't be afraid backporting it give it's a simple change and if we have confidence.

Note I prefer to backporting it, but I am not insist on backporting it if there is strong objection from other maintainers.

stricter lock is true, it's also wrong lock. It's a bug to me.

Without the change system behaves correctly, with the change only thing you get is performance improvement. How this is a bug?

I would say it's harmless (on functionality) bug, obviously the lock isn't correctly be used. Let alone it has impact on performance.

Bugs is when locks don't protect the critical section, relaxing locks is performance improvement. Nothing is harmless until proven, let alone concurrency change.

tests/e2e/corrupt_test.go

jmhbnz

LGTM

ahrtr · 2023-07-14T12:15:24Z

server/mvcc/kvstore.go

+	_, scheduledCompactBytes := tx.UnsafeRange(buckets.Meta, scheduledCompactKeyName, nil, 0)
+	scheduledCompact := int64(0)
+	if len(scheduledCompactBytes) != 0 {
+		scheduledCompact = bytesToRev(scheduledCompactBytes[0]).main
+	}
+
+	_, finishedCompactBytes := tx.UnsafeRange(buckets.Meta, finishedCompactKeyName, nil, 0)
+	finishedCompact := int64(0)
+	if len(finishedCompactBytes) != 0 {
+		finishedCompact = bytesToRev(finishedCompactBytes[0]).main
+
+	}


Please add a UnsafeReadScheduledCompact and UnsafeReadFinishedCompact in server/mvcc/util.go for 3.5.

The two functions should be reused in multiple places, (Please search scheduledCompactKeyName and finishedCompactKeyName in the code base)

It's accepted to fix it in a separate PR. Please also raise a followup ticket.

ahrtr · 2023-07-14T12:17:01Z

Please raise two followup ticket as mentioned in #16068 (comment) and #16068 (comment).

Pls also rebase this PR.

…tKeyName before writing hash to release-3.5. Fix etcd-io#15919. Check ScheduledCompactKeyName and FinishedCompactKeyName before writing hash to hashstore. If they do not match, then it means this compaction has once been interrupted and its hash value is invalid. In such cases, we won't write the hash values to the hashstore, and avoids the incorrect corruption alarm. Signed-off-by: caojiamingalan <[email protected]>

Replace unnecessary Lock()/Unlock()s with RLock()/RUnlock()s Signed-off-by: caojiamingalan <[email protected]>

CaojiamingAlan · 2023-07-15T04:44:39Z

Just raised #16247. I will solve #16068 (comment) real quick, so no need for another PR.

ahrtr

LGTM

thx @CaojiamingAlan

Add a UnsafeReadScheduledCompact and UnsafeReadFinishedCompact Signed-off-by: caojiamingalan <[email protected]>

serathius approved these changes Jun 13, 2023

View reviewed changes

chaochn47 reviewed Jun 13, 2023

View reviewed changes

chaochn47 approved these changes Jun 13, 2023

View reviewed changes

jmhbnz reviewed Jun 13, 2023

View reviewed changes

tests/e2e/corrupt_test.go Show resolved Hide resolved

jmhbnz approved these changes Jun 13, 2023

View reviewed changes

ahrtr changed the title ~~etcdserver: backport check scheduledCompactKeyName and finishedCompac…~~ [3.5] etcdserver: backport check scheduledCompactKeyName and finishedCompac… Jul 14, 2023

ahrtr reviewed Jul 14, 2023

View reviewed changes

CaojiamingAlan force-pushed the release-3.5 branch from f4b2f7a to 6ac9d94 Compare July 15, 2023 00:25

CaojiamingAlan mentioned this pull request Jul 15, 2023

Replace unnecessary Lock()/Unlock()s with RLock()/RUnlock()s #16247

Open

3 tasks

CaojiamingAlan added a commit to CaojiamingAlan/etcd that referenced this pull request Jul 15, 2023

Follow up etcd-io#16068 (comment)

bc97a94

Replace unnecessary Lock()/Unlock()s with RLock()/RUnlock()s Signed-off-by: caojiamingalan <[email protected]>

CaojiamingAlan mentioned this pull request Jul 15, 2023

Replace unnecessary Lock()/Unlock()s with RLock()/RUnlock()s #16248

Merged

ahrtr approved these changes Jul 15, 2023

View reviewed changes

ahrtr merged commit 8f4b6c9 into etcd-io:release-3.5 Jul 15, 2023

CaojiamingAlan deleted the release-3.5 branch July 17, 2023 18:47

CaojiamingAlan added a commit to CaojiamingAlan/etcd that referenced this pull request Jul 17, 2023

Follow up etcd-io#16068 (comment)

dd78032

Add a UnsafeReadScheduledCompact and UnsafeReadFinishedCompact Signed-off-by: caojiamingalan <[email protected]>

CaojiamingAlan added a commit to CaojiamingAlan/etcd that referenced this pull request Jul 17, 2023

Follow up etcd-io#16068 (comment)

5c7e802

Add a UnsafeReadScheduledCompact and UnsafeReadFinishedCompact Signed-off-by: caojiamingalan <[email protected]>

CaojiamingAlan mentioned this pull request Jul 17, 2023

[3.5] Add UnsafeReadScheduledCompact and UnsafeReadFinishedCompact #16262

Merged

CaojiamingAlan added a commit to CaojiamingAlan/etcd that referenced this pull request Jul 18, 2023

Follow up etcd-io#16068 (comment)

eb9bfaa

Add a UnsafeReadScheduledCompact and UnsafeReadFinishedCompact Signed-off-by: caojiamingalan <[email protected]>

This was referenced Jul 24, 2023

[3.5]Replace unnecessary Lock()/Unlock()s with RLock()/RUnlock()s #16297

Open

[3.4]Replace unnecessary Lock()/Unlock()s with RLock()/RUnlock()s #16298

Closed

serathius mentioned this pull request Oct 12, 2023

Plan release v3.5.10 #16733

Closed

This was referenced Oct 31, 2023

NO-ISSUE: rebase-main-4.15.0-0.nightly-2023-10-30-224022_amd64-2023-10-30_arm64-2023-10-30 openshift/microshift#2533

Merged

NO-ISSUE: rebase-4.14.0-0.nightly-2023-11-02-185339_amd64-2023-11-02_arm64-2023-11-02 openshift/microshift#2550

Merged

jmhbnz mentioned this pull request Aug 11, 2024

[3.5] Backport github/workflows: set read-only default permissions to approve workflow #18429

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3.5] etcdserver: backport check scheduledCompactKeyName and finishedCompac… #16068

[3.5] etcdserver: backport check scheduledCompactKeyName and finishedCompac… #16068

CaojiamingAlan commented Jun 12, 2023 •

edited

Loading

chaochn47 Jun 13, 2023

serathius Jun 13, 2023

chaochn47 Jun 13, 2023

CaojiamingAlan Jun 13, 2023 •

edited

Loading

chaochn47 Jun 13, 2023

ahrtr Jul 25, 2023

ahrtr Jul 25, 2023

serathius Jul 25, 2023 •

edited

Loading

ahrtr Jul 25, 2023

serathius Jul 25, 2023

jmhbnz left a comment

ahrtr Jul 14, 2023

ahrtr commented Jul 14, 2023

CaojiamingAlan commented Jul 15, 2023

ahrtr left a comment

	func (rt *readTx) Lock() { rt.mu.Lock() }
	func (rt *readTx) Unlock() { rt.mu.Unlock() }
	func (rt *readTx) RLock() { rt.mu.RLock() }
	func (rt *readTx) RUnlock() { rt.mu.RUnlock() }

[3.5] etcdserver: backport check scheduledCompactKeyName and finishedCompac… #16068

[3.5] etcdserver: backport check scheduledCompactKeyName and finishedCompac… #16068

Conversation

CaojiamingAlan commented Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaojiamingAlan Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serathius Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmhbnz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Jul 14, 2023

CaojiamingAlan commented Jul 15, 2023

ahrtr left a comment

Choose a reason for hiding this comment

CaojiamingAlan commented Jun 12, 2023 •

edited

Loading

CaojiamingAlan Jun 13, 2023 •

edited

Loading

serathius Jul 25, 2023 •

edited

Loading