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

Rework Kafka health check #23020

Merged
merged 3 commits into from
May 3, 2024

Conversation

nasark
Copy link
Member

@nasark nasark commented May 2, 2024

  • The push/pop approach for Kafka health checks is not ideal because this can lead to timing issues when dealing with multiple consumers (multiple appliances). Therefore using a simple call to retrieve metadata from the Kafka server, this will fail if the server is down which is ideal for a health check
  • Kafka client connections should be closed to prevent issues with multiple consumers on the same topic

Depends on:

@miq-bot assign @agrare
@miq-bot add_reviewer @agrare
@miq-bot add_labels enhancement, bug

@nasark
Copy link
Member Author

nasark commented May 2, 2024

We will need to bump manageiq-messaging once ManageIQ/manageiq-messaging#88 is merged and a new version is released

@nasark nasark force-pushed the get_metadata_for_kafka_health_check branch from d95e9fb to 45546fb Compare May 3, 2024 16:03
Comment on lines 630 to 631
# Fail health check if list of topics can't be retrieved
broker.topics
Copy link
Member Author

Choose a reason for hiding this comment

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

@agrare Should we log on success here? The push/pop approach had logging baked in by the messaging client

Copy link
Member

Choose a reason for hiding this comment

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

I think we're okay to just log on failure

rescue => err
_log.error("Messaging health check failed: #{err}")
shutdown_and_exit(1)
ensure
broker.close
Copy link
Member

Choose a reason for hiding this comment

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

You need to handle the case where broker is nil here because ensure will still trigger on the return up here https://github.com/ManageIQ/manageiq/pull/23020/files#diff-0c71d22135646ed908fa6a66bec47aa6024935e7d9ed66e4a99c64f9355b3307R628

The other option is to have an explicit begin/rescue/ensure like

def messaging_health_check
  broker =
  return if broker.nil?

  begin
    ...
  rescue => err
    ...
  ensure
    broker.close
  end
end

@agrare
Copy link
Member

agrare commented May 3, 2024

@nasark manageiq-messaging v1.4.2 released with ManageIQ/manageiq-messaging#88

@nasark nasark requested a review from Fryguy as a code owner May 3, 2024 19:25
@nasark nasark force-pushed the get_metadata_for_kafka_health_check branch from c320447 to 4f4a4c4 Compare May 3, 2024 19:30
@nasark nasark force-pushed the get_metadata_for_kafka_health_check branch from 4f4a4c4 to 8229620 Compare May 3, 2024 20:13
@miq-bot
Copy link
Member

miq-bot commented May 3, 2024

Checked commits nasark/manageiq@38debb3~...8229620 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@agrare agrare merged commit 6351fd6 into ManageIQ:master May 3, 2024
8 checks passed
@Fryguy
Copy link
Member

Fryguy commented May 8, 2024

Backported to radjabov in commit 29a2b3c.

commit 29a2b3cc9da76d0f21cf2f597dd4dfc14e8e613b
Author: Adam Grare <[email protected]>
Date:   Fri May 3 16:30:12 2024 -0400

    Merge pull request #23020 from nasark/get_metadata_for_kafka_health_check
    
    Rework Kafka health check
    
    (cherry picked from commit 6351fd6e13bd3b36f64088e7da6571c4f2e624b3)

Fryguy pushed a commit that referenced this pull request May 8, 2024
…heck

Rework Kafka health check

(cherry picked from commit 6351fd6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants