Add ParseNumeric and replace uses of ParseInt/Float

[isugimpy]

Signed-off-by: Jayme Howard [email protected]

This is a potential implementation of a solution for the problem I described in #2299 with YAML conversion causing problematic behavior. The ParseNumeric function should be close to a drop-in replacement for strconv.Parse{Int,Float}, but with the ability to parse strings with a type hint that I've described in the issue. As an example, d(1.0) would hint at a decimal (float64) value to be returned. I did not make this change to any other scalers as I'm unsure that this change will be accepted as is and didn't want to make the investment. This should, practically speaking, retain backwards compatibility with all existing scaling behavior, as far as I can tell from my tests.

If the maintainers consider this acceptable, I can go through and update other things that currently use Parse{Int,Float} to use this function, update the changelog, and will open a PR to update the docs as well.

Checklist

• [X] Commits are signed with Developer Certificate of Origin (DCO - learn more)
• [X] Tests have been added
• [ ] A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
• [ ] A PR is opened to update the documentation on (repo) (if applicable)
• [ ] Changelog has been updated

Fixes #2299

[isugimpy]

I don't think I have the ability to fix this last one, as it's a timeout on your linting steps.

[JorTurFer]

BTW, Thanks a ton for the contribution ❤️

[isugimpy]

BTW, Thanks a ton for the contribution heart

Glad to do it. Getting this solved would be a huge win for me and my team, so I've got a vested interest in making sure it gets done. If I can take a little burden off y'all and make it happen, well worth the time investment to me.

[JorTurFer]

Hi, Sorry for the slow response, the weeks previous x-mas are crazy... I think that I'm still missing something :( Your problem is that the automated tools that you use automatically converts for example "1" into 1, so the ScaledObject fails during to apply because of the type, right? Your proposal basically is to add support to specify "1" in this other way "d(1)" in order to avoid automatic conversions using other tools during the process. Am I right? In that case, shouldn't this change be applied in all scalers?

Personally, I'm not totally sure about this notation but in base of the tests we can still use the old one at the same time and we provide a fallback way for these automatizations 🤔 Maybe explaining it in a section inside the docs? WDTY @zroubalik , @tomkerkhove @ahmelsayed ?

[isugimpy]

Apologies, I may not have explained it as well as I had previously thought.

When you're using anything that parses the YAML and outputs it again, with Kustomize being the best example, the object gets loaded in, parsed to native Go objects, and eventually output back as YAML. Now, let's say we start with a ScaledObject with quoted numeric values like "1". The YAML gets read, and a string with the contents 1 is created. At output time, what goes into the resulting output is a 1 with no quotes, because YAML implicitly treats strings as quoted. When the next thing goes to read the YAML, it sees an unquoted value which "looks" like a numeric, and parses it as a number, storing it as an integer. That integer no longer meets the OpenAPIv3 spec for the CRD, and the apply fails because of it.

There are two ways to solve this. One would be to rewrite the spec to allow either a numeric or string value, and adjust everything accordingly. The other would be the approach above, where type hinting is provided in the string. Because the value doesn't "look" like a numeric, a YAML parser will always treat it as a string. This provides a nice fallback mechanism for users who do use tools like Kustomize (or other tools which work the same way), allowing a mechanism to provide a value which clearly doesn't "look" like a number, while maintaining compatibility with the existing schema and behavior.

I agree this should go into other scalers. In fact, I'm willing to do the work to do so if that's desired. But I didn't want to make the change significantly larger and touch the other scalers without having gotten buy-in from maintainers first. Also very happy to write the relevant docs, but again didn't want to go down that road without ensuring that there would be buy-in first.

[isugimpy]

@arschles Thank you for the feedback! Both addressed as requested. Hopefully that error message provides what you're looking for there.

[isugimpy]

Just following up on this. I know it's hard to get time around stuff like this especially during the holidays, but I want to make sure it doesn't get lost in the shuffle.

[arschles]

My approval review stands, but looks like the TestCPUMemoryParseMetadata test is failing currently, due to a nil pointer exception...

[isugimpy]

You're sure right, sorry about that. I saw what looked like a different failure and completely missed that my own changes were failing tests. I've written a fix that should solve the problem, and found a linting error I had previously missed.

I can't seem to get the full test suite to run locally, but the tests for the code I touched pass on my machine at this point.

As previously offered, I'll still be glad to go through the other scalers I haven't touched and convert references from ParseInt/ParseFloat to ParseNumeric if this work is accepted.

[isugimpy]

@zroubalik Apologies, time got away from me and I just realized I left this hanging this whole time. I don't know if this would satisfy what you're wanting, but I made a small change to one of the existing e2e tests which should exercise the behavior of parsing with the hint. Existing tests would use the hintless behavior already. As far as I know that should work transparently.

However, I can't make the e2e actually run because of the labels on the PR.

[zroubalik]

@isugimpy sorry for the delay, that e2e test has been migrated to go in the meantime. Do you think you can modify it? Also please don't change the original test, rather extend it to cover this new scenario.

I have been thinking about this for some time (sorry for that 😅 ), and I think that we can go ahead with this change.

[tomkerkhove]

Any update on this?

[tomkerkhove]

Closing due to inactivity for a long period, feel free to re-open if you have time to complete it anyway.