* [pdm-devel] [PATCH 0/2] fix reload behaviour @ 2024-12-23 13:08 Fabian Grünbichler 2024-12-23 13:08 ` [pdm-devel] [PATCH 1/2] privileged api server: properly handle socket on reload Fabian Grünbichler ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Fabian Grünbichler @ 2024-12-23 13:08 UTC (permalink / raw) To: pdm-devel Fabian Grünbichler (2): privileged api server: properly handle socket on reload build: properly reload services after upgrade debian/proxmox-datacenter-manager.postinst | 28 +++++++++++ debian/proxmox-datacenter-manager.prerm | 14 ++++++ debian/rules | 5 ++ .../bin/proxmox-datacenter-privileged-api.rs | 48 ++++++++++--------- 4 files changed, 73 insertions(+), 22 deletions(-) create mode 100755 debian/proxmox-datacenter-manager.postinst create mode 100755 debian/proxmox-datacenter-manager.prerm -- 2.39.5 _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [pdm-devel] [PATCH 1/2] privileged api server: properly handle socket on reload 2024-12-23 13:08 [pdm-devel] [PATCH 0/2] fix reload behaviour Fabian Grünbichler @ 2024-12-23 13:08 ` Fabian Grünbichler 2024-12-23 16:16 ` Thomas Lamprecht 2024-12-23 13:08 ` [pdm-devel] [PATCH 2/2] build: properly reload services after upgrade Fabian Grünbichler 2024-12-23 15:39 ` [pdm-devel] applied: [PATCH 0/2] fix reload behaviour Thomas Lamprecht 2 siblings, 1 reply; 6+ messages in thread From: Fabian Grünbichler @ 2024-12-23 13:08 UTC (permalink / raw) To: pdm-devel the permission/ownership change fails during reload because the socket doesn't exist on-disk anymore, it is only passed along as previously opened FD in that case.. Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com> --- as the referenced comment indicates, this should probably all move to the rest server itself.. this is rather a stop-gap measure. .../bin/proxmox-datacenter-privileged-api.rs | 48 ++++++++++--------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/server/src/bin/proxmox-datacenter-privileged-api.rs b/server/src/bin/proxmox-datacenter-privileged-api.rs index d4e8817..2276a78 100644 --- a/server/src/bin/proxmox-datacenter-privileged-api.rs +++ b/server/src/bin/proxmox-datacenter-privileged-api.rs @@ -147,28 +147,32 @@ async fn run() -> Result<(), Error> { .expect("bad api socket path"), move |listener: tokio::net::UnixListener| { let sockpath = pdm_buildcfg::PDM_PRIVILEGED_API_SOCKET_FN; - - // NOTE: NoFollowSymlink is apparently not implemented in fchmodat()... - fchmodat( - Some(libc::AT_FDCWD), - sockpath, - Mode::from_bits_truncate(0o660), - FchmodatFlags::FollowSymlink, - ) - .map_err(|err| { - format_err!("unable to set mode for api socket '{sockpath:?}' - {err}") - })?; - - fchownat( - None, - sockpath, - None, - Some(api_user.gid), - FchownatFlags::FollowSymlink, - ) - .map_err(|err| { - format_err!("unable to set ownership for api socket '{sockpath}' - {err}") - })?; + // FIXME: needs to be adapted if socket is no longer removed above + // the socket only exists on the first start/restart, it's passed along as open FD for + // reloads.. + if Path::new(sockpath).exists() { + // NOTE: NoFollowSymlink is apparently not implemented in fchmodat()... + fchmodat( + Some(libc::AT_FDCWD), + sockpath, + Mode::from_bits_truncate(0o660), + FchmodatFlags::FollowSymlink, + ) + .map_err(|err| { + format_err!("unable to set mode for api socket '{sockpath:?}' - {err}") + })?; + + fchownat( + None, + sockpath, + None, + Some(api_user.gid), + FchownatFlags::FollowSymlink, + ) + .map_err(|err| { + format_err!("unable to set ownership for api socket '{sockpath}' - {err}") + })?; + } let incoming = UnixAcceptor::from(listener); -- 2.39.5 _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pdm-devel] [PATCH 1/2] privileged api server: properly handle socket on reload 2024-12-23 13:08 ` [pdm-devel] [PATCH 1/2] privileged api server: properly handle socket on reload Fabian Grünbichler @ 2024-12-23 16:16 ` Thomas Lamprecht 2024-12-30 15:00 ` Thomas Lamprecht 0 siblings, 1 reply; 6+ messages in thread From: Thomas Lamprecht @ 2024-12-23 16:16 UTC (permalink / raw) To: Proxmox Datacenter Manager development discussion, Fabian Grünbichler On 23/12/2024 14:08, Fabian Grünbichler wrote: > the permission/ownership change fails during reload because the socket doesn't > exist on-disk anymore, it is only passed along as previously opened FD in that > case.. > something seems still off here, after a reload of the privileged daemon the unprivileged one cannot connect to it anymore. Restarts works fine. _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pdm-devel] [PATCH 1/2] privileged api server: properly handle socket on reload 2024-12-23 16:16 ` Thomas Lamprecht @ 2024-12-30 15:00 ` Thomas Lamprecht 0 siblings, 0 replies; 6+ messages in thread From: Thomas Lamprecht @ 2024-12-30 15:00 UTC (permalink / raw) To: Proxmox Datacenter Manager development discussion, Fabian Grünbichler On 23/12/2024 17:16, Thomas Lamprecht wrote: > On 23/12/2024 14:08, Fabian Grünbichler wrote: >> the permission/ownership change fails during reload because the socket doesn't >> exist on-disk anymore, it is only passed along as previously opened FD in that >> case.. >> > > something seems still off here, after a reload of the privileged daemon > the unprivileged one cannot connect to it anymore. Restarts works fine. > Should be fixed now. We really must not delete the socket file unconditionally on daemon start, as it's used from the unprivileged, main API daemon to proxy to on-demand. So, while on a fresh start it did not matter as the socket including the file was newly created anyway, on a reload the file is not newly created again, as there we just re-create the UnixListener from the inherited FD, thus breaking the proxying completely. I lost some time as I initially suspected the FD passing to be broken, which would not matter for the TCP socket based PBS and thus explain why it "works" there, but the socket FD passing actually works and is done quite elegantly, so I finally saw the obvious: no socket file == not being able to connect. To fix this I moved the removal into the bind fn from the impl of the Listenable trait for the tokio::net::UnixListener type used here, that way it's only done on fresh socket creation. As alternative we could keep the proxy FD open and transparently reconnect to the new FD (e.g., comparing inode with our cached one, or just opportunistically if the old one stops accepting connections). _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [pdm-devel] [PATCH 2/2] build: properly reload services after upgrade 2024-12-23 13:08 [pdm-devel] [PATCH 0/2] fix reload behaviour Fabian Grünbichler 2024-12-23 13:08 ` [pdm-devel] [PATCH 1/2] privileged api server: properly handle socket on reload Fabian Grünbichler @ 2024-12-23 13:08 ` Fabian Grünbichler 2024-12-23 15:39 ` [pdm-devel] applied: [PATCH 0/2] fix reload behaviour Thomas Lamprecht 2 siblings, 0 replies; 6+ messages in thread From: Fabian Grünbichler @ 2024-12-23 13:08 UTC (permalink / raw) To: pdm-devel instead of using the default restart action. Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com> --- debian/proxmox-datacenter-manager.postinst | 28 ++++++++++++++++++++++ debian/proxmox-datacenter-manager.prerm | 14 +++++++++++ debian/rules | 5 ++++ 3 files changed, 47 insertions(+) create mode 100755 debian/proxmox-datacenter-manager.postinst create mode 100755 debian/proxmox-datacenter-manager.prerm diff --git a/debian/proxmox-datacenter-manager.postinst b/debian/proxmox-datacenter-manager.postinst new file mode 100755 index 0000000..7e07d01 --- /dev/null +++ b/debian/proxmox-datacenter-manager.postinst @@ -0,0 +1,28 @@ +#!/bin/sh + +set -e + +#DEBHELPER# + +case "$1" in + configure) + # modeled after dh_systemd_start output + systemctl --system daemon-reload >/dev/null || true + if [ -n "$2" ]; then + _dh_action=try-reload-or-restart + else + _dh_action=start + fi + deb-systemd-invoke $_dh_action 'proxmox-datacenter-api.service' 'proxmox-datacenter-privileged-api.service' >/dev/null || true + ;; + + abort-upgrade|abort-remove|abort-deconfigure) + ;; + + *) + echo "postinst called with unknown argument \`$1'" >&2 + exit 1 + ;; +esac + +exit 0 diff --git a/debian/proxmox-datacenter-manager.prerm b/debian/proxmox-datacenter-manager.prerm new file mode 100755 index 0000000..efeeebc --- /dev/null +++ b/debian/proxmox-datacenter-manager.prerm @@ -0,0 +1,14 @@ +#!/bin/sh + +set -e + +#DEBHELPER# + +# modeled after dh_systemd_start output +if [ -d /run/systemd/system ] && [ "$1" = remove ]; then + deb-systemd-invoke stop 'proxmox-datacenter-api.service' \ + 'proxmox-datacenter-manager-banner.service' \ + 'proxmox-datacenter-manager-daily-update.timer' \ + 'proxmox-datacenter-privileged-api.service' \ + >/dev/null || true +fi diff --git a/debian/rules b/debian/rules index b2dc88d..86652b0 100755 --- a/debian/rules +++ b/debian/rules @@ -35,5 +35,10 @@ override_dh_strip: debian/scripts/elf-strip-unused-dependencies.sh "$$exe" || true; \ done +override_dh_installsystemd: + dh_installsystemd -pproxmox-datacenter-manager proxmox-datacenter-manager-daily-update.timer + # note: we start/try-reload-restart services manually in postinst + dh_installsystemd --no-start --no-restart-after-upgrade --no-stop-on-upgrade + override_dh_missing: dh_missing --fail-missing -- 2.39.5 _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [pdm-devel] applied: [PATCH 0/2] fix reload behaviour 2024-12-23 13:08 [pdm-devel] [PATCH 0/2] fix reload behaviour Fabian Grünbichler 2024-12-23 13:08 ` [pdm-devel] [PATCH 1/2] privileged api server: properly handle socket on reload Fabian Grünbichler 2024-12-23 13:08 ` [pdm-devel] [PATCH 2/2] build: properly reload services after upgrade Fabian Grünbichler @ 2024-12-23 15:39 ` Thomas Lamprecht 2 siblings, 0 replies; 6+ messages in thread From: Thomas Lamprecht @ 2024-12-23 15:39 UTC (permalink / raw) To: Proxmox Datacenter Manager development discussion, Fabian Grünbichler On 23/12/2024 14:08, Fabian Grünbichler wrote: > Fabian Grünbichler (2): > privileged api server: properly handle socket on reload > build: properly reload services after upgrade > > debian/proxmox-datacenter-manager.postinst | 28 +++++++++++ > debian/proxmox-datacenter-manager.prerm | 14 ++++++ > debian/rules | 5 ++ > .../bin/proxmox-datacenter-privileged-api.rs | 48 ++++++++++--------- > 4 files changed, 73 insertions(+), 22 deletions(-) > create mode 100755 debian/proxmox-datacenter-manager.postinst > create mode 100755 debian/proxmox-datacenter-manager.prerm > applied both patches, thanks! Had the second part already prepared locally but not yet applied so I took yours – <=70 cc limits for commit message would be nice though :) _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-12-30 15:00 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-12-23 13:08 [pdm-devel] [PATCH 0/2] fix reload behaviour Fabian Grünbichler 2024-12-23 13:08 ` [pdm-devel] [PATCH 1/2] privileged api server: properly handle socket on reload Fabian Grünbichler 2024-12-23 16:16 ` Thomas Lamprecht 2024-12-30 15:00 ` Thomas Lamprecht 2024-12-23 13:08 ` [pdm-devel] [PATCH 2/2] build: properly reload services after upgrade Fabian Grünbichler 2024-12-23 15:39 ` [pdm-devel] applied: [PATCH 0/2] fix reload behaviour Thomas Lamprecht
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox