-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
WIP - fix: trim so name when is longer than 63 chars #2995
Conversation
Signed-off-by: Jorge Turrado <[email protected]>
/run-e2e |
Signed-off-by: Jorge Turrado <[email protected]>
How do you resolve it when |
In than case there is a conflict and the hpa will not be created. We can remove the extra characters from the beginning or from the middle, but there are cases for every situation. |
Or we could also remove the odd characters or something like that 🤔 |
Or we assign a guid internally and use that but I guess that defeats the discoverability for our end-users - unless it's surfaced in our k get scaledobject |
You are right, it's another option, but I agree that the user experience will be horrible in that case. The main problem is that it's not only an internal thing (like label selectors), the HPA name is generated based on SO Name and it has a length constraint AFAIK |
As mentioned, this could introdue conflict when there will be a couple of SO with a similar long name. Also the label set on ScaledObject is being used to identify correct SO that is needed by HPA controller to drive the scaling. We should design some robust solution (probably introducing some uids), but I think that we don't have to block 2.7 on this and postpone it to 2.8 |
Agreed - I think unique IDs are the only solid approach for this which we can surface in our end-user experience. |
I agree too. Let's talk about this with calm for v2.8. |
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 for sake of process
labels := map[string]string{ | ||
"app.kubernetes.io/name": labelName, | ||
"app.kubernetes.io/version": version.Version, | ||
"app.kubernetes.io/part-of": scaledObject.Name, | ||
"app.kubernetes.io/part-of": kedacontrollerutil.Trim(scaledObject.Name, 63), |
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.
As discussed, we need a more robust solution
Another approach I've seen is if the string is over the limit (ex. >63), cut it down to a smaller number (63-7=56) and add a hash of the full original value ( Doesn't guarantee there won't be a collision, but it's extremely unlikely. |
Agree, I have been thinking about something like this. We need to evaluate all the options and select the best one. We still have a time for 2.8 |
/run-e2e |
Signed-off-by: Jorge Turrado [email protected]
If the SO Name is longer than 63 characters, KEDA fails because that length is valid for the SO Name but not for other resources where it's used. This PR trims the SO Name when is longer than 63 characters on every place where is needed
Checklist
Fixes #2915