JonathanTreffler / backblaze-personal-wine-container

Run the Backblaze personal backup client in a docker container
https://hub.docker.com/r/tessypowder/backblaze-personal-wine
GNU Affero General Public License v3.0
374 stars 35 forks source link

(refactor): startapp.sh script #110

Closed xela1 closed 7 months ago

xela1 commented 7 months ago

Found a few issues with this if someone wants to confirm

  1. log_message function was within the 'if client already exists' context, so couldn't be called from the doesn't exist else
  2. on first install from scratch, the log location doesn't exist yet
  3. on first install from scratch, it was attempting to download the installer to / which it has no permissions to
traktuner commented 7 months ago

HEy @xela1 Thanks for the PR! When I have time I'll review the changes (might take some time). Regarding your changes to the path: Don't write the installer to /tmp in the container It should be in /config/wine on disk (cdright after the mkdir) The permissions issue won't be fixed and has nothing to do with our implementation. It's the baseimage we're using, which is a very recent one since v1.6 and the permission scheme was tightened (which is good). Some people just need to add USER_ID and GROP_ID to adjust the permissions, for other users it works out-of-the-box.

xela1 commented 7 months ago

Don't write the installer to /tmp in the container It should be in /config/wine on disk (cdright after the mkdir)

This is missing in the section where it installs the client if it doesn't already exist, so I just added cd /tmp, but I can alter that

https://github.com/JonathanTreffler/backblaze-personal-wine-container/blob/615dd05ce7a04a6eb4ed53a57eaa7263ec4339f2/startapp.sh#L152-L153

Whereas it exists in the update code

https://github.com/JonathanTreffler/backblaze-personal-wine-container/blob/615dd05ce7a04a6eb4ed53a57eaa7263ec4339f2/startapp.sh#L79-L80

Thinking about it more, is there a reason why it doesn't just call the fetch_and_install if there is no current installed client?

I have a few 'enhancements' I've being playing with that I'm happy to run past you before I raise a PR for them

  1. If wine is not installed (first launch) deliberately call wineboot -i to initialise it before progressing
  2. checking for mounted devices (following a pattern /drive_d /drive_e etc) and automatically create the symlink (prior to installing/starting backblaze)
  3. Allow users to set a backblaze username and password environment variable and silently install the client
traktuner commented 7 months ago

Hey! Thanks. It's currently a code-mix, I just added the autoupdater function and left the existing code as-is since it's working. You can overhaul the function to directly call fetch_and_install also on first launch.

1) What would be the benefit of adding wineboot for new installs?

2) that's a good idea, however I'm one of the users who alter drives a lot, so if a symlink already exists or and the mount is not there, nothing should happen. So symlinks should be created and not removed imo. Backblaze is really picky there and just throws a "drive missing" warning if it does not find the correct volume ID. So I'm more cautious with automating things because we don't want to harm peoples's backups.

3) How would you handle the silent install/update? Only the MSI package supports silent deployment but only with business deployments (group ID and group token are mandatory for new and existing installs).

xela1 commented 7 months ago
  1. It ensures wine is configured and running before proceeding with anything further in the startapp script, for example, it doesn't need the work around I proposed in the log_message function that was attempting to write to the /config/wine directory
  2. My POC code just adds symlinks, it will never remove, if you have a volume mounted in the container following the /drive_x pattern it will create the symlink. It's not necessary, but it means one less manual action on setting up the container
  3. Personal accounts can be passed as a parameter also, I've got it working on a test build at the moment, the only issue I've got at the moment is it doesn't fully start the control panel after installing and has to be manually opened, I'll play around some more

"install_backblaze.exe" -nogui -signinaccount $BB_USER $BB_PASS

image

image

traktuner commented 7 months ago

Hey, let's try adding wineboot and symlinks first (and the fetch_and_install optimization to reduce redundant code) I am not sure about the silent install. Officially, I have the info from support that this is only available with the MSI package (plus the organization-specific parameters). I would rather not use undocumented and not officially supported parameters, as it can break anytime. Would be interesting to hear @JonathanTreffler's opinion on this.

xela1 commented 7 months ago

Hey, let's try adding wineboot and symlinks first (and the fetch_and_install optimization to reduce redundant code) I am not sure about the silent install. Officially, I have the info from support that this is only available with the MSI package (plus the organization-specific parameters). I would rather not use undocumented and not officially supported parameters, as it can break anytime. Would be interesting to hear @JonathanTreffler's opinion on this.

The exe help shows the parameters, but I'll leave this for now. I'll submit a PR tomorrow for the other bits for you to review

image

JonathanTreffler commented 7 months ago

I would rather not use undocumented and not officially supported parameters, as it can break anytime.

I agree and I am also concerned about 2FA support on the silent install. If accounts with OTP don't work, that would not be acceptable.

JonathanTreffler commented 7 months ago

2. Backblaze is really picky there and just throws a "drive missing" warning if it does not find the correct volume ID.

I think Backblaze recognizes drives by a file it writes on the filesystem on the disk. So I think re-adding symlinks would not hurt, but testing of that would be needed.

xela1 commented 7 months ago

I would rather not use undocumented and not officially supported parameters, as it can break anytime.

I agree and I am also concerned about 2FA support on the silent install. If accounts with OTP don't work, that would not be acceptable.

Good point, hadn't thought of this, I'll see how it behaves

xela1 commented 7 months ago

I think Backblaze recognizes drives by a file it writes on the filesystem on the disk. So I think re-adding symlinks would not hurt, but testing of that would be needed.

It doesn't seem to like the drive letter being changed, the original drive shows as unplugged and doesn't show as the new letter, but when reverted to the original letter it recovers fine.

Adding new disks (following the /drive_x pattern) are in the backblaze client to be selected on the first container startup after adding, no need to restart the container after symlink

traktuner commented 7 months ago

I would rather not use undocumented and not officially supported parameters, as it can break anytime.

I agree and I am also concerned about 2FA support on the silent install. If accounts with OTP don't work, that would not be acceptable.

Good point, hadn't thought of this, I'll see how it behaves

I asked Backblaze support a 2nd time about this, and they mentioned that this silent install is - even with the exe installer - only available with groupID and group token and will misbehave when those parameters are missing. 2FA would also be a problem. So a hard no for me.

traktuner commented 7 months ago

I think Backblaze recognizes drives by a file it writes on the filesystem on the disk. So I think re-adding symlinks would not hurt, but testing of that would be needed.

It doesn't seem to like the drive letter being changed, the original drive shows as unplugged and doesn't show as the new letter, but when reverted to the original letter it recovers fine.

Adding new disks (following the /drive_x pattern) are in the backblaze client to be selected on the first container startup after adding, no need to restart the container after symlink

Volume ID and drive letter are "married" for Backblaze. If it sees the drive "X" with a different volume ID it's unplugged for Backblaze.

Adding new symlinks for new mounts is good 👍

xela1 commented 7 months ago

I asked Backblaze support a 2nd time about this, and they mentioned that this silent install is - even with the exe installer - only available with groupID and group token and will misbehave when those parameters are missing.

I mean they're incorrect, as it works and has options to do it without the groupID etc, but I'm really not bothered enough to push for this, it was just a nice to have on my list where we can remove as much user interaction as possible

xela1 commented 7 months ago

I pushed my latest commits into the PR last night which contains the refactoring of the update code, the initialization of wine and the auto-symlinking of mounted volumes.

I've tested this in every way I can think possible, but would appreciate someone else performing some testing

Wine Initialization

  1. New container deploy - wine initializes
  2. Existing container start - wine initializing skipped

Symlinking

  1. Added volume - detected on next start of container and symlink created
  2. Removal of volume - nothing happens
  3. Change of drive letter of existing volume - symlink is created, but Backblaze ignores until the original drive letter is restored
  4. Replacing an existing drive letter with a new device - Backblaze offers to change the drive letter or replace the unplugged device - backblaze is unable to change the drive letter, but this is existing behavior if someone did this manually
  5. Adding 'replaced' device back as new drive letter - Backblaze picks up as new device now

Install/Updating

  1. New container with pinned version - installs
  2. Auto update with new pinned version - upgrades
  3. Forced update with no update available - advises no updates available
  4. Forced update with update available - updates
JonathanTreffler commented 7 months ago

Thanks @xela1 for your work. To me this seems to meet the quality requirements for a merge, but I will leave the final decision of that to @traktuner because he originally wrote the update code.

Also the new symlinking functionality requires the README to be changed: The unnecessary step(s) in the install process, that are now automated should be removed and new advisories on how to handle the drive letters, like you have already outlined above are needed. It would be nice of you to add these readme changes to the PR :)

xela1 commented 7 months ago

Also the new symlinking functionality requires the README to be changed: The unnecessary step(s) in the install process, that are now automated should be removed and new advisories on how to handle the drive letters, like you have already outlined above are needed. It would be nice of you to add these readme changes to the PR :)

Yes, I was thinking that earlier, I'll work on that when I finish work