itzg / docker-mc-backup

Provides a side-car container to backup itzg/minecraft-server world data
https://hub.docker.com/r/itzg/mc-backup
MIT License
298 stars 51 forks source link

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

Open dreylok opened 1 month ago

dreylok commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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?

itzg commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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:

https://github.com/itzg/docker-mc-backup/commit/ce2ea1327e2b515313652c39068566339872b7d4#diff-26c954788cf358925d71c7e1ef6dd3824e2884ee29afe73d58091f0775cc5611R286

In conclusion :)

So, just close the issue?