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

PostgresSQL DB v15 and health check endpoint #5312

Merged
merged 26 commits into from
Dec 10, 2022
Merged

Conversation

azhavoro
Copy link
Contributor

@azhavoro azhavoro commented Nov 18, 2022

Motivation and context

How has this been tested?

Checklist

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

@azhavoro azhavoro marked this pull request as ready for review November 18, 2022 08:35
@azhavoro
Copy link
Contributor Author

/check

@github-actions
Copy link
Contributor

github-actions bot commented Nov 18, 2022

❌ Some checks failed
📄 See logs here

@azhavoro azhavoro changed the title Az/update postgres [WIP]Az/update postgres Nov 18, 2022
@nmanovic
Copy link
Contributor

@azhavoro , I think a little bit and I propose another solution. Could you please think and give me your opinion?

Let's have a health check endpoint for CVAT. Thus we can use it in our UI and show that the server is not ready yet. Of course, we don't know the reason why, but we can show possible reasons and recommendations, a link to our documentation. I believe it is a step in the right direction.

I will recommend to integrate https://github.com/revsys/django-health-check app. Also we can write small custom plugins and detect some popular problems. For example, OPA isn't running. The problem already can be shown in UI.

@azhavoro
Copy link
Contributor Author

/check

@github-actions
Copy link
Contributor

github-actions bot commented Nov 29, 2022

❌ Some checks failed
📄 See logs here

@azhavoro
Copy link
Contributor Author

/check

@github-actions
Copy link
Contributor

github-actions bot commented Nov 29, 2022

✔️ All checks completed successfully
📄 See logs here

@sizov-kirill
Copy link
Contributor

sizov-kirill commented Nov 29, 2022

Probably we can try to speed up our tests by simplifying restore.sql script:

I would suggest:

DROP DATABASE IF EXISTS :to WITH FORCE;

instead of current:

SELECT pg_terminate_backend(pg_stat_activity.pid)
FROM pg_stat_activity
WHERE pg_stat_activity.datname = 'cvat' AND pid <> pg_backend_pid();

DROP DATABASE IF EXISTS :to;

But it's necessary to ensure that such solution will give speeding up

@azhavoro azhavoro changed the title [WIP]Az/update postgres Az/update postgres Dec 6, 2022
cvat-ui/src/reducers/health-check-reducer.ts Outdated Show resolved Hide resolved
cvat-ui/src/components/cvat-app.tsx Outdated Show resolved Hide resolved
cvat-ui/src/components/cvat-app.tsx Outdated Show resolved Hide resolved
cvat/urls.py Outdated Show resolved Hide resolved
@nmanovic nmanovic changed the title Az/update postgres PostgresSQL DB v15 and health check endpoint Dec 8, 2022
@nmanovic
Copy link
Contributor

nmanovic commented Dec 8, 2022

@azhavoro , healthcheck django package provides a command health_check. It can be used to implement https://docs.docker.com/compose/compose-file/compose-file-v3/#healthcheck. Thus we will have a status for cvat_server.

https://docs.docker.com/engine/reference/builder/#healthcheck

@nmanovic
Copy link
Contributor

nmanovic commented Dec 8, 2022

Need to document the command as well.

image

@nmanovic
Copy link
Contributor

nmanovic commented Dec 8, 2022

Need to improve the initial page

image

@nmanovic
Copy link
Contributor

nmanovic commented Dec 8, 2022

No need to write the header one more time in the body. Also at the end there is no a space between the and Upgrade Guide.

image

@azhavoro
Copy link
Contributor Author

azhavoro commented Dec 8, 2022

/check

@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2022

❌ Some checks failed
📄 See logs here

@@ -155,13 +155,14 @@ services:
labels:
- traefik.enable=true
- traefik.http.services.cvat-ui.loadbalancer.server.port=80
- traefik.http.routers.cvat-ui.rule=Host(`${CVAT_HOST:-localhost}`)
- traefik.http.routers.cvat-ui.rule=Host(`${CVAT_HOST:-localhost}`) &&
! PathPrefix(`/api/`, `/git/`, `/opencv/`, `/static/`, `/admin`, `/documentation/`, `/django-rq`)
Copy link
Contributor

Choose a reason for hiding this comment

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

@azhavoro , why do we need the line now? How is it connected with the patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nmanovic I think it's better to remove the HEALTHCHECK command from the Dockerfile for this PR, this currently leads to routing issues related to Traefik container health check behavior traefik/traefik#7732. At the moment I don't find a suitable workaround, so I propose to create a separate issue about adding HEALTHCHECK instruction to CVAT server docker image.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Let's do that in this case.

Copy link
Contributor

@nmanovic nmanovic left a comment

Choose a reason for hiding this comment

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

LGTM

@nmanovic nmanovic merged commit 980c019 into develop Dec 10, 2022
@nmanovic nmanovic deleted the az/update_postgres branch December 10, 2022 08:25
mikhail-treskin pushed a commit to retailnext/cvat that referenced this pull request Jul 1, 2023
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

4 participants