Add unmanaged tablet flag at vttablet level

[beingnoble03]

Description

Adds a --unmanaged flag for vttablet, which Indicates an unmanaged tablet, i.e. using an external mysql-compatible database. It will help add both better visibility and user experience while setting up unmanaged tablets.

Related Issue(s)

• Fixes: vttablet: better visibility for unmanaged tablets #12092

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

[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.

[beingnoble03]

@GuptaManan100 can you please have a look?

[GuptaManan100]

Do we have end to end tests for unmanaged tablets @rohit-nayak-ps? If we do, we should add this flag there to make sure that the validations are correct.

We also need to add summary changes.

[rohit-nayak-ps]

Do we have end to end tests for unmanaged tablets @rohit-nayak-ps? If we do, we should add this flag there to make sure that the validations are correct.

We don't explicitly setup unmanaged tablets in e2e tests currently. @beingnoble03, how did you test these changes?

[rohit-nayak-ps]

Thanks for this, @beingnoble03 . I wanted to note that this PR doesn't fix #12092 entirely. These have not been addressed:

* Add validations to check that incompatible flags are not defined
* Display this in relevant vttablet APIs and expose in /debug/vars

@deepthi, one of the reasons that issue was originally created was, iirc, that incompatible flags were being specified when vttablets were started which did not apply and were causing either errors or unexpected behavior. (I forget the details). If we have (or can enumerate) the list of such flags, we can add it to the validations added in this PR.

Aside:
We also need to update/refresh the unmanaged tablet documentation. We don't provide a proper motivation and examples for the use of unmanaged tablets as far as I can find. Also https://vitess.io/docs/18.0/user-guides/configuration-advanced/unmanaged-tablet/ is still referring to the --init_populate_metadata flag.

[beingnoble03]

Do we have end to end tests for unmanaged tablets @rohit-nayak-ps? If we do, we should add this flag there to make sure that the validations are correct.

We don't explicitly setup unmanaged tablets in e2e tests currently. @beingnoble03, how did you test these changes?

@rohit-nayak-ps thanks for the review. I changed examples/common/scripts/vttablet-up.sh script accordingly (adding --unmanaged flag and testing some of the validations I've added) to test the changes and running the 101_initial_cluster.sh script.

[beingnoble03]

Thanks for this, @beingnoble03 . I wanted to note that this PR doesn't fix #12092 entirely. These have not been addressed:

* Add validations to check that incompatible flags are not defined
* Display this in relevant vttablet APIs and expose in /debug/vars

@deepthi, one of the reasons that issue was originally created was, iirc, that incompatible flags were being specified when vttablets were started which did not apply and were causing either errors or unexpected behavior. (I forget the details). If we have (or can enumerate) the list of such flags, we can add it to the validations added in this PR.

Aside: We also need to update/refresh the unmanaged tablet documentation. We don't provide a proper motivation and examples for the use of unmanaged tablets as far as I can find. Also https://vitess.io/docs/18.0/user-guides/configuration-advanced/unmanaged-tablet/ is still referring to the --init_populate_metadata flag.

Yes, This PR needs to address these points too. I added some validations which I thought were necessary, but if we can enumerate a list of incompatible flags, I can proceed further adding checks for these flags.

I think we can update the docs, in a separate follow-up PR.

[GuptaManan100]

Apologies it took a while for me to review this PR, I had some other more urgent things to work on.

@deepthi Can you think of other validations that we want to make behind this --unmanaged flag? I looked at the doc https://vitess.io/docs/18.0/user-guides/configuration-advanced/unmanaged-tablet/ and couldn't find anything else.

That doc itself feels a lot out of date, because it still mentions init_populate_metadata which I think doesn't exist anymore in vttablets.

[beingnoble03]

@deepthi @GuptaManan100 is there any other validation which we need to include here?

[codecov]

Codecov Report

Attention: Patch coverage is 81.81818% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 65.64%. Comparing base (f889134) to head (e957a3b).

Files	Patch %	Lines
go/vt/mysqlctl/mysqld.go	50.00%	3 Missing ⚠️
go/vt/vttablet/tabletserver/tabletenv/config.go	88.88%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14871      +/-   ##
==========================================
- Coverage   65.65%   65.64%   -0.01%     
==========================================
  Files        1563     1563              
  Lines      194395   194427      +32     
==========================================
+ Hits       127627   127632       +5     
- Misses      66768    66795      +27     

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

[GuptaManan100]

Rest looks good to me, we would need to add summary changes for the same for 20.0.0. @deepthi Do you have any other change in mind?

[ajm188]

@GuptaManan100 we should also add some unit tests for verifyUnmanagedTabletConfig

[beingnoble03]

@GuptaManan100 @ajm188 added required tests for verifyUnmanagedTabletConfig

[deepthi]

This PR itself looks mostly fine, except we may need to add some more validations.

[deepthi]

People can still not specify this, and just specify the existing set of flags, correct?
We should make this language stronger, and say that we recommend setting this flag so that we can validate all the related flags.

[deepthi]

I was thinking about this again, and I believe we should deprecate --disable_active_reparents along with adding this flag.
For backwards compatibility, we should deprecate --disable_active_reparents in v20, and delete it in v21.
The internal value of mysqlctl.DisableActiveReparents should be the OR of --unmanaged and --disable_active_reparents in v20, and just the value of --unmanaged in v21.

[GuptaManan100]

disable_active_reparents is used in vtctldclient as well to prevent that specific instance from running reparent commands. I don't think we can get rid of that. We could rename the flag on vtctld too?

[deepthi]

It should be fine to leave that flag on vtctld. Having the same flag name on vttablet and vtctld was always confusing. I'm not sure if the two are sharing code which might make this separation more complicated, though.

[GuptaManan100]

Done 😎 I made the required changes and deprecated the flag only on the required binaries.

[deepthi]

We do need to update the website docs for Unmanaged tablet, so I added that label back

[deepthi]

I was thinking about this again, and I believe we should deprecate --disable_active_reparents along with adding this flag.
For backwards compatibility, we should deprecate --disable_active_reparents in v20, and delete it in v21.
The internal value of mysqlctl.DisableActiveReparents should be the OR of --unmanaged and --disable_active_reparents in v20, and just the value of --unmanaged in v21.

[deepthi]

LGTM. Only thing left is website documentation.

[GuptaManan100]

I'll do the website docs update by EOD.

Update - Here is the docs PR - vitessio/website#1702