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

Correctly parse cell names with dashes in tablet aliases #8167

Merged
merged 1 commit into from
May 28, 2021

Conversation

hkdsun
Copy link
Member

@hkdsun hkdsun commented May 21, 2021

Signed-off-by: Hormoz Kheradmand hormoz.kheradmand@shopify.com

Description

Closes #6041

vttablet fails to parse tablet-aliases with more than one - in the cell name.

This is especially problematic because cells are often named after GCP/AWS zones, which are often valid DNS-1123 labels (consists of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character. see similar validation in the kubernetes/kubernetes repository)

Since this parser has been out in the wild for a while, I am not sure how strict we want to make the format. I chose to include the capital alphabetic characters in addition to ., _, and of course -.

I am happy to change the expected format to be any more or any less strict, as maintainers see fit.

Checklist

  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

There are chances of invalid cell names out in the wild the more strict we make the expected format.

@deepthi @gedgar @harshit-gangal
cc @jeremycole @acharis

go/vt/topo/topoproto/tablet.go Outdated Show resolved Hide resolved
go/vt/topo/topoproto/tablet.go Outdated Show resolved Hide resolved
go/vt/topo/topoproto/tablet.go Outdated Show resolved Hide resolved
@rafael
Copy link
Member

rafael commented May 24, 2021

Hello @hkdsun , @jeremycole ! Good to see you in Vitess land 🎉

How cell is inferred in tablets is something that has bugged for a long time. Thank you for fixing this. In my view, this will work and after addressing that outstanding comments we should proceed with the approach proposed here.

Having said this, there is an alternative approach that we could consider that I would like us to consider. All other vitess components are explicit about which cell they are registering to. I can't think of a good reason why that shouldn't be the case here as well. Curious if @deepthi or @sougou remember something that I might be missing. If we can make it explicit, I think we can do something as follows:

  1. Add two flags to the tablet (-cell and -uid).
  2. If those attributes are not set, default to existent behavior.
  3. If they are set, they take precedence and we don't have to rely on processing the tablet_path to infer what portion is the cell and which one is the uid.
  4. Eventually deprecate tablet_path.

In the long term, this might be a better approach. Of course, that will increase the scope of what you are actually trying to fix. It will not only mean updating o tablet, but other places where this assumption is also being made (e.g some of the vtctld operation like reparents).

I think we can merge this, but I would be curious of people thoughts on the alternate approach.

Signed-off-by: Hormoz Kheradmand <hormoz.kheradmand@shopify.com>
@hkdsun
Copy link
Member Author

hkdsun commented May 27, 2021

Thanks for the comments everyone and the excellent context, Rafael.

Using separate -uid and -cell arguments makes a lot of sense to me, though I can only commit to this simple fix at the moment for the stage of the project we are in – happy to reconsider in the future.

I would still love to have this PR be merged if we agree on it as a good short-term fix.

@deepthi
Copy link
Member

deepthi commented May 28, 2021

Thanks for the comments everyone and the excellent context, Rafael.

Using separate -uid and -cell arguments makes a lot of sense to me, though I can only commit to this simple fix at the moment for the stage of the project we are in – happy to reconsider in the future.

I would still love to have this PR be merged if we agree on it as a good short-term fix.

I like the long-term approach proposed by @rafael but understand your position too. We can merge this as-is and revisit later.

@deepthi deepthi merged commit 2b0c8fd into vitessio:master May 28, 2021
@rafael
Copy link
Member

rafael commented Jun 1, 2021

I will create an issue to keep track of the long term fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vttablet fails to parse cell names with dashes
5 participants