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

WIP - fix: trim so name when is longer than 63 chars #2995

Closed
wants to merge 4 commits into from

Conversation

JorTurFer
Copy link
Member

@JorTurFer JorTurFer commented May 3, 2022

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

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • Changelog has been updated and is aligned with our changelog requirements

Fixes #2915

@JorTurFer JorTurFer requested a review from a team as a code owner May 3, 2022 23:31
@JorTurFer
Copy link
Member Author

JorTurFer commented May 3, 2022

/run-e2e
Update: You can check the progres here

Signed-off-by: Jorge Turrado <[email protected]>
@tomkerkhove
Copy link
Member

How do you resolve it when verylongnaaa[..]aame-1 & verylongnaaa[..]aame-2 are 65 chars long?

@JorTurFer
Copy link
Member Author

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.
Other option to simplify this is just throw an error if the scaled object is longer than IDK, 55 characters (remember that the add things to the so name in some cases so we extend the length)

@JorTurFer
Copy link
Member Author

Or we could also remove the odd characters or something like that 🤔

@tomkerkhove
Copy link
Member

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

@JorTurFer
Copy link
Member Author

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

@zroubalik
Copy link
Member

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

@tomkerkhove
Copy link
Member

Agreed - I think unique IDs are the only solid approach for this which we can surface in our end-user experience.

@JorTurFer
Copy link
Member Author

JorTurFer commented May 4, 2022

I agree too. Let's talk about this with calm for v2.8.
I keep this PR open as starting point ,when we decided the approach, we only need to update the rows changed in this PR and we already have the e2e test, 😄

@JorTurFer JorTurFer changed the title fix: trim so name when is longer than 63 chars WIP - fix: trim so name when is longer than 63 chars May 4, 2022
Copy link
Member

@tomkerkhove tomkerkhove left a 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),
Copy link
Member

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

@rwkarg
Copy link
Contributor

rwkarg commented May 4, 2022

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 (- plus a 6 character hash).
So long-value-...-the-rest-of-the-thing becomes long-value-...-the-re-39f5a2.

Doesn't guarantee there won't be a collision, but it's extremely unlikely.

@zroubalik
Copy link
Member

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

@JorTurFer
Copy link
Member Author

JorTurFer commented May 9, 2022

/run-e2e
Update: You can check the progres here

@JorTurFer JorTurFer closed this May 9, 2022
@JorTurFer JorTurFer deleted the fix-error-hpa branch July 19, 2022 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HPA is not created for deployments with long names
4 participants