[pve-devel] [PATCH docs] pveproxy: improve LISTEN_IP doc

[pve-devel] [PATCH docs] pveproxy: improve LISTEN_IP doc

[Oguz Bektas]

* fix small typo
* add details for link-local addresses
* mention that pveproxy needs to be restarted

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
 pveproxy.adoc | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/pveproxy.adoc b/pveproxy.adoc
index 08c5f63..b9f8ade 100644
--- a/pveproxy.adoc
+++ b/pveproxy.adoc
@@ -71,10 +71,18 @@ exposure to the public internet:
 
  LISTEN_IP="192.0.2.1"
 
-Similarly you can also set a n IPv6 address:
+Similarly you can also set an IPv6 address:
 
  LISTEN_IP="2001:db8:85a3::1"
 
+And for a link-local IPv6 address on vmbr0 (interface name is necessary in this case):
+
+ LISTEN_IP="fe80::d8ee:34ff:fe37:4579%vmbr0"
+
+After the change you have to restart `pveproxy` for it to take effect:
+
+ systemctl restart pveproxy
+
 WARNING: The nodes in a cluster need access to `pveproxy` for communication,
 possibly on different sub-nets. It is **not recommended** to set `LISTEN_IP` on
 clustered systems.
-- 
2.20.1

Re: [pve-devel] [PATCH docs] pveproxy: improve LISTEN_IP doc

[Thomas Lamprecht]

On 28.04.21 13:16, Oguz Bektas wrote:
> * fix small typo
> * add details for link-local addresses
> * mention that pveproxy needs to be restarted
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>  pveproxy.adoc | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/pveproxy.adoc b/pveproxy.adoc
> index 08c5f63..b9f8ade 100644
> --- a/pveproxy.adoc
> +++ b/pveproxy.adoc
> @@ -71,10 +71,18 @@ exposure to the public internet:
>  
>   LISTEN_IP="192.0.2.1"
>  
> -Similarly you can also set a n IPv6 address:
> +Similarly you can also set an IPv6 address:
>  
>   LISTEN_IP="2001:db8:85a3::1"
>  
> +And for a link-local IPv6 address on vmbr0 (interface name is necessary in this case):

Does not reads like an actual sentence... I'd write something a long the lines of:

"Note, if you want to specify a link-local IPv6 address, you need to provide the interface name itself:"

> +
> + LISTEN_IP="fe80::d8ee:34ff:fe37:4579%vmbr0"
> +
> +After the change you have to restart `pveproxy` for it to take effect:

I'd specifically state that a reload is not enough and then add a small warning that
a restart can stop some existing workers (not all, but e.g., shell connection is stopped
and reconnected which may loose info on CTs without a screen/tmux instance running).
Also, there's a short time window where no new connections are accepted IIRC (albeit
I was the one fixing that for reload it's been to long since then, so not sure anymore)

> +
> + systemctl restart pveproxy

and spiceproxy?

> +
>  WARNING: The nodes in a cluster need access to `pveproxy` for communication,
>  possibly on different sub-nets. It is **not recommended** to set `LISTEN_IP` on
>  clustered systems.
> 

Re: [pve-devel] [PATCH docs] pveproxy: improve LISTEN_IP doc

[Oguz Bektas]

hi,

thanks for checking.

On Wed, Apr 28, 2021 at 01:35:40PM +0200, Thomas Lamprecht wrote:
> On 28.04.21 13:16, Oguz Bektas wrote:
> > * fix small typo
> > * add details for link-local addresses
> > * mention that pveproxy needs to be restarted
> > 
> > Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> > ---
> >  pveproxy.adoc | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/pveproxy.adoc b/pveproxy.adoc
> > index 08c5f63..b9f8ade 100644
> > --- a/pveproxy.adoc
> > +++ b/pveproxy.adoc
> > @@ -71,10 +71,18 @@ exposure to the public internet:
> >  
> >   LISTEN_IP="192.0.2.1"
> >  
> > -Similarly you can also set a n IPv6 address:
> > +Similarly you can also set an IPv6 address:
> >  
> >   LISTEN_IP="2001:db8:85a3::1"
> >  
> > +And for a link-local IPv6 address on vmbr0 (interface name is necessary in this case):
> 
> Does not reads like an actual sentence... I'd write something a long the lines of:
> 
> "Note, if you want to specify a link-local IPv6 address, you need to provide the interface name itself:"

okay

> 
> > +
> > + LISTEN_IP="fe80::d8ee:34ff:fe37:4579%vmbr0"
> > +
> > +After the change you have to restart `pveproxy` for it to take effect:
> 
> I'd specifically state that a reload is not enough and then add a small warning that
> a restart can stop some existing workers (not all, but e.g., shell connection is stopped
> and reconnected which may loose info on CTs without a screen/tmux instance running).
> Also, there's a short time window where no new connections are accepted IIRC (albeit
> I was the one fixing that for reload it's been to long since then, so not sure anymore)

i think the phrasing "you have to restart" already emphasizes this,
adding too many warnings or notes would just confuse users in my
opinion.

though i don't see any harm in making the **restart** bold in that
sentence and adding that small warning about possible connection drop.

> 
> > +
> > + systemctl restart pveproxy
> 
> and spiceproxy?
ah yes forgot that, also adding to v2
> 
> > +
> >  WARNING: The nodes in a cluster need access to `pveproxy` for communication,
> >  possibly on different sub-nets. It is **not recommended** to set `LISTEN_IP` on
> >  clustered systems.
> > 

Re: [pve-devel] [PATCH docs] pveproxy: improve LISTEN_IP doc

[Thomas Lamprecht]

On 28.04.21 13:42, Oguz Bektas wrote:
>>> +
>>> + LISTEN_IP="fe80::d8ee:34ff:fe37:4579%vmbr0"
>>> +
>>> +After the change you have to restart `pveproxy` for it to take effect:
>>
>> I'd specifically state that a reload is not enough and then add a small warning that
>> a restart can stop some existing workers (not all, but e.g., shell connection is stopped
>> and reconnected which may loose info on CTs without a screen/tmux instance running).
>> Also, there's a short time window where no new connections are accepted IIRC (albeit
>> I was the one fixing that for reload it's been to long since then, so not sure anymore)
> 
> i think the phrasing "you have to restart" already emphasizes this,
> adding too many warnings or notes would just confuse users in my
> opinion.

No, it's clear that something needs to be restarted, but "restart" is a general
overused term which can mean lots of things (even reboot for some).

> 
> though i don't see any harm in making the **restart** bold in that
> sentence and adding that small warning about possible connection drop.

as said, restart is often used for the general semantic thing, be it reload or restart,
so this is really not clear. The gui also only triggers a reload, IIRC (pls. check)
and thus "restarting" (it's named that way there) from there would not help.

You just need to write it in such a way that it is not confusing, then it is not
a problem.

Re: [pve-devel] [PATCH docs] pveproxy: improve LISTEN_IP doc

[Oguz Bektas]

On Wed, Apr 28, 2021 at 01:51:51PM +0200, Thomas Lamprecht wrote:
> On 28.04.21 13:42, Oguz Bektas wrote:
> >>> +
> >>> + LISTEN_IP="fe80::d8ee:34ff:fe37:4579%vmbr0"
> >>> +
> >>> +After the change you have to restart `pveproxy` for it to take effect:
> >>
> >> I'd specifically state that a reload is not enough and then add a small warning that
> >> a restart can stop some existing workers (not all, but e.g., shell connection is stopped
> >> and reconnected which may loose info on CTs without a screen/tmux instance running).
> >> Also, there's a short time window where no new connections are accepted IIRC (albeit
> >> I was the one fixing that for reload it's been to long since then, so not sure anymore)
> > 
> > i think the phrasing "you have to restart" already emphasizes this,
> > adding too many warnings or notes would just confuse users in my
> > opinion.
> 
> No, it's clear that something needs to be restarted, but "restart" is a general
> overused term which can mean lots of things (even reboot for some).
> 
> > 
> > though i don't see any harm in making the **restart** bold in that
> > sentence and adding that small warning about possible connection drop.
> 
> as said, restart is often used for the general semantic thing, be it reload or restart,
> so this is really not clear. 

why is it not clear? there's not a single instance of 'systemctl reload'
command in the documentation, and in instances where a service restart
is required, we specify 'systemctl restart' without any mention, e.g.,
in pvenode.adoc when setting up certificates)

so i don't see why users would imagine to 'reload' the service instead
of 'restart' when it's clearly written in bold, and the command is
right beneath the explanation...

> The gui also only triggers a reload, IIRC (pls. check)
> and thus "restarting" (it's named that way there) from there would not help.

yes, on the button it says "restart" but the tasklog says "reload".
so to me that sounds like a mislabeled button with wrong/lax use
of "restart" instead of the correct "reload".

> 
> You just need to write it in such a way that it is not confusing, then it is not
> a problem.

there's really nothing confusing IMO (besides the button). but if you
insist i will send another patch with the changes you recommended.

some possible alternative phrasing i'd suggest:

=====
After the change you have to **restart** `pveproxy` and `spiceproxy` for
it to take effect (**reload** or restart from GUI does not suffice):
 systemctl restart pveproxy spiceproxy
=====

or much simpler and less confusing:
=====
Run the following command to restart the proxy servers and apply the change:
 systemctl restart pveproxy spiceproxy
=====

with a note about the connection drop during restart.

Re: [pve-devel] [PATCH docs] pveproxy: improve LISTEN_IP doc

[Thomas Lamprecht]

On 28.04.21 14:20, Oguz Bektas wrote:
> On Wed, Apr 28, 2021 at 01:51:51PM +0200, Thomas Lamprecht wrote:
>> On 28.04.21 13:42, Oguz Bektas wrote:
>>>>> +
>>>>> + LISTEN_IP="fe80::d8ee:34ff:fe37:4579%vmbr0"
>>>>> +
>>>>> +After the change you have to restart `pveproxy` for it to take effect:
>>>>
>>>> I'd specifically state that a reload is not enough and then add a small warning that
>>>> a restart can stop some existing workers (not all, but e.g., shell connection is stopped
>>>> and reconnected which may loose info on CTs without a screen/tmux instance running).
>>>> Also, there's a short time window where no new connections are accepted IIRC (albeit
>>>> I was the one fixing that for reload it's been to long since then, so not sure anymore)
>>>
>>> i think the phrasing "you have to restart" already emphasizes this,
>>> adding too many warnings or notes would just confuse users in my
>>> opinion.
>>
>> No, it's clear that something needs to be restarted, but "restart" is a general
>> overused term which can mean lots of things (even reboot for some).
>>
>>>
>>> though i don't see any harm in making the **restart** bold in that
>>> sentence and adding that small warning about possible connection drop.
>>
>> as said, restart is often used for the general semantic thing, be it reload or restart,
>> so this is really not clear. 
> 
> why is it not clear? there's not a single instance of 'systemctl reload'
> command in the documentation, and in instances where a service restart
> is required, we specify 'systemctl restart' without any mention, e.g.,
> in pvenode.adoc when setting up certificates)
> 
> so i don't see why users would imagine to 'reload' the service instead
> of 'restart' when it's clearly written in bold, and the command is
> right beneath the explanation...
> 
>> The gui also only triggers a reload, IIRC (pls. check)
>> and thus "restarting" (it's named that way there) from there would not help.
> 
> yes, on the button it says "restart" but the tasklog says "reload".
> so to me that sounds like a mislabeled button with wrong/lax use
> of "restart" instead of the correct "reload".

I *always* use try-reload-or-restart, as I want to avoid service disruption,
as most people actually running relevant systems do...
What I surely do not is reading all the docs, comparing how often reload vs.
restart is used and then try to read into that and conclude something from
that...

I *never* saw anybody complaining about an additional note of some side-effects
that may come unexpected, but I saw quite some situations where there was no 
such note and user run into issues, I do not suggest such changes just for the
hell of it...

> 
>>
>> You just need to write it in such a way that it is not confusing, then it is not
>> a problem.
> 
> there's really nothing confusing IMO (besides the button). but if you
> insist i will send another patch with the changes you recommended.

You said you would write it in a confusing way when adding an explicit note,
so I suggest doing it in a not confusing manner ;)

> 
> some possible alternative phrasing i'd suggest:
> 
> =====
> After the change you have to **restart** `pveproxy` and `spiceproxy` for
> it to take effect (**reload** or restart from GUI does not suffice):
>  systemctl restart pveproxy spiceproxy
> =====
> 
> or much simpler and less confusing:
> =====
> Run the following command to restart the proxy servers and apply the change:
>  systemctl restart pveproxy spiceproxy
> =====
> 
> with a note about the connection drop during restart.
> 

I applied the fixes, the notes and another small grammar style fix for the comma
after "Similarly," myself, seemed to be the faster way to resolve this nicely..