CI upgrade/downgrade tests for Online DDL / throttler / vreplication flow #16017

shlomi-noach · 2024-05-27T13:22:00Z

Description

Adding upgrade/downgrade tests as follows:

The tests will run an X version VTTablet primary, with Y VTTablet replica. See breakdown below.
Enable tablet throttler.
Create and populate some table.
Create a load. The load takes throttling into account so that it does not overwhelm the cluster.
Run Online DDL with vitess strategy, thereby running VReplication that is also using the throttler.
Throttle and unthrottle the migration.
Validate migration behavior while under load. Complete the migration.
Expect successful completion of the migration, with cut-over as graceful as possible (but will resort to forced cut-over if unable to complete in a timely fashion).

See more details in https://github.com/vitessio/vitess/pull/16017/files#diff-dc6e3889488266071984d6da1f44795b7b06bf5ff6504c8e9aa359598918230aR17-R37

This will test cross version compatibility of the throttler, of vreplication, of Online DDL. It runs onlineddl_flow in the following setups:

Primary at version N (current) and replica at N-1
Primary at version N-1 and replica at N
Primary at version N+1 and replica at N
Primary at version N and replica at N+1

Each such flow runs at about 1 minute, which is why I chose to aggregate them all in the same workflow test (as opposed to splitting to "new" and "old")

EDIT: sample execution flow: https://github.com/vitessio/vitess/actions/runs/9348746367/job/25728525330?pr=16017

Related Issue(s)

No specific issue, but with #15988 in mind.

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…ctionality) Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

vitess-bot · 2024-05-27T13:22:03Z

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…ve more chance, terminate the workload and expect it to compelete Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…ironment variable values to vttablet and mysqlctl binary paths Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

codecov · 2024-06-03T09:56:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.22%. Comparing base (faa2e2e) to head (79256c7).
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16017      +/-   ##
==========================================
- Coverage   68.23%   68.22%   -0.02%     
==========================================
  Files        1541     1541              
  Lines      197234   197330      +96     
==========================================
+ Hits       134592   134634      +42     
- Misses      62642    62696      +54

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2024-06-03T10:45:17Z

So weird. The workflow onlineddl_flow doesn't run now. It ran just fine when I was testing with the rest of the workflows removed (e.g. 42692c2), but now that I returned all the original workflows, I don't see it on the CI checks list.

shlomi-noach · 2024-06-03T10:47:37Z

OK its just reappeared. Fine.

shlomi-noach · 2024-06-03T10:52:16Z

Sample execution flow: https://github.com/vitessio/vitess/actions/runs/9348746367/job/25728525330?pr=16017

frouioui

Some nits, otherwise it looks good to me. We will have to remember to make this new workflow required on main once we merge the PR.

frouioui · 2024-06-03T14:44:07Z

.github/workflows/upgrade_downgrade_test_onlineddl_flow.yml

+    - name: Check for changes in relevant files
+      if: steps.skip-workflow.outputs.skip-workflow == 'false'
+      uses: dorny/paths-filter@v3.0.1
+      id: changes
+      with:
+        token: ''
+        filters: |
+          end_to_end:
+            - 'go/**'
+            - 'go/**/*.go'
+            - 'test.go'
+            - 'Makefile'
+            - 'build.env'
+            - 'go.sum'
+            - 'go.mod'
+            - 'proto/*.proto'
+            - 'tools/**'
+            - 'config/**'
+            - 'bootstrap.sh'
+            - '.github/workflows/upgrade_downgrade_test_onlineddl_flow.yml'


nit: this can be moved two steps up, after checking out

.github/workflows/upgrade_downgrade_test_onlineddl_flow.yml

go/test/endtoend/onlineddl/flow/onlineddl_flow_test.go

.github/workflows/upgrade_downgrade_test_onlineddl_flow.yml

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Co-authored-by: Florent Poinsard <35779988+frouioui@users.noreply.github.com> Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

mattlord

Thank you for working on this! ❤️

mattlord · 2024-06-03T21:49:53Z

.github/workflows/upgrade_downgrade_test_onlineddl_flow.yml

+        mkdir -p /tmp/vitess-build-current/
+        cp -R bin /tmp/vitess-build-current/
+
+    # Swap the binaries. Use vtctl version n and keep vttablet at version n-1


This should be checking the vtctldclient version rather than vtctl.

Oh we don't use vtctl and that comment is leftover.

mattlord · 2024-06-03T21:50:06Z

.github/workflows/upgrade_downgrade_test_onlineddl_flow.yml

+        cp /tmp/vitess-build-next/bin/vttablet $PWD/bin/vttablet-next
+        cp /tmp/vitess-build-next/bin/mysqlctl $PWD/bin/mysqlctl-next
+        cp /tmp/vitess-build-next/bin/mysqlctld $PWD/bin/mysqlctld-next
+        vtctl --version


Same here, should be vtctldclient --version. The tests are using vtctldclient (cluster.VtctldClientProcess) so not a big deal.

Should actually be vttablet, this is again an oversight. Fixing in next push.

mattlord · 2024-06-03T21:56:13Z

go/test/endtoend/onlineddl/flow/onlineddl_flow_test.go

+		t.Logf("Using REPLICA_TABLET_BINARY_SUFFIX: %s", binarySuffix)
+	}
+
+	shards = clusterInstance.Keyspaces[0].Shards


Probably worth a require call before here too:

require.Greater(t, len(clusterInstance.Keyspaces), 0)

mattlord · 2024-06-03T21:59:59Z

go/test/endtoend/onlineddl/flow/onlineddl_flow_test.go

+			t.Run("additional wait", func(t *testing.T) {
+				select {
+				case <-time.After(3 * time.Second):
+				case <-ctx.Done():
+					require.Fail(t, "context cancelled")
+				}
+			})


Curious what we're waiting for here? If nothing else, it will give us a clue if this becomes flaky. Are we waiting for the schema migration to reach the ready-to-complete stage? If we're just waiting for load generation then it's fine. Although we could also wait for the rows in a table to be greater than X. We have helpers for that too.

Waiting just so that we generate more DMLs, and give migration/vreplication more "opportunities" to throttle or to make progress. Adding clarification.

mattlord · 2024-06-03T22:08:25Z

go/test/endtoend/onlineddl/flow/onlineddl_flow_test.go

+				select {
+				case <-time.After(10 * time.Second):
+				case <-ctx.Done():
+					require.Fail(t, "context cancelled")
+				}
+
+				status := onlineddl.WaitForMigrationStatus(t, &vtParams, shards, uuid, migrationWaitTimeout, schema.OnlineDDLStatusRunning, schema.OnlineDDLStatusComplete)


I'm curious why in this spot and similar ones we're not instead just adding 10 seconds to the migrationWaitTimeout we pass in?

Huh. You're right and this is not needed. Removing altogether.

mattlord · 2024-06-03T22:12:28Z

go/test/endtoend/onlineddl/flow/onlineddl_flow_test.go

+	row := onlineddl.VtgateExecDDL(t, &vtParams, ddlStrategy, alterStatement, "").Named().Row()
+	if row != nil {
+		uuid = row.AsString("uuid", "")
+	}


Should we not use require/assert to fail here if we don't get a row and/or uuid?

Yes. This is copy+paste from other tests where a nil result is in fact possible (where we expect an error in submission).

mattlord · 2024-06-03T22:15:42Z

go/test/endtoend/onlineddl/flow/onlineddl_flow_test.go

+				if expected {
+					// if migration is ready to complete, the timestamp should be non-null
+					assert.False(t, row["ready_to_complete_timestamp"].IsNull())
+				} else {
+					assert.True(t, row["ready_to_complete_timestamp"].IsNull())
+				}


Seems like we could condense this to:

assert.Equal(t, expected, row["ready_to_complete_timestamp"].IsNull())

It's the negation actually (IsNull() should be !expected) which is why I broke it into explicit if-else, as I find negation so confusing sometimes.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2024-06-04T06:15:42Z

We wanted this to be in time for v20 RC1 code freeze, but did not make it. Seeing that this is a tests-only PR, it complies with code freeze guidelines. There is an advantage to having this in release-20.0 branch and so once merged we will backport this PR to both release-20.0 and release-20.0-rc.

…flow (vitessio#16017) Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Co-authored-by: Florent Poinsard <35779988+frouioui@users.noreply.github.com>

shlomi-noach added 2 commits May 27, 2024 16:17

upgrade_downgrade_test_onlineddl_old.yml (with reparent functionality)

b0cedb0

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

isolating upgrade_downgrade_test_onlineddl_old.yml (with reparent fun…

e861c70

…ctionality) Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach added Component: Build/CI Component: VReplication Type: CI/Build Component: Online DDL Online DDL (vitess/native/gh-ost/pt-osc) Component: Throttler labels May 27, 2024

github-actions bot added this to the v20.0.0 milestone May 27, 2024

shlomi-noach added 17 commits May 30, 2024 07:43

disable VTADMIN builds

4272a93

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

initial endtoend test, still single versioned

615d436

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

validate applied DML is at sufficient quantities

5727a3b

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Describe purpose of test

80c53a7

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

create and use 'upgrade_downgrade_onlineddl_flow' tag

b0cdddc

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

stricted lag threshold

357ed1a

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

give more chance for the migration to complete, force cut-over and gi…

6bf9fe1

…ve more chance, terminate the workload and expect it to compelete Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

remvoe debug info

f51d0ad

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

use correct file name

ddea1ad

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

build both last and next versions

13c5a2d

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

add PRIMARY_TABLET_BINARY_SUFFIX and REPLICA_TABLET_BINARY_SUFFIX env…

1a226de

…ironment variable values to vttablet and mysqlctl binary paths Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Populate PRIMARY_TABLET_BINARY_SUFFIX and REPLICA_TABLET_BINARY_SUFFIX

3be74e8

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

rename CI workflow to 'upgrade_downgrade_test_onlineddl_flow'

1f77101

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

fix file name

78a8d79

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

increase timeout

7516ea1

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

step description

c0ecf21

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

resolved conflict

1a8b81d

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach requested a review from a team June 3, 2024 09:43

shlomi-noach added 2 commits June 3, 2024 13:02

code comment

f663903

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

step naming

3abe3cd

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

frouioui approved these changes Jun 3, 2024

View reviewed changes

frouioui reviewed Jun 3, 2024

View reviewed changes

.github/workflows/upgrade_downgrade_test_onlineddl_flow.yml Outdated Show resolved Hide resolved

shlomi-noach and others added 4 commits June 3, 2024 17:57

potentially skip test

0759d68

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Update go/test/endtoend/onlineddl/flow/onlineddl_flow_test.go

a1e64ab

Co-authored-by: Florent Poinsard <35779988+frouioui@users.noreply.github.com> Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

remove SKIP_VTADMIN

ab2f1f9

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

reorder steps

17407d2

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach mentioned this pull request Jun 3, 2024

Release of v20.0.0-RC1 #16010

Closed

49 tasks

mattlord approved these changes Jun 3, 2024

View reviewed changes

shlomi-noach added 6 commits June 4, 2024 07:48

fix comment

b56a9dc

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

better version check for all vttablet binaries

364f58c

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

verify keyspaces is not empty

dbba616

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

clarifying wait

082efe4

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

remove excessive wait

aa18f6d

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

validate uuid

79256c7

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach merged commit 2df3545 into vitessio:main Jun 5, 2024
93 checks passed

shlomi-noach deleted the ci-upgrade-downgrade-onlineddl branch June 5, 2024 03:58

shlomi-noach added Backport to: release-20.0-rc Backport to: release-20.0 Needs to be backport to release-20.0 labels Jun 5, 2024

This was referenced Jun 5, 2024

v20 backport: CI upgrade/downgrade tests for Online DDL / throttler / vreplication flow #16065

Merged

Online DDL flow CI: Update golang version to 1.22.4 #16066

Merged

shlomi-noach mentioned this pull request Jun 17, 2024

CI: wait-for rather than 'assume' in Online DDL flow #16210

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI upgrade/downgrade tests for Online DDL / throttler / vreplication flow #16017

CI upgrade/downgrade tests for Online DDL / throttler / vreplication flow #16017

shlomi-noach commented May 27, 2024 •

edited

Loading

vitess-bot bot commented May 27, 2024

codecov bot commented Jun 3, 2024 •

edited

Loading

shlomi-noach commented Jun 3, 2024 •

edited

Loading

shlomi-noach commented Jun 3, 2024

shlomi-noach commented Jun 3, 2024

frouioui left a comment

frouioui Jun 3, 2024

shlomi-noach Jun 3, 2024

mattlord left a comment

mattlord Jun 3, 2024

shlomi-noach Jun 4, 2024

shlomi-noach Jun 4, 2024

mattlord Jun 3, 2024

shlomi-noach Jun 4, 2024

shlomi-noach Jun 4, 2024

mattlord Jun 3, 2024

shlomi-noach Jun 4, 2024

mattlord Jun 3, 2024

shlomi-noach Jun 4, 2024

shlomi-noach Jun 4, 2024

mattlord Jun 3, 2024

shlomi-noach Jun 4, 2024

shlomi-noach Jun 4, 2024

mattlord Jun 3, 2024

shlomi-noach Jun 4, 2024

shlomi-noach Jun 4, 2024

mattlord Jun 3, 2024

shlomi-noach Jun 4, 2024

shlomi-noach commented Jun 4, 2024

CI upgrade/downgrade tests for Online DDL / throttler / vreplication flow #16017

CI upgrade/downgrade tests for Online DDL / throttler / vreplication flow #16017

Conversation

shlomi-noach commented May 27, 2024 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented May 27, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Jun 3, 2024 • edited Loading

Codecov Report

shlomi-noach commented Jun 3, 2024 • edited Loading

shlomi-noach commented Jun 3, 2024

shlomi-noach commented Jun 3, 2024

frouioui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattlord left a comment

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

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

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

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

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

shlomi-noach commented Jun 4, 2024

shlomi-noach commented May 27, 2024 •

edited

Loading

codecov bot commented Jun 3, 2024 •

edited

Loading

shlomi-noach commented Jun 3, 2024 •

edited

Loading