Re: [pve-devel] [PATCH storage] volume import: assume target API version is at least 9
Am 04/07/2024 um 14:11 schrieb Fiona Ebner: > Yes, next time we introduce an apiinfo call, we can just have it fail > hard upon errors. Oh, and just to avoid potential future error potential here: For a new topic-specific API version call that might not work, as the fallback and (lacking) error handling here was added explicitly for backward https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH pve-container 2/2] fix #5339: api: lxc: ip: add 'all' option so that all addresses can be returned.
On Tue, Jul 02, 2024 at 04:29:25PM GMT, Fabian Grünbichler wrote:
> apologies again for the long delay!
>
> > Johannes Cornelis Draaijer via pve-devel hat
> > am 18.04.2024 22:49 CEST geschrieben:
>
> > Signed-off-by: Johannes Cornelis Draaijer
> > ---
> > src/PVE/API2/LXC.pm | 16 +---
> > src/PVE/LXC.pm | 9 +++--
> > 2 files changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> > index 89ba64c..3561317 100644
> > --- a/src/PVE/API2/LXC.pm
> > +++ b/src/PVE/API2/LXC.pm
> > @@ -2533,6 +2533,12 @@ __PACKAGE__->register_method({
> > properties => {
> > node => get_standard_option('pve-node'),
> > vmid => get_standard_option('pve-vmid', { completion =>
> > \&PVE::LXC::complete_ctid }),
> > + all => {
> > + type => 'boolean',
> > + default => 0,
> > + optional => 1,
> > + description => 'Return all adresses of each interface instead
> > of only one',
>
> typo: s/adresses/addresses
>
> > + }
> > },
> > },
> > returns => {
> > @@ -2552,12 +2558,14 @@ __PACKAGE__->register_method({
> > },
> > inet => {
> > type => 'string',
> > - description => 'The IPv4 address of the interface',
> > + format => 'CIDRv4-list',
>
> this format here and the code below don't agree. a string type with the -list
> suffix needs actually be a string with the list elements delimited by either
> space, ',' or ';'. in this case, comma or semicolon is probably okay.
>
Since the caller needs to specify a new parameter, shouldn't we just
return an actual array instead?
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH pve-container 2/2] fix #5339: api: lxc: ip: add 'all' option so that all addresses can be returned.
> Wolfgang Bumiller hat am 05.07.2024 09:45 CEST
> geschrieben:
>
>
> On Tue, Jul 02, 2024 at 04:29:25PM GMT, Fabian Grünbichler wrote:
> > apologies again for the long delay!
> >
> > > Johannes Cornelis Draaijer via pve-devel
> > > hat am 18.04.2024 22:49 CEST geschrieben:
> >
> > > Signed-off-by: Johannes Cornelis Draaijer
> > > ---
> > > src/PVE/API2/LXC.pm | 16 +---
> > > src/PVE/LXC.pm | 9 +++--
> > > 2 files changed, 20 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> > > index 89ba64c..3561317 100644
> > > --- a/src/PVE/API2/LXC.pm
> > > +++ b/src/PVE/API2/LXC.pm
> > > @@ -2533,6 +2533,12 @@ __PACKAGE__->register_method({
> > > properties => {
> > > node => get_standard_option('pve-node'),
> > > vmid => get_standard_option('pve-vmid', { completion =>
> > > \&PVE::LXC::complete_ctid }),
> > > + all => {
> > > + type => 'boolean',
> > > + default => 0,
> > > + optional => 1,
> > > + description => 'Return all adresses of each interface instead
> > > of only one',
> >
> > typo: s/adresses/addresses
> >
> > > + }
> > > },
> > > },
> > > returns => {
> > > @@ -2552,12 +2558,14 @@ __PACKAGE__->register_method({
> > > },
> > > inet => {
> > > type => 'string',
> > > - description => 'The IPv4 address of the interface',
> > > + format => 'CIDRv4-list',
> >
> > this format here and the code below don't agree. a string type with the
> > -list suffix needs actually be a string with the list elements delimited by
> > either space, ',' or ';'. in this case, comma or semicolon is probably okay.
> >
>
> Since the caller needs to specify a new parameter, shouldn't we just
> return an actual array instead?
the retun schema wouldn't match then for either variant of the call..
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH ifupdown2] patch : addons: vxlan: fix VNI filter on single VXLAN device
Am 19/09/2023 um 16:10 schrieb Alexandre Derumier: > Requested by a customer using setup with single vxlan devices. > --- > debian/patches/series | 3 ++- > .../upstream/0001-vxlan-fix-vni-filter.patch | 27 +++ > 2 files changed, 29 insertions(+), 1 deletion(-) > create mode 100644 debian/patches/upstream/0001-vxlan-fix-vni-filter.patch > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH installer] auto-installer: fix answer-request charset
Important fix, apparently slipped through in April.
"Re-discovered" it yesterday by coincidence while talking to Aaron.
On Fri, Apr 26, 2024 at 10:12:55AM GMT, Fabian Grünbichler wrote:
> 'utf-' is a typo, and can trip up some servers that do strict
> checking/matching.
>
> Signed-off-by: Fabian Grünbichler
Reviewed-by: Christoph Heiss
> ---
>
> Notes:
> see
> https://forum.proxmox.com/threads/invalid-charset-on-automated-install-answer-http-fetch.145856/
>
> proxmox-fetch-answer/src/fetch_plugins/http.rs | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/proxmox-fetch-answer/src/fetch_plugins/http.rs
> b/proxmox-fetch-answer/src/fetch_plugins/http.rs
> index 1c5e9ea..21bc6a2 100644
> --- a/proxmox-fetch-answer/src/fetch_plugins/http.rs
> +++ b/proxmox-fetch-answer/src/fetch_plugins/http.rs
> @@ -211,7 +211,7 @@ mod http_post {
>
> answer = agent
> .post(&url)
> -.set("Content-type", "application/json; charset=utf-")
> +.set("Content-type", "application/json; charset=utf-8")
> .send_string(&payload)?
> .into_string()?;
> } else {
> @@ -231,7 +231,7 @@ mod http_post {
> .build();
> answer = agent
> .post(&url)
> -.set("Content-type", "application/json; charset=utf-")
> +.set("Content-type", "application/json; charset=utf-8")
> .timeout(std::time::Duration::from_secs(60))
> .send_string(&payload)?
> .into_string()?;
> --
> 2.39.2
>
>
>
> ___
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH installer] auto-installer: fix answer-request charset
Am 26/04/2024 um 10:12 schrieb Fabian Grünbichler: > 'utf-' is a typo, and can trip up some servers that do strict > checking/matching. > > Signed-off-by: Fabian Grünbichler > --- > > Notes: > see > https://forum.proxmox.com/threads/invalid-charset-on-automated-install-answer-http-fetch.145856/ > > proxmox-fetch-answer/src/fetch_plugins/http.rs | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > applied, with Christoph's R-b, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage] volume import: assume target API version is at least 9
Am 04.07.24 um 19:45 schrieb Thomas Lamprecht: > Am 04/07/2024 um 14:11 schrieb Fiona Ebner: >> There is no apiinfo call required anymore. No code is the cleanest kind > > Yeah, by the assumption you self choose to use and that I question, so > not really a useful argument. > > In practice, users can upgrade a from one major release to the next one, > nothing is really blocking them w.r.t apt, that cannot be said for jumping > releases, so one should definitively have different levels of standard for > "any X to any X + 1" compared to "X to X + >= 2". > > What we recommend users (run latest 7 before upgrade) is not necessarily > the exact same boundary of what we should hedge against to reduce negative > feelings of users and resulting work/soothing our support has to do as > a result; I'd think of it more like the minimum and recommended system > requirements. > Okay, I understand. >> of code IMHO. > > Less code can be nicer as long as all features and edge cases are still > handled properly and also still easy to read, not code golf minimalism. > > Or, to exaggerate for the points' sake, should I just delete all our > (user) error code handling and tell any users complaining that they just > need to do it right the next time? While that would result in quite > a few lines less, it definitively won't help our popularity. > Of course I'm not arguing for such a thing. Being accused of arguing for that hurts :(, especially when coming from somebody I highly respect. I honestly did not know that we consider having a node with PVE 7.0 while upgrading to PVE 8 supported. >>> Besides that: no, I definitively place UX over some abstract "code >>> cleanliness" >>> criteria – if, it can be fair to set the barrier such that one should >>> achieve >>> both, but putting UX below code tidiness is IMO just not acceptable. >> >> I do put UX for users running ancient versions below code cleanliness, >> but not UX for current versions of course. > > PVE 7 is still supported, so definitively not ancient! We normally put > in quite a bit of work to ensure that systems talking with other > incompatible versions get a good error, why bother adding versioning > else in the first place? And while there's definitively areas that can > still be improved, that's no argument for going backwards again. > Again, it's not all of PVE 7, just an early subset of it that's about three years without updates. If it weren't that, I wouldn't have it considered safe to remove the call. But okay, I'm fine with being more strict. > I mean this specific case here might be relatively harmless in effect, > but if you really apply this philosophy to all development here then I > wish you good luck into explaining angry users and our support agents > that had to answer them, why it was good for them to remove some simple > check for code cleanliness, because that should not be relevant if > the users did everything 100% correct. We already need to justify > us too much about not being hacky as a FLOSS solution, after having > to use the shell occasionally any missing/incomplete error handling is > the next big reason for people to complain. > And sure, most alternative (proprietary) solutions are far from being > better, and while it's annoying that some people are hypocrites here, > I do not have anything against being held to a high(er) standard, > as one can really stick out with good UX, which always consists of tons > of little things. > > Anyhow, as mentioned, this might not fall back on us here, but I hope > I could convince to not use that philosophy unconditionally, and I > wished for some more background explanation on this in the commit message. > > I just cannot think that there could have any negative effect, be it in > using nor maintaining that code for neither users nor developers, if we > just fixed the actual (error handling) behavior of querying the API > version instead, and possibly dropped the real old version checks, i.e. > those not just a single major version apart, in a separate patch. Let's just revert it then and drop the eval and the fallback. Fine by me. Or actually, a fresh install of PVE 7.0 comes with libpve-storage-perl = 7.0-7 (respectively 7.0-10 for the second release of the ISO). The API version was bumped to 9 in 7.0-4, so actually the subset of PVE 7 that's affected is empty. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH guest-common v4 2/6] mapping: pci: assert_valid: reword error messages
Am 06/06/2024 um 11:21 schrieb Dominik Csapak: > makes them a bit clearer > > Signed-off-by: Dominik Csapak > --- > split out in v4 > src/PVE/Mapping/PCI.pm | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH guest-common v4 1/6] mapping: pci: assert_valid: rename cfg to mapping
Am 06/06/2024 um 11:21 schrieb Dominik Csapak: > to make it clearer what it actually is. Also we want to add the > 'real' config as parameter too, and so it's less confusing. > > Signed-off-by: Dominik Csapak > --- > split out in v4 > src/PVE/Mapping/PCI.pm | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH guest-common v4 3/6] mapping: pci: make sure all desired properties are checked
Am 06/06/2024 um 11:21 schrieb Dominik Csapak: > by placing all expected properties from the hardware into an 'expected_props' > and those fromt he config into 'configured_props' > > the names makes clearer what's what, and we can easily extend it, even > if the data does not come from the mapping (like we'll do with 'mdev') > > Signed-off-by: Dominik Csapak > --- > changes from v3: > * rebased on split out patches before > * don't merge keys but add all to expected_props instead > src/PVE/Mapping/PCI.pm | 27 --- > 1 file changed, 16 insertions(+), 11 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH guest-common v4 4/6] mapping: pci: check the mdev configuration on the device too
Am 06/06/2024 um 11:21 schrieb Dominik Csapak:
> but that lives int he 'global' part of the mapping config, not in a
> specific mapping. To check that, add it to the relevant hashes here.
>
> Signed-off-by: Dominik Csapak
> ---
> changes from v3:
> * leave $cfg optional
>
> src/PVE/Mapping/PCI.pm | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/src/PVE/Mapping/PCI.pm b/src/PVE/Mapping/PCI.pm
> index aa56496..1b2ac52 100644
> --- a/src/PVE/Mapping/PCI.pm
> +++ b/src/PVE/Mapping/PCI.pm
> @@ -131,7 +131,7 @@ sub options {
>
> # checks if the given config is valid for the current node
> sub assert_valid {
> -my ($name, $mapping) = @_;
> +my ($name, $mapping, $cfg) = @_;
Which config is this? As is its IMO a bit to opaque. I even thought first
that this is the VM config, or well the config of a specific VM PCI
passthrough property, which I would not have liked much as that would add
a coupling, or maybe better said, hidden cyclic dependency between
guest-common and qemu-server.
Naming this such that it's clearer what config we're talking about here
would help to avoid such thought digressions, at least me.
Maybe `$global_mapping_cfg` or `$cluster_mapping_cfg` (as we do not really
use the term "global" much IIRC).
>
> my @paths = split(';', $mapping->{path} // '');
>
> @@ -161,6 +161,12 @@ sub assert_valid {
>
> my $configured_props = { $mapping->%{qw(id iommugroup subsystem-id)} };
>
> + # check mdev from globabl mapping config, if that is given
> + if (defined($cfg)) {
> + $expected_props->{mdev} = $info->{mdev} ? 1 : 0;
> + $configured_props->{mdev} = $cfg->{mdev} ? 1 : 0;
> + }
> +
> for my $prop (sort keys $expected_props->%*) {
> next if $prop eq 'iommugroup' && $idx > 0; # check iommu only on
> the first device
>
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH qemu-server v4 04/12] stop cleanup: remove unnecessary tpmstate cleanup
Am 06/06/2024 um 11:22 schrieb Dominik Csapak: > tpmstate0 is already included in `get_vm_volumes`, and our only storage > plugin that has unmap_volume implemented is the RBDPlugin, where we call > unmap in `deactivate_volume`. So it's already ummapped by the > `deactivate_volumes` calls above. > > For third-party storage plugins, it's natural to expect that > deactivate_volume() would also remove a mapping for the volume just > like RBDPlugin does. > > While there is an explicit map_volume() call in start_swtpm(), a > third-party plugin might expect an explicit unmap_volume() call too. > However, the order of calls right now is > 1. activate_volume() > 2. map_volume() > 3. deactivate_volume() > 4. unmap_volume() > > Which seems like it could cause problems already for third-party > plugins relying on an explicit unmap call. > > All that said, it is unlikely that a third-party plugin breaks. If it > really happens, it can be discussed/adapted to the actual needs still. > > Signed-off-by: Dominik Csapak > Acked-by: Fiona Ebner > --- > changes from v3: > * include rationale for 3rd party plugins (thanks @fiona) > > PVE/QemuServer.pm | 8 > 1 file changed, 8 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH guest-common/qemu-server/manager/docs v4] implement experimental vgpu live migration
Am 06/06/2024 um 11:21 schrieb Dominik Csapak: > guest-common 1-4; qemu-server 1-6; pve-manager 1,2 > are preparations/cleanups mostly and could be applied independently Well, yes and no, they have some interdependency between themselves, so not full independent. It would be great if you would note such inter-patch dependencies also in each patches' dev/review note section (where patch revision changelog lives too). E.g. I cannot apply manager's 1/5: "mapping: pci: include mdev in config checks" without the guest-common's 4/6: "mapping: pci: check the mdev configuration on the device too" but I probably can apply manager's 2/5: "bulk migrate: improve precondition checks", but as I noticed the missing dependency documentation for the first pair I was now slightly hesitant to do so without a much bigger amount of work to check this very closely myself; that is IMO extra reviewer work cost that can be avoided with not too much work from patch series author. Also, having that info makes it a bit easier to not miss any d/control `Depends` or `Breaks` version bump/update. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC qemu 3/7] block/backup: allow passing additional options for copy-before-write upon job creation
Quoting Fiona Ebner (2024-06-10 14:59:38)
> In particular, useful for setting the 'on-cbw-error' and 'cbw-timeout'
> options (see BlockdevOptionsCbw in QAPI).
>
> Signed-off-by: Fiona Ebner
> ---
> block/backup.c | 10 +++---
> block/replication.c| 2 +-
> blockdev.c | 2 +-
> include/block/block_int-global-state.h | 2 ++
> pve-backup.c | 2 +-
> 5 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index e0acfe6758..82fedf1680 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -336,6 +336,7 @@ BlockJob *backup_job_create(const char *job_id,
> BlockDriverState *bs,
>bool compress, bool discard_source,
>const char *filter_node_name,
>BackupPerf *perf,
> + QDict *cbw_opts,
>BlockdevOnError on_source_error,
>BlockdevOnError on_target_error,
>int creation_flags,
> @@ -347,7 +348,6 @@ BlockJob *backup_job_create(const char *job_id,
> BlockDriverState *bs,
> int64_t cluster_size;
> BlockDriverState *cbw = NULL;
> BlockCopyState *bcs = NULL;
> -QDict *cbw_opts = NULL;
>
> assert(bs);
> assert(target);
> @@ -436,8 +436,12 @@ BlockJob *backup_job_create(const char *job_id,
> BlockDriverState *bs,
> }
>
> if (perf->has_min_cluster_size) {
> -cbw_opts = qdict_new();
> -qdict_put_int(cbw_opts, "min-cluster-size", perf->min_cluster_size);
> +if (!cbw_opts) {
> +cbw_opts = qdict_new();
> +}
> +if (!qdict_haskey(cbw_opts, "min-cluster-size")) {
> +qdict_put_int(cbw_opts, "min-cluster-size",
> perf->min_cluster_size);
so here cbw_opts "wins" without any indication that this happens
> +}
> }
>
> cbw = bdrv_cbw_append(bs, target, filter_node_name, discard_source,
> diff --git a/block/replication.c b/block/replication.c
> index 0415a5e8b7..c5a27f593e 100644
> --- a/block/replication.c
> +++ b/block/replication.c
> @@ -583,7 +583,7 @@ static void replication_start(ReplicationState *rs,
> ReplicationMode mode,
> s->backup_job = backup_job_create(
> NULL, s->secondary_disk->bs,
> s->hidden_disk->bs,
> 0, MIRROR_SYNC_MODE_NONE, NULL, 0, false,
> false,
> -NULL, &perf,
> +NULL, &perf, NULL,
> BLOCKDEV_ON_ERROR_REPORT,
> BLOCKDEV_ON_ERROR_REPORT, JOB_INTERNAL,
> backup_job_completed, bs, NULL, &local_err);
> diff --git a/blockdev.c b/blockdev.c
> index cbe224387b..55ca967430 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2732,7 +2732,7 @@ static BlockJob *do_backup_common(BackupCommon *backup,
> backup->sync, bmap, backup->bitmap_mode,
> backup->compress, backup->discard_source,
> backup->filter_node_name,
> -&perf,
> +&perf, NULL,
> backup->on_source_error,
> backup->on_target_error,
> job_flags, NULL, NULL, txn, errp);
> diff --git a/include/block/block_int-global-state.h
> b/include/block/block_int-global-state.h
> index f0c642b194..7332680087 100644
> --- a/include/block/block_int-global-state.h
> +++ b/include/block/block_int-global-state.h
> @@ -179,6 +179,7 @@ void mirror_start(const char *job_id, BlockDriverState
> *bs,
> * @bitmap_mode: The bitmap synchronization policy to use.
> * @perf: Performance options. All actual fields assumed to be present,
> *all ".has_*" fields are ignored.
> + * @cbw_opts: Additional options to configure cbw filter with.
from this, I'd have guessed the other way round ;) should the precedence be
made explicit somewhere? or, for this particular option, should the higher
value win? but such logic might quickly get complicated once more parameters
need to be handled..
> * @on_source_error: The action to take upon error reading from the source.
> * @on_target_error: The action to take upon error writing to the target.
> * @creation_flags: Flags that control the behavior of the Job lifetime.
> @@ -198,6 +199,7 @@ BlockJob *backup_job_create(const char *job_id,
> BlockDriverState *bs,
> bool compress, bool discard_source,
> const char *filter_node_name,
> BackupPerf *perf,
> +QDict *cbw_opts,
> BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> int creation_flags,
>
Re: [pve-devel] [RFC qemu 7/7] block/backup: set callback for cbw errors
Quoting Fiona Ebner (2024-06-10 14:59:42)
> The callback is invoked when cbw is configured to not break the guest
> write and will abort a backup job immediately. Currently the backup
> has to wait for the rest of the block copy operation to finish before
> checking the cbw error state.
>
> Signed-off-by: Fiona Ebner
> ---
>
> Note for testers: if e.g. the PBS is compeletly unreachable, the
> backup job still will need to wait until the in-flight request is
> aborted after 15 minutes. But the guest writes should be fast again.
could we improve that by checking the status in the pbs-qemu lib periodically,
and aborting there as well?
how is the bitmap handled in case of a cbw-timeout/error?
>
> block/backup.c | 41 +
> 1 file changed, 41 insertions(+)
>
> diff --git a/block/backup.c b/block/backup.c
> index ba153110d3..43d34ce4c2 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -32,6 +32,45 @@
>
> static const BlockJobDriver backup_job_driver;
>
> +typedef struct {
> +Job *job;
> +int ret;
> +} BackupOnCbwError;
> +
> +static void backup_on_cbw_error_cb_bh(void *opaque)
> +{
> +BackupOnCbwError *data = opaque;
> +if (data->job) {
> +WITH_JOB_LOCK_GUARD() {
> +if (!job_is_completed_locked(data->job)) {
> +error_report("backup was cancelled because of
> copy-before-write error: %s",
> + strerror(-data->ret));
> +job_cancel_locked(data->job, true);
> +}
> +}
> +} else {
> +error_report("backup_on_cbw_error_cb_bh: no job! Error: %s",
> strerror(-data->ret));
> +}
> +
> +g_free(data);
> +}
> +
> +static void backup_on_cbw_error_cb(void *opaque, int ret)
> +{
> +BackupOnCbwError *data = g_new0(BackupOnCbwError, 1);
> +data->job = opaque;
> +data->ret = ret;
> +
> +/*
> + * backup_cancel() cannot run in coroutine context.
> + */
> +if (qemu_in_coroutine()) {
> +aio_bh_schedule_oneshot(qemu_get_aio_context(),
> backup_on_cbw_error_cb_bh, data);
> +} else {
> +backup_on_cbw_error_cb_bh(data);
> +}
> +}
> +
> static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
> {
> BdrvDirtyBitmap *bm;
> @@ -477,6 +516,8 @@ BlockJob *backup_job_create(const char *job_id,
> BlockDriverState *bs,
> goto error;
> }
>
> +bdrv_cbw_set_error_cb(cbw, backup_on_cbw_error_cb, job);
> +
> job->cbw = cbw;
> job->source_bs = bs;
> job->target_bs = target;
> --
> 2.39.2
>
>
>
> ___
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC qemu] fix #3231+#3631: PVE backup: fail backup rather than guest write when backup target cannot be reached or is too slow
Quoting Fiona Ebner (2024-06-10 14:59:35) > A long-standing issue with VM backups in Proxmox VE is that a slow or > unreachable target would lead to a copy-before-write (cbw) operation > to break the guest write rather than abort the backup. This is > unexpected to users and the will end up without a successful backup > and without a working guest in such cases. This series prevents the > latter by changing the behavior to fail the backup instead of the > guest write. > > This is done by re-using the already existing 'on-cbw-error' and > 'cbw-timeout' options that are already used for fleecing and having > regular backup also check for the cbw's snapshot_error (unfortunately > this becomes a bit of a misnomer). If a given copy-before-write > operation cannot complete within 45 seconds, it's extremely likely > that aborting the backup is the better choice than keeping the guest > IO blocked. > > Just checking for the error already makes it work (i.e. without the > last two patches), but backup can only check the error at the end. To > abort backup immediately, an error callback for the copy-before-write > node is introduced. A potential alternative would be give the > block-copy operation a pointer to the snapshot_error and have it check > it during its operation, but my initial attempt failed. Likely I > missed adapting certain logic that checks for whether the block-copy > operation failed and it's questionable if this approach would be > cleaner. An error callback is nice and explicit. > > Note for testers: if e.g. the PBS is compeletly unreachable, the > backup job still will need to wait until the in-flight request is > aborted after 15 minutes. But the guest writes should be fast again. > > Should it really be required to make the option more flexible, i.e. > allow users to specify a custom timeout or go back to the old behavior > then the 'backup' QMP call can be extended with those parameters. > > Unfortunately, this is a non-trivial amount of code to make it work, > but there is quite a bit of boilerplate and some comments, so > hopefully the logic is straight-forward enough. this sentence here made me except a lot worse ;) code seems very straight-forward and clean, two small comments inline. not sure whether we want to entangle this with 9.0, but I think this could be applied soonish after some more in-depth testing, since it should solve a pretty big pain point user consistenly run into.. I am sure we will have users clamoring for a configurable timeout soon after though ;) > > The first patch can be applied regardless of whether we want to go > with this or not. > > > Fiona Ebner (7): > PVE backup: fleecing: properly set minimum cluster size > block/copy-before-write: allow passing additional options for > bdrv_cbw_append() > block/backup: allow passing additional options for copy-before-write > upon job creation > block/backup: make cbw error also fail backup that does not use > fleecing > fix #3231+#3631: PVE backup: add timeout for copy-before-write > operations and fail backup instead of guest writes > block/copy-before-write: allow specifying error callback > block/backup: set callback for cbw errors > > block/backup.c | 57 +- > block/copy-before-write.c | 41 +++--- > block/copy-before-write.h | 9 +++- > block/replication.c| 2 +- > blockdev.c | 2 +- > include/block/block_int-global-state.h | 2 + > pve-backup.c | 13 +- > 7 files changed, 115 insertions(+), 11 deletions(-) > > -- > 2.39.2 > > > > ___ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage v3] fix #4272: btrfs: add rename feature
Aaron Lauterer writes:
> gave it a try and it does what it should.
> by enabling the rename feature only for `raw` we avoid potential pitfalls if
> we
> encounter a non regular situation on BTRFS. For example, an
> images/{vmid}/vm-{vmid}-disk-X.qcow2 file directly instead of the
> images/{vmid}/vm-{vmid}-disk-X/disk.raw as is the way the BTRFS plugin handles
> it in subvolumes.
>
> But if we add the following diff, it seems to handle the case of a qcow2 file
> in
> the same directory structure just fine:
Did you try it without your patch? I tested it here and it seemed to
work from my limited testing.
--
Maximiliano
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage v3] fix #4272: btrfs: add rename feature
Without my changes I get the following error when I try to do it with
the manually places qcow2 file:
Storage does not support moving of this disk to another VM (500)
And if I enable qcow2 for the rename feature without adding the format
check in the rename_volume function:
diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index 7376ae4..56730d1 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -619,7 +619,7 @@ sub volume_has_feature {
current => { qcow2 => 1, raw => 1, vmdk => 1 },
},
rename => {
- current => { raw => 1 },
+ current => { qcow2 => 1, raw => 1 },
},
};
it fails with the following error:
internal error: bad disk name: 104/vm-104-disk-1.qcow2 at
/usr/share/perl5/PVE/Storage/BTRFSPlugin.pm: 951
On 2024-07-05 14:41, Maximiliano Sandoval wrote:
Aaron Lauterer writes:
gave it a try and it does what it should.
by enabling the rename feature only for `raw` we avoid potential pitfalls if we
encounter a non regular situation on BTRFS. For example, an
images/{vmid}/vm-{vmid}-disk-X.qcow2 file directly instead of the
images/{vmid}/vm-{vmid}-disk-X/disk.raw as is the way the BTRFS plugin handles
it in subvolumes.
But if we add the following diff, it seems to handle the case of a qcow2 file in
the same directory structure just fine:
Did you try it without your patch? I tested it here and it seemed to
work from my limited testing.
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH storage v4] fix #4272: btrfs: add rename feature
Adds the ability to change the owner of a guest image.
Btrfs does not need special commands to rename a subvolume and this can
be achieved the same as in Storage/plugin.pm's rename_volume taking
special care of how the directory structure used by Btrfs.
Signed-off-by: Maximiliano Sandoval
---
Differences from v3:
- add qcow2 and vmdk support for rename
- remove unused $ppath variable
Differences from v2:
- use indices instead of assigning to undef 5 times
Differences from v1:
- avoid assigning unused values of returned list to variables
src/PVE/Storage/BTRFSPlugin.pm | 33 +
1 file changed, 33 insertions(+)
diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index 42815cb..385ac30 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -618,6 +618,9 @@ sub volume_has_feature {
base => { qcow2 => 1, raw => 1, vmdk => 1 },
current => { qcow2 => 1, raw => 1, vmdk => 1 },
},
+ rename => {
+ current => { qcow2 => 1, raw => 1, vmdk => 1 },
+ },
};
my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
$class->parse_volname($volname);
@@ -930,4 +933,34 @@ sub volume_import {
return "$storeid:$volname";
}
+sub rename_volume {
+my ($class, $scfg, $storeid, $source_volname, $target_vmid,
$target_volname) = @_;
+die "no path found\n" if !$scfg->{path};
+
+my $format = ($class->parse_volname($source_volname))[6];
+
+if ($format ne 'raw' && $format ne 'subvol') {
+ return $class->SUPER::rename_volume($scfg, $storeid, $source_volname,
$target_vmid, $target_volname);
+}
+
+$target_volname = $class->find_free_diskname($storeid, $scfg,
$target_vmid, $format, 1)
+ if !$target_volname;
+$target_volname = "$target_vmid/$target_volname";
+
+my $basedir = $class->get_subdir($scfg, 'images');
+
+mkpath "${basedir}/${target_vmid}";
+my $source_dir = raw_name_to_dir($source_volname);
+my $target_dir = raw_name_to_dir($target_volname);
+
+my $old_path = "${basedir}/${source_dir}";
+my $new_path = "${basedir}/${target_dir}";
+
+die "target volume '${target_volname}' already exists\n" if -e $new_path;
+rename $old_path, $new_path ||
+ die "rename '$old_path' to '$new_path' failed - $!\n";
+
+return "${storeid}:$target_volname";
+}
+
1
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage v3] fix #4272: btrfs: add rename feature
Aaron Lauterer writes:
> gave it a try and it does what it should.
> by enabling the rename feature only for `raw` we avoid potential pitfalls if
> we
> encounter a non regular situation on BTRFS. For example, an
> images/{vmid}/vm-{vmid}-disk-X.qcow2 file directly instead of the
> images/{vmid}/vm-{vmid}-disk-X/disk.raw as is the way the BTRFS plugin handles
> it in subvolumes.
>
> But if we add the following diff, it seems to handle the case of a qcow2 file
> in
> the same directory structure just fine:
>
> diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
> index 7376ae4..143442c 100644
> --- a/src/PVE/Storage/BTRFSPlugin.pm
> +++ b/src/PVE/Storage/BTRFSPlugin.pm
> @@ -619,7 +619,7 @@ sub volume_has_feature {
> current => { qcow2 => 1, raw => 1, vmdk => 1 },
> },
> rename => {
> - current => { raw => 1 },
> + current => { qcow2 => 1, raw => 1},
> },
> };
>
> @@ -939,6 +939,10 @@ sub rename_volume {
>
> my $format = ($class->parse_volname($source_volname))[6];
>
> +if ($format ne 'raw' && $format ne 'subvol') {
> + return $class->SUPER::rename_volume($scfg, $storeid, $source_volname,
> $target_vmid, $target_volname);
> +}
> +
> my $ppath = $class->filesystem_path($scfg, $source_volname);
>
> $target_volname = $class->find_free_diskname($storeid, $scfg,
> $target_vmid,
> $format, 1)
>
>
> Since we do have that in the other functions (alloc_image, free_image), we
> might
> want to add it here as well, just to be safe.
>
> If we aren't concerned about this, then consider this:
>
> Reviewed-By: Aaron Lauterer
> Tested-By: Aaron Lauterer
I added your suggestion on v4.
--
Maximiliano
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager v8 03/13] ui: dc: backup: allow to set custom job id in advanced settings
This might be useful if somebody wants to match on the new
'backup-job' field in a notification match rule.
Signed-off-by: Lukas Wagner
---
www/manager6/dc/Backup.js | 4
www/manager6/panel/BackupAdvancedOptions.js | 23 +
2 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
index 4ba80b31..381402ca 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -45,10 +45,6 @@ Ext.define('PVE.dc.BackupEdit', {
Proxmox.Utils.assemble_field_data(values, { 'delete':
'notification-target' });
}
- if (!values.id && isCreate) {
- values.id = 'backup-' +
Ext.data.identifier.Uuid.Global.generate().slice(0, 13);
- }
-
let selMode = values.selMode;
delete values.selMode;
diff --git a/www/manager6/panel/BackupAdvancedOptions.js
b/www/manager6/panel/BackupAdvancedOptions.js
index 7dd19f96..acb2fbd0 100644
--- a/www/manager6/panel/BackupAdvancedOptions.js
+++ b/www/manager6/panel/BackupAdvancedOptions.js
@@ -37,6 +37,10 @@ Ext.define('PVE.panel.BackupAdvancedOptions', {
return {};
}
+ if (!formValues.id && me.isCreate) {
+ formValues.id = 'backup-' +
Ext.data.identifier.Uuid.Global.generate().slice(0, 13);
+ }
+
let options = {};
if (!me.isCreate) {
@@ -108,6 +112,25 @@ Ext.define('PVE.panel.BackupAdvancedOptions', {
},
items: [
+ {
+ xtype: 'pveTwoColumnContainer',
+ startColumn: {
+ xtype: 'pmxDisplayEditField',
+ vtype: 'ConfigId',
+ fieldLabel: gettext('Job ID'),
+ emptyText: gettext('Autogenerate'),
+ name: 'id',
+ allowBlank: true,
+ cbind: {
+ editable: '{isCreate}',
+ },
+ },
+ endFlex: 2,
+ endColumn: {
+ xtype: 'displayfield',
+ value: gettext('Can be used in notification matchers to match
this job.'),
+ },
+ },
{
xtype: 'pveTwoColumnContainer',
startColumn: {
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager v8 02/13] api: jobs: vzdump: pass job 'job-id' parameter
This allows us to access us the backup job id in the send_notification
function, where we can set it as metadata for the notification.
The 'job-id' parameter can only be used by 'root@pam' to prevent
abuse. This has the side effect that manually triggered backup jobs
cannot have the 'job-id' parameter at the moment. To mitigate that,
manually triggered backup jobs could be changed so that they
are not performed by a direct API call by the UI, but by requesting
pvescheduler to execute the job in the near future (similar to how
manually triggered replication jobs work).
Signed-off-by: Lukas Wagner
---
PVE/API2/Backup.pm | 2 +-
PVE/API2/VZDump.pm | 13 +++--
PVE/Jobs/VZDump.pm | 4 +++-
PVE/VZDump.pm | 6 +++---
4 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index 88140323..48598b8f 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -45,7 +45,7 @@ sub assert_param_permission_common {
my ($rpcenv, $user, $param, $is_delete) = @_;
return if $user eq 'root@pam'; # always OK
-for my $key (qw(tmpdir dumpdir script)) {
+for my $key (qw(tmpdir dumpdir script job-id)) {
raise_param_exc({ $key => "Only root may set this option."}) if exists
$param->{$key};
}
diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index 7f92e7ec..15c9b0dc 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -42,8 +42,8 @@ __PACKAGE__->register_method ({
permissions => {
description => "The user needs 'VM.Backup' permissions on any VM, and "
."'Datastore.AllocateSpace' on the backup storage (and fleecing
storage when fleecing "
- ."is used). The 'tmpdir', 'dumpdir' and 'script' parameters are
restricted to the "
- ."'root\@pam' user. The 'maxfiles' and 'prune-backups' settings
require "
+ ."is used). The 'tmpdir', 'dumpdir', 'script' and 'job-id'
parameters are restricted "
+ ."to the 'root\@pam' user. The 'maxfiles' and 'prune-backups'
settings require "
."'Datastore.Allocate' on the backup storage. The 'bwlimit',
'performance' and "
."'ionice' parameters require 'Sys.Modify' on '/'.",
user => 'all',
@@ -53,6 +53,15 @@ __PACKAGE__->register_method ({
parameters => {
additionalProperties => 0,
properties => PVE::VZDump::Common::json_config_properties({
+ 'job-id' => {
+ description => "The ID of the backup job. If set, the
'backup-job' metadata field"
+ . " of the backup notification will be set to this value.
Only root\@pam"
+ . " can set this parameter.",
+ type => 'string',
+ format => 'pve-configid',
+ maxLength => 256,
+ optional => 1,
+ },
stdout => {
type => 'boolean',
description => "Write tar to stdout, not to a file.",
diff --git a/PVE/Jobs/VZDump.pm b/PVE/Jobs/VZDump.pm
index b8e57945..2dad3f55 100644
--- a/PVE/Jobs/VZDump.pm
+++ b/PVE/Jobs/VZDump.pm
@@ -12,7 +12,7 @@ use PVE::API2::VZDump;
use base qw(PVE::VZDump::JobBase);
sub run {
-my ($class, $conf) = @_;
+my ($class, $conf, $job_id) = @_;
my $props = $class->properties();
# remove all non vzdump related options
@@ -20,6 +20,8 @@ sub run {
delete $conf->{$opt} if !defined($props->{$opt});
}
+$conf->{'job-id'} = $job_id;
+
# Required as string parameters # FIXME why?! we could just check ref()
for my $key (keys $PVE::VZDump::Common::PROPERTY_STRINGS->%*) {
if ($conf->{$key} && ref($conf->{$key}) eq 'HASH') {
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 8dbcc4a9..f1a6b220 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -483,6 +483,7 @@ sub send_notification {
my ($self, $tasklist, $total_time, $err, $detail_pre, $detail_post) = @_;
my $opts = $self->{opts};
+my $job_id = $opts->{'job-id'};
my $mailto = $opts->{mailto};
my $cmdline = $self->{cmdline};
my $policy = $opts->{mailnotification} // 'always';
@@ -528,13 +529,12 @@ sub send_notification {
};
my $fields = {
- # TODO: There is no straight-forward way yet to get the
- # backup job id here... (I think pvescheduler would need
- # to pass that to the vzdump call?)
type => "vzdump",
# Hostname (without domain part)
hostname => PVE::INotify::nodename(),
};
+# Add backup-job metadata field in case this is a backup job.
+$fields->{'job-id'} = $job_id if $job_id;
my $severity = $failed ? "error" : "info";
my $email_configured = $mailto && scalar(@$mailto);
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager v8 04/13] api: notification: add API for getting known metadata fields/values
This new API route returns known notification metadata fields and
a list of known possible values. This will be used by the UI to
provide suggestions when adding/modifying match rules.
Signed-off-by: Lukas Wagner
---
PVE/API2/Cluster/Notifications.pm | 139 ++
1 file changed, 139 insertions(+)
diff --git a/PVE/API2/Cluster/Notifications.pm
b/PVE/API2/Cluster/Notifications.pm
index 68fdda2a..2b202c28 100644
--- a/PVE/API2/Cluster/Notifications.pm
+++ b/PVE/API2/Cluster/Notifications.pm
@@ -79,12 +79,151 @@ __PACKAGE__->register_method ({
{ name => 'endpoints' },
{ name => 'matchers' },
{ name => 'targets' },
+ { name => 'matcher-fields' },
+ { name => 'matcher-field-values' },
];
return $result;
}
});
+__PACKAGE__->register_method ({
+name => 'get_matcher_fields',
+path => 'matcher-fields',
+method => 'GET',
+description => 'Returns known notification metadata fields',
+permissions => {
+ check => ['or',
+ ['perm', '/mapping/notifications', ['Mapping.Modify']],
+ ['perm', '/mapping/notifications', ['Mapping.Audit']],
+ ],
+},
+protected => 0,
+parameters => {
+ additionalProperties => 0,
+ properties => {},
+},
+returns => {
+ type => 'array',
+ items => {
+ type => 'object',
+ properties => {
+ name => {
+ description => 'Name of the field.',
+ type => 'string',
+ },
+ },
+ },
+ links => [ { rel => 'child', href => '{name}' } ],
+},
+code => sub {
+ # TODO: Adapt this API handler once we have a 'notification registry'
+
+ my $result = [
+ { name => 'type' },
+ { name => 'hostname' },
+ { name => 'job-id' },
+ ];
+
+ return $result;
+}
+});
+
+__PACKAGE__->register_method ({
+name => 'get_matcher_field_values',
+path => 'matcher-field-values',
+method => 'GET',
+description => 'Returns known notification metadata fields and their known
values',
+permissions => {
+ check => ['or',
+ ['perm', '/mapping/notifications', ['Mapping.Modify']],
+ ['perm', '/mapping/notifications', ['Mapping.Audit']],
+ ],
+},
+protected => 1,
+parameters => {
+ additionalProperties => 0,
+},
+returns => {
+ type => 'array',
+ items => {
+ type => 'object',
+ properties => {
+ 'value' => {
+ description => 'Notification metadata value known by the
system.',
+ type => 'string'
+ },
+ 'comment' => {
+ description => 'Additional comment for this value.',
+ type => 'string',
+ optional => 1,
+ },
+ 'field' => {
+ description => 'Field this value belongs to.',
+ type => 'string',
+ },
+ },
+ },
+},
+code => sub {
+ # TODO: Adapt this API handler once we have a 'notification registry'
+ my $rpcenv = PVE::RPCEnvironment::get();
+ my $user = $rpcenv->get_user();
+
+ my $values = [
+ {
+ value => 'package-updates',
+ field => 'type',
+ },
+ {
+ value => 'fencing',
+ field => 'type',
+ },
+ {
+ value => 'replication',
+ field => 'type',
+ },
+ {
+ value => 'vzdump',
+ field => 'type',
+ },
+ {
+ value => 'system-mail',
+ field => 'type',
+ },
+ ];
+
+ # Here we need a manual permission check.
+ if ($rpcenv->check($user, "/", ["Sys.Audit"], 1)) {
+ for my $backup_job (@{PVE::API2::Backup->index({})}) {
+ push @$values, {
+ value => $backup_job->{id},
+ comment => $backup_job->{comment},
+ field => 'job-id'
+ };
+ }
+ }
+ # The API call returns only returns jobs for which the user
+ # has adequate permissions.
+ for my $sync_job (@{PVE::API2::ReplicationConfig->index({})}) {
+ push @$values, {
+ value => $sync_job->{id},
+ comment => $sync_job->{comment},
+ field => 'job-id'
+ };
+ }
+
+ for my $node (@{PVE::Cluster::get_nodelist()}) {
+ push @$values, {
+ value => $node,
+ field => 'hostname',
+ }
+ }
+
+ return $values;
+}
+});
+
__PACKAGE__->register_method ({
name => 'endpoints_index',
path => 'endpoints',
--
2.39.2
___
pve-devel ma
[pve-devel] [PATCH manager v8 05/13] ui: utils: add overrides for translatable notification fields/values
Signed-off-by: Lukas Wagner
---
www/manager6/Utils.js | 11 +++
1 file changed, 11 insertions(+)
diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index f5608944..5b9d86ca 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -2059,6 +2059,17 @@ Ext.define('PVE.Utils', {
zfscreate: [gettext('ZFS Storage'), gettext('Create')],
zfsremove: ['ZFS Pool', gettext('Remove')],
});
+
+ Proxmox.Utils.overrideNotificationFieldName({
+ 'job-id': gettext('Job ID'),
+ });
+
+ Proxmox.Utils.overrideNotificationFieldValue({
+ 'package-updates': gettext('Package updates are available'),
+ 'vzdump': gettext('Backup notifications'),
+ 'replication': gettext('Replication job notifications'),
+ 'fencing': gettext('Node fencing notifications'),
+ });
},
});
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH docs v8 12/13] notifications: match-field 'exact'-mode can now match multiple values
Signed-off-by: Lukas Wagner --- notifications.adoc | 18 ++ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/notifications.adoc b/notifications.adoc index acca19b..bdfebd0 100644 --- a/notifications.adoc +++ b/notifications.adoc @@ -233,11 +233,16 @@ configurable schedule. Field Matching Rules Notifications have a selection of metadata fields that can be matched. +When using `exact` as a matching mode, a `,` can be used as a separator. +The matching rule then matches if the metadata field has *any* of the specified +values. * `match-field exact:type=vzdump` Only match notifications about backups. +* `match-field exact:type=replication,fencing` Match `replication` and `fencing` notifications. * `match-field regex:hostname=^.+\.example\.com$` Match the hostname of the node. + If a matched metadata field does not exist, the notification will not be matched. For instance, a `match-field regex:hostname=.*` directive will only match @@ -279,18 +284,7 @@ matcher: backup-failures comment Send notifications about backup failures to one group of admins matcher: cluster-failures -match-field exact:type=replication -match-field exact:type=fencing -mode any -target cluster-admins -comment Send cluster-related notifications to other group of admins - - -The last matcher could also be rewritten using a field matcher with a regular -expression: - -matcher: cluster-failures -match-field regex:type=^(replication|fencing)$ +match-field exact:type=replication,fencing target cluster-admins comment Send cluster-related notifications to other group of admins -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH widget-toolkit v8 09/13] notification: matcher: move match-calendar fields to panel
Also introduce a local viewModel that is linked to a parent viewModel,
allowing us to move the formulas to the panel.
This should make the code more cohesive and easier to follow.
No functional changes.
Signed-off-by: Lukas Wagner
Tested-by: Maximiliano Sandoval
---
src/window/NotificationMatcherEdit.js | 92 +--
1 file changed, 60 insertions(+), 32 deletions(-)
diff --git a/src/window/NotificationMatcherEdit.js
b/src/window/NotificationMatcherEdit.js
index 559b405..50145e3 100644
--- a/src/window/NotificationMatcherEdit.js
+++ b/src/window/NotificationMatcherEdit.js
@@ -389,15 +389,6 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
return record?.get('type') === 'match-severity';
},
},
- typeIsMatchCalendar: {
- bind: {
- bindTo: '{selectedRecord}',
- deep: true,
- },
- get: function(record) {
- return record?.get('type') === 'match-calendar';
- },
- },
matchSeverityValue: {
bind: {
bindTo: '{selectedRecord}',
@@ -417,26 +408,6 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
return record?.get('data')?.value;
},
},
- matchCalendarValue: {
- bind: {
- bindTo: '{selectedRecord}',
- deep: true,
- },
- set: function(value) {
- let me = this;
- let record = me.get('selectedRecord');
- let currentData = record.get('data');
- record.set({
- data: {
- ...currentData,
- value: value,
- },
- });
- },
- get: function(record) {
- return record?.get('data')?.value;
- },
- },
rootMode: {
bind: {
bindTo: '{selectedRecord}',
@@ -995,6 +966,58 @@ Ext.define('Proxmox.panel.NotificationMatchRuleSettings', {
['unknown', gettext('Unknown')],
],
},
+ {
+ xtype: 'pmxNotificationMatchCalendarSettings',
+ },
+],
+});
+
+Ext.define('Proxmox.panel.MatchCalendarSettings', {
+extend: 'Ext.panel.Panel',
+xtype: 'pmxNotificationMatchCalendarSettings',
+border: false,
+layout: 'anchor',
+// Hide initially to avoid glitches when opening the window
+hidden: true,
+bind: {
+ hidden: '{!typeIsMatchCalendar}',
+},
+viewModel: {
+ // parent is set in `initComponents`
+ formulas: {
+ typeIsMatchCalendar: {
+ bind: {
+ bindTo: '{selectedRecord}',
+ deep: true,
+ },
+ get: function(record) {
+ return record?.get('type') === 'match-calendar';
+ },
+ },
+
+ matchCalendarValue: {
+ bind: {
+ bindTo: '{selectedRecord}',
+ deep: true,
+ },
+ set: function(value) {
+ let me = this;
+ let record = me.get('selectedRecord');
+ let currentData = record.get('data');
+ record.set({
+ data: {
+ ...currentData,
+ value: value,
+ },
+ });
+ },
+ get: function(record) {
+ return record?.get('data')?.value;
+ },
+ },
+ },
+},
+items: [
{
xtype: 'proxmoxKVComboBox',
fieldLabel: gettext('Timespan to match'),
@@ -1003,11 +1026,8 @@
Ext.define('Proxmox.panel.NotificationMatchRuleSettings', {
editable: true,
displayField: 'key',
field: 'value',
- // Hide initially to avoid glitches when opening the window
- hidden: true,
bind: {
value: '{matchCalendarValue}',
- hidden: '{!typeIsMatchCalendar}',
disabled: '{!typeIsMatchCalender}',
},
@@ -1017,6 +1037,14 @@
Ext.define('Proxmox.panel.NotificationMatchRuleSettings', {
],
},
],
+
+initComponent: function() {
+ let me = this;
+ Ext.apply(me.viewModel, {
+ parent: me.up('pmxNotificationMatchRulesEditPanel').getViewModel(),
+ });
+ me.callParent();
+},
});
Ext.define('Proxmox.panel.MatchFieldSettings', {
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH pve-guest-common v8 01/13] vzdump: common: allow 'job-id' as a parameter without being in schema
'job-id' is passed when a backup as started as a job and will be
passed to the notification system as matchable metadata. It it
can be considered 'internal'.
Signed-off-by: Lukas Wagner
---
src/PVE/VZDump/Common.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/PVE/VZDump/Common.pm b/src/PVE/VZDump/Common.pm
index 1996c5b..2532b42 100644
--- a/src/PVE/VZDump/Common.pm
+++ b/src/PVE/VZDump/Common.pm
@@ -503,7 +503,7 @@ sub command_line {
foreach my $p (keys %$param) {
next if $p eq 'id' || $p eq 'vmid' || $p eq 'starttime' ||
- $p eq 'dow' || $p eq 'stdout' || $p eq 'enabled';
+ $p eq 'dow' || $p eq 'stdout' || $p eq 'enabled' || $p eq
'job-id';
my $v = $param->{$p};
my $pd = $confdesc->{$p} || die "no such vzdump option '$p'\n";
if ($p eq 'exclude-path') {
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager v8 06/13] d/control: bump proxmox-widget-toolkit dependency to 4.1.4
We need "utils: add mechanism to add and override translatable notification event descriptions in the product specific UIs" otherwise there is an error in the browser console. Signed-off-by: Lukas Wagner --- debian/control | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/debian/control b/debian/control index d4c254d4..bc9c7218 100644 --- a/debian/control +++ b/debian/control @@ -20,7 +20,7 @@ Build-Depends: debhelper-compat (= 13), libtemplate-perl, libtest-mockmodule-perl, lintian, - proxmox-widget-toolkit (>= 4.0.7), + proxmox-widget-toolkit (>= 4.1.4), pve-cluster, pve-container, pve-doc-generator (>= 8.0.5), -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH many v8 00/13] notifications: notification metadata matching improvements
This patch series attempts to improve the user experience when creating notification matchers. Some of the noteworthy changes: - Allow setting a custom backup job ID, similar how we handle it for sync/prune jobs in PBS (to allow recognizable names used in matchers) - New metadata fields: - job-id: Job ID for backup-jobs or replication-jobs - Add an API that enumerates known notification metadata fields/values - Suggest known fields/values in match rule window - Some code clean up for match rule edit window - Extended the 'exact' match-field mode - it now allows setting multiple allowed values, separated via ',': e.g. `match-field exact:type=replication,fencing Originally, I created a separate 'list' match type for this, but since the semantics for a list with one value and 'exact' mode are identical, I decided to just extend 'exact'. This is a safe change since there are are no values where a ',' makes any sense (config IDs, hostnames) NOTE: Might need a versionened break, since the widget-toolkit-patches depend on new APIs provided by pve-manager. If the API is not present, creating matchers with 'match-field' does not work (cannot load lists of known values/fields) Inter-Dependencies: - the widget-toolkit dep in pve-manager needs to be bumped to at least 4.1.4 (we need "utils: add mechanism to add and override translatable notification event descriptions in the product specific UIs", otherwise the UI breaks with the pve-manager patches applied) --> already included a patch for this - widget-toolkit relies on a new API endpoint provided by pve-manager: --> we require a versioned break in widget-toolkit on pve-manager - pve-manager needs bumped pve-guest-common (thx @Fabian) Changelog: - v8: incorporate feedback from @Fabian, thx a lot! - Made 'job-id' API param usable by root@pam only - this should prevent abuse by spoofing job-id, potentially bothering other users with bogus notifications. - Don't set 'job-id' when starting a backup job via 'Run now' in the UI - Add a note to the docs explaining when job-id is set and when not. - Drop already applied patches - v7: incorporated some more feedback from @Fiona, thx! - Fixed error when switching from 'exact' to 'regex' if the text field was empty - rebased to latest master - 'backport' doc improvements from PBS - bumped widget-toolkit dep - v6: incorporate feedback from @Fiona, thx! - rename 'id' -> 'job-id' in VZDump API handler - consolidate 'replication-job'/'backup-job' to 'job-id' - Move 'job-id' setting to advanced tab in backup job edit. - Don't use 'internal' flag to mark translatable fields, since the only field where that's necessary is 'type' for now - so just add a hardcoded check - v5: - Rebased onto latest master, resolving some small conflict - v4: - widget-toolkit: break out changes for the utils module so that they can be applied ahead of time to ease dep bumping - don't show Job IDs in the backup/replication job columns - v3: - Drop already applied patches for `proxmox` - Rebase onto latest master - minor conflict resolution was needed - v2: - include 'type' metadata field for forwarded mails --> otherwise it's not possible to match them - include Maximilliano's T-b trailer in UI patches pve-guest-common: Lukas Wagner (1): vzdump: common: allow 'job-id' as a parameter without being in schema src/PVE/VZDump/Common.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) pve-manager: Lukas Wagner (5): api: jobs: vzdump: pass job 'job-id' parameter ui: dc: backup: allow to set custom job id in advanced settings api: notification: add API for getting known metadata fields/values ui: utils: add overrides for translatable notification fields/values d/control: bump proxmox-widget-toolkit dependency to 4.1.4 PVE/API2/Backup.pm | 2 +- PVE/API2/Cluster/Notifications.pm | 139 PVE/API2/VZDump.pm | 13 +- PVE/Jobs/VZDump.pm | 4 +- PVE/VZDump.pm | 6 +- debian/control | 2 +- www/manager6/Utils.js | 11 ++ www/manager6/dc/Backup.js | 4 - www/manager6/panel/BackupAdvancedOptions.js | 23 9 files changed, 192 insertions(+), 12 deletions(-) proxmox-widget-toolkit: Lukas Wagner (4): notification: matcher: match-field: show known fields/values notification: matcher: move match-field formulas to local viewModel notification: matcher: move match-calendar fields to panel notification: matcher: move match-severity fields to panel src/data/model/NotificationConfig.js | 12 + src/window/NotificationMatcherEdit.js | 613 ++ 2 files changed, 441 insertions(+), 184 deletions
[pve-devel] [PATCH docs v8 13/13] notifications: add note regarding when 'job-id' is set for backups
Signed-off-by: Lukas Wagner --- notifications.adoc | 4 1 file changed, 4 insertions(+) diff --git a/notifications.adoc b/notifications.adoc index bdfebd0..6425e6c 100644 --- a/notifications.adoc +++ b/notifications.adoc @@ -312,6 +312,10 @@ Notification Events | `job-id` | Job ID |=== +NOTE: Backup job notifications only have `job-id` set if the backup job + was executed automatically based on its schedule, but not if it was triggered + manually by the 'Run now' button in the UI. + System Mail Forwarding - -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH docs v8 11/13] notifications: describe new notification metadata fields
Signed-off-by: Lukas Wagner --- notifications.adoc | 20 +++- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/notifications.adoc b/notifications.adoc index 25a9391..acca19b 100644 --- a/notifications.adoc +++ b/notifications.adoc @@ -301,19 +301,21 @@ Notification Events [width="100%",options="header"] |=== -| Event| `type`| Severity | Metadata fields (in addition to `type`) -| System updates available |`package-updates` | `info` | `hostname` -| Cluster node fenced |`fencing` | `error` | `hostname` -| Storage replication failed |`replication` | `error` | - -| Backup finished |`vzdump` | `info` (`error` on failure) | `hostname` -| Mail for root|`system-mail` | `unknown`| - +| Event| `type`| Severity | Metadata fields (in addition to `type`) +| System updates available |`package-updates` | `info` | `hostname` +| Cluster node fenced |`fencing` | `error` | `hostname` +| Storage replication job failed |`replication` | `error` | `hostname`, `job-id` +| Backup succeeded |`vzdump` | `info` | `hostname`, `job-id` (only for backup jobs) +| Backup failed|`vzdump` | `error` | `hostname`, `job-id` (only for backup jobs) +| Mail for root|`system-mail` | `unknown`| `hostname` |=== [width="100%",options="header"] |=== -| Field name | Description -| `type` | Type of the notification -| `hostname` | Hostname, without domain (e.g. `pve1`) +| Field name| Description +| `type`| Type of the notification +| `hostname`| Hostname, without domain (e.g. `pve1`) +| `job-id` | Job ID |=== System Mail Forwarding -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH widget-toolkit v8 08/13] notification: matcher: move match-field formulas to local viewModel
This should make the code more cohesive and easier to follow.
No functional changes.
Signed-off-by: Lukas Wagner
Tested-by: Maximiliano Sandoval
---
src/window/NotificationMatcherEdit.js | 189 +-
1 file changed, 95 insertions(+), 94 deletions(-)
diff --git a/src/window/NotificationMatcherEdit.js
b/src/window/NotificationMatcherEdit.js
index be33efe..559b405 100644
--- a/src/window/NotificationMatcherEdit.js
+++ b/src/window/NotificationMatcherEdit.js
@@ -380,15 +380,6 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
}
return !record.isRoot();
},
- typeIsMatchField: {
- bind: {
- bindTo: '{selectedRecord}',
- deep: true,
- },
- get: function(record) {
- return record?.get('type') === 'match-field';
- },
- },
typeIsMatchSeverity: {
bind: {
bindTo: '{selectedRecord}',
@@ -407,89 +398,13 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
return record?.get('type') === 'match-calendar';
},
},
- matchFieldType: {
- bind: {
- bindTo: '{selectedRecord}',
- deep: true,
- },
- set: function(value) {
- let me = this;
- let record = me.get('selectedRecord');
- let currentData = record.get('data');
-
- let newValue = [];
-
- // Build equivalent regular expression if switching
- // to 'regex' mode
- if (value === 'regex') {
- let regexVal = "^(";
- regexVal += currentData.value.join('|') + ")$";
- newValue.push(regexVal);
- }
-
- record.set({
- data: {
- ...currentData,
- type: value,
- value: newValue,
- },
- });
- },
- get: function(record) {
- return record?.get('data')?.type;
- },
- },
- matchFieldField: {
- bind: {
- bindTo: '{selectedRecord}',
- deep: true,
- },
- set: function(value) {
- let me = this;
- let record = me.get('selectedRecord');
- let currentData = record.get('data');
-
- record.set({
- data: {
- ...currentData,
- field: value,
- // Reset value if field changes
- value: [],
- },
- });
- },
- get: function(record) {
- return record?.get('data')?.field;
- },
- },
- matchFieldValue: {
- bind: {
- bindTo: '{selectedRecord}',
- deep: true,
- },
- set: function(value) {
- let me = this;
- let record = me.get('selectedRecord');
- let currentData = record.get('data');
- record.set({
- data: {
- ...currentData,
- value: value,
- },
- });
- },
- get: function(record) {
- return record?.get('data')?.value;
- },
- },
matchSeverityValue: {
bind: {
bindTo: '{selectedRecord}',
deep: true,
},
set: function(value) {
- let me = this;
- let record = me.get('selectedRecord');
+ let record = this.get('selectedRecord');
let currentData = record.get('data');
record.set({
data: {
@@ -1137,7 +1052,98 @@ Ext.define('Proxmox.panel.MatchFieldSettings', {
},
},
},
+viewModel: {
+ // parent is set in `initComponents`
+ formulas: {
+ typeIsMatchField: {
+ bind: {
+ bindTo: '{selectedRecord}',
+ deep: true,
+ },
+ get: function(record) {
+ return record?.get('type') === 'match-field';
+ },
+ },
+ isRegex: function(get) {
+ return get('matchFieldType') === 'regex';
+ },
+ matchFieldType: {
+ bind: {
+ bindTo: '{selecte
[pve-devel] [PATCH widget-toolkit v8 10/13] notification: matcher: move match-severity fields to panel
Also introduce a local viewModel that is linked to a parent viewModel,
allowing us to move the formulas to the panel.
This should make the code more cohesive and easier to follow.
No functional changes.
Signed-off-by: Lukas Wagner
Tested-by: Maximiliano Sandoval
---
src/window/NotificationMatcherEdit.js | 129 --
1 file changed, 80 insertions(+), 49 deletions(-)
diff --git a/src/window/NotificationMatcherEdit.js
b/src/window/NotificationMatcherEdit.js
index 50145e3..9ab443f 100644
--- a/src/window/NotificationMatcherEdit.js
+++ b/src/window/NotificationMatcherEdit.js
@@ -380,34 +380,7 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
}
return !record.isRoot();
},
- typeIsMatchSeverity: {
- bind: {
- bindTo: '{selectedRecord}',
- deep: true,
- },
- get: function(record) {
- return record?.get('type') === 'match-severity';
- },
- },
- matchSeverityValue: {
- bind: {
- bindTo: '{selectedRecord}',
- deep: true,
- },
- set: function(value) {
- let record = this.get('selectedRecord');
- let currentData = record.get('data');
- record.set({
- data: {
- ...currentData,
- value: value,
- },
- });
- },
- get: function(record) {
- return record?.get('data')?.value;
- },
- },
+
rootMode: {
bind: {
bindTo: '{selectedRecord}',
@@ -944,27 +917,7 @@ Ext.define('Proxmox.panel.NotificationMatchRuleSettings', {
},
},
{
- xtype: 'proxmoxKVComboBox',
- fieldLabel: gettext('Severities to match'),
- isFormField: false,
- allowBlank: true,
- multiSelect: true,
- field: 'value',
- // Hide initially to avoid glitches when opening the window
- hidden: true,
- bind: {
- value: '{matchSeverityValue}',
- hidden: '{!typeIsMatchSeverity}',
- disabled: '{!typeIsMatchSeverity}',
- },
-
- comboItems: [
- ['info', gettext('Info')],
- ['notice', gettext('Notice')],
- ['warning', gettext('Warning')],
- ['error', gettext('Error')],
- ['unknown', gettext('Unknown')],
- ],
+ xtype: 'pmxNotificationMatchSeveritySettings',
},
{
xtype: 'pmxNotificationMatchCalendarSettings',
@@ -1047,6 +1000,84 @@ Ext.define('Proxmox.panel.MatchCalendarSettings', {
},
});
+Ext.define('Proxmox.panel.MatchSeveritySettings', {
+extend: 'Ext.panel.Panel',
+xtype: 'pmxNotificationMatchSeveritySettings',
+border: false,
+layout: 'anchor',
+// Hide initially to avoid glitches when opening the window
+hidden: true,
+bind: {
+ hidden: '{!typeIsMatchSeverity}',
+},
+viewModel: {
+ // parent is set in `initComponents`
+ formulas: {
+ typeIsMatchSeverity: {
+ bind: {
+ bindTo: '{selectedRecord}',
+ deep: true,
+ },
+ get: function(record) {
+ return record?.get('type') === 'match-severity';
+ },
+ },
+ matchSeverityValue: {
+ bind: {
+ bindTo: '{selectedRecord}',
+ deep: true,
+ },
+ set: function(value) {
+ let record = this.get('selectedRecord');
+ let currentData = record.get('data');
+ record.set({
+ data: {
+ ...currentData,
+ value: value,
+ },
+ });
+ },
+ get: function(record) {
+ return record?.get('data')?.value;
+ },
+ },
+ },
+},
+items: [
+ {
+ xtype: 'proxmoxKVComboBox',
+ fieldLabel: gettext('Severities to match'),
+ isFormField: false,
+ allowBlank: true,
+ multiSelect: true,
+ field: 'value',
+ // Hide initially to avoid glitches when opening the window
+ hidden: true,
+ bind: {
+ value: '{matchSeverityValue}',
+ hidden: '{!typeIsMatchSeverity}',
+ disabled: '{!typeIsMatchSeverity}',
+ },
+
+ comboItems: [
+ ['info', gettext('Info')],
+ ['notice', gettext('Notice')],
+ ['war
[pve-devel] [PATCH widget-toolkit v8 07/13] notification: matcher: match-field: show known fields/values
These changes introduce combogrid pickers for the 'field' and 'value'
form elements for 'match-field' match rules. The 'field' picker shows
a list of all known metadata fields, while the 'value' picker shows a
list of all known values, filtered depending on the current value of
'field'.
The list of known fields/values is retrieved from new API endpoints.
Some values are marked 'internal' by the backend. This means that the
'value' field was not user-created (counter example: backup job
IDs) and can therefore be used as a base for translations.
Signed-off-by: Lukas Wagner
Tested-by: Maximiliano Sandoval
---
src/data/model/NotificationConfig.js | 12 ++
src/window/NotificationMatcherEdit.js | 297 +-
2 files changed, 253 insertions(+), 56 deletions(-)
diff --git a/src/data/model/NotificationConfig.js
b/src/data/model/NotificationConfig.js
index e8ebf28..03cf317 100644
--- a/src/data/model/NotificationConfig.js
+++ b/src/data/model/NotificationConfig.js
@@ -15,3 +15,15 @@ Ext.define('proxmox-notification-matchers', {
},
idProperty: 'name',
});
+
+Ext.define('proxmox-notification-fields', {
+extend: 'Ext.data.Model',
+fields: ['name', 'description'],
+idProperty: 'name',
+});
+
+Ext.define('proxmox-notification-field-values', {
+extend: 'Ext.data.Model',
+fields: ['value', 'comment', 'field'],
+idProperty: 'value',
+});
diff --git a/src/window/NotificationMatcherEdit.js
b/src/window/NotificationMatcherEdit.js
index e717ad7..be33efe 100644
--- a/src/window/NotificationMatcherEdit.js
+++ b/src/window/NotificationMatcherEdit.js
@@ -79,7 +79,7 @@ Ext.define('Proxmox.window.NotificationMatcherEdit', {
labelWidth: 120,
},
-width: 700,
+width: 800,
initComponent: function() {
let me = this;
@@ -416,10 +416,22 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
let me = this;
let record = me.get('selectedRecord');
let currentData = record.get('data');
+
+ let newValue = [];
+
+ // Build equivalent regular expression if switching
+ // to 'regex' mode
+ if (value === 'regex') {
+ let regexVal = "^(";
+ regexVal += currentData.value.join('|') + ")$";
+ newValue.push(regexVal);
+ }
+
record.set({
data: {
...currentData,
type: value,
+ value: newValue,
},
});
},
@@ -441,6 +453,8 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
data: {
...currentData,
field: value,
+ // Reset value if field changes
+ value: [],
},
});
},
@@ -549,6 +563,9 @@ Ext.define('Proxmox.panel.NotificationRulesEditPanel', {
column2: [
{
xtype: 'pmxNotificationMatchRuleSettings',
+ cbind: {
+ baseUrl: '{baseUrl}',
+ },
},
],
@@ -601,7 +618,7 @@ Ext.define('Proxmox.panel.NotificationMatchRuleTree', {
let value = data.value;
text = Ext.String.format(gettext("Match field: {0}={1}"),
field, value);
iconCls = 'fa fa-square-o';
- if (!field || !value) {
+ if (!field || !value || (Ext.isArray(value) && !value.length)) {
iconCls += ' internal-error';
}
} break;
@@ -821,6 +838,11 @@ Ext.define('Proxmox.panel.NotificationMatchRuleTree', {
if (type === undefined) {
type = "exact";
}
+
+ if (type === 'exact') {
+ matchedValue = matchedValue.split(',');
+ }
+
return {
type: 'match-field',
data: {
@@ -982,7 +1004,9 @@ Ext.define('Proxmox.panel.NotificationMatchRuleTree', {
Ext.define('Proxmox.panel.NotificationMatchRuleSettings', {
extend: 'Ext.panel.Panel',
xtype: 'pmxNotificationMatchRuleSettings',
+mixins: ['Proxmox.Mixin.CBind'],
border: false,
+layout: 'anchor',
items: [
{
@@ -1000,6 +1024,8 @@ Ext.define('Proxmox.panel.NotificationMatchRuleSettings',
{
['notall', gettext('At least one rule does not match')],
['notany', gettext('No rule matches')],
],
+ // Hide initially to avoid glitches when opening the window
+ hidden: true,
bind: {
hidden: '{!showMatchingMode}',
disabled: '{!showMatchingMode}',
@@ -1011,7 +1037,8 @@ Ext.define('Proxmox.panel.NotificationMatchR
Re: [pve-devel] superseded: [PATCH many v7 00/19] notifications: notification metadata matching improvements
superseded by v8! On 2024-06-10 10:40, Lukas Wagner wrote: > This patch series attempts to improve the user experience when creating > notification matchers. > > Some of the noteworthy changes: > - Fixup inconsistent 'hostname' field. Some notification events sent > the hostname including a domain, while other did not. > This series unifies the behavior, now the field only includes the hostname > without a domain. Also updated the docs to reflect this change. > - Allow setting a custom backup job ID, similar how we handle it for > sync/prune jobs in PBS (to allow recognizable names used in matchers) > - Adding columns for backup job ID/replication job ID in the UI > - New metadata fields: > - job-id: Job ID for backup-jobs or replication-jobs > - Add an API that enumerates known notification metadata fields/values > - Suggest known fields/values in match rule window > - Some code clean up for match rule edit window > - Extended the 'exact' match-field mode - it now allows setting multiple > allowed values, separated via ',': > e.g. `match-field exact:type=replication,fencing > Originally, I created a separate 'list' match type for this, but > since the semantics for a list with one value and 'exact' mode > are identical, I decided to just extend 'exact'. > This is a safe change since there are are no values where a ',' > makes any sense (config IDs, hostnames) > > NOTE: Might need a versionened break, since the widget-toolkit-patches > depend on new APIs provided by pve-manager. If the API is not present, > creating matchers with 'match-field' does not work (cannot load lists > of known values/fields) > > Inter-Dependencies: > - the widget-toolkit dep in pve-manager needs to be bumped > to at least 4.1.4 > (we need "utils: add mechanism to add and override translatable > notification event > descriptions in the product specific UIs", otherwise the UI breaks > with the pve-manager patches applied) --> already included a patch for > this > - widget-toolkit relies on a new API endpoint provided by pve-manager: > --> we require a versioned break in widget-toolkit on pve-manager > > Changelog: > - v7: incorporated some more feedback from @Fiona, thx! > - Fixed error when switching from 'exact' to 'regex' if the text field > was empty > - rebased to latest master > - 'backport' doc improvements from PBS > - bumped widget-toolkit dep > - v6: incorporate feedback from @Fiona, thx! > - rename 'id' -> 'job-id' in VZDump API handler > - consolidate 'replication-job'/'backup-job' to 'job-id' > - Move 'job-id' setting to advanced tab in backup job edit. > - Don't use 'internal' flag to mark translatable fields, since > the only field where that's necessary is 'type' for now - so > just add a hardcoded check > - v5: > - Rebased onto latest master, resolving some small conflict > - v4: > - widget-toolkit: break out changes for the utils module so that they > can be applied ahead of time to ease dep bumping > - don't show Job IDs in the backup/replication job columns > - v3: > - Drop already applied patches for `proxmox` > - Rebase onto latest master - minor conflict resolution was needed > - v2: > - include 'type' metadata field for forwarded mails > --> otherwise it's not possible to match them > - include Maximilliano's T-b trailer in UI patches > > pve-guest-common: > > Lukas Wagner (1): > vzdump: common: allow 'job-id' as a parameter without being in schema > > src/PVE/VZDump/Common.pm | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > > pve-manager: > > Lukas Wagner (9): > api: jobs: vzdump: pass job 'job-id' parameter > ui: dc: backup: send 'job-id' property when starting a backup job > manually > ui: dc: backup: allow to set custom job id in advanced settings > api: replication: add 'job-id' to notification metadata > vzdump: apt: notification: do not include domain in 'hostname' field > api: replication: include 'hostname' field for notifications > api: notification: add API for getting known metadata fields/values > ui: utils: add overrides for translatable notification fields/values > d/control: bump proxmox-widget-toolkit dependency to 4.1.4 > > PVE/API2/APT.pm | 3 +- > PVE/API2/Cluster/Notifications.pm | 139 > PVE/API2/Replication.pm | 4 +- > PVE/API2/VZDump.pm | 8 ++ > PVE/Jobs/VZDump.pm | 4 +- > PVE/VZDump.pm | 14 +- > debian/control | 2 +- > www/manager6/Utils.js | 12 ++ > www/manager6/dc/Backup.js | 7 +- > www/manager6/panel/BackupAdvancedOptions.js | 23 > 10 files changed, 200 insertions(+), 16 deletions(-) > > > pr
[pve-devel] [PATCH v1 proxmox 0/3] Fix #5105: Overhaul TLS Handshake Checking Logic
Fix #5105: Overhaul TLS Handshake Checking Logic This series fixes bug #5105 [1] by overhauling the TLS handshake checking logic, which is performed when using a connection acceptor variant with optional TLS. In the case of PBS (the only place where this is used, to my knowledge), any requests made over plain HTTP are redirected to the same host, but clients are instructed to use HTTPS instead. The TLS handshake checking logic determines whether the client uses HTTP or HTTPS by peeking into the stream buffer -- if the first 5 received bytes look like a TLS handshake fragment, the connection is passed on to OpenSSL before being accepted. Otherwise the connection is assumed to be unencrypted, i.e. plain HTTP. However, this logic contains two errors: 1. The timeout duration is too short - one second is too little 2. When a timeout occurs, the connection is assumed to be unencrypted (and thus plain HTTP) The patches 01 and 02 are mainly done in preparation for patch 03 (which contains the actual fix), improving the overall quality of the code and including the peer's address in error logs. Please see the individual patches for more information. Special thanks go to Stefan Hanreich whose advice helped identifying many individual puzzle pieces comprising this issue. References -- [1]: https://bugzilla.proxmox.com/show_bug.cgi?id=5105 Summary of Changes -- Max Carrara (3): rest-server: connection: clean up accept data flow rest-server: connection: log peer address on error fix #5105: rest-server: connection: overhaul TLS handshake check logic proxmox-rest-server/src/connection.rs | 165 +- 1 file changed, 85 insertions(+), 80 deletions(-) -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v1 proxmox 1/3] rest-server: connection: clean up accept data flow
This adds the structs `AcceptState` and `AcceptFlags` and adapts
relevant method signatures of `AcceptBuilder` accordingly. This makes
it easier to add further parameters in the future.
Signed-off-by: Max Carrara
---
proxmox-rest-server/src/connection.rs | 72 ++-
1 file changed, 38 insertions(+), 34 deletions(-)
diff --git a/proxmox-rest-server/src/connection.rs
b/proxmox-rest-server/src/connection.rs
index 34b585cb..243348c0 100644
--- a/proxmox-rest-server/src/connection.rs
+++ b/proxmox-rest-server/src/connection.rs
@@ -255,6 +255,16 @@ impl From<(ClientSender, InsecureClientSender)> for Sender
{
}
}
+struct AcceptState {
+pub socket: InsecureClientStream,
+pub acceptor: Arc>,
+pub accept_counter: Arc<()>,
+}
+
+struct AcceptFlags {
+pub is_debug: bool,
+}
+
impl AcceptBuilder {
async fn accept_connections(
self,
@@ -285,24 +295,26 @@ impl AcceptBuilder {
continue;
}
+let state = AcceptState {
+socket,
+acceptor,
+accept_counter,
+};
+
+let flags = AcceptFlags {
+is_debug: self.debug,
+};
+
match sender {
Sender::Secure(ref secure_sender) => {
-let accept_future = Self::do_accept_tls(
-socket,
-acceptor,
-accept_counter,
-self.debug,
-secure_sender.clone(),
-);
+let accept_future = Self::do_accept_tls(state, flags,
secure_sender.clone());
tokio::spawn(accept_future);
}
Sender::SecureAndInsecure(ref secure_sender, ref
insecure_sender) => {
let accept_future = Self::do_accept_tls_optional(
-socket,
-acceptor,
-accept_counter,
-self.debug,
+state,
+flags,
secure_sender.clone(),
insecure_sender.clone(),
);
@@ -343,17 +355,11 @@ impl AcceptBuilder {
Ok(socket)
}
-async fn do_accept_tls(
-socket: InsecureClientStream,
-acceptor: Arc>,
-accept_counter: Arc<()>,
-debug: bool,
-secure_sender: ClientSender,
-) {
+async fn do_accept_tls(state: AcceptState, flags: AcceptFlags,
secure_sender: ClientSender) {
let ssl = {
// limit acceptor_guard scope
// Acceptor can be reloaded using the command socket
"reload-certificate" command
-let acceptor_guard = acceptor.lock().unwrap();
+let acceptor_guard = state.acceptor.lock().unwrap();
match openssl::ssl::Ssl::new(acceptor_guard.context()) {
Ok(ssl) => ssl,
@@ -364,7 +370,7 @@ impl AcceptBuilder {
}
};
-let secure_stream = match tokio_openssl::SslStream::new(ssl, socket) {
+let secure_stream = match tokio_openssl::SslStream::new(ssl,
state.socket) {
Ok(stream) => stream,
Err(err) => {
log::error!("failed to create SslStream using ssl and
connection socket - {err}");
@@ -381,41 +387,39 @@ impl AcceptBuilder {
match result {
Ok(Ok(())) => {
-if secure_sender.send(Ok(secure_stream)).await.is_err() &&
debug {
+if secure_sender.send(Ok(secure_stream)).await.is_err() &&
flags.is_debug {
log::error!("detected closed connection channel");
}
}
Ok(Err(err)) => {
-if debug {
+if flags.is_debug {
log::error!("https handshake failed - {err}");
}
}
Err(_) => {
-if debug {
+if flags.is_debug {
log::error!("https handshake timeout");
}
}
}
-drop(accept_counter); // decrease reference count
+drop(state.accept_counter); // decrease reference count
}
async fn do_accept_tls_optional(
-socket: InsecureClientStream,
-acceptor: Arc>,
-accept_counter: Arc<()>,
-debug: bool,
+state: AcceptState,
+flags: AcceptFlags,
secure_sender: ClientSender,
insecure_sender: InsecureClientSender,
) {
let client_initiates_handshake = {
#[cfg(feature = "rate-limited-stream")]
-let socket = socket.inner();
+let socket_ref = state.socket.inner();
#[cfg(not(feature = "rate-limited-stream"))]
-let socket = &socket;
+let socket_ref = &state.socket;
-match Self::wait_for_
[pve-devel] [PATCH v1 proxmox 3/3] fix #5105: rest-server: connection: overhaul TLS handshake check logic
On rare occasions, the TLS "client hello" message [1] is delayed after
a connection with the server was established, which causes HTTPS
requests to fail before TLS was even negotiated. In these cases, the
server would incorrectly respond with "HTTP/1.1 400 Bad Request"
instead of closing the connection (or similar).
The reasons for the "client hello" being delayed seem to vary; one
user noticed that the issue went away completely after they turned off
UFW [2]. Another user noticed (during private correspondence) that the
issue only appeared when connecting to their PBS instance via WAN, but
not from within their VPN. In the WAN case a firewall was also
present. The same user kindly provided tcpdumps and strace logs on
request.
The issue was finally reproduced with the following Python script:
import socket
import time
HOST: str = ...
PORT: int = ...
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock:
sock.connect((HOST, PORT))
time.sleep(1.5) # simulate firewall / proxy / etc. delay
sock.sendall(b"\x16\x03\x01\x02\x00")
data = sock.recv(256)
print(data)
The additional delay before sending the first 5 bytes of the "client
hello" message causes the handshake checking logic to incorrectly fall
back to plain HTTP.
All of this is fixed by the following:
1. Increase the timeout duration to 10 seconds (from 1)
2. Instead of falling back to plain HTTP, refuse to accept the
connection if the TLS handshake wasn't initiated before the
timeout limit is reached
3. Only accept plain HTTP if the first 5 bytes do not correspond to
a TLS handshake fragment [3]
4. Do not take the last number of bytes that were in the buffer into
account; instead, only perform the actual handshake check if
5 bytes are in the peek buffer
Regarding 1.: This should be generous enough for any client to be able
to initiate a TLS handshake, despite its surrounding circumstances.
Regarding 4.: While this is not 100% related to the issue, altering
this condition simplifies the overall logic and removes a potential
source of unintended behaviour.
[1]: https://www.rfc-editor.org/rfc/rfc8446.html#section-4.1.2
[2]:
https://forum.proxmox.com/threads/disable-default-http-redirects-on-8007.142312/post-675352
[3]: https://www.rfc-editor.org/rfc/rfc8446.html#section-4.1.2
Signed-off-by: Max Carrara
Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5105
---
proxmox-rest-server/src/connection.rs | 69 ---
1 file changed, 31 insertions(+), 38 deletions(-)
diff --git a/proxmox-rest-server/src/connection.rs
b/proxmox-rest-server/src/connection.rs
index 470021d7..a2d54b18 100644
--- a/proxmox-rest-server/src/connection.rs
+++ b/proxmox-rest-server/src/connection.rs
@@ -418,70 +418,63 @@ impl AcceptBuilder {
secure_sender: ClientSender,
insecure_sender: InsecureClientSender,
) {
-let peer = state.peer;
+const CLIENT_HANDSHAKE_TIMEOUT: Duration = Duration::from_secs(10);
-let client_initiates_handshake = {
-#[cfg(feature = "rate-limited-stream")]
-let socket_ref = state.socket.inner();
+let peer = state.peer;
-#[cfg(not(feature = "rate-limited-stream"))]
-let socket_ref = &state.socket;
+#[cfg(feature = "rate-limited-stream")]
+let socket_ref = state.socket.inner();
-match Self::wait_for_client_tls_handshake(socket_ref).await {
-Ok(initiates_handshake) => initiates_handshake,
-Err(err) => {
-log::error!("[{peer}] error checking for TLS handshake:
{err}");
-return;
-}
-}
-};
+#[cfg(not(feature = "rate-limited-stream"))]
+let socket_ref = &state.socket;
-if !client_initiates_handshake {
-let insecure_stream = Box::pin(state.socket);
+let handshake_res =
+Self::wait_for_client_tls_handshake(socket_ref,
CLIENT_HANDSHAKE_TIMEOUT).await;
-if insecure_sender.send(Ok(insecure_stream)).await.is_err() &&
flags.is_debug {
-log::error!("[{peer}] detected closed connection channel")
+match handshake_res {
+Ok(true) => {
+Self::do_accept_tls(state, flags, secure_sender).await;
}
+Ok(false) => {
+let insecure_stream = Box::pin(state.socket);
-return;
+if let Err(send_err) =
insecure_sender.send(Ok(insecure_stream)).await {
+log::error!("[{peer}] failed to accept connection -
connection channel closed: {send_err}");
+}
+}
+Err(err) => {
+log::error!("[{peer}] failed to check for TLS handshake:
{err}");
+}
}
-
-Self::do_accept_tls(state, flags, secure_sender).await
}
-async fn wait_for_client_tls_handshak
[pve-devel] [PATCH v1 proxmox 2/3] rest-server: connection: log peer address on error
.. in order to make debugging easier and logs more helpful.
Signed-off-by: Max Carrara
---
proxmox-rest-server/src/connection.rs | 42 ---
1 file changed, 25 insertions(+), 17 deletions(-)
diff --git a/proxmox-rest-server/src/connection.rs
b/proxmox-rest-server/src/connection.rs
index 243348c0..470021d7 100644
--- a/proxmox-rest-server/src/connection.rs
+++ b/proxmox-rest-server/src/connection.rs
@@ -2,6 +2,7 @@
//!
//! Hyper building block.
+use std::net::SocketAddr;
use std::os::unix::io::AsRawFd;
use std::path::PathBuf;
use std::pin::Pin;
@@ -257,6 +258,7 @@ impl From<(ClientSender, InsecureClientSender)> for Sender {
struct AcceptState {
pub socket: InsecureClientStream,
+pub peer: SocketAddr,
pub acceptor: Arc>,
pub accept_counter: Arc<()>,
}
@@ -276,9 +278,9 @@ impl AcceptBuilder {
let mut shutdown_future = crate::shutdown_future().fuse();
loop {
-let socket = futures::select! {
+let (socket, peer) = futures::select! {
res = self.try_setup_socket(&listener).fuse() => match res {
-Ok(socket) => socket,
+Ok(socket_peer) => socket_peer,
Err(err) => {
log::error!("couldn't set up TCP socket: {err}");
continue;
@@ -291,12 +293,13 @@ impl AcceptBuilder {
let accept_counter = Arc::clone(&accept_counter);
if Arc::strong_count(&accept_counter) > self.max_pending_accepts {
-log::error!("connection rejected - too many open connections");
+log::error!("[{peer}] connection rejected - too many open
connections");
continue;
}
let state = AcceptState {
socket,
+peer,
acceptor,
accept_counter,
};
@@ -328,7 +331,7 @@ impl AcceptBuilder {
async fn try_setup_socket(
&self,
listener: &TcpListener,
-) -> Result {
+) -> Result<(InsecureClientStream, SocketAddr), Error> {
let (socket, peer) = match listener.accept().await {
Ok(connection) => connection,
Err(error) => {
@@ -338,10 +341,10 @@ impl AcceptBuilder {
socket
.set_nodelay(true)
-.context("error while setting TCP_NODELAY on socket")?;
+.with_context(|| format!("[{peer}] error while setting TCP_NODELAY
on socket"))?;
proxmox_sys::linux::socket::set_tcp_keepalive(socket.as_raw_fd(),
self.tcp_keepalive_time)
-.context("error while setting SO_KEEPALIVE on socket")?;
+.with_context(|| format!("[{peer}] error while setting
SO_KEEPALIVE on socket"))?;
#[cfg(feature = "rate-limited-stream")]
let socket = match self.lookup_rate_limiter.clone() {
@@ -349,13 +352,12 @@ impl AcceptBuilder {
None => RateLimitedStream::with_limiter(socket, None, None),
};
-#[cfg(not(feature = "rate-limited-stream"))]
-let _peer = peer;
-
-Ok(socket)
+Ok((socket, peer))
}
async fn do_accept_tls(state: AcceptState, flags: AcceptFlags,
secure_sender: ClientSender) {
+let peer = state.peer;
+
let ssl = {
// limit acceptor_guard scope
// Acceptor can be reloaded using the command socket
"reload-certificate" command
@@ -364,7 +366,9 @@ impl AcceptBuilder {
match openssl::ssl::Ssl::new(acceptor_guard.context()) {
Ok(ssl) => ssl,
Err(err) => {
-log::error!("failed to create Ssl object from Acceptor
context - {err}");
+log::error!(
+"[{peer}] failed to create Ssl object from Acceptor
context - {err}"
+);
return;
}
}
@@ -373,7 +377,9 @@ impl AcceptBuilder {
let secure_stream = match tokio_openssl::SslStream::new(ssl,
state.socket) {
Ok(stream) => stream,
Err(err) => {
-log::error!("failed to create SslStream using ssl and
connection socket - {err}");
+log::error!(
+"[{peer}] failed to create SslStream using ssl and
connection socket - {err}"
+);
return;
}
};
@@ -388,17 +394,17 @@ impl AcceptBuilder {
match result {
Ok(Ok(())) => {
if secure_sender.send(Ok(secure_stream)).await.is_err() &&
flags.is_debug {
-log::error!("detected closed connection channel");
+log::error!("[{peer}] detected closed connection channel");
}
}
Ok(Err(err)) => {
if flags.is_debug {
-log::error!("https handshake failed - {err}");
+log::error!("
