Bug fix: Use target tablet from health stats cache when checking replication status #14436

austenLacy · 2023-11-02T15:42:38Z

Description

When determining which tablets to show in show vitess_replication_status Vitess is excluding tablets if their states were changed. This now uses the target tablet from the heath check cache which represents the expected state of the tablets.

Related Issue(s)

Fixes: Bug Report: Changing a tablet's type to REPLICA does not update results of SHOW vitess_replication_status #14432

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Did the new or modified tests pass consistently locally and on the CI
Documentation was added or is not required

Deployment Notes

…tatus Signed-off-by: Austen Lacy <[email protected]>

vitess-bot · 2023-11-02T15:42:40Z

…blets changing type Signed-off-by: Austen Lacy <[email protected]>

mattlord

Looks good. Thanks, @austenLacy ! ❤️

I only had a couple of minor comments -- let me know what you think. Have you already run the new test locally in a loop to check for flakiness?

I can come back to this quickly when I hear back.

go/test/endtoend/cluster/cluster_process.go

mattlord · 2023-11-02T21:18:26Z

go/test/endtoend/tabletgateway/vtgate_test.go

+func TestVtgateReplicationStatusCheckWithTabletTypeChange(t *testing.T) {
+	defer cluster.PanicHandler(t)
+	// Healthcheck interval on tablet is set to 1s, so sleep for 2s
+	time.Sleep(2 * time.Second)


This is likely to be flaky in the CI due to unpredictable performance and occasional machine pauses. Any reason not to use the variable value (if we can access it) * 10 or something?

I copied this from the other tests in this file. I can try running the test in a loop to see if it has any flakiness.

I ran it 10 times in a loop 2-3 times and didn't see any flakiness.

for i in {1..10}; do go test -count=1 -timeout 30s -run ^TestVtgateReplicationStatusCheckWithTabletTypeChange$ vitess.io/vitess/go/test/endtoend/tabletgateway; sleep 1; done ok vitess.io/vitess/go/test/endtoend/tabletgateway 23.902s ok vitess.io/vitess/go/test/endtoend/tabletgateway 25.247s ok vitess.io/vitess/go/test/endtoend/tabletgateway 23.130s ok vitess.io/vitess/go/test/endtoend/tabletgateway 23.091s ok vitess.io/vitess/go/test/endtoend/tabletgateway 25.394s ok vitess.io/vitess/go/test/endtoend/tabletgateway 25.882s ok vitess.io/vitess/go/test/endtoend/tabletgateway 23.308s ok vitess.io/vitess/go/test/endtoend/tabletgateway 23.246s ok vitess.io/vitess/go/test/endtoend/tabletgateway 25.528s ok vitess.io/vitess/go/test/endtoend/tabletgateway 25.797s

mattlord

Found one more issue that I think we should address after having looked at other usage of TabletsStats.

@deepthi could you please review this when you have time? No rush, but you know the healthcheck cache related code the best. Thanks!

mattlord · 2023-11-03T05:02:37Z

go/vt/vtgate/executor.go

@@ -901,7 +901,7 @@ func (e *Executor) showVitessReplicationStatus(ctx context.Context, filter *sqlp
 	for _, s := range status {
 		for _, ts := range s.TabletsStats {
 			// We only want to show REPLICA and RDONLY tablets
-			if ts.Tablet.Type != topodatapb.TabletType_REPLICA && ts.Tablet.Type != topodatapb.TabletType_RDONLY {
+			if ts.Target.TabletType != topodatapb.TabletType_REPLICA && ts.Target.TabletType != topodatapb.TabletType_RDONLY {


I did confirm that Target.TabletType is what's being used elsewhere when examining/showing the type for tablets in the healthcheck cache. So this part seems right.

Following on from that, I think that we should change all usage of ts.Tablet.* in this function to ts.Target.*. For example, below we should change ts.Tablet.Keyspace to ts.Target.Keyspace and ts.Tablet.Shard to ts.Target.Shard.

…r#showVitessReplicationStatus Signed-off-by: Austen Lacy <[email protected]>

mattlord

Thanks, @austenLacy ! ❤️

deepthi

Nice work on tests!

deepthi · 2023-11-03T18:41:23Z

go/vt/vtgate/executor.go

@@ -901,14 +901,14 @@ func (e *Executor) showVitessReplicationStatus(ctx context.Context, filter *sqlp
 	for _, s := range status {
 		for _, ts := range s.TabletsStats {
 			// We only want to show REPLICA and RDONLY tablets
-			if ts.Tablet.Type != topodatapb.TabletType_REPLICA && ts.Tablet.Type != topodatapb.TabletType_RDONLY {
+			if ts.Target.TabletType != topodatapb.TabletType_REPLICA && ts.Target.TabletType != topodatapb.TabletType_RDONLY {


It's actually worrisome that these two fields are not in sync. That is something we'll need to go back and review. Doesn't need to block this PR though.

…ication status (#14436) Signed-off-by: Austen Lacy <[email protected]> Co-authored-by: Austen Lacy <[email protected]>

…ication status (vitessio#14436) Signed-off-by: Austen Lacy <[email protected]> Co-authored-by: Austen Lacy <[email protected]>

…ication status (vitessio#14436) (#128) Signed-off-by: Austen Lacy <[email protected]> Co-authored-by: Austen Lacy <[email protected]>

…n checking replication status (#14436) (#14456) Signed-off-by: Austen Lacy <[email protected]> Signed-off-by: Harshit Gangal <[email protected]> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com> Co-authored-by: Austen Lacy <[email protected]> Co-authored-by: Harshit Gangal <[email protected]>

…ication status (vitessio#14436) (#128) Signed-off-by: Austen Lacy <[email protected]> Co-authored-by: Austen Lacy <[email protected]>

…ication status (vitessio#14436) (#128) Signed-off-by: Austen Lacy <[email protected]> Co-authored-by: Austen Lacy <[email protected]> (cherry picked from commit f757ff2)

…ication status (vitessio#14436) (#128) Signed-off-by: Austen Lacy <[email protected]> Co-authored-by: Austen Lacy <[email protected]>

…ication status (vitessio#14436) (#128) Signed-off-by: Austen Lacy <[email protected]> Co-authored-by: Austen Lacy <[email protected]> (cherry picked from commit f757ff2)

…ication status (vitessio#14436) (#128) Signed-off-by: Austen Lacy <[email protected]> Co-authored-by: Austen Lacy <[email protected]> (cherry picked from commit f757ff2) (cherry picked from commit 7fabb2d)

…ication status (vitessio#14436) (#128) Signed-off-by: Austen Lacy <[email protected]> Co-authored-by: Austen Lacy <[email protected]> (cherry picked from commit f757ff2)

…ication status (vitessio#14436) (#128) Signed-off-by: Austen Lacy <[email protected]> Co-authored-by: Austen Lacy <[email protected]> (cherry picked from commit f757ff2) (cherry picked from commit 7fabb2d) (cherry picked from commit a5cc396)

use target tablet from health stats cache when checking replication s…

e8211df

…tatus Signed-off-by: Austen Lacy <[email protected]>

austenLacy requested review from harshit-gangal, systay, frouioui and GuptaManan100 as code owners November 2, 2023 15:42

vitess-bot bot added NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Nov 2, 2023

github-actions bot added this to the v19.0.0 milestone Nov 2, 2023

austenLacy requested a review from deepthi as a code owner November 2, 2023 19:03

added test in vtgate for show vitess_replication_status to support ta…

f4920fe

…blets changing type Signed-off-by: Austen Lacy <[email protected]>

austenLacy force-pushed the austenlacy/fix-show-vitess-replication-status-bug branch from 126dd17 to f4920fe Compare November 2, 2023 20:24

mattlord reviewed Nov 2, 2023

View reviewed changes

mattlord reviewed Nov 3, 2023

View reviewed changes

simplify VtctlclientChangeTabletType and use target tablet in executo…

d7a4665

…r#showVitessReplicationStatus Signed-off-by: Austen Lacy <[email protected]>

mattlord approved these changes Nov 3, 2023

View reviewed changes

deepthi approved these changes Nov 3, 2023

View reviewed changes

deepthi reviewed Nov 3, 2023

View reviewed changes

deepthi merged commit 366f7d2 into vitessio:main Nov 3, 2023
115 checks passed

vitess-bot bot mentioned this pull request Nov 3, 2023

[release-18.0] Bug fix: Use target tablet from health stats cache when checking replication status (#14436) #14456

Merged

vitess-bot pushed a commit that referenced this pull request Nov 3, 2023

Bug fix: Use target tablet from health stats cache when checking repl…

87ff3a1

…ication status (#14436) Signed-off-by: Austen Lacy <[email protected]> Co-authored-by: Austen Lacy <[email protected]>

austenLacy deleted the austenlacy/fix-show-vitess-replication-status-bug branch November 3, 2023 20:08

austenLacy mentioned this pull request Nov 6, 2023

[BACK-PORT] Bug fix: Use target tablet from health stats cache when checking replication status (https://github.com/vitessio/vitess/pull/14436) Shopify/vitess#128

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fix: Use target tablet from health stats cache when checking replication status #14436

Bug fix: Use target tablet from health stats cache when checking replication status #14436

austenLacy commented Nov 2, 2023 •

edited by mattlord

Loading

vitess-bot bot commented Nov 2, 2023

mattlord left a comment

mattlord Nov 2, 2023 •

edited

Loading

austenLacy Nov 3, 2023

austenLacy Nov 3, 2023

mattlord left a comment

mattlord Nov 3, 2023

mattlord left a comment

deepthi left a comment

deepthi Nov 3, 2023

Bug fix: Use target tablet from health stats cache when checking replication status #14436

Bug fix: Use target tablet from health stats cache when checking replication status #14436

Conversation

austenLacy commented Nov 2, 2023 • edited by mattlord Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Nov 2, 2023

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

mattlord left a comment

Choose a reason for hiding this comment

mattlord Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

austenLacy Nov 3, 2023

Choose a reason for hiding this comment

austenLacy Nov 3, 2023

Choose a reason for hiding this comment

mattlord left a comment

Choose a reason for hiding this comment

mattlord Nov 3, 2023

Choose a reason for hiding this comment

mattlord left a comment

Choose a reason for hiding this comment

deepthi left a comment

Choose a reason for hiding this comment

deepthi Nov 3, 2023

Choose a reason for hiding this comment

austenLacy commented Nov 2, 2023 •

edited by mattlord

Loading

mattlord Nov 2, 2023 •

edited

Loading