Skip to content

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

@JorTurFer JorTurFer requested a review from a team as a code owner January 8, 2024 22:19
@github-actions
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 <[email protected]>
.
Signed-off-by: Jorge Turrado <[email protected]>
@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
@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
@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 <[email protected]>

* Add error on failed leader search

Signed-off-by: Jorge Turrado <[email protected]>

* update changelog

Signed-off-by: Jorge Turrado <[email protected]>

* .

Signed-off-by: Jorge Turrado <[email protected]>

---------

Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: anton.lysina <[email protected]>
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.

3 participants