-
Notifications
You must be signed in to change notification settings - Fork 25
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
Support subcluster names with underscore #362
Conversation
I had a problem with problem with golangci-lint. The current version consumes way to much memory, I had to upgrade to v1.51.1 to run it locally. We may want to change version. Also there are some linter that are deprecated since v1.49.0. |
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.
Just a few minor suggestions
api/v1beta1/verticadb_types.go
Outdated
// GenNameWithoutUnderscore returns the subcluster name after | ||
// replacing all `_` occurrences with `-` | ||
func (s *Subcluster) GenNameWithoutUnderscore() 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.
Suggest a slightly different function name. Something that doesn't expose the internals in case we ever add to it in the future.
// GenNameWithoutUnderscore returns the subcluster name after | |
// replacing all `_` occurrences with `-` | |
func (s *Subcluster) GenNameWithoutUnderscore() string { | |
// GenCompatibleFQDN returns a name of the subcluster that is | |
// compatible inside a fully-qualified domain name. | |
func (s *Subcluster) GenCompatibleFQDN() string { |
api/v1beta1/verticadb_types.go
Outdated
@@ -1049,7 +1049,9 @@ func (v *VerticaDB) GenSubclusterMap() map[string]*Subcluster { | |||
// about its name because it is included in the name of the statefulset, so we | |||
// must adhere to the Kubernetes rules for object names. | |||
func IsValidSubclusterName(scName string) bool { | |||
r := regexp.MustCompile(`^[a-z0-9](?:[a-z0-9\-]{0,61}[a-z0-9])?$`) |
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 suggest that we keep the regex the same. But, we should pass scName through our new Gen function that removes the underscore.
I was aware that some of the linters were deprecated. There is no replacements for them, so I am inclined to keep using them for now. If you want to upgrade the linter, feel free to provide that in a separate PR. |
Before this, a subcluster's name couldn't contain the underscore character because it is part of sts and pod names and according to FQDN rules, an underscore is not a valid character.
This adds better support by simply mapping '_' to a '-' before building pod/service/sts name. This will allow us to support the default subcluster name in vertica – default_subcluster.
The webhook now allows subcluster name with '_'. However, two subclusters could have have identical sts names: e.g. default_subcluster and default-subcluster. We reject that case now in the webhook.