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

The "sync" command in backup-loop.sh #189

Open
dreylok opened this issue Jun 16, 2024 · 5 comments
Open

The "sync" command in backup-loop.sh #189

dreylok opened this issue Jun 16, 2024 · 5 comments

Comments

@dreylok
Copy link

dreylok commented Jun 16, 2024

The following line in backup-loop.sh is missing the "rcon-cli" portion of the command:
retry ${RCON_RETRIES} ${RCON_RETRY_INTERVAL} sync

This results in the command being executed in the mc-backup container instead, calling the Linux "sync" command. There is no need for this command to run inside the mc-backup container and it appears to have been intended to be the sync command in Minecraft server, which should follow the save-all command.

Suggested fix is to simply add the "rcon-cli" command to this line. Will submit a PR; opening this issue for tracking purposes.

@itzg
Copy link
Owner

itzg commented Jun 16, 2024

I looked later and there is no vanilla sync command

https://minecraft.wiki/w/Commands

I think the original author really meant the Linux sync command. Now how important that was is a good question. I looked and I must have missed the change during PR and didn't ask.

@dreylok
Copy link
Author

dreylok commented Jun 16, 2024

I stand corrected. I swear I found something obscure that indicated there was a sync command for Minecraft server and even double checked in ChatGPT (for what that's worth) and it even went so far as to elaborate the difference between "sync" and "save-all flush" commands. But you're right, there is no reference to it and using the command in Minecraft server returns an error.

With that said, I agree the original author intended to use the Linux command, but it somehow got wrapped in the rcon-cli retry loop erroneously. However, running the sync command in the mc-backup container would not be effective since any potential data would be resident in the memory of the minecraft-server container.

Docker should be handling this at the volume level itself from what I understand since it's intended that data can be shared between services, but if we wanted to be absolutely certain, the command would need to be executed in the server container. Is there a way to facilitate this from the backup container?

@dreylok dreylok changed the title The "sync" command needs "rcon-cli" added to it The "sync" command in backup-loop.sh Jun 16, 2024
@itzg
Copy link
Owner

itzg commented Jun 16, 2024

Docker should be handling this at the volume level itself from what I understand since it's intended that data can be shared between services, but if we wanted to be absolutely certain, the command would need to be executed in the server container. Is there a way to facilitate this from the backup container?

I don't believe Docker would be doing anything special since it's all just Linux inodes mounted into the container/process. Even being called from an adjacent container, it's likely it works as intended since again it's the same filesystem/inode mounted into both containers.

...however, if we're doubting its effectiveness and why it's even there, then probably should just remove the call entirely.

@dreylok
Copy link
Author

dreylok commented Jun 16, 2024

From what I'm reading, the Docker volume "driver" does consistent writes by default to ensure data is immediately available on the host, and assumedly the other containers accessing named volumes. Nothing to worry about here.

However, the OS in the minecraft-server container could potentially have data in cache that has not yet been submitted to storage. Docker cannot do anything about that, and that's where the sync command could help ensure data is truly flushed to the storage layer. If we're erring on the side of caution, perhaps it should be included?

@itzg
Copy link
Owner

itzg commented Jun 16, 2024

From what I'm reading, the Docker volume "driver" does consistent writes by default to ensure data is immediately available on the host, and assumedly the other containers accessing named volumes. Nothing to worry about here.

That'll be for Docker Desktop, right? I was thinking about fully native Docker on Linux on where everything (host and containers) are running in the same kernel against the same filesystems and inodes.

However, the OS in the minecraft-server container could potentially have data in cache that has not yet been submitted to storage. Docker cannot do anything about that, and that's where the sync command could help ensure data is truly flushed to the storage layer. If we're erring on the side of caution, perhaps it should be included?

But yes, a point I forgot I was going to make is that it has been working fine for 5 years, so no need to mess with it:

ce2ea13#diff-26c954788cf358925d71c7e1ef6dd3824e2884ee29afe73d58091f0775cc5611R286

In conclusion :)

  • it's not a typo
  • it was intended to serve a purpose
  • it's possibly fulfilling that purpose in some cases (fully native Linux)
  • even if/when it doesn't do any useful operation, it apparently doesn't hurt anything since it's been there for ages

So, just close the issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants