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.

Sign up for GitHub

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

Jump to bottom

Healthchecks provided for all context storages #279

Merged
merged 9 commits into from
Nov 23, 2023
Merged

Healthchecks provided for all context storages #279

merged 9 commits into from
Nov 23, 2023

Conversation

pseusys
Copy link
Collaborator

@pseusys pseusys commented Nov 13, 2023 •
edited
Loading

Description

Also make command wait_db was removed and replaced with --wait flag (it will be also valid for #132).
Also docker-compose commands renamed to docker compose + file renamed (as it is preferred now).

Checklist

  • I have performed a self-review of the changes

To Consider

  • Add tests (if functionality is changed)
  • Update API reference / tutorials / guides
  • Update CONTRIBUTING.md (if devel workflow is changed)
  • Update .ignore files, scripts (such as lint), distribution manifest (if files are added/deleted)
  • Search for references to changed entities in the codebase

@pseusys pseusys added the enhancement New feature or request label Nov 13, 2023
@pseusys pseusys self-assigned this Nov 13, 2023
@RLKRo RLKRo changed the base branch from master to dev November 20, 2023 10:50
RLKRo added 3 commits November 21, 2023 03:52
@RLKRo
remove redundant command
bde8480
@RLKRo
replace CMD-SHELL with string commands
facea50 
The two forms are equivalent but our usage of them was inconsistent (some containers had string commands; others -- cmd-shell).

I think this form looks better; could change all commands to `CMD-SHELL` if needed.
@RLKRo
return docker dependency for test_all target
c60d4b3
RLKRo
RLKRo requested changes Nov 21, 2023
View reviewed changes
compose.yml
Comment on lines +16 to +19
interval: 5s
timeout: 10s
retries: 5
start_period: 30s
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain how these numbers were chosen?
If we used the default ones we wouldn't have to specify anything and have


    healthcheck:
      test: mysql -u $${MYSQL_USERNAME} -p$${MYSQL_PASSWORD} -e "select 1;"

  psql:

instead.

Copy link
Collaborator Author

@pseusys pseusys Nov 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

These numbers were chosen after local testing; some DB images already have healthchecks included, other do not.
I wanted all of them to be in equal conditions.

compose.yml
timeout: 10s
retries: 5
start_period: 30s

dashboard:
Copy link
Member

Choose a reason for hiding this comment

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

Do we need health checks for dashboard and otelcol?
AFAIK dashboard performs its own health checks that are unrelated to docker.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can, but I am not very familiar with what properties should we test in that cases. If you have a suggestion - I'll research how to add some.

compose.yml Outdated
@@ -41,6 +63,13 @@ services:
- 27017:27017
volumes:
- mongo-data:/data/db
healthcheck:
test: ["CMD-SHELL", "mongosh --norc --quiet --eval 'db.runCommand(\"ping\").ok' localhost:27017/test"]
Copy link
Member

Choose a reason for hiding this comment

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

Now mongo spams docker logs every 5 seconds with

mongo-1               | {"t":{"$date":"2023-11-21T00:43:27.123+00:00"},"s":"I",  "c":"NETWORK",  "id":22943,   "ctx":"listener","msg":"Connection accepted","attr":{"remote":"127.0.0.1:38256","uuid":"3b900426-57ec-4b36-80d3-cbcd0ec55cbe","connectionId":51,"connectionCount":3}}
mongo-1               | {"t":{"$date":"2023-11-21T00:43:27.124+00:00"},"s":"I",  "c":"NETWORK",  "id":51800,   "ctx":"conn50","msg":"client metadata","attr":{"remote":"127.0.0.1:38248","client":"conn50","doc":{"application":{"name":"mongosh 1.10.0"},"driver":{"name":"nodejs|mongosh","version":"5.6.0|1.10.0"},"platform":"Node.js v16.20.0, LE","os":{"name":"linux","architecture":"x64","version":"6.6.1-arch1-1","type":"Linux"}}}}
mongo-1               | {"t":{"$date":"2023-11-21T00:43:27.124+00:00"},"s":"I",  "c":"NETWORK",  "id":51800,   "ctx":"conn51","msg":"client metadata","attr":{"remote":"127.0.0.1:38256","client":"conn51","doc":{"application":{"name":"mongosh 1.10.0"},"driver":{"name":"nodejs|mongosh","version":"5.6.0|1.10.0"},"platform":"Node.js v16.20.0, LE","os":{"name":"linux","architecture":"x64","version":"6.6.1-arch1-1","type":"Linux"}}}}
mongo-1               | {"t":{"$date":"2023-11-21T00:43:27.127+00:00"},"s":"I",  "c":"NETWORK",  "id":22943,   "ctx":"listener","msg":"Connection accepted","attr":{"remote":"127.0.0.1:38264","uuid":"19309746-b63b-42a1-baa3-83ee8ed8b244","connectionId":52,"connectionCount":4}}
mongo-1               | {"t":{"$date":"2023-11-21T00:43:27.127+00:00"},"s":"I",  "c":"NETWORK",  "id":51800,   "ctx":"conn52","msg":"client metadata","attr":{"remote":"127.0.0.1:38264","client":"conn52","doc":{"application":{"name":"mongosh 1.10.0"},"driver":{"name":"nodejs|mongosh","version":"5.6.0|1.10.0"},"platform":"Node.js v16.20.0, LE","os":{"name":"linux","architecture":"x64","version":"6.6.1-arch1-1","type":"Linux"}}}}
mongo-1               | {"t":{"$date":"2023-11-21T00:43:27.257+00:00"},"s":"I",  "c":"NETWORK",  "id":22944,   "ctx":"conn51","msg":"Connection ended","attr":{"remote":"127.0.0.1:38256","uuid":"3b900426-57ec-4b36-80d3-cbcd0ec55cbe","connectionId":51,"connectionCount":3}}
mongo-1               | {"t":{"$date":"2023-11-21T00:43:27.257+00:00"},"s":"I",  "c":"NETWORK",  "id":22944,   "ctx":"conn50","msg":"Connection ended","attr":{"remote":"127.0.0.1:38248","uuid":"272ddbb7-be66-4f8a-ad36-3895ee6c01a2","connectionId":50,"connectionCount":0}}
mongo-1               | {"t":{"$date":"2023-11-21T00:43:27.257+00:00"},"s":"I",  "c":"NETWORK",  "id":22944,   "ctx":"conn52","msg":"Connection ended","attr":{"remote":"127.0.0.1:38264","uuid":"19309746-b63b-42a1-baa3-83ee8ed8b244","connectionId":52,"connectionCount":2}}
mongo-1               | {"t":{"$date":"2023-11-21T00:43:27.257+00:00"},"s":"I",  "c":"NETWORK",  "id":22944,   "ctx":"conn49","msg":"Connection ended","attr":{"remote":"127.0.0.1:38240","uuid":"e1760d4a-303b-40a7-a279-d87747596239","connectionId":49,"connectionCount":1}}

Could we do something about that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not really posible: we can not disable logging for 1 command only.

@RLKRo RLKRo merged commit cd6bf77 into dev Nov 23, 2023
16 checks passed
@RLKRo RLKRo deleted the feat/healthchecks_instead_of_make_command branch November 23, 2023 13:53
@RLKRo RLKRo mentioned this pull request Dec 7, 2023
Merged
1 task
RLKRo added a commit that referenced this pull request Dec 11, 2023
@RLKRo
Release v0.6.4
d388322 
# Release Notes

- Fix attachment sending via Telegram interfaces (81673b5)
- Fix PollingTelegramInterface dropping whenever an error occurred while responding (ec19921)
- Fix `Pipeline._run_pipeline` failing when `ctx_id=None` (5c22cc7)
- Docker improvements (#279)
- Various tutorial improvements
- Add release checklist
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RLKRo RLKRo RLKRo approved these changes

@ruthenian8 ruthenian8 ruthenian8 approved these changes

@kudep kudep Awaiting requested review from kudep

Assignees

@pseusys pseusys

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pseusys @ruthenian8 @RLKRo