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.

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

dmctl 8.5.1 can not config export from 7.5.1 cluster #12067

Open
bolt-leandro opened this issue Feb 21, 2025 · 10 comments
Open

dmctl 8.5.1 can not config export from 7.5.1 cluster #12067

bolt-leandro opened this issue Feb 21, 2025 · 10 comments
Labels
area/dm Issues or PRs related to DM. type/bug The issue is confirmed as a bug.

Comments

@bolt-leandro
Copy link

What did you do?

We upgraded tiup dmctl from v.7.5.4 to v.8.5.1.

  • dmctl:v7.5.4 can config export from DM cluster running on 7.5.1 without problems
  • dmctl:v8.5.1 gives an error on config export from DM cluster running on 7.5.1
shell> tiup dm display dm-cluster
Starting component dm: /root/.tiup/components/dm/v1.16.1/tiup-dm display dm-cluster
Cluster type:       dm
Cluster name:       dm-cluster
Cluster version:    v7.5.1

shell> tiup dmctl:v7.5.4 --master-addr dm-cluster-dm_master-1.domain:8261 config export
Starting component dmctl: /root/.tiup/components/dmctl/v7.5.4/dmctl/dmctl --master-addr dm-cluster-dm_master-1.domain:8261 config export
export configs to directory `configs` succeed

shell> tiup dmctl:v8.5.1 --master-addr dm-cluster-dm_master-1.domain:8261 config export
Starting component dmctl: /root/.tiup/components/dmctl/v8.5.1/dmctl/dmctl --master-addr dm-cluster-dm_master-1.domain:8261 config export
Error: rpc error: code = Unimplemented desc = unknown method ListSourceConfigs for service pb.Master

What did you expect to see?

A successful backup of configs created:
export configs to directory `configs` succeed

  • dmctl 8.5.1 should be backwards compatible with previous 7.5.x major version

What did you see instead?

An error and no task and source backup files were created.
Error: rpc error: code = Unimplemented desc = unknown method ListSourceConfigs for service pb.Master

Versions of the cluster

DM version (run dmctl -V or dm-worker -V or dm-master -V):

Starting component dmctl: /root/.tiup/components/dmctl/v8.5.1/dmctl/dmctl -V
Release Version: v8.5.1
Git Commit Hash: 0a10d1fae9245c480e31df30f8b13ca4810c3b04
Git Branch: HEAD
UTC Build Time: 2025-01-10 06:50:25
Go Version: go1.23.4
Failpoint Build: false

How did you deploy DM: tiup or manually?

TiUP

current status of DM cluster (execute query-status <task-name> in dmctl)

(paste current status of DM cluster here)
@bolt-leandro bolt-leandro added area/dm Issues or PRs related to DM. type/bug The issue is confirmed as a bug. labels Feb 21, 2025
@dveeden
Copy link
Contributor

dveeden commented Feb 21, 2025

I think this should either work, or there should be a version check when connecting to DM master. And maybe a version matrix in the docs?

@lance6716
Copy link
Contributor

/cc @OliverS929

@OliverS929
Copy link
Contributor

I will take a look.

@OliverS929
Copy link
Contributor

OliverS929 commented Feb 27, 2025

/assign OliverS929

@OliverS929
Copy link
Contributor

OliverS929 commented Feb 27, 2025

This backward incompatibility was introduced in this pull request, which decoupled encryption from dmctl and introduced new RPC calls: Encrypt, ListTaskConfigs, and ListSourceConfigs. As a result, you are encountering the error:

Error: rpc error: code = Unimplemented desc = unknown method ListSourceConfigs for service pb.Master.

However, to my knowledge, dmctl does not explicitly support backward compatibility. dmctl is tightly coupled with DMMaster and DMWorker in each version. As new features are introduced and refactoring takes place, changes to the RPC calls and protobuf structures used between dmctl and DM instances are often required. Therefore, it seems reasonable to assume that dmctl and DM should be aligned to the same version.

Additionally, the functionality provided by dmctl depends on support from DM instances. As such, I don’t foresee many use cases that would justify upgrading dmctl independently of the DM version. I would appreciate it if you could clarify the specific scenarios you have in mind.

However, I agree with @dveeden that we should consider implementing a version check to prevent this type of error, as it could be confusing for users.

/cc @alastori Please take a look and assess whether a version check is necessary for dmctl.

@dveeden
Copy link
Contributor

dveeden commented Feb 27, 2025

I think for people managing multiple DM clusters it would be reasonable to expect these to be the current LTS version and one LTS version below that.

So if you have say 10 DM clusters you upgrade one at a time.
Then either you upgrade dmctl before the upgrade (which requires it to be compatible with n and n-1) or after the upgrade which requires DM master to be compatible with n and n-1.
The benefit of this is that one version of dmctl can be used with all clusters.

And if that doesn't work a 1-to-1 mapping between dmctl and dm master would be ok.

@alastori
Copy link

Given that it is easy to run multiple dmctl versions via TiUP on the same machine, I'll assume it is not very disruptive to specify the target version explicitly (i.e., tiup dmctl:v7.5.4). Please let me know if this assumption is incorrect for your use case.

Must-have: Version check & clear error message

I agree that we must implement a version check in dmctl and provide a clear, actionable error message when there is a version mismatch. The error should:

  1. Explicitly state the version mismatch instead of failing with an unclear error.
  2. Suggest the correct dmctl version and usage based on the detected DM cluster version.

This will help prevent confusion and silent issues.

Specific concern in cases of errors before connection

I'm not sure if dmctl can perform the version check early enough before encountering protocol-related errors. In the case discussed here, it seems we can do it (before ListSourceConfigs). If not, at the very least, we should:

  • Improve pre-version-check error messages where possible.
  • Document known errors due to compatibility issues clearly so users can reference them easily.

LTS compatibility discussion

I acknowledge @dveeden's point that dmctl, as a client, should ideally support v(LTS) and v(LTS-1) for smoother multi-cluster management. However, due to dmctl's tight coupling with DM internals and dependencies, maintaining backward compatibility would require significant effort, which may not always be justified if we can avoid confusion with clear error messages and when the corrective action (specifying the correct version) is straightforward.

Let me know your thoughts.

@dveeden
Copy link
Contributor

dveeden commented Feb 28, 2025

@alastori I 100% agree with that

@bolt-leandro
Copy link
Author

The reason we were expecting backwards compatibility is that in all our previous upgrades, dmctl was able to talk to LTS and LTS-1 versions. We were caught off guard with 8.5.1 not having such capability. Curiously, dmctl 7.5.4 can talk to LTS, LTS-1 and LTS+1. This means that we don't have any logic in our "config export" tool to cross check dmctcl->dm-master versions. Going forward we will have to introduce version checking.

In summary, I agree with @alastori:

  • Improve pre-version-check error messages where possible.
  • Document known errors due to compatibility issues clearly so users can reference them easily.
  • ideally support v(LTS) and v(LTS-1)

@OliverS929
Copy link
Contributor

Thank you for all the feedback—the scenarios described above are very insightful. Ideally, it would be beneficial to support version checks and backward compatibility.

For version checks, we could introduce a dedicated RPC call before any existing RPC communication between dmctl and DM instances to retrieve and validate their versions. Without this, we wouldn’t have control over the response behavior, as changes to the RPC interface between versions could lead to unpredictable results. However, implementing this would require changes to how dmctl executes commands and interacts with DM instances, so it’s not something we can introduce instantly.

For backward compatibility, we would need a structured versioning mechanism for RPC interfaces, in addition to the version validation mentioned above. This mechanism would have to be carefully designed, including a deprecation strategy to phase out outdated interface versions while minimizing legacy code accumulation. Implementing this would require dedicated planning and substantial effort.

I’ll conduct a deeper investigation and evaluation of this issue to determine the best path forward. I’ll provide further updates as I gather more information.

/cc @alastori

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dm Issues or PRs related to DM. type/bug The issue is confirmed as a bug.
Projects
None yet
Development

No branches or pull requests

5 participants