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

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

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..