Re: [pve-devel] [RFC guest-common 4/4] guest helpers: add helper to overrule active tasks of a specific type
On 17/11/2023 13:53, Wolfgang Bumiller wrote: > Patch itself LGTM, just a note on sending patch series in general: > > If you number patches throughout a whole series rather than the > individual repositories (as in, this one is labeled 4/4 instead of 1/1), > it would be nice if the order also helps determine dependencies. Makes total sense, I'll keep this in mind for the future. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC manager/container/qemu-server/guest-common 0/4] fix #4474: stop tasks may overrule shutdown tasks
Thanks for the review! I'll prepare a v2 that incorporates the UI changes I suggested earlier. I do have some questions regarding the concurrent tasks scenario in patch #2, see my separate mail. On 17/11/2023 13:31, Wolfgang Bumiller wrote: [...] >> On 26/01/2023 09:32, Friedrich Weber wrote: >>> * Does it make sense to have overruling optional? Or should "stop" >>> generally overrule shutdown? This might lead to confusing >>> interactions, as Thomas noted [0]. > > Although whenever I ran into that I had simply misclicked shutdown or > became impatient. I never had any automated shutdown tasks happen. > Yet I still feel like this should be optional ;-) > (I usually just ended up using `qm stop` on the cli :P) Makes sense! (And using `qm stop` sounds like it might save some clicks ...) >>> * Backend: Is there a more elegant way to overrule shutdown tasks, >>> and a better place than pve-guest-common? >>> * Frontend: When stopping a VM/CT, we already ask for confirmation. >>> Is an (occasional) second modal dialog with a lot of text a good user >>> experience? Alternatively, I could imagine a checkbox in the first >>> dialog saying "Overrule any active shutdown tasks". >> >> Actually I don't really like the second modal dialog. What about the >> following: When the user clicks "Stop" and the frontend detects an >> active shutdown task, the already-existing "Confirm" dialog has an >> additional default-off checkbox "Kill active shutdown tasks" (or >> similar). This way the default behavior does not change, but users do >> not have to kill active shutdown tasks manually anymore. > > Sounds good to me. > But maybe don't use the word "kill" 😄 "Replace/Override" should work. Fair point. :) >>> * This patch series forbids `overrule-shutdown=1` for HA-managed VMs/CTs >>> because I didn't know how overruling should work in a HA setting. Do >>> you have any suggestions? > > I think it's okay to disable this for now. Alright! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC container 2/4] fix #4474: lxc api: add overrule-shutdown parameter to stop endpoint
Thanks for looking into this! On 17/11/2023 14:09, Wolfgang Bumiller wrote: [...] >> return PVE::LXC::Config->lock_config($vmid, $lockcmd); > > ^ Here we lock first, then fork the worker, then do `vm_stop` with the > config lock inherited. > > This means that creating multiple shutdown tasks before using one with > override=true could cause the override task to cancel the *first* ongoing > shutdown task, then move on to the `lock_config` call - in the meantime > a second shutdown task acquires this very lock and performs another > long-running shutdown, causing the `override` parameter to be > ineffective. Just to make sure I understand correctly, the scenario is (please correct me if I'm wrong): * shutdown task #1 has the lock and starts long-running shutdown * stop API handler with override kills shutdown task #1, but does not acquire the lock yet * shutdown task #2 starts, acquires the lock and starts long-running shutdown * stop task waits for the lock => override flag was ineffective > We should switch the ordering here: first fork the worker, then lock. > (¹ And your new chunk would go into the worker as well) > > Unless I'm missing something, but AFAICT the current ordering there is > rather ... bad :-) Would this actually prevent the scenario above? We cannot put my new chunk into the locked section (because then it couldn't kill an active shutdown task that has the lock), but if we put it into the worker before the locked section, couldn't the same thing as above happen? Meaning the stop task with override kills shutdown tasks but doesn't have the lock yet, a new shutdown task acquires the lock, makes the stop task wait for it, and renders the override flag ineffective just the same? ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [RFC manager] api: replication: allow users to enumerate accessible replication jobs
Previously, the /cluster/replication API handler would fail completely
with a HTTP 403 if a user does have VM.Audit permissions for
a single VM/CT. That was due to the 'noerr' parameter not set for
$rpcenv->check()
Signed-off-by: Lukas Wagner
---
Not sure if this violates our API stability guarantees, so I'm sending
this as an RFC in advance. If this change is problematic, we could
hide the new behavior behind an optional flag.
This change is necessary for retrieving a list of known job-ids for
enhancements to the notification matching rule edit window.
PVE/API2/ReplicationConfig.pm | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/PVE/API2/ReplicationConfig.pm b/PVE/API2/ReplicationConfig.pm
index 8af62621..d0e8a49e 100644
--- a/PVE/API2/ReplicationConfig.pm
+++ b/PVE/API2/ReplicationConfig.pm
@@ -20,7 +20,8 @@ __PACKAGE__->register_method ({
method => 'GET',
description => "List replication jobs.",
permissions => {
- description => "Requires the VM.Audit permission on /vms/.",
+ description => "Will only return replication jobs for which the calling
user has"
+ . " VM.Audit permission on /vms/.",
user => 'all',
},
parameters => {
@@ -47,7 +48,7 @@ __PACKAGE__->register_method ({
foreach my $id (sort keys %{$cfg->{ids}}) {
my $d = $cfg->{ids}->{$id};
my $vmid = $d->{guest};
- next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Audit' ]);
+ next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Audit' ], 1);
$d->{id} = $id;
push @$res, $d;
}
--
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] pve7to8: fix and improve wording
Signed-off-by: Alexander Zeidler --- bin/Makefile | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/bin/Makefile b/bin/Makefile index 06d8148e..2185b009 100644 --- a/bin/Makefile +++ b/bin/Makefile @@ -62,10 +62,11 @@ pve6to7.1: pve7to8.1: printf ".TH PVE7TO8 1\n.SH NAME\npve7to8 \- Proxmox VE upgrade checker script for 7.4+ to current 8.x\n" > $@.tmp - printf ".SH DESCRIPTION\nThis tool will help you to detect common pitfalls and misconfguration\ -before, and during the upgrade of a Proxmox VE system\n" >> $@.tmp - printf "Any failure must be addressed before the upgrade, and any waring must be addressed, \ -or at least carefully evaluated, if a false-positive is suspected\n" >> $@.tmp + printf ".SH DESCRIPTION\nThis tool will help you to detect common pitfalls and misconfiguration\ +before, and during the upgrade of a Proxmox VE system.\n" >> $@.tmp + printf "Any failures or warnings must be addressed prior to the upgrade.\n" >> $@.tmp + printf "If you suspect that a message is a false positive, you have to\ +make carefully sure that it really is.\n" >> $@.tmp printf ".SH SYNOPSIS\npve7to8 [--full]\n" >> $@.tmp mv $@.tmp $@ -- 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 proxmox-i18n] use xgettext to extract translatable strings
xgettext is a robust tool to extract translatable strings from source
code.
Using msgcat for concatenating pot files is not recommended, hence we
also switch to xgettext. It also added garbage when there were comments.
What do we get for free:
- It de-escapes strings. there are 3 cases in our code base where
single-quoted strings were used and its `'` had to be escaped, these
were not de-escaped properly when presented to translators. This is one
such example
```diff
#: proxmox-widget-toolkit/src/panel/EmailRecipientPanel.js:39
-msgid "The notification will be sent to the user\\'s configured mail address"
+#, fuzzy
+msgid "The notification will be sent to the user's configured mail address"
msgstr "La notificación sera enviada a el correo configurado del usuario"
```
- xgettext can detect when strings use a certain style of substitutions,
but I was not able to detect the conditions and it only affects a single
string in the entire code base.
```diff
#: proxmox-widget-toolkit/src/Utils.js:995
+#, javascript-format
msgid "{0}% of {1}"
msgstr "{0}% de {1}"
```
- Correct POT-Creation-Date, note how the new one matches the
Revision-Date's format.
```diff
@@ -7,7 +7,7 @@ msgid ""
msgstr ""
"Project-Id-Version: proxmox translations\n"
"Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: Wed Nov 22 18:17:30 2023\n"
+"POT-Creation-Date: 2023-12-01 11:25+0100\n"
"PO-Revision-Date: 2023-11-27 16:43+0100\n"
"Last-Translator: Maximiliano Sandoval \n"
"Language-Team: Spanish\n"
```
- Extraction of strings using ngettext, pgettext, etc. Even if we don't
have js wrappers for these at the moment, they are critical to provide
good-quality translations and could be added in the future.
- We can extract comments from the source code with `xgettext -c`.
Newly added comments won't mark strings as fuzzy but can provide
helpful context to translators.
Comments are additive, if for example two sources contain
the same string with different comments and it appears a third time
without comments, the three sources and the two comments will be shown
to translators.
These are a few examples that could be implemented in our codebase:
It is not clear if "Prune Options" prunes the options or configures
pruning.
```js
// TRANSLATORS: Opens the panel that allows configuring how Pruning works
let s = gettext("Prune Options");
```
Adding a source for a concept or its expanded name can help
translators decide whats the gender for a word in their language.
```js
// TRANSLATORS: TOTP stands for Time-based one-time password
let s = gettext("Add a TOTP login factor");
```
Some strings are not marked for translation to avoid translating
certain parts of it, this is a change that could be made
```diff
-fieldLabel: 'Crush Rule', // do not localize
+// TRANSLATORS: Do not translate 'Crush', its a proper name
+fieldLabel: gettext('Crush Rule'),
```
Or simply to give more context when substitutions are involved.
```
// TRANSLATORS: For example 'Join CLUSTER_NAME'
return Ext.String.format(gettext('Join {0}'), `'${cn}'`);
```
Cons:
- In total 3 translations were marked as fuzzy. Translators will have to
review and mark them as translated again.
- If using -c, gettext can't distinguish if the comment above is useful
for translators. The common practice is to add a `TRANSLATORS:` tag to
these comments.
- The reordering of sources for each msgstr will create an unnecessarily
massive (yet ultimately harmless) diff (approx. 50k insertions(+) 50k
deletions(-)).
Signed-off-by: Maximiliano Sandoval
Thomas: Should this be merged, please run `make do_update` and commit the
changes to each .po{,t} file. I am not sure if it is possible to even
send an email with over 100k lines of text.
---
Makefile | 10 +++-
jsgettext.pl | 135 ---
2 files changed, 8 insertions(+), 137 deletions(-)
delete mode 100755 jsgettext.pl
diff --git a/Makefile b/Makefile
index 1d7af6e..4776e02 100644
--- a/Makefile
+++ b/Makefile
@@ -97,7 +97,13 @@ pbs-lang-%.js: %.po
# parameter 1 is the name
# parameter 2 is the directory
define potupdate
-./jsgettext.pl -p "$(1) $(shell cd $(2);git rev-parse HEAD)" -o $(1).pot
$(2)
+ find . -iname "*.js" -path "./$(2)*" | xargs xgettext -c -s \
+ --from-code="UTF-8" \
+ --package-name="$(1)" \
+ --package-version="$(shell cd $(2);git rev-parse HEAD)" \
+ --msgid-bugs-address="" \
+ --copyright-holder="Copyright (C) Proxmox Server Solutions GmbH
& the translation contributors." \
+ --output="$(1)".pot
endef
.PHONY: update update_pot do_update
@@ -124,7 +130,7 @@ init-%.po: messages.pot
.INTERMEDIATE: messages.pot
messages.pot: proxmox-widget-toolkit.pot proxmox-mailgateway.pot
pve-manager.pot proxmox-backup.pot
- msgcat $^ > $@
+ xgettext $^ --msgid-bugs-address="" -o $@
