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

[O11y][SQL Input] Add healthcheck to all images #9861

Merged
merged 4 commits into from
May 17, 2024

Conversation

aliabbas-elastic
Copy link
Contributor

@aliabbas-elastic aliabbas-elastic commented May 14, 2024

Proposed commit message

Fixes the flaky tests which result in error message one or more errors found in documents stored in metrics-sql.sql-ep data stream: [0] found error.message in event: cannot open connection: testing connection: dial tcp 172.18.0.7:3306: connect: connection refused

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have added an entry to my package's changelog.yml file.
    • Not updating the changelog as this change doesn't relate to the changes that should be visible at user's end
  • I have verified that Kibana version constraints are current according to guidelines.

How to test this PR locally

  1. elastic-package check
  2. elastic-package stack up -d -v
  3. elastic-package test -v

Related issues

Screenshots

image

@aliabbas-elastic aliabbas-elastic self-assigned this May 14, 2024
@aliabbas-elastic aliabbas-elastic changed the title [O11y][SQL Input] Add healthcheck for all images [O11y][SQL Input] Add healthcheck to all images May 14, 2024
@aliabbas-elastic aliabbas-elastic marked this pull request as ready for review May 14, 2024 10:45
@aliabbas-elastic aliabbas-elastic requested a review from a team as a code owner May 14, 2024 10:45
@aliabbas-elastic
Copy link
Contributor Author

/test

Comment on lines 25 to 26
interval: 1s
retries: 90
Copy link
Member

@shmsr shmsr May 15, 2024

Choose a reason for hiding this comment

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

Running healthcheck every second (in case it is failing) is just too much unnecessary overhead.

Probably these are more sane defaults:

  interval: 10s
  timeout: <decide some sane default>
  retries: 5
  start_period: <decide some sane default>

If Postgres container on averages takes time to start up, let's set a start_period. If this healthcheck command using sqlcmd is quick, probably setting a small timeout is good. I think interval 5/10s and retries 5/10 should be more than enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Setting interval and retries to values like 5/10 would make sense. But since the container does not take a lot time to start I think we can skip start_period and timeout IMO. Similar config was there in microsoft_sqlserver package but it's good to keep some proper values in retries and interval.

Copy link
Member

Choose a reason for hiding this comment

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

I know. Many healthchecks are improper in our packages.

USER root
ENV MYSQL_ROOT_PASSWORD test

HEALTHCHECK --interval=1s --retries=90 CMD /healthcheck.sh
Copy link
Member

Choose a reason for hiding this comment

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

Let's set better defaults. Retrying after every 1s is never good.

@shmsr shmsr added the bug Something isn't working label May 15, 2024
Copy link
Contributor

@niraj-elastic niraj-elastic left a comment

Choose a reason for hiding this comment

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

LGTM

@aliabbas-elastic
Copy link
Contributor Author

/test

@elasticmachine
Copy link

💚 Build Succeeded

History

cc @aliabbas-elastic

Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@aliabbas-elastic aliabbas-elastic merged commit 985181e into elastic:main May 17, 2024
5 checks passed
@aliabbas-elastic aliabbas-elastic deleted the fix_sql_input_tests branch May 17, 2024 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants