Re: [pve-devel] [PATCH installer 00/14] fix #5536: implement post-(auto-)installation notification mechanism

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 10/07/2024 um 15:27 schrieb Christoph Heiss:
> This implements a mechanism for post-installation "notifications" via a
> POST request [0] when using the auto-installer.
> 
> It's implemented as a separate, small utility to facilitate separation
> of concerns and make the information gathering easier by having it
> isolated in one place.
> 
> Patches #1 through #10 are simply clean-ups, refactors, etc. that were
> done along the way, which do not impact functionality in any way.
> 
> Most interesting here will be patch #12, which adds the actual
> implementation of the post-hook. (Bind-)mounting the installed host
> system is done using the existing `proxmox-chroot` utility, and the HTTP
> POST functionality can fortunately be re-used 1:1 from
> `proxmox-fetch-answer`.
> 
> I've also included an example of how the JSON body (pretty-printed for
> readability) of such a post-installation request would look like below,
> for reference.
> 
> Tested this with both PVE and PBS ISOs, PMG did not (yet) have a
> release with an auto-installation capable ISO. The only product-specific
> code is the version detection in `proxmox-post-hook`, which - since it
> works the same for PVE and PMG - be no obstacle.
> 
> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=5536
> 
> {
>   "debian-version": "12.5",
>   "product-version": "pve-manager/8.2.2/9355359cd7afbae4",
>   "kernel-version": "proxmox-kernel-6.8.4-2-pve-signed",

no hard feelings, but from a gut feeling I'd probably move this to a
"version" object and potentially use a more reduced, easier to parse, value.
Maybe also as an object so that both, a simple X.Y(.Z) release and a full
string are given, as we changed (kernel) packages name in the past, and I
could imagine that there will be a LTS and non LTS variant if we ever rework
on where we base our kernel on, so this might change in the future too.
While the simple X.Y.Z version will more likely stay as is.

And I do not want to move the goal post here to far, but isn't some of this
information potentially interesting to have sent to a metric server?
At least with a low frequency (or just once on every boot) so that one has a
somewhat recent externally saved set of information that can be used to
identify machines more easily and be aware of some changes to correlate
regressions or strange (load) metrics with.

No need to do that now, but maybe something to keep in mind to allow easier
reuse of this.

IMO it's a big plus if we manage to keep information laid out the same way,
or at list in a similar one, wherever it's included. And that doesn't have
to mean that the whole struct has to be shared, maybe it just could be just
a collection of types that stem from common crates outside the installer
(at least in the long run, as said, no need to completely block this on
scope extension now).

>   "boot-type": "bios",

We call this "mode" in the product APIs [0], might potentially make sense
to use the same schema here? Else I'd at least name this boot-mode and use
the same keys.

[0]: https://pve.proxmox.com/pve-docs/api-viewer/index.html#/nodes/{node}/status

>   "filesystem": "zfs (RAID1)",
>   "fqdn": "host.domain",
>   "machine-id": "f4bf9711783248b7aaffe3ccbca3e3dc",
>   "bootdisk": [

could be also interesting to get all disks and flag the ones used for booting
with a boolean "bootdisk" flag. E.g. to make additional storage provisioning
later on slightly more convenient.

>     {
>       "size": 8589934592,
>       "udev-properties": {
>         "DEVNAME": "/dev/vda", [..]
>       }
>     },
>     {
>       "size": 8589934592,
>       "udev-properties": {
>         "DEVNAME": "/dev/vdb", [..]
>       }
>     }
>   ],
>   "management-nic": {
>     "mac": "de:ad:f0:0d:12:34",
>     "address": "10.0.0.10/24",
>     "udev-properties": {
>       "INTERFACE": "enp6s18", [..]
>     }
>   },
>   "ssh-public-host-keys": {
>     "ecdsa": "ecdsa-sha2-nistp256 [..] root@host",
>     "ed25519": "ssh-ed25519 [..] root@host",
>     "rsa": "ssh-rsa [..] root@host",
>   }
> }
> 
> Christoph Heiss (14):  chroot: print full anyhow message
>   tree-wide: fix some typos
>   tree-wide: collect hardcoded installer runtime directory strings into
>     constant
>   common: simplify filesystem type serializing & Display trait impl
>   common: setup: serialize `target_hd` as string explicitly
>   common: split out installer setup files loading functionality
>   debian: strip unused library dependencies
>   fetch-answer: move http-related code to gated module in
>     installer-common
>   tree-wide: convert some more crates to use workspace dependencies
>   auto-installer: tests: replace left/right with got/expected in output
>   auto-installer: answer: add `posthook` section
>   fix #5536: add post-hook utility for sending notifications after
>     auto-install
>   fix #5536: post-hook: add some unit tests
>   unconfigured.sh: run proxmox-post-hook after successful auto-install
> 
>  Cargo.toml                                    |  11 +
>  Makefile                                      |   8 +-
>  debian/control                                |   1 +
>  debian/install                                |   1 +
>  debian/rules                                  |   9 +
>  proxmox-auto-install-assistant/Cargo.toml     |  14 +-
>  proxmox-auto-installer/Cargo.toml             |  15 +-
>  proxmox-auto-installer/src/answer.rs          |  27 +-
>  .../src/bin/proxmox-auto-installer.rs         |  15 +-
>  proxmox-auto-installer/src/sysinfo.rs         |  10 +-
>  proxmox-auto-installer/src/utils.rs           |  15 +-
>  proxmox-auto-installer/tests/parse-answer.rs  |  42 +-
>  proxmox-chroot/Cargo.toml                     |   8 +-
>  proxmox-chroot/src/main.rs                    |  19 +-
>  proxmox-fetch-answer/Cargo.toml               |  17 +-
>  .../src/fetch_plugins/http.rs                 | 100 +---
>  .../src/fetch_plugins/partition.rs            |  14 +-
>  proxmox-installer-common/Cargo.toml           |  26 +-
>  proxmox-installer-common/src/http.rs          |  94 ++++
>  proxmox-installer-common/src/lib.rs           |   5 +
>  proxmox-installer-common/src/options.rs       | 109 ++--
>  proxmox-installer-common/src/setup.rs         | 108 +---
>  proxmox-installer-common/src/utils.rs         |   2 +
>  proxmox-post-hook/Cargo.toml                  |  19 +
>  proxmox-post-hook/src/main.rs                 | 498 ++++++++++++++++++
>  proxmox-tui-installer/Cargo.toml              |   8 +-
>  proxmox-tui-installer/src/setup.rs            |   2 +-
>  unconfigured.sh                               |   7 +-
>  28 files changed, 862 insertions(+), 342 deletions(-)
>  create mode 100644 proxmox-installer-common/src/http.rs
>  create mode 100644 proxmox-post-hook/Cargo.toml
>  create mode 100644 proxmox-post-hook/src/main.rs
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel