-
Notifications
You must be signed in to change notification settings - Fork 75
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
Add support for user-defined per-keyspace images #524
Conversation
Signed-off-by: Max Englander <[email protected]>
fb0c294
to
831f35a
Compare
Signed-off-by: Max Englander <[email protected]>
This is a good enhancement, thank you @maxenglander ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it! Makes sense to me. I only had some minor comments.
mariadb103Compatible: | ||
type: string | ||
mariadbCompatible: | ||
type: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we can remove these from all of the CRDs on main now, but for now I think we can NOT propagate them here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, done
docs/api/index.html
Outdated
<p>Images are not customizable by users at the keyspace level because version | ||
skew across the cluster is discouraged except during rolling updates, | ||
in which case this field is automatically managed by the VitessCluster | ||
controller that owns this VitessKeyspace.</p> | ||
<p>Images are inherited from the VitessCluster spec, unless the user has | ||
specified keyspace-level overrides. Version skew across the cluster is | ||
discouraged except during rolling updates, in which case this field is | ||
automatically managed by the VitessCluster controller that owns this | ||
VitessKeyspace.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the version here was more about Vitess than MySQL. I think that we're now focusing on MySQL version rather than Vitess version, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Yes indeed, just focusing on MySQL.
Hm..with that observation in mind, do you think it would make sense to scope this PR to only allow MySQL image overrides?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead an removed support for user-definable per-keyspace images for vttablet, vtorc, myqsld exporter along w/ removing support for setting mariadb images.
…nts about version skew Signed-off-by: Max Englander <[email protected]>
Description
Currently the way to upgrade from mysql 57 to 80 in the operator is to change
mysqld
image version. This causes operator to do a rolling upgrade of tablets. There's a lot that could go wrong here, and it would be nice if there were an option to do a controlled migration from 57 to 80.One way this could work in theory is to create a new keyspace configured with mysql 80, and do a
MoveTables
from the 57 keyspace to the 80. This would allow carefully testing and validating that the upgrade succeeded before doingSwitchTraffic
, and in theory would allow switching back to 57 if issues were discovered in 80 afterSwitchTraffic
.This PR allows the user to specify per-keyspace images, relaxing this prohibition:
vitess-operator/docs/api.md
Lines 5280 to 5283 in 5bf4279
Issues
Fixes: #446
Validation
I validated the change by deploying a local build to a local
kind
cluster, and deploying aVitessCluster
with one keyspace inheriting images from the cluster spec, and the other using user-defined overrides.kind.yaml
vitesscluster.yaml
Results