vtadmin onlineddl endpoints

[ajm188]

Description

This hooks up all the vtadmin API endpoints to make use of vtctld's schema migration management endpoints which were added in the previous version.

Quick demo for sanity:

➜  vitess git:(andrew/vtadmin-onlineddl) ✗ vtctldclient --server ":15999" ApplySchema commerce --sql "alter table customer add column first_name varchar(255)" --ddl-strategy online
22bcf2fa_c13b_11ee_89ab_2e0cac6950df
➜  vitess git:(andrew/vtadmin-onlineddl) ✗ curl localhost:14200/api/migrations/
{"result":{"schema_migrations":[{"cluster":{"id":"local","name":"local"},"schema_migration":{"uuid":"22bcf2fa_c13b_11ee_89ab_2e0cac6950df","keyspace":"commerce","shard":"0","schema":"vt_commerce","table":"customer","migration_statement":"alter table customer add column first_name varchar(255)","added_at":{"seconds":1706817022},"requested_at":{"seconds":1706817023},"started_at":{"seconds":1706817024},"liveness_timestamp":{"seconds":1706817024},"status":5,"artifacts":"_22bcf2fa_c13b_11ee_89ab_2e0cac6950df_20240201145023_vrepl,","tablet":{"cell":"zone1","uid":101},"progress":100,"migration_context":"vtctl:22b6ef36-c13b-11ee-89ab-2e0cac6950df","ddl_action":"alter","artifact_retention":{"seconds":86400},"vitess_liveness_indicator":1706817023,"reviewed_at":{"seconds":1706817024}}}]},"ok":true}%  

Related Issue(s)

Closes #15113

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

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[codecov]

Codecov Report

Attention: 144 lines in your changes are missing coverage. Please review.

Comparison is base (eddb39e) 47.29% compared to head (77fd6bf) 70.61%.
Report is 108 commits behind head on main.

Files	Patch %	Lines
go/vt/mysqlctl/builtinbackupengine.go	12.90%	54 Missing ⚠️
go/vt/mysqlctl/backupengine.go	0.00%	20 Missing ⚠️
go/vt/mysqlctl/xtrabackupengine.go	0.00%	15 Missing ⚠️
go/vt/topo/keyspace.go	78.72%	10 Missing ⚠️
go/mysql/conn.go	53.84%	6 Missing ⚠️
go/vt/mysqlctl/backup.go	37.50%	5 Missing ⚠️
go/mysql/capabilities/capability.go	91.48%	4 Missing ⚠️
go/mysql/query.go	77.77%	4 Missing ⚠️
go/vt/mysqlctl/schema.go	72.72%	3 Missing ⚠️
go/vt/schemadiff/column.go	50.00%	3 Missing ⚠️
... and 15 more

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #15114       +/-   ##
===========================================
+ Coverage   47.29%   70.61%   +23.31%     
===========================================
  Files        1137     1377      +240     
  Lines      238684   182789    -55895     
===========================================
+ Hits       112895   129075    +16180     
+ Misses     117168    53714    -63454     
+ Partials     8621        0     -8621     

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

[mattlord]

Other than the 1 noted typo (I'm assuming), this looks good to me. I think you're planning to push more code though?

[ajm188]

I think you're planning to push more code though?

yeah i'll be pushing up unit tests today! then we should be good to go

[ajm188]

bump on this, i'd like it to make it into #15136

[mattlord]

Hopefully my comments make sense. I was trying to review just the commits added since my previous review commit by commit.

[mattlord]

IMO it's worth a safety check that requestsByCluster[c.ID] is not nil — even though it should not be. But we don't want to crash vtadmin if something odd happens, right? Perhaps we're recovering elsewhere though.

[ajm188]

good call

as to your second question, we do have error recovery in both the http and grpc stacks:

• grpc
• http

[mattlord]

Any reason not to use an errgroup? Seems like it would be a perfect fit -- making the code simpler to read and ending earlier.

[ajm188]

the main difference between errgroup and this pattern is the following (emphasis mine):

Wait blocks until all function calls from the Go method have returned, then returns the first non-nil error (if any) from them.

we would be losing information by switching, as we currently collect all errors. and (as i understand it) errgroup doesn't actually terminate other goroutines early if one returns an error, it just limits the amount of error information you get, so it doesn't end up ending earlier

[mattlord]

Yeah, it doesn't forcefully end anything AFAIUI — it cancels the context (assuming errgroup.WithContext() was used) so it should cause things to end earlier assuming that the errgroup's context is used by calls within the errgroup's Go function AND that the code executed in the goroutines check for context cancellation. Even so, the point about losing error information is valid. I was just curious -- the current code is fine.

[mattlord]

My only remaining valid comments/suggestions were minor, so will approve and you can address those as you feel is best.

[notfelineit]

The interfaces look good to me, pending @mattlord 's comments!