Fix for running signoz-schema-migrator under docker-swarm

[Ruppsn]

Fixes: #6477

---

Important

Add sync and --up= arguments to otel-collector-migrator in docker-compose.yaml for Docker Swarm compatibility.

• Behavior:
  • Add sync and --up= arguments to otel-collector-migrator service in docker-compose.yaml to ensure proper execution under Docker Swarm.
• Misc:
  • No changes to other services or files.

^{This description was created by for 0cc22df. It will automatically update as commits are pushed.}

[welcome]

Welcome to the SigNoz community! Thank you for your first pull request and making this project better. 🤗

[CLAassistant]

All committers have signed the CLA.

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to 0cc22df in 15 seconds

More details

• Looked at 15 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. deploy/docker-swarm/clickhouse-setup/docker-compose.yaml:248

• Draft comment:
  The --up= parameter seems unnecessary and might cause issues. Consider removing it if it's not required.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment is speculative, suggesting that the --up= parameter might be unnecessary and could cause issues. However, it does not provide strong evidence that this is indeed a problem. The rules state that speculative comments should be removed unless there is strong evidence of an issue.
  I might be missing specific knowledge about the otel-collector-migrator service and whether the --up= parameter is valid or required. However, the comment does not provide evidence or reasoning to support its claim.
  Even if I lack specific knowledge about the service, the comment should provide evidence or reasoning to support its claim, which it does not. Therefore, it should be removed according to the rules.
  The comment is speculative and lacks strong evidence of an issue. According to the rules, it should be removed.

2. deploy/docker-swarm/clickhouse-setup/docker-compose.yaml:246

• Draft comment:
  No issues found with the changes in this PR.
• Reason this comment was not posted:
  Confidence changes required: 0%
  The PR does not violate any of the specified rules. The changes are related to the command arguments for the otel-collector-migrator service, which are appropriate for the context.

Workflow ID: wflow_bGk6fQaiHeI7fUZ7

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[prashant-shahi]

Both sync and async schema migrators are needed.
Only after the sync is successfully executed, the async migration must run.

In Docker Compose, we achieved this using depends_on.<service-name>.condition:

    depends_on:
      otel-collector-migrator-sync:
        condition: service_completed_successfully

However, this exact behaviour will not work in Docker Swarm.
We will need to compromise for some other solution to achieve this behaviour.

Ref: docker/cli#3880

[Ruppsn]

Does this mean that meanwhile docker swarm will not be runnable with signoz?

[dieterblancke]

Wouldn't it then be better to change the migrator to run both (first sync, then async) migrations instead of spawning different containers for it?

[prashant-shahi]

Does this mean that meanwhile docker swarm will not be runnable with signoz?

With your PR changes, it should work fine for now as we do not have any async migrations yet.

However after we include async migrations, we may need to spawn async migration container post-install manually for Docker Swarm.

[prashant-shahi]

Wouldn't it then be better to change the migrator to run both (first sync, then async) migrations instead of spawning different containers for it?

Technically, we could do that as well. But it's best to separate these two for the ease of running only either of them when needed.

@srikanthccv any thoughts here?

[dieterblancke]

Wouldn't it then be better to change the migrator to run both (first sync, then async) migrations instead of spawning different containers for it?

Technically, we could do that as well. But it's best to separate these two for the ease of running only either of them when needed.

@srikanthccv any thoughts here?

Could still do it as it works now and accept both "sync" and "async" commands, and if both are present make sure it executes in the order of "first sync, then async".

I'm not sure as to how easy/hard that would be to achieve as I haven't gotten the time to look into how the migrator service works but in my head it sounds like it should be possible (basically one container that can handle both types instead of spawning it in two different containers).