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

fix: update graphite e2e test to generate empty metric #2954

Merged
merged 2 commits into from
May 2, 2022

Conversation

JorTurFer
Copy link
Member

@JorTurFer JorTurFer commented Apr 30, 2022

Signed-off-by: Jorge Turrado jorge_turrado@hotmail.es

After this PR the graphite e2e test is broken (sorry, I forgot to trigger e2e tests). Even though the behaviour is correct, our e2e test doesn't update the metric to 0, only stops sending it (in my really short experience with graphite, it's not the real scenario).
The e2e test is based on the "null as zero" behaviour, and that's why it fails.

This PR adds another job to continue registering the metric with 0 value, simulating the real behaviour to allow the scale in and fix the e2e test.

Am I right @bpinske ?

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #2944

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer JorTurFer requested a review from a team as a code owner April 30, 2022 10:43
@JorTurFer
Copy link
Member Author

JorTurFer commented Apr 30, 2022

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

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@bpinske
Copy link
Contributor

bpinske commented Apr 30, 2022

Signed-off-by: Jorge Turrado jorge_turrado@hotmail.es

After this PR the graphite e2e test is broken (sorry, I forgot to trigger e2e tests). Even though the behaviour is correct, our e2e test doesn't update the metric to 0, only stops sending it (in my really short experience with graphite, it's not the real scenario). The e2e test is based on the "null as zero" behaviour, and that's why it fails.

This PR adds another job to continue registering the metric with 0 value, simulating the real behaviour to allow the scale in and fix the e2e test.

Am I right @bpinske ?

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #2944

Oops on breaking e2e.
Yes, you're right. Explicitly sending zeros will allow everything to scale down properly again.

@tomkerkhove tomkerkhove merged commit 865cc9e into kedacore:main May 2, 2022
@JorTurFer JorTurFer deleted the fix-grafite-scaler branch May 2, 2022 09:46
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.

Graphite Scaler doesn't properly handle null responses
3 participants