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

feat: add docker service availability monitoring #5215

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SheepReaper
Copy link

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Introduces Docker service availability to monitor uptime and status of Docker services alongside existing types. Updates the database schema to support "docker_service". Enhances the user interface to allow configuration of Docker service parameters within the monitor setup.

This change allows for more granular monitoring capabilities by checking the running state of services in a Docker environment.

Relates to implementing extended Docker monitoring functionalities.

implements #5214

Type of change

  • User interface (UI)
  • New feature (non-breaking change which adds functionality)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • My changes generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

image image

@louislam

@SheepReaper SheepReaper force-pushed the feature/add-swarm-service-support branch from 9f988bc to bd719d9 Compare October 18, 2024 05:21
Introduces Docker service availability to monitor uptime
and status of Docker services alongside existing types.
Updates the database schema to support "docker_service".
Enhances the user interface to allow configuration of Docker
service parameters within the monitor setup.

This change allows for more granular monitoring capabilities
by checking the running state of services in a Docker environment.

Relates to implementing extended Docker monitoring functionalities.

implements louislam#5214

Signed-off-by: Bryan Gonzalez <[email protected]>
@CommanderStorm CommanderStorm changed the title GH-5214: feat: Add Docker service availability monitoring feat: add docker service availability monitoring Oct 18, 2024
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Lets go back one step to the conceptual level how this should be done:

We are currently in the migration towards a few things that are relevant for this PR:

  • we are migrating from the ginormous monior.js to more maintainable monitoring types. See reddis for a similar PR.
    => This PR needs to use the newer variant.
  • having two really close monitors is not ideal for users. Please merge this with the docker monitor unless there is a reason this should not happen
  • For monitors, we need testcases (f.ex. via testcontainers) to prevent stupid regressions in the future. If this is not possible, that is another story, but I think it likely is => we should try to do so.
    See feat: add RabbitMQ monitor #5199 for a PR on how to use testcontainers
  • Please have a look at this file and check if the newer conditional approach can work for this monitor

I have changed this to a draft to communicate the early stage of the PR to others better. Feel free to change once this fits better. ^^
If you have any trouble with one of the comments above, feel free to comment.

@CommanderStorm CommanderStorm added the pr:please address review comments this PR needs a bit more work to be mergable label Oct 20, 2024
@CommanderStorm CommanderStorm marked this pull request as draft October 20, 2024 15:00
@SheepReaper
Copy link
Author

  • I looked for a docker monitor type, there does not appear to be one in the master branch. Is there another branch where this refactor exists?
  • Am I being asked to perform said refactor?
  • The existing method of checking container status, and the new one I've introduced here are extremely similar code-wise, but they are checking different things. Namely, the existing checks via the container API, which is fine for a single-node docker deployment, whereas mine checks the service, via the services API. It's fundamentally a different concept. A service is UP if at least 1 container that supports it is running.
  • I also wanted to eventually implement a 3rd monitor type to check the "health" of a service since kuma has an intermediate status. In this implementation we care about the exact number of running containers supporting a service. But it's more complex since I need to make use of multiple docker API's in order to get all of the information needed to make that determination.
  • I've never used the @testcontainers package, but a quick glance here: https://testcontainers.com/modules/ doesn't seem to indicate that a "docker" container exists, nor is that really what I want. Since this series of monitors checks the system running the containers.
  • I can instead achieve the tests you're asking for by just mocking the axios responses, no need for @testcontainers.

Please clarify what you're asking of me. I only wanted to contribute a simple addition to the platform because I personally have need of it. But I'll consider contributing the refactor if the scope is clear.

@SheepReaper
Copy link
Author

SheepReaper commented Oct 20, 2024

forgot tag: @CommanderStorm
for reference the existing docker monitor is embedded here: https://github.com/louislam/uptime-kuma/blob/master/server/model/monitor.js#L712

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Oct 20, 2024

Am I being asked to perform said refactor?

Yes, please migrate to the newer monitoring type structure.
Adding new features to the unmaintainable pile is not workable.

It's fundamentally a different concept

For you it might, I don't think it is for other users (f.ex. me).
To name similarities, Oauth2 vs no auth are both inside of the http monitor.

If you look at this file, I think we can have a much nicer experience with a bit more effort ^^

doesn't seem to indicate that a "docker" container exists

For checking if the docker monitor can monitor docker containers (spawned via a testcontaier), there also does not need to be one.
Not sure if what I have in mind fully works => feel free to point out if it does not ^^

I can instead achieve the tests you're asking for by just mocking the axios responses

That is an alternative, but lets try to do it properly first

@dylnmatthewswork
Copy link

@SheepReaper @CommanderStorm

I would really love this feature, it would be good if container hashes change

@SheepReaper
Copy link
Author

I'm still tracking this, but I haven't had time lately to do the refactor. I'm happy to do it, but I won't feel bad if someone beats me to it. I just need to find the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:please address review comments this PR needs a bit more work to be mergable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants