Skip to content
This repository has been archived by the owner on May 31, 2021. It is now read-only.

Hide docker mounts from diskusage by default #836

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tillkrischer
Copy link
Contributor

Description

I think the docker related mount points should be hidden by default since they don't add any information.

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

before:
before
after:
after

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@@ -15,6 +15,24 @@ const CONFIG = {
ssl: process.env.FLOOD_ENABLE_SSL === 'true' || process.env.FLOOD_ENABLE_SSL === true,
sslKey: '/data/flood_ssl.key',
sslCert: '/data/flood_ssl.cert',
diskUsageService: {
dontWatchMountPoints: [
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can't just enumerate files our mountpoints here because they can be different for each distro. We need to find a cross-distro way to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are paths in the container, I don't know if the host distro affects this. I only tried on ubuntu.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, I missed it was for docker only.

@sktt
Copy link
Contributor

sktt commented Sep 24, 2019

Couldn't we suggest to the user to configure watchMountPoints instead? Perhaps we can give a notification on the terminal output when starting flood in lines of "You probably want to specify the mountpoints that you want to see reported disk usage for". For example in your case you'd just do
watchMountPoints: ["/downloads"]

@tillkrischer
Copy link
Contributor Author

Couldn't we suggest to the user to configure watchMountPoints instead? Perhaps we can give a notification on the terminal output when starting flood in lines of "You probably want to specify the mountpoints that you want to see reported disk usage for". For example in your case you'd just do
watchMountPoints: ["/downloads"]

The config.js is part of the docker image. You can't edit it easily. (You can open a shell inside the container and edit using vi). Or you would have to build a custom image.

Ideally I think this should be configurable in the GUI, but I don't know how to do that.

@sktt
Copy link
Contributor

sktt commented Sep 24, 2019

Ah! ok that's even better, this way we know which partition to watch right?

@sktt
Copy link
Contributor

sktt commented Sep 24, 2019

It seems that by default we're using /data as volume https://github.com/Flood-UI/flood/blob/master/Dockerfile#L46

perhaps that should be preset in config.docker.js instead?

@tillkrischer
Copy link
Contributor Author

It seems that by default we're using /data as volume https://github.com/Flood-UI/flood/blob/master/Dockerfile#L46

perhaps that should be preset in config.docker.js instead?

Well the user can call their mounts whatever they want. I'm doing "/downloads" for example. That's why I'm proposing the blacklist option.

@sktt
Copy link
Contributor

sktt commented Sep 24, 2019

Just trying to figure out whether this could be solved without introducing extra configuration, which would be preferable.

Another way of seeing this issue is that docker users who attach additional volumes to their container, don't have a straightforward method of adding those to the config. Or that docker users can't easily edit the config file. (something along the lines of docker exec -ti rtorrent-flood vi config.js and docker restart rtorrent-flood. Which would be lost after an upgrade)

The option to store to make this configurable from the GUI would mean that we'd store this information in the database.

@tillkrischer
Copy link
Contributor Author

Hardcoding a bunch of paths in configuration file is certainly not great. And as @noraj points out it might not cover all docker implementations.

I will try to make a GUI configurable version and do a new pull request.

@MacTwister
Copy link

Another idea could be simply to add a new environment variable for the "watch mounts" setting (ie. comma separated value), like we do for FLOOD_SECRET?

Also the /data volume is hard-coded in Dockerfile therefore can not be changed internally in the container from the perspective of the app meaning having a default in config.docker.js for watchMountPoints: ["/data"] is a suitable in my opinion.
@tillkrischer You'r talking about your volume mount point on the host which is not what the app can see.
If a user did manage changed the default DB path, that would mean they'd had made local modification as-well, meaning they'd be able to change the config values as-well.

I had to create my own Dockerfile and config.js locally to build a new image so I could have custom watch mount config value, see below, which as easy and simple to do. Only s downside is I'd have to manually update my config.js during any update where config changed.

FROM jfurrow/flood-ui:latest

COPY config.custom.js ./config.js

@tillkrischer
Copy link
Contributor Author

@tillkrischer You'r talking about your volume mount point on the host which is not what the app can see.

I'm actually mounting /downloads in flood, but only to match my rtorrent container, so the paths in the file picker are correct. Now that you mention it I think it's more of a niche configuration.

Also the /data volume is hard-coded in Dockerfile therefore can not be changed internally in the container from the perspective of the app meaning having a default in config.docker.js for watchMountPoints: ["/data"] is a suitable in my opinion.

I guess that is a reasonable default. It would work in my setup, since /data and /downloads are on the same partition.

Ultimately I think the best would still be a GUI option, maybe I can figure out how to implement that.

@jfurrow
Copy link
Member

jfurrow commented Sep 26, 2019

Ultimately I think the best would still be a GUI option, maybe I can figure out how to implement that.

Totally agree, this is the best solution. I think it should be a per-user setting, and the values would be stored in the database. The user should be able to view all available mounts somewhere in the settings modal and disable/enable them at will.

Are you up for tackling this? If not, I think this config solution would be fine for now.

@tillkrischer
Copy link
Contributor Author

Are you up for tackling this? If not, I think this config solution would be fine for now.

Yes I will spend some time on it today.
If I fail I will let you know, then you can merge this.

@jesec
Copy link
Member

jesec commented Jan 31, 2021

Resolved by jesec/flood@ee83e8f, jesec/flood@b6ebee6.

You may close the PR if it is no longer relevant.

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

Successfully merging this pull request may close these issues.

6 participants