Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RFC: Separate Component Version in Cluster #2010

Merged
merged 5 commits into from
Nov 17, 2023
Merged

RFC: Separate Component Version in Cluster #2010

merged 5 commits into from
Nov 17, 2023

Conversation

nexustar
Copy link
Collaborator

@nexustar nexustar commented Aug 7, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change
  • Has persistent data change

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Release notes:

NONE

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot requested review from breezewish and lonng August 7, 2022 08:05
@ti-chi-bot ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 7, 2022
breezewish
breezewish reviewed Aug 17, 2022
View reviewed changes
doc/rfcs/0001-separate-component-version-in-cluster.md

## Motivation

- New component TiDB Dashboard need to deployed with TiDB but not release with TiDB. Need a way to specify Dashboard version when deployment and upgrade.
Copy link
Member

@breezewish breezewish Aug 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect whether TiDB Dashboard is really capable of a standalone version.

This means it is possible that user deploy a TiDB Dashboard with a very old TiDB, for example, TiDB 4.0 with TiDB Dashboard master. I guess this will just make TiDB Dashboard not working at all. It would also be a disaster to make sure that latest TiDB Dashboard is compatible with all existing TiDB cluster versions.

I don't think PM have considered this defect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think use different component for TiDB cluster should not be considered by TiUP PM. Because TiDB cannot ensure different versions can work well together. We should not do this if no one can guarantee the behaviors of a TiDB cluster containing the different versions of components.

Implementing this feature is very easy and the hard part is the different version compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There comes already a demand that TiDB-Dashboard want's to release standalone versions different with TiDB's, so we as a framework need to be prepared with such usage cases.

I do agree we should only enable this config for certain components, at this time, maybe only dashboard, in the Specification definitions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think use different component for TiDB cluster should not be considered by TiUP PM. Because TiDB cannot ensure different versions can work well together. We should not do this if no one can guarantee the behaviors of a TiDB cluster containing the different versions of components.

Implementing this feature is very easy and the hard part is the different version compatibility.

tiup cluster is not only used in prod environment. I think we cloud only allow user to specify tidb-dashboard version by default. And add a flag like --allow-separate-vertion that allow user to specify version of any component to test or patch component.

AstroProfundis
AstroProfundis reviewed Aug 18, 2022
View reviewed changes
doc/rfcs/0001-separate-component-version-in-cluster.md Outdated

4. if user not specify version, those version has default value latest which means use newest stable version in tiup repo.

5. maybe add version field to all other component, deafult value is empty which means use global cluster version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As comments above, we should only add component specified version fields to those claims to work with other components on different versions.

Copy link
Member

@breezewish breezewish Aug 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe some version constraint is also needed. For example, TiDB Dashboard vX.X requires TiDB >= vX.X and certain constraint break should not be allowed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe some version constraint is also needed. For example, TiDB Dashboard vX.X requires TiDB >= vX.X and certain constraint break should not be allowed.

maybe we do not need it? if latest tidb-dashboard always support all exist tidb (>=6.2.0). just leave "latest" alisas as tidb-dashboard version. If user upgrade tidb cluster, tidb-dashboard upgrade to latest.If user do not upgrade tidb cluster, tidb-dashboard remains old version. We cloud just to warning user not to upgrade single component if he don't know what will happen.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is easy to add something like new dashboard do not support old tidb, but not easy to add information like old dashboard do not support new tidb

@nexustar
Copy link
Collaborator Author

On TiDB v6.3.0, we will just make TiDB-Dashboard use the same version as TiDB. This RFC will not be implement recently.

@kaaaaaaang kaaaaaaang added this to the 1.13.0 milestone Aug 14, 2023
@ti-chi-bot ti-chi-bot bot added lgtm and removed lgtm labels Aug 25, 2023
@ti-chi-bot ti-chi-bot bot added the lgtm label Aug 25, 2023
@kaaaaaaang kaaaaaaang modified the milestones: 1.13.0, v1.14.0 Aug 25, 2023
@ti-chi-bot ti-chi-bot bot removed the lgtm label Nov 17, 2023
@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 17, 2023

[LGTM Timeline notifier]

Timeline:

  • 2023-08-25 09:36:15.102532553 +0000 UTC m=+1487739.651548538: ☑️ agreed by kaaaaaaang.
  • 2023-08-25 10:32:59.522909404 +0000 UTC m=+1491144.071925388: ✖️🔁 reset by kaaaaaaang.
  • 2023-08-25 13:10:16.956991749 +0000 UTC m=+1500581.506007736: ☑️ agreed by kaaaaaaang.
  • 2023-11-17 08:46:39.264507562 +0000 UTC m=+4411596.851617705: ✖️🔁 reset by nexustar.

@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 17, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kaaaaaaang for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 17, 2023

New changes are detected. LGTM label has been removed.

@kaaaaaaang kaaaaaaang merged commit 7dfc35b into pingcap:master Nov 17, 2023
8 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@breezewish breezewish breezewish left review comments

@AstroProfundis AstroProfundis AstroProfundis left review comments

@kaaaaaaang kaaaaaaang kaaaaaaang approved these changes

@lonng lonng Awaiting requested review from lonng

Assignees
No one assigned
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.14.0
Development

Successfully merging this pull request may close these issues.
7 participants
@nexustar @ti-chi-bot @AstroProfundis @lonng @breezewish @kaaaaaaang @srstack