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(jetstream): Raise an error if leader not found and added v2.10 e2e coverage #5358

Merged
merged 6 commits into from Jan 9, 2024

Conversation

JorTurFer
Copy link
Member

@JorTurFer JorTurFer commented Jan 8, 2024

Fixing this issue generated by NATS arch changes, we added a logic that iterates over all the nodes of the cluster looking for the leader, but if the leader isn't found KEDA doesn't raise any error and silently fails.

This PR revisit that logic adding an error in that case, and also removing an unnecessary iteration over a repeated element (current leader is already inside the cluster_uls). As an extra point, I've added explicit coverage to NATS JetStream v2.10 as they introduced the changes of the issue, enforcing the usage of the headless service.

Checklist

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

Relates to #4524

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@JorTurFer JorTurFer requested a review from a team as a code owner January 8, 2024 22:19
Copy link

github-actions bot commented Jan 8, 2024

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

While you are waiting, make sure to:

Learn more about:

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
.
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@JorTurFer
Copy link
Member Author

JorTurFer commented Jan 8, 2024

/run-e2e jetstream
Update: You can check the progress here

@JorTurFer JorTurFer enabled auto-merge (squash) January 8, 2024 22:25
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@JorTurFer
Copy link
Member Author

JorTurFer commented Jan 8, 2024

/run-e2e jetstream
Update: You can check the progress here

Copy link
Member

@wozniakjan wozniakjan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

just mind the unit test failure, not sure if it's a flake or related

    nats_jetstream_scaler_test.go:311: Expected success for 'All Good - messages waiting (clustered)' but got error leader node not found for consumer pull_consumer

@JorTurFer
Copy link
Member Author

I've triggered it again and it's worked, so maybe it's been any kind of transient error. Just in case, I'm going to check it again to prevent flaky tests

@JorTurFer
Copy link
Member Author

JorTurFer commented Jan 9, 2024

/run-e2e jetstream
Update: You can check the progress here

@JorTurFer JorTurFer merged commit c5fa2ba into kedacore:main Jan 9, 2024
19 checks passed
@JorTurFer JorTurFer deleted the fix-jetstream branch January 9, 2024 16:17
toniiiik pushed a commit to toniiiik/keda that referenced this pull request Jan 15, 2024
…e coverage (kedacore#5358)

* fix(jetstream): Scaler doesn't print multiple (wrong) errors

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* Add error on failed leader search

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* update changelog

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

* .

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

---------

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: anton.lysina <alysina@gmail.com>
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.

None yet

3 participants