[pve-devel] [PATCH container 2/5] fix #1423: add timezone config option

[Oguz Bektas]

hi,

On Tue, Jun 16, 2020 at 04:45:34PM +0200, Thomas Lamprecht wrote:
> Am 6/16/20 um 3:36 PM schrieb Oguz Bektas:
> > optionally enabled.
> > 
> > adds the 'timezone' option to config, which takes a valid timezone (i.e.
> > Europe/Vienna) to set in the container.
> > 
> > if nothing is selected, then it will show as 'container managed' in
> > GUI, and nothing will be done.
> > 
> > if set to 'host', the /etc/localtime symlink from the host node will be
> > cached and set in the container rootfs.
> > 
> > Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> > ---
> >  src/PVE/LXC/Config.pm     |  5 +++++
> >  src/PVE/LXC/Setup.pm      | 13 +++++++++++++
> >  src/PVE/LXC/Setup/Base.pm | 28 +++++++++++++++++++++++++---
> >  3 files changed, 43 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> > index 8d1854a..0ebf517 100644
> > --- a/src/PVE/LXC/Config.pm
> > +++ b/src/PVE/LXC/Config.pm
> > @@ -444,6 +444,11 @@ my $confdesc = {
> >  	type => 'string', format => 'address-list',
> >  	description => "Sets DNS server IP address for a container. Create will automatically use the setting from the host if you neither set searchdomain nor nameserver.",
> >      },
> > +    timezone => {
> > +	optional => 1,
> > +	type => 'string', format => 'timezone',
> > +	description => "Time zone to use in the container. If option isn't set, then nothing will be done. Can be set to 'host' to match the host time zone, or an arbitrary time zone option from /usr/share/zoneinfo/zone1970.tab",
> > +    },
> >      rootfs => get_standard_option('pve-ct-rootfs'),
> >      parent => {
> >  	optional => 1,
> > diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm
> > index c738e64..0e07796 100644
> > --- a/src/PVE/LXC/Setup.pm
> > +++ b/src/PVE/LXC/Setup.pm
> > @@ -5,6 +5,8 @@ use warnings;
> >  use POSIX;
> >  use PVE::Tools;
> >  
> > +use Cwd 'abs_path';
> > +
> >  use PVE::LXC::Setup::Debian;
> >  use PVE::LXC::Setup::Ubuntu;
> >  use PVE::LXC::Setup::CentOS;
> > @@ -103,6 +105,7 @@ sub new {
> >  
> >      # Cache some host files we need access to:
> >      $plugin->{host_resolv_conf} = PVE::INotify::read_file('resolvconf');
> > +    $plugin->{host_localtime} = abs_path('/etc/localtime');
> 
> Hmm, I'd not save that in the $plugin, makes no sense to me.
> I mean, I see where this comes from but that also doesn't makes much sense to me.
> 
> This isn't both expected to be used often, so just using it directly without cache
> or at least cache only if at least used once would be nicer IMO.


this was the simplest way i found, and it's just caching a ~50 byte
string (/etc/localtime symlink) to be used in the setup.

how else can i access that in the setup routines
which are running in CT rootfs?


> 
> >  
> >      # pass on user namespace information:
> >      my ($id_map, $rootuid, $rootgid) = PVE::LXC::parse_id_maps($conf);
> > @@ -205,6 +208,16 @@ sub set_dns {
> >      $self->protected_call($code);
> >  }
> >  
> > +sub set_timezone {
> > +    my ($self) = @_;
> > +
> > +    return if !$self->{plugin}; # unmanaged
> > +    my $code = sub {
> > +	$self->{plugin}->set_timezone($self->{conf});
> > +    };
> > +    $self->protected_call($code);
> > +}
> > +
> >  sub setup_init {
> >      my ($self) = @_;
> >  
> > diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
> > index 93dace7..4c7c2e6 100644
> > --- a/src/PVE/LXC/Setup/Base.pm
> > +++ b/src/PVE/LXC/Setup/Base.pm
> > @@ -451,6 +451,26 @@ my $randomize_crontab = sub {
> >     }
> >  };
> >  
> > +sub set_timezone {
> > +    my ($self, $conf) = @_;
> > +
> > +    my $zoneinfo = $conf->{timezone};
> > +
> > +    return if !defined($zoneinfo);
> > +
> > +    my $tz_path = "/usr/share/zoneinfo/$zoneinfo";
> > +    if ($zoneinfo eq 'host') {
> > +	$tz_path= $self->{host_localtime};
> 
> whitespace error before =
> 
> > +    }
> > +
> > +    if ($self->ct_file_exists($tz_path)) {
> > +	$self->ct_unlink("/etc/localtime");
> > +	$self->ct_symlink($tz_path, "/etc/localtime");
> 
> could be nicer to do a atomic move, i.e. symlink to some "localtime.$$.new.tmpfile" and do a
> rename (move) afterwards to "/etc/localtime".
> 
> Also it would be nice to avoid the change if it points already to the correct zone, avoiding
> IO even if minimal is always good.

alright

> 
> > +    } else {
> > +	warn "container does not have $tz_path, timezone can not be modified\n";
> > +    }
> > +}
> > +
> >  sub pre_start_hook {
> >      my ($self, $conf) = @_;
> >  
> > @@ -458,6 +478,7 @@ sub pre_start_hook {
> >      $self->setup_network($conf);
> >      $self->set_hostname($conf);
> >      $self->set_dns($conf);
> > +    $self->set_timezone($conf);
> >  
> >      # fixme: what else ?
> >  }
> > @@ -466,16 +487,17 @@ sub post_create_hook {
> >      my ($self, $conf, $root_password, $ssh_keys) = @_;
> >  
> >      $self->template_fixup($conf);
> > -    
> > +
> >      &$randomize_crontab($self, $conf);
> > -    
> > +
> >      $self->set_user_password($conf, 'root', $root_password);
> >      $self->set_user_authorized_ssh_keys($conf, 'root', $ssh_keys) if $ssh_keys;
> >      $self->setup_init($conf);
> >      $self->setup_network($conf);
> >      $self->set_hostname($conf);
> >      $self->set_dns($conf);
> > -    
> > +    $self->set_timezone($conf);
> > +
> >      # fixme: what else ?
> >  }
> >  
> > 
>