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

gui, api: Indicate running under container #8728

Merged
merged 3 commits into from Dec 25, 2022
Merged

Conversation

calmh
Copy link
Member

@calmh calmh commented Dec 24, 2022

This adds a word to the version string when running containerized. The purpose is mostly to facilitate troubleshooting via screenshot by "leaking" this rather important aspect of the setup. Additionally, the version row gets "no-overflow-ellipsis" treatment so that the whole thing is actually visible in the GUI and the (now useless) tooltip is removed. In production releases this won't make a difference as the whole thing will typically fit, but in odd setups it provides more info up front.

Screenshot 2022-12-24 at 13 00 49

Arguably this thing should be localized, but ... the version strings aren't currently, because they're proper names of sort, the "64-bit" part could be...

This adds a word to the version string when running containerized. The
purpose is mostly to facilitate troubleshooting via screenshot by
"leaking" this rather important aspect of the setup. Additionally, the
version row gets "no-overflow-ellipsis" treatment so that the whole
thing is actually visible in the GUI and the (now useless) tooltip is
removed. In production releases this won't make a difference as the
whole thing will typically fit, but in odd setups it provides more info
up front.
@bt90
Copy link
Contributor

bt90 commented Dec 24, 2022

Could you add an environment detection for podman?

/run/.containerenv

The other two detection strategies don't seem to work in an alpine container (quickly tested in my ntfy container):

cat /proc/self/cgroup

returns 0::/

cat /proc/1/environ

doesn't contain anything container related.

I think those detection strategies are not valid anymore with cgroupv2?

@bt90
Copy link
Contributor

bt90 commented Dec 24, 2022

Looks like detection isn't that easy: https://github.com/systemd/systemd/blob/5c8e19cc1cfc58cd65a8bfe0fea4ea16ea513dcf/src/basic/virt.c

I think we should check for docker, podman and kubernetes. If anybody has a valid strategy for LXC that might also be nice.

As for kubernetes: https://kubernetes.io/docs/tutorials/services/connect-applications-service/#environment-variables

It should be enough to check for the presence of KUBERNETES_SERVICE_HOST.

@bt90
Copy link
Contributor

bt90 commented Dec 24, 2022

This might also be a nice addition for the usage report.

@calmh
Copy link
Member Author

calmh commented Dec 24, 2022

Yeah I can add more detection mechanisms. Note that I explicitly externalised the detection library because I don't want syncthing to have to maintain that as well.

@bt90
Copy link
Contributor

bt90 commented Dec 24, 2022

Sounds good but my point is that the /proc stuff is going to be less useful as cgroupv1 being phased out. The other mechanisms cover both of them.

I would also add PID as a detection mechanism. If syncthing is running with PID 1, it's 99.999% running inside a container.

@calmh
Copy link
Member Author

calmh commented Dec 24, 2022

In syncthings case the process running the check won't be pid 1 unfortunately because of wrappers and monitor process, but it's a good additional check.

@bt90
Copy link
Contributor

bt90 commented Dec 24, 2022

@calmh
Copy link
Member Author

calmh commented Dec 24, 2022

Yeah I don't particularly care about bsd here.

@tomasz1986
Copy link
Contributor

tomasz1986 commented Dec 24, 2022

Arguably this thing should be localized, but ... the version strings aren't currently, because they're proper names of sort, the "64-bit" part could be...

If this was to be translated, it should probably be treated as a whole with variables, as the word order differs between languages (e.g. "64-bit ARM" vs "ARM 64-bit", etc.) unless each part was divided with commas, e.g. "64-bit, ARM, container". Without any of those changes, I think it's probably better to leave "container" in English (especially since also the single word translation with no context may lead to funny things here), at least for the time being.

@acolomb
Copy link
Member

acolomb commented Dec 24, 2022

Agreed, let's not open a can of worms regarding translations here.

Does the mechanism tell us what container technology is being used? If so, I'd put the specific abbreviation there instead of the generic word "container".

@bt90
Copy link
Contributor

bt90 commented Dec 24, 2022

That would be neat. Some detection mechanisms would allow to do that. For others we would only get the information that it's some kind of container.

Maybe an enum like Docker, Podman, Generic and None ?

@calmh
Copy link
Member Author

calmh commented Dec 25, 2022

Thanks guys for the feedback, those are all good ideas that someone could contribute in the future! :)

@calmh calmh merged commit c4e69cd into syncthing:main Dec 25, 2022
@calmh calmh deleted the detectcontainer branch December 25, 2022 07:10
@calmh calmh added this to the v1.22.3 milestone Dec 27, 2022
calmh added a commit to calmh/syncthing that referenced this pull request Jan 23, 2023
* main: (69 commits)
  Handle relay connect timeout (fixes syncthing#8749) (syncthing#8755)
  gui, man, authors: Update docs, translations, and contributors
  build: Go 1.19.5
  gui, man, authors: Update docs, translations, and contributors
  script: Add weblatedl.go for downloading updated translations (syncthing#8723)
  gui: Allow to translate action and type in Recent Changes modal (syncthing#8548)
  gui, man, authors: Update docs, translations, and contributors
  gui: Fix undefined lastSeenDays error in disconnected-inactive status check (ref syncthing#8530) (syncthing#8730)
  gui, man, authors: Update docs, translations, and contributors
  gui, api: Indicate running under container (syncthing#8728)
  lib/fs: Use io/fs errors as recommended in std lib (syncthing#8726)
  build: Handle co-authors (ref syncthing#3744) (syncthing#8708)
  lib/fs: Watching is unsupported on android/amd64 (fixes syncthing#8709) (syncthing#8710)
  lib/model: Only log at info level if setting change time fails (syncthing#8725)
  lib/model: Don't lower rescan interval from default on auto accepted enc folder (fixes syncthing#8572) (syncthing#8573)
  gui, man, authors: Update docs, translations, and contributors
  gui: Remove unmaintained language variant nl-BE (syncthing#8722)
  gui, script: Fix indentation in lang-en.json to match others (syncthing#8721)
  docker: Ensure entrypoint is executable (syncthing#8719)
  Go 1.19.4
  ...
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Dec 25, 2023
@syncthing syncthing locked and limited conversation to collaborators Dec 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants