[pve-devel] [PATCH v2 container 17/18] rework update_pct_config to write into pending section
Fabian Grünbichler
f.gruenbichler at proxmox.com
Wed Oct 2 13:53:27 CEST 2019
On September 30, 2019 2:44 pm, Oguz Bektas wrote:
> use vmconfig_hotplug_pending or vmconfig_apply_pending to apply the
> pending changes, depending on whether the CT is on- or offline.
>
> Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> ---
> src/PVE/LXC/Config.pm | 334 ++++++++++++++----------------------------
> 1 file changed, 106 insertions(+), 228 deletions(-)
>
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index 47bd4bb..14c26bc 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm
> @@ -4,11 +4,13 @@ use strict;
> use warnings;
>
> use PVE::AbstractConfig;
> +use PVE::RPCEnvironment;
> 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);
>
> @@ -900,267 +902,143 @@ sub write_pct_config {
> }
>
> sub update_pct_config {
> - my ($class, $vmid, $conf, $running, $param, $delete) = @_;
> + my ($class, $vmid, $conf, $running, $param) = @_;
>
> - my @nohotplug;
> + my $rpcenv = PVE::RPCEnvironment::get();
> + my $authuser = $rpcenv->get_user();
>
> - my $new_disks = 0;
> - my @deleted_volumes;
> + my $delete_str = extract_param($param, 'delete');
> + my @delete = PVE::Tools::split_list($delete_str);
> + my $revert_str = extract_param($param, 'revert');
> + my @revert = PVE::Tools::split_list($revert_str);
I am still not convinced that hashes for delete and revert would not be
more readable down below.
>
> - my $rootdir;
> - if ($running) {
> - my $pid = PVE::LXC::find_lxc_pid($vmid);
> - $rootdir = "/proc/$pid/root";
> + PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, {}, [@delete]);
still no permission checks for reverting?
> +
> + foreach my $opt (PVE::Tools::split_list($revert_str)) {
should be @revert..
> + raise_param_exc({ revert => "unknown option '$opt'" })
> + if !$class->option_exists($opt);
> +
> + raise_param_exc({ delete => "you can't use '-$opt' and " .
wrong parameter to raise the exception for
> + "'-revert $opt' at the same time" })
> + if defined($param->{$opt});
> +
> + push @revert, $opt;
> }
@revert now contains each reverted option twice ;)
>
> - my $hotplug_error = sub {
> - if ($running) {
> - push @nohotplug, @_;
> - return 1;
> - } else {
> - return 0;
> - }
> - };
> + foreach my $opt (@revert) {
> + delete $conf->{pending}->{$opt};
> + # FIXME: maybe check before doing this?
check what?
> + $class->remove_from_pending_delete($conf, $opt); # remove from deletion queue
> + }
> + $class->write_config($vmid, $conf);
do we really need to write_config here? this was just a logical change,
and if we die afterwards the API call has failed and the changes so
far where side-effect free anyway..
>
> - if (defined($delete)) {
> - foreach my $opt (@$delete) {
> - if (!exists($conf->{$opt})) {
> - # silently ignore
> - next;
> - }
> + foreach my $opt (@delete) {
> + raise_param_exc({ delete => "you can't use '-$opt' and -delete $opt' at the same time" })
> + if defined($param->{$opt});
>
> - if ($opt eq 'memory' || $opt eq 'rootfs') {
> - die "unable to delete required option '$opt'\n";
> - } elsif ($opt eq 'hostname') {
> - delete $conf->{$opt};
> - } elsif ($opt eq 'swap') {
> - delete $conf->{$opt};
> - PVE::LXC::write_cgroup_value("memory", $vmid,
> - "memory.memsw.limit_in_bytes", -1);
> - } elsif ($opt eq 'description' || $opt eq 'onboot' || $opt eq 'startup' || $opt eq 'hookscript') {
> - delete $conf->{$opt};
> - } elsif ($opt eq 'nameserver' || $opt eq 'searchdomain' ||
> - $opt eq 'tty' || $opt eq 'console' || $opt eq 'cmode') {
> - next if $hotplug_error->($opt);
> - delete $conf->{$opt};
> - } elsif ($opt eq 'cores') {
> - delete $conf->{$opt}; # rest is handled by pvestatd
> - } elsif ($opt eq 'cpulimit') {
> - PVE::LXC::write_cgroup_value("cpu", $vmid, "cpu.cfs_quota_us", -1);
> - delete $conf->{$opt};
> - } elsif ($opt eq 'cpuunits') {
> - PVE::LXC::write_cgroup_value("cpu", $vmid, "cpu.shares", $confdesc->{cpuunits}->{default});
> - delete $conf->{$opt};
> - } elsif ($opt =~ m/^net(\d)$/) {
> - delete $conf->{$opt};
> - next if !$running;
> - my $netid = $1;
> - PVE::Network::veth_delete("veth${vmid}i$netid");
> - } elsif ($opt eq 'protection') {
> - delete $conf->{$opt};
> - } elsif ($opt =~ m/^unused(\d+)$/) {
> - next if $hotplug_error->($opt);
> - PVE::LXC::Config->check_protection($conf, "can't remove CT $vmid drive '$opt'");
> - push @deleted_volumes, $conf->{$opt};
> - delete $conf->{$opt};
> - } elsif ($opt =~ m/^mp(\d+)$/) {
> - next if $hotplug_error->($opt);
> - PVE::LXC::Config->check_protection($conf, "can't remove CT $vmid drive '$opt'");
> - my $mp = PVE::LXC::Config->parse_ct_mountpoint($conf->{$opt});
> - delete $conf->{$opt};
> - if ($mp->{type} eq 'volume') {
> - PVE::LXC::Config->add_unused_volume($conf, $mp->{volume});
> - }
> - } elsif ($opt eq 'unprivileged') {
> - die "unable to delete read-only option: '$opt'\n";
> - } elsif ($opt eq 'features') {
> - next if $hotplug_error->($opt);
> - delete $conf->{$opt};
> - } else {
> - die "implement me (delete: $opt)"
> - }
> - PVE::LXC::Config->write_config($vmid, $conf) if $running;
> - }
> + raise_param_exc({ delete => "you can't use '-delete $opt' and " .
> + "'-revert $opt' at the same time" })
> + if grep(/^$opt$/, @revert);
> +
> +
> + raise_param_exc({ delete => "unknown option '$opt'" })
> + if !$class->option_exists($opt);
> }
>
> - # 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 $wanted_memory = PVE::Tools::extract_param($param, 'memory');
> - my $wanted_swap = PVE::Tools::extract_param($param, 'swap');
> - if (defined($wanted_memory) || defined($wanted_swap)) {
> -
> - my $old_memory = ($conf->{memory} || 512);
> - my $old_swap = ($conf->{swap} || 0);
> -
> - $wanted_memory //= $old_memory;
> - $wanted_swap //= $old_swap;
> -
> - my $total = $wanted_memory + $wanted_swap;
> - if ($running) {
> - my $old_total = $old_memory + $old_swap;
> - if ($total > $old_total) {
> - 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));
> + PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef, $param, []);
> +
> + my $storage_cfg = PVE::Storage::config();
> +
> + my $repl_conf = PVE::ReplicationConfig->new();
still missing use PVE::ReplicationConfig (but see comments in cover
letter)
> + my $is_replicated = $repl_conf->check_for_existing_jobs($vmid, 1);
> + if ($is_replicated) {
> + $class->foreach_mountpoint_full($param, 0, sub {
> + my ($opt, $mountpoint) = @_;
> + my $volid = $mountpoint->{volume};
> + return if !$volid || !($mountpoint->{replicate}//1);
> + if ($mountpoint->{type} eq 'volume') {
> + my ($storeid, $format);
> + if ($volid =~ $PVE::LXC::NEW_DISK_RE) {
> + $storeid = $1;
> + $format = $mountpoint->{format} || PVE::Storage::storage_default_format($storage_cfg, $storeid);
> } 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));
> + ($storeid, undef) = PVE::Storage::parse_volume_id($volid, 1);
> + $format = (PVE::Storage::parse_volname($storage_cfg, $volid))[6];
> }
> + return if PVE::Storage::storage_can_replicate($storage_cfg, $storeid, $format);
> + my $scfg = PVE::Storage::storage_config($storage_cfg, $storeid);
> + return if $scfg->{shared};
> }
> - $conf->{memory} = $wanted_memory;
> - $conf->{swap} = $wanted_swap;
> -
> - PVE::LXC::Config->write_config($vmid, $conf) if $running;
> + die "cannot add non-replicatable volume to a replicated VM\n";
> + });
> }
>
> - my $storecfg = PVE::Storage::config();
> + # write updates to pending section
> + my $modified = {}; # record modified options
> +
> + foreach my $opt (@delete) {
> + if (!defined($conf->{$opt}) && !defined($conf->{pending}->{$opt})) {
> + warn "cannot delete '$opt' - not set in current configuration!\n";
> + next;
> + }
> + $modified->{$opt} = 1;
> + if ($opt eq 'memory' || $opt eq 'rootfs' || $opt eq 'ostype') {
> + die "unable to delete required option '$opt'\n";
> + } elsif ($opt =~ m/^unused(\d+)$/) {
> + $class->check_protection($conf, "can't remove CT $vmid drive '$opt'");
> + } elsif ($opt =~ m/^mp(\d+)$/) {
> + $class->check_protection($conf, "can't remove CT $vmid drive '$opt'");
> + } elsif ($opt eq 'unprivileged') {
> + die "unable to delete read-only option: '$opt'\n";
> + }
> + $class->add_to_pending_delete($conf, $opt);
> + $class->write_config($vmid, $conf);
same here, no changes with side-effects so far, no need to persist
$conf (yet)..
> + }
>
> - my $used_volids = {};
> my $check_content_type = sub {
> my ($mp) = @_;
> my $sid = PVE::Storage::parse_volume_id($mp->{volume});
> - my $storage_config = PVE::Storage::storage_config($storecfg, $sid);
> + my $storage_config = PVE::Storage::storage_config($storage_cfg, $sid);
> die "storage '$sid' does not allow content type 'rootdir' (Container)\n"
> if !$storage_config->{content}->{rootdir};
> };
>
> - my $rescan_volume = sub {
> - my ($mp) = @_;
> - eval {
> - $mp->{size} = PVE::Storage::volume_size_info($storecfg, $mp->{volume}, 5)
> - if !defined($mp->{size});
> - };
> - warn "Could not rescan volume size - $@\n" if $@;
> - };
> -
> - foreach my $opt (keys %$param) {
> + foreach my $opt (keys %$param) { # add/change
> + $modified->{$opt} = 1;
> my $value = $param->{$opt};
> - my $check_protection_msg = "can't update CT $vmid drive '$opt'";
> - if ($opt eq 'hostname' || $opt eq 'arch') {
> - $conf->{$opt} = $value;
> - } elsif ($opt eq 'onboot') {
> - $conf->{$opt} = $value ? 1 : 0;
> - } elsif ($opt eq 'startup') {
> - $conf->{$opt} = $value;
> - } elsif ($opt eq 'tty' || $opt eq 'console' || $opt eq 'cmode') {
> - next if $hotplug_error->($opt);
> - $conf->{$opt} = $value;
> + if ($opt =~ m/^mp(\d+)$/ || $opt eq 'rootfs') {
> + $class->check_protection($conf, "can't update CT $vmid drive '$opt'");
> + my $mp = $opt eq 'rootfs' ? $class->parse_ct_rootfs($value) : $class->parse_ct_mountpoint($value);
> + $check_content_type->($mp);
> + $conf->{pending}->{$opt} = $value;
drop the last line
> + } elsif ($opt eq 'hookscript') {
> + PVE::GuestHelpers::check_hookscript($value);
> + $conf->{pending}->{$opt} = $value;
drop the last line
> } elsif ($opt eq 'nameserver') {
> - next if $hotplug_error->($opt);
> my $list = PVE::LXC::verify_nameserver_list($value);
$value =
> - $conf->{$opt} = $list;
> + $conf->{pending}->{$opt} = $list;
drop this line
> } elsif ($opt eq 'searchdomain') {
> - next if $hotplug_error->($opt);
> my $list = PVE::LXC::verify_searchdomain_list($value);
$value =
> - $conf->{$opt} = $list;
> - } elsif ($opt eq 'cores') {
> - $conf->{$opt} = $value;# rest is handled by pvestatd
> - } elsif ($opt eq 'cpulimit') {
> - if ($value == 0) {
> - PVE::LXC::write_cgroup_value("cpu", $vmid, "cpu.cfs_quota_us", -1);
> - } else {
> - PVE::LXC::write_cgroup_value("cpu", $vmid, "cpu.cfs_quota_us", int(100000*$value));
> - }
> - $conf->{$opt} = $value;
> - } elsif ($opt eq 'cpuunits') {
> - $conf->{$opt} = $value;
> - PVE::LXC::write_cgroup_value("cpu", $vmid, "cpu.shares", $value);
> - } elsif ($opt eq 'description') {
> - $conf->{$opt} = $value;
> - } elsif ($opt =~ m/^net(\d+)$/) {
> - my $netid = $1;
> - my $net = PVE::LXC::Config->parse_lxc_network($value);
> - if (!$running) {
> - $conf->{$opt} = PVE::LXC::Config->print_lxc_network($net);
> - } else {
> - PVE::LXC::update_net($vmid, $conf, $opt, $net, $netid, $rootdir);
> - }
> - } elsif ($opt eq 'protection') {
> - $conf->{$opt} = $value ? 1 : 0;
> - } elsif ($opt =~ m/^mp(\d+)$/) {
> - next if $hotplug_error->($opt);
> - PVE::LXC::Config->check_protection($conf, $check_protection_msg);
> - my $old = $conf->{$opt};
> - my $mp = PVE::LXC::Config->parse_ct_mountpoint($value);
> - if ($mp->{type} eq 'volume') {
> - &$check_content_type($mp);
> - $used_volids->{$mp->{volume}} = 1;
> - &$rescan_volume($mp);
> - $conf->{$opt} = PVE::LXC::Config->print_ct_mountpoint($mp);
> - } else {
> - $conf->{$opt} = $value;
> - }
> - if (defined($old)) {
> - my $mp = PVE::LXC::Config->parse_ct_mountpoint($old);
> - if ($mp->{type} eq 'volume') {
> - PVE::LXC::Config->add_unused_volume($conf, $mp->{volume});
> - }
> - }
> - $new_disks = 1;
> - } elsif ($opt eq 'rootfs') {
> - next if $hotplug_error->($opt);
> - PVE::LXC::Config->check_protection($conf, $check_protection_msg);
> - my $old = $conf->{$opt};
> - my $mp = PVE::LXC::Config->parse_ct_rootfs($value);
> - if ($mp->{type} eq 'volume') {
> - &$check_content_type($mp);
> - $used_volids->{$mp->{volume}} = 1;
> - &$rescan_volume($mp);
> - $conf->{$opt} = PVE::LXC::Config->print_ct_mountpoint($mp, 1);
> - } else {
> - $conf->{$opt} = $value;
> - }
> - if (defined($old)) {
> - my $mp = PVE::LXC::Config->parse_ct_rootfs($old);
> - if ($mp->{type} eq 'volume') {
> - PVE::LXC::Config->add_unused_volume($conf, $mp->{volume});
> - }
> - }
> - $new_disks = 1;
> + $conf->{pending}->{$opt} = $list;
drop this line
> } elsif ($opt eq 'unprivileged') {
> die "unable to modify read-only option: '$opt'\n";
> - } elsif ($opt eq 'ostype') {
> - next if $hotplug_error->($opt);
> - $conf->{$opt} = $value;
> - } elsif ($opt eq 'features') {
> - next if $hotplug_error->($opt);
> - $conf->{$opt} = $value;
> - } elsif ($opt eq 'hookscript') {
> - PVE::GuestHelpers::check_hookscript($value);
> - $conf->{$opt} = $value;
> } else {
drop the else
> - die "implement me: $opt";
> + $conf->{pending}->{$opt} = $param->{$opt};
and unconditionally do
$conf->{pending}->{$opt} = $value;
> }
> -
> - PVE::LXC::Config->write_config($vmid, $conf) if $running;
> + $class->remove_from_pending_delete($conf, $opt);
> + $class->write_config($vmid, $conf);
again, no need to write_config here - we haven't done any changes that
have side-effects..
> }
>
> - # Apply deletions and creations of new volumes
> - if (@deleted_volumes) {
> - my $storage_cfg = PVE::Storage::config();
> - foreach my $volume (@deleted_volumes) {
> - next if $used_volids->{$volume}; # could have been re-added, too
> - # also check for references in snapshots
> - next if $class->is_volume_in_use($conf, $volume, 1);
> - PVE::LXC::delete_mountpoint_volume($storage_cfg, $vmid, $volume);
> - }
> - }
> + $conf = $class->load_config($vmid); # update/reload
given that we don't need to do a single write_config up to this point,
we also don't need to reload here ;)
> + my $changes = $class->cleanup_pending($conf);
> + $class->write_config($vmid, $conf) if $changes;
same, cleanup_pending only changes $conf, but has no side-effects
otherwise.
>
> - if ($new_disks) {
> - my $storage_cfg = PVE::Storage::config();
> - PVE::LXC::create_disks($storage_cfg, $vmid, $conf, $conf);
> - }
> -
> - # This should be the last thing we do here
> - if ($running && scalar(@nohotplug)) {
> - die "unable to modify " . join(',', @nohotplug) . " while container is running\n";
> + if ($running) {
> + my $errors = {};
> + $class->vmconfig_hotplug_pending($vmid, $conf, $storage_cfg, $modified, $errors);
> + raise_param_exc($errors) if %$errors;
> + } else {
> + $class->vmconfig_apply_pending($vmid, $conf, $storage_cfg);
so now only the questions remains whether we need to write_config before
calling hotplug/apply, depending on how those handle $conf ;) see
comments on #18
> }
> }
>
> --
> 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