Skip to content

Conversation

@aldy505
Copy link
Collaborator

@aldy505 aldy505 commented Oct 14, 2025

This is a followup for #3991 and an alternative of #3999 that doesn't use a lock file.

…ta exists in `/tmp` for SeaweedFS

This is a followup for #3991 and an alternative of #3999 that doesn't use a lock file.
echo "${_group}Turning things off ..."

# Only execute this when `seaweedfs` container is running
if ! $dc ps --quiet seaweedfs; then

Choose a reason for hiding this comment

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

Potential bug: The condition if ! $dc ps --quiet seaweedfs incorrectly checks the command's exit code instead of its output, causing it to always execute the else block.
  • Description: The condition if ! $dc ps --quiet seaweedfs incorrectly evaluates the exit code of the docker-compose command instead of its output. Since $dc ps --quiet seaweedfs always has an exit code of 0, the condition is always false, causing the else block to execute unconditionally. This block attempts to run docker compose exec on the seaweedfs container. On fresh installations or when containers are stopped, the seaweedfs container isn't running. The exec command fails, and because the parent install.sh script uses set -e, the entire installation or upgrade process will crash.

  • Suggested fix: The check should be changed to verify if the command's output is empty, which correctly indicates whether the container is running. Use if [ -z "$($dc ps --quiet seaweedfs)" ] to skip the migration logic when the seaweedfs container is not found.
    severity: 0.9, confidence: 0.95

Did we get this right? 👍 / 👎 to inform future reviews.

@codecov
Copy link

codecov bot commented Oct 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.49%. Comparing base (da1f546) to head (9c5e5d8).
⚠️ Report is 5 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4000   +/-   ##
=======================================
  Coverage   99.49%   99.49%           
=======================================
  Files           3        3           
  Lines         197      197           
=======================================
  Hits          196      196           
  Misses          1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aldy505 aldy505 mentioned this pull request Oct 15, 2025
1 task
Copy link
Collaborator

@aminvakil aminvakil left a comment

Choose a reason for hiding this comment

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

LGTM

@aldy505 aldy505 enabled auto-merge (squash) October 15, 2025 13:31
@aldy505 aldy505 disabled auto-merge October 15, 2025 22:05
@aldy505
Copy link
Collaborator Author

aldy505 commented Oct 15, 2025

25.10.0 is released. Too late / not relevant anymore.

@aldy505 aldy505 closed this Oct 15, 2025
@aldy505 aldy505 deleted the aldy505/fix/seaweedfs-mv-tmp branch October 15, 2025 22:50
@BYK
Copy link
Member

BYK commented Oct 16, 2025

@aldy505 why? We can always make a patch release and tell people to skip this one?

@aldy505
Copy link
Collaborator Author

aldy505 commented Oct 16, 2025

@aldy505 why? We can always make a patch release and tell people to skip this one?

@BYK Because I've already merged the fix in another PR. If I can say, folks should skip 25.9.0 instead.

@BYK
Copy link
Member

BYK commented Oct 16, 2025

My suggestion was for folks who have already upgraded to 25.9 but not to 25.10 yet as if they do, they'll lose data, right?

@aldy505
Copy link
Collaborator Author

aldy505 commented Oct 16, 2025

@aldy505 why? We can always make a patch release and tell people to skip this one?

@BYK Because I've already merged the fix in another PR. If I can say, folks should skip 25.9.0 instead.

My suggestion was for folks who have already upgraded to 25.9 but not to 25.10 yet as if they do, they'll lose data, right?

Yeah, that's true.

@gertvdijk
Copy link

Got hit by this and wanted to say it may be worth to enable read-only root fs for the containers by default; it will reject writes to non-volume paths ie prevent writes data to volatile paths like this. For actual temp data paths one can use the tmpfs volume type in Docker to explicitly declare such volatile paths and allow writes there. Much less error prone. 😃

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

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants