-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: Docker volume migration - OSS improvements Part 2 #6962
Conversation
Signed-off-by: Prashant Shahi <prashant@signoz.io>
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to dcc8906 in 2 minutes and 1 seconds
More details
- Looked at
880
lines of code in7
files - Skipped
1
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. scripts/volume-migration/migrate.sh:350
- Draft comment:
Check if the volume already exists before creating it to avoid conflicts or data overwrites. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
- Docker volume create is idempotent - it safely handles the case where a volume already exists. 2. The command is already handling errors by redirecting output. 3. Adding an explicit check would be redundant and add complexity without benefit. 4. The current implementation is following Docker best practices.
The comment might be concerned about race conditions or concurrent migrations that could cause data corruption.
Docker handles volume creation atomically and safely - there's no risk of data corruption from concurrent creation attempts. The existing code is actually the safer approach.
Delete the comment. The current implementation using Docker's idempotent volume create is the correct and safe approach.
2. deploy/docker-swarm/docker-compose.yaml:85
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This applies to other similar instances in this PR. - Reason this comment was not posted:
Comment was on unchanged code.
3. deploy/docker/docker-compose.ha.yaml:92
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This applies to other similar instances in this PR. - Reason this comment was not posted:
Marked as duplicate.
4. deploy/docker/docker-compose.testing.yaml:88
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This applies to other similar instances in this PR. - Reason this comment was not posted:
Marked as duplicate.
5. deploy/docker/docker-compose.yaml:88
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This applies to other similar instances in this PR. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_c1HgCsaMby72Y6HM
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 360e0b8 in 29 seconds
More details
- Looked at
138
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. scripts/volume-migration/migrate.sh:362
- Draft comment:
Consider adding error handling for thedocker run
command to ensure Docker is installed and running. This will prevent misleading error messages if Docker is not available. - Reason this comment was not posted:
Comment was on unchanged code.
2. scripts/volume-migration/migrate.sh:385
- Draft comment:
Consider adding error handling for thedocker run
command to ensure Docker is installed and running. This will prevent misleading error messages if Docker is not available. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_LCuhjl2sVmpLJFrf
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Prashant Shahi <prashant@signoz.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on ab46dd2 in 14 seconds
More details
- Looked at
21
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. scripts/volume-migration/migrate.sh:518
- Draft comment:
Changingreturn 1
toexit 1
is appropriate here to ensure the script terminates on invalid options, preventing further execution with incorrect input. - Reason this comment was not posted:
Confidence changes required:0%
The change fromreturn 1
toexit 1
in the parse_args function is appropriate because the script should terminate if there's an error in parsing arguments. This ensures that the script does not proceed with invalid or incomplete input.
Workflow ID: wflow_XsjNz6TUzeCyhVdl
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Signed-off-by: Prashant Shahi <prashant@signoz.io>
Summary
This is the second part of the OSS installation and improvement PR.
Users must run the migration script to migrate an existing SigNoz installation from Docker Standalone or Docker Swarm.
Related Issues / PR's
NA
Screenshots
NA
Affected Areas and Manually Tested Areas
NA
Important
Introduces a migration script and updates Docker Compose files to transition SigNoz installations from bind mounts to Docker volumes.
migrate.sh
inscripts/volume-migration
to migrate data from bind mounts to Docker volumes.standalone
andswarm
deployments.clickhouse
,zookeeper
,signoz
,alertmanager
.migrate()
andpost_migrate()
functions for data migration and cleanup.docker-compose.yaml
,docker-compose.ha.yaml
,docker-compose.testing.yaml
to use named volumes instead of bind mounts forclickhouse
,zookeeper
,alertmanager
, andsqlite
.signoz-clickhouse
,signoz-zookeeper-1
, etc..deprecated
file indeploy/docker/clickhouse-setup
indicating the deprecation of the old data directory and instructing users to use the migration script.This description was created by
for ab46dd2. It will automatically update as commits are pushed.