[pve-devel] [PATCH container 7/9] add vmconfig_hotplug_pending and vmconfig_apply_pending
Fabian Grünbichler
f.gruenbichler at proxmox.com
Wed Sep 11 09:40:18 CEST 2019
On September 5, 2019 4:11 pm, Oguz Bektas wrote:
> vmconfig_hotplug_pending is responsible for checking if a key in the
> pending section is hotpluggable, if yes; perform a generic config value
> replace or perform specific actions if a special case.
>
> vmconfig_apply_pending is only supposed to be called when ct isn't live.
>
> Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> ---
> src/PVE/LXC/Config.pm | 186 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 184 insertions(+), 2 deletions(-)
>
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index 08b958f..26c694f 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm
> @@ -7,11 +7,13 @@ use PVE::AbstractConfig;
> use PVE::Cluster qw(cfs_register_file);
> use PVE::GuestHelpers;
> use PVE::INotify;
> +use PVE::Exception qw(raise_param_exc);
> use PVE::JSONSchema qw(get_standard_option);
> -use PVE::Tools;
> +use PVE::Tools qw(extract_param);
>
> use base qw(PVE::AbstractConfig);
>
> +my $confdesc;
see other comment - I'd prefer not making vmconfig_hotplug_pending and
vmconfig_apply_pending methods in AbstractConfig, which means they can
be moved below the existing $confdesc.
> my $nodename = PVE::INotify::nodename();
> my $lock_handles = {};
> my $lockdir = "/run/lock/lxc";
> @@ -76,6 +78,186 @@ sub has_feature {
> return $err ? 0 : 1;
> }
>
> +my $LXC_FASTPLUG_OPTIONS= {
> + 'description' => 1,
> + 'onboot' => 1,
> + 'startup' => 1,
> + 'protection' => 1,
> + 'hostname' => 1,
hostname is reflected inside the container though? you'd need a trip
through Setup.pm or something to actually hotplug it..
> + 'hookscript' => 1,
> + 'cores' => 1,
> +};
> +
> +sub vmconfig_hotplug_pending {
> + my ($class, $vmid, $conf, $storecfg, $selection, $errors) = @_;
> +
> + my $pid = PVE::LXC::find_lxc_pid($vmid);
> + my $rootdir = "/proc/$pid/root";
> +
> + my $add_error = sub {
> + my ($opt, $msg) = @_;
> + $errors->{$opt} = "hotplug problem - $msg";
> + };
> +
> + my $changes;
> + foreach my $opt (keys %{$conf->{pending}}) { # add/change
> + if ($LXC_FASTPLUG_OPTIONS->{$opt}) {
> + $conf->{$opt} = delete $conf->{pending}->{$opt};
> + $changes = 1;
> + }
ignores $selection?
> + }
> +
> + if ($changes) {
> + $class->write_config($vmid, $conf);
> + $conf = $class->load_config($vmid); # update/reload
should not be needed, this needs to be called while holding an flock
anyway..
> + }
> +
> + my $pending_delete_hash = $class->split_flagged_list($conf->{pending}->{delete});
> + while (my ($opt, $force) = each %$pending_delete_hash) {
> + next if $selection && !$selection->{$opt};
> + eval {
> + if ($LXC_FASTPLUG_OPTIONS->{$opt}) {
> + # pass
> + } elsif ($opt eq 'swap') { # TODO FIXME: unlimited swap instead of no swap
wouldn't this just be able to re-use the hotplug_memory code from below?
> + PVE::LXC::write_cgroup_value("memory", $vmid,
> + "memory.memsw.limit_in_bytes", -1);
> + } elsif ($opt eq 'cpulimit') {
> + PVE::LXC::write_cgroup_value("cpu", $vmid, "cpu.cfs_quota_us", -1);
> + } elsif ($opt eq 'cpuunits') {
> + PVE::LXC::write_cgroup_value("cpu", $vmid, "cpu.shared", $confdesc->{cpuunits}->{default});
> + } elsif ($opt =~ m/^net(\d)$/) {
> + my $netid = $1;
> + PVE::Network::veth_delete("veth${vmid}i$netid");
> + } else {
> + die "skip\n"; # skip non-hotpluggable opts
> + }
> + };
> + if (my $err = $@) {
> + $add_error->($opt, $err) if $err ne "skip\n";
> + } else {
> + delete $conf->{$opt};
> + $class->vmconfig_undelete_pending_option($conf, $opt);
> + $class->write_config($vmid, $conf);
> + $conf = $class->load_config($vmid); # update/reload
reload shouldn't be needed
> + }
> + }
> +
> + # There's no separate swap size to configure, there's memory and "total"
> + # memory (iow. memory+swap). This means we have to change them together.
> + my $hotplug_memory_done;
> + my $hotplug_memory = sub {
> + my ($wanted_memory, $wanted_swap) = @_;
> + my $old_memory = ($conf->{memory} || 512);
> + my $old_swap = ($conf->{swap} || 0);
$confdesc says swap also defaults to 512? which one is wrong?
why not use the defaults here, instead of hard-coding them?
> +
> + $wanted_memory //= $old_memory;
> + $wanted_swap //= $old_swap;
> +
> + my $total = $wanted_memory + $wanted_swap;
> + my $old_total = $old_memory + $old_swap;
> +
> +
superfluous blank line
> + if ($total > $old_total) {
does this fail if the order is wrong? if so, wouldn't it be better to
read the current value instead of guessing it based on $conf? still
racy, but that can probably not be avoided..
> + PVE::LXC::write_cgroup_value("memory", $vmid,
> + "memory.memsw.limit_in_bytes",
> + int($total*1024*1024));
> + PVE::LXC::write_cgroup_value("memory", $vmid,
> + "memory.limit_in_bytes",
> + int($wanted_memory*1024*1024));
> + } else {
> + PVE::LXC::write_cgroup_value("memory", $vmid,
> + "memory.limit_in_bytes",
> + int($wanted_memory*1024*1024));
> + PVE::LXC::write_cgroup_value("memory", $vmid,
> + "memory.memsw.limit_in_bytes",
> + int($total*1024*1024));
> + }
> + $hotplug_memory_done = 1;
> + };
> +
> + foreach my $opt (keys %{$conf->{pending}}) {
next if $opt eq 'delete'; # just to be safe?
> + next if $selection && !$selection->{$opt};
> + my $value = $conf->{pending}->{$opt};
> + eval {
> + if ($LXC_FASTPLUG_OPTIONS->{$opt}) {
> + # pass
this branch should be unreachable? they are already handled and removed
from $conf->{pending} early on..
> + } elsif ($opt eq 'cpulimit') {
> + if ($value == 0) {
> + PVE::LXC::write_cgroup_value("cpu", $vmid, "cpu.cfs_quota_us", -1);
duplicated with deletion branch..
> + } else {
> + PVE::LXC::write_cgroup_value("cpu", $vmid, "cpu.cfs_quota_us", int(100000*$value));
when setting this up via the LXC config, we also ensure that the period
is actually 100000. don't we need to do the same here?
> + }
> + } elsif ($opt eq 'cpuunits') {
> + PVE::LXC::write_cgroup_value("cpu", $vmid, "cpu.shares", $value);
> + } elsif ($opt =~ m/^net(\d+)$/) {
> + my $netid = $1;
> + my $net = PVE::LXC::Config->parse_lxc_network($value);
> + PVE::LXC::update_net($vmid, $conf, $opt, $net, $netid, $rootdir);
> + } elsif ($opt eq 'memory' || $opt eq 'swap') {
> + if (!$hotplug_memory_done) { # don't call twice if both opts are passed
> + $hotplug_memory->($conf->{pending}->{memory}, $conf->{pending}->{swap});
> + }
> + } else {
> + die "skip\n"; # skip non-hotpluggable
> + }
> + };
> + if (my $err = $@) {
> + $add_error->($opt, $err) if $err ne "skip\n";
here a reload might make sense but is missing - we don't know whether
any of the actual hotplug helpers we called modified $conf without
persisting it before die-ing.. reloading would reset any such incomplete
modifications.
> + } else {
> + $conf->{$opt} = $value;
> + delete $conf->{pending}->{$opt};
> + $class->write_config($vmid, $conf);
> + $conf = $class->load_config($vmid); # update/reload
this reload should not be needed..
> + }
> + }
> +}
> +
> +sub vmconfig_apply_pending {
> + my ($class, $vmid, $conf, $storecfg) = @_;
> +
> +
superfluous blank line
> + my $pending_delete_hash = $class->split_flagged_list($conf->{pending}->{delete});
> + while (my ($opt, $force) = each %$pending_delete_hash) {
> + $conf = $class->load_config($vmid); # update/reload
again, should not be needed..
> + if (!defined($conf->{$opt})) {
> + $class->vmconfig_undelete_pending_option($conf, $opt);
isn't this what vmconfig_cleanup_pending is for?
> + $class->write_config($vmid, $conf);
single write_config at the end of applying should be enough - we are
only modifying $conf here..
> + } elsif ($opt =~ m/^mp(\d+)$/) {
> + my $old = $conf->{$opt};
unused variable
> + my $mp = PVE::LXC::Config->parse_ct_mountpoint($conf->{$opt});
> + if ($mp->{type} eq 'volume') {
> + PVE::LXC::Config->add_unused_volume($conf, $mp->{volume});
> + }
> + $class->vmconfig_undelete_pending_option($conf, $opt);
> + delete $conf->{$opt};
> + $class->write_config($vmid, $conf);
same, write_config is not needed here
> + } else {
> + $class->vmconfig_undelete_pending_option($conf, $opt);
> + delete $conf->{$opt};
> + $class->write_config($vmid, $conf);
same, write_config is not needed here
> + }
> + }
if you call vmconfig_cleanup_pending in the beginning, this whole loop
body can become
if ($mp) {
parse;
if ($mp->{volume}) {
add_unused;
}
}
delete $conf->{opt}
undelete $conf->{pending}->{$opt}
> +
> + $conf = $class->load_config($vmid); # update/reload
> +
shouldn't be needed either
> +
> + foreach my $opt (keys %{$conf->{pending}}) { # add/change
next if $opt eq 'delete'; # just to be safe?
> + $conf = $class->load_config($vmid); # update/reload
shouldn't be needed
> + my $value = $conf->{pending}->{$opt};
> +
> + if (defined($conf->{$opt}) && ($conf->{$opt} eq $conf->{pending}->{$opt})) {
> + # skip if nothing changed
would also be handled by vmconfig_cleanup_pending
then this whole loop body could be
$conf->{$opt} = delete $conf->{pending}->{$opt};
> + } else {
> + $conf->{$opt} = $value;
> + }
> +
> + delete $conf->{pending}->{$opt};
> + $class->write_config($vmid, $conf);
move the write_config out of the loop to have a single write_config at
the end, for ALL the changes we made in this sub.
> + }
> +
> +
> +}
> +
> sub __snapshot_save_vmstate {
> my ($class, $vmid, $conf, $snapname, $storecfg) = @_;
> die "implement me - snapshot_save_vmstate\n";
> @@ -324,7 +506,7 @@ my $features_desc = {
> },
> };
>
> -my $confdesc = {
> +$confdesc = {
> lock => {
> optional => 1,
> type => 'string',
> --
> 2.20.1
>
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
More information about the pve-devel
mailing list