Skip to content

fix(monitoring): remove legacy container and default empty cronJob - #5141

Merged
Siumauricio merged 3 commits into
canaryfrom
fix/monitoring-zombie-container
Aug 27, 2026
Merged

Siumauricio merged 3 commits into
canaryfrom
fix/monitoring-zombie-container

Conversation

@narcisonunez

@narcisonunez narcisonunez commented Aug 20, 2026 •

Copy link
Copy Markdown
Collaborator

The swarm migration in 3848fa9c0 dropped the container.remove({force:true}) that the standalone deploy path used to run. Swarm tasks are named dokploy-monitoring.<slot>.<id>, so there is no name collision and the pre-v0.30.0 container survives every redeploy. It also stays pinned to an orphaned image ID once pullRemoteImage moves the latest tag, so neither a pull nor a Save clears it and it restarts forever.

Cloud setup spread metricsConfig straight from the row, shipping cronJob: "" to the agent. robfig/cron rejects an empty spec, so the Go binary exits before Fiber binds 4500 and Docker restarts it every ~60s.

  • remove the legacy container in deployMonitoringService, which covers both setupMonitoring and setupWebMonitoring. Cleanup is best effort: a failure is logged and the deploy continues, matching the pre-migration behaviour
  • default cronJob when configuring monitoring for cloud
  • on build servers, clean up the legacy container but deploy no service. They never join the swarm, yet cloud setup did create the standalone container there before v0.30.0, and the monitoring form has always been hidden for them, so those agents are all stuck with an empty cronJob

What is this PR about?

Please describe in a short paragraph what this PR is about.

Checklist

Before submitting this PR, please make sure that:

Issues related (if applicable)

Screenshots (if applicable)

Screenshot 2026-08-20 at 11 23 35 AM

Greptile Summary

This PR removes legacy standalone monitoring containers before deploying the current Swarm service and supplies a default retention cron expression during cloud server setup.

  • Adds best-effort cleanup for the legacy dokploy-monitoring container.
  • Defaults an empty cloud monitoring cron expression to a daily schedule.
  • Adds real-Docker coverage for legacy cleanup and service deployment.

Confidence Score: 5/5

The PR appears safe to merge because no eligible blocking failure remains from the provided follow-up-review context.

No blocking failure remains.

Reviews (3): Last reviewed commit: "mend" | Re-trigger Greptile

Context used:

The swarm migration in 3848fa9 dropped the container.remove({force:true})
that the standalone deploy path used to run. Swarm tasks are named
dokploy-monitoring.<slot>.<id>, so there is no name collision and the
pre-v0.30.0 container survives every redeploy. It also stays pinned to an
orphaned image ID once pullRemoteImage moves the latest tag, so neither a
pull nor a Save clears it and it restarts forever.

Cloud setup spread metricsConfig straight from the row, shipping
cronJob: "" to the agent. robfig/cron rejects an empty spec, so the Go
binary exits before Fiber binds 4500 and Docker restarts it every ~60s.

- remove the legacy container in deployMonitoringService, which covers both
  setupMonitoring and setupWebMonitoring. Cleanup is best effort: a failure
  is logged and the deploy continues, matching the pre-migration behaviour
- default cronJob when configuring monitoring for cloud
- on build servers, clean up the legacy container but deploy no service.
  They never join the swarm, yet cloud setup did create the standalone
  container there before v0.30.0, and the monitoring form has always been
  hidden for them, so those agents are all stuck with an empty cronJob
- cover the above with real-docker tests
@narcisonunez
narcisonunez marked this pull request as ready for review August 20, 2026 15:31
@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Aug 20, 2026
@Siumauricio
Siumauricio merged commit 3c6a96a into canary Aug 27, 2026
5 checks passed
@Siumauricio
Siumauricio deleted the fix/monitoring-zombie-container branch August 27, 2026 06:03
@imrja8

imrja8 commented Sep 2, 2026 •

Copy link
Copy Markdown
Collaborator

Hey @narcisonunez, this PR introduced monitoring-setup.real.test.ts, which is currently breaking the pnpm test CI job for all open PRs synced with canary.

The test runs flawlessly locally, but on the GitHub Actions runner, it fails instantly with Error: connect ENOENT /var/run/docker.sock. It looks like the Node test runner in CI doesn't have the proper socket permissions or configuration to reach the host's Docker Daemon when querying for zombie containers.

We might need to either add .skipIf(!!process.env.GITHUB_ACTIONS) to this test or fix the docker socket setup in .github/workflows/pull-request.yml.

@narcisonunez

Copy link
Copy Markdown
Collaborator Author

@imrja8 can you please send a PR with the change?

@imrja8

imrja8 commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

@imrja8 can you please send a PR with the change?

Yes, I’ll surely give it a try.

Also, could you please have a look at the other two PRs when you get a chance: #5231 and #5197

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants