Re: [pve-devel] [PATCH ha-manager 3/4] config, env: allow bulk updates with update_resources_config

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

Am 15.12.25 um 11:24 AM schrieb Daniel Kral:
> diff --git a/src/PVE/HA/Config.pm b/src/PVE/HA/Config.pm
> index 04e039e0..159798a7 100644
> --- a/src/PVE/HA/Config.pm
> +++ b/src/PVE/HA/Config.pm
> @@ -136,37 +136,46 @@ sub read_and_check_resources_config {
>      return wantarray ? ($conf, $res->{digest}) : $conf;
>  }
>  
> +my sub update_single_resource_config_inplace {
> +    my ($cfg, $sid, $param, $delete) = @_;
> +
> +    ($sid, my $type, my $name) = parse_sid($sid);
> +
> +    my $scfg = $cfg->{ids}->{$sid}
> +        || die "no such resource '$sid'\n";
> +
> +    my $plugin = PVE::HA::Resources->lookup($scfg->{type});
> +    my $opts = $plugin->check_config($sid, $param, 0, 1);
> +
> +    foreach my $k (%$opts) {
> +        $scfg->{$k} = $opts->{$k};
> +    }
> +
> +    if ($delete) {
> +        my $options = $plugin->private()->{options}->{$ytype};
> +        foreach my $k (PVE::Tools::split_list($delete)) {
> +            my $d = $options->{$k}
> +                || die "no such option '$k'\n";
> +            die "unable to delete required option '$k'\n"
> +                if !$d->{optional};
> +            die "unable to delete fixed option '$k'\n"
> +                if $d->{fixed};
> +            delete $scfg->{$k};
> +        }
> +    }
> +}
> +
>  sub update_resources_config {
> -    my ($sid, $param, $delete, $digest) = @_;
> +    my ($changes, $delete, $digest) = @_;

Nit: I feel like the $delete belongs inside the $changes for a clean
interface. So something like
$changes = {
  $sid => {
    param => $param,
    delete => $delete,
  }
}

I know the single caller using bulk won't need it, but the current
interface does feel slightly off to me. What do you think?

>  
>      lock_ha_domain(
>          sub {
>              my $cfg = read_resources_config();
> -            ($sid, my $type, my $name) = parse_sid($sid);
> -
>              PVE::SectionConfig::assert_if_modified($cfg, $digest);
>  
> -            my $scfg = $cfg->{ids}->{$sid}
> -                || die "no such resource '$sid'\n";
> -
> -            my $plugin = PVE::HA::Resources->lookup($scfg->{type});
> -            my $opts = $plugin->check_config($sid, $param, 0, 1);
> -
> -            foreach my $k (%$opts) {
> -                $scfg->{$k} = $opts->{$k};
> -            }
> -
> -            if ($delete) {
> -                my $options = $plugin->private()->{options}->{$type};
> -                foreach my $k (PVE::Tools::split_list($delete)) {
> -                    my $d = $options->{$k}
> -                        || die "no such option '$k'\n";
> -                    die "unable to delete required option '$k'\n"
> -                        if !$d->{optional};
> -                    die "unable to delete fixed option '$k'\n"
> -                        if $d->{fixed};
> -                    delete $scfg->{$k};
> -                }
> +            for my $sid (keys %$changes) {

Nit: I'd prefer to iterate after 'sort'

> +                my $param = $changes->{$sid};
> +                update_single_resource_config_inplace($cfg, $sid, $param, $delete);
>              }
>  
>              write_resources_config($cfg);

---snip---

> diff --git a/src/PVE/HA/Sim/Hardware.pm b/src/PVE/HA/Sim/Hardware.pm
> index 0848d18a..b19a23b4 100644
> --- a/src/PVE/HA/Sim/Hardware.pm
> +++ b/src/PVE/HA/Sim/Hardware.pm
> @@ -114,10 +114,8 @@ sub read_service_config {
>      return $conf;
>  }
>  
> -sub update_service_config {
> -    my ($self, $sid, $param, $delete) = @_;
> -
> -    my $conf = $self->read_service_config();
> +my sub update_single_service_config_inplace {
> +    my ($conf, $sid, $param, $delete) = @_;
>  
>      my $sconf = $conf->{$sid} || die "no such resource '$sid'\n";
>  
> @@ -130,6 +128,17 @@ sub update_service_config {
>              delete $sconf->{$k};
>          }
>      }
> +}
> +
> +sub update_service_config {
> +    my ($self, $changes, $delete) = @_;
> +
> +    my $conf = $self->read_service_config();
> +
> +    for my $sid (keys %$changes) {

Nit: I'd prefer to iterate after 'sort'

> +        my $param = $changes->{$sid};
> +        update_single_service_config_inplace($conf, $sid, $param, $delete);
> +    }
>  
>      $self->write_service_config($conf);
>  }



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel