Re: [pve-devel] [PATCH container] setup: nixos: set cmode to console by default

[Fiona Ebner <f.ebner@proxmox.com>]

Am 14.02.24 um 14:04 schrieb Christoph Heiss:
> Thanks for the review!
> 
> On Fri, Jan 26, 2024 at 03:47:34PM +0100, Fiona Ebner wrote:
>> Am 21.11.23 um 10:40 schrieb Christoph Heiss:
> [..]
>>> diff --git a/src/PVE/LXC/Setup/NixOS.pm b/src/PVE/LXC/Setup/NixOS.pm
>>> index c702f3d..7f23111 100644
>>> --- a/src/PVE/LXC/Setup/NixOS.pm
>>> +++ b/src/PVE/LXC/Setup/NixOS.pm
>>> @@ -17,6 +17,11 @@ sub new {
>>>
>>>      $conf->{ostype} = "nixos";
>>>
>>> +    # Set `cmode` to `console` for NixOS containers, as getty is only configured for /dev/console by
>>> +    # default, but not any TTY ports. This way, users still get a login/shell instead of just a
>>> +    # blank screen when openinng the console in the web UI.
>>> +    $conf->{cmode} = 'console';
>>> +
>>>      return bless $self, $class;
>>>  }
>>>
>>
>> Won't this override any pre-existing setting (from user or backup)?
>> Even if checking for that, it could still be considered a breaking
>> change, i.e. other people might have configured their NixOS container
>> differently, so maybe best to wait for the next major release?
> 
> Yep, just tried that out - so that's definitely a no-go.
> 
>>
>> For now, we could log a warning if no explicit 'cmode' was specified for
>> NixOS.
>>
>> I don't see any other implementation of new() overriding any defaults.
>> Should we even start doing that?
> 
> Well, looking at the code again, there is the ->template_fixup() sub,
> which gets called on restores after ->new(). This does seem to override
> some things (e.g. ->setup_securetty()) for most of the distros.
> 

But template_fixup() doesn't change any config values either, right?

If we think that most NixOS users can benefit from the different default
and that the change is not likely to break existing containers/backups,
then I'm not fundamentally against the change. In any case, it would
need to be documented in the configuration schema that the default for
NixOS is different.

Two approaches would be:

1. If done in new(), it needs to be a '//=' assignment, so explicit
value wins.

2. Could also be done in PVE::LXC::Config->get_cmode(), might be
slightly cleaner (explicit value has to win too of course).

> So I guess this would be correct place after all for this, if we
> override something.
> 
> Anyway, so just a
> 
>   warn "..." if `$conf->{cmode} ne 'console'`;
> 
> if ->new() would be acceptable?
> 

Is 'shell' also problematic? The check should use
PVE::LXC::Config->get_cmode(), to avoid potential comparison against undef.