[pdm-devel] [PATCH 0/2] fix reload behaviour

[pdm-devel] [PATCH 0/2] fix reload behaviour

[Fabian Grünbichler]

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

[pdm-devel] [PATCH 1/2] privileged api server: properly handle socket on reload

[Fabian Grünbichler]

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

[pdm-devel] [PATCH 2/2] build: properly reload services after upgrade

[Fabian Grünbichler]

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

[pdm-devel] applied: [PATCH 0/2] fix reload behaviour

[Thomas Lamprecht]

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

Re: [pdm-devel] [PATCH 1/2] privileged api server: properly handle socket on reload

[Thomas Lamprecht]

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

Re: [pdm-devel] [PATCH 1/2] privileged api server: properly handle socket on reload

[Thomas Lamprecht]

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