[pve-devel] [PATCH v2 ha-manager 02/12] Move code updating resource config from API2::HA::Resources to HA::Config

[Thomas Lamprecht]

On 9/30/19 9:22 AM, Fabian Ebner wrote:
> This makes it easier to update the resource configuration from within the CRM/LRM stack,
> which is needed for the new 'stop' command.
> 
> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
> ---
>  src/PVE/API2/HA/Resources.pm | 34 +--------------------------------
>  src/PVE/HA/Config.pm         | 37 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+), 33 deletions(-)
> 
> diff --git a/src/PVE/API2/HA/Resources.pm b/src/PVE/API2/HA/Resources.pm
> index 22d7f28..2b62ee8 100644
> --- a/src/PVE/API2/HA/Resources.pm
> +++ b/src/PVE/API2/HA/Resources.pm
> @@ -237,39 +237,7 @@ __PACKAGE__->register_method ({
>  
>  	check_service_state($sid, $param->{state});
>  
> -	PVE::HA::Config::lock_ha_domain(
> -	    sub {
> -
> -		my $cfg = PVE::HA::Config::read_resources_config();
> -
> -		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};
> -		    }
> -		}
> -
> -		PVE::HA::Config::write_resources_config($cfg)
> -
> -	    }, "update resource failed");
> +	PVE::HA::Config::update_resources_config($digest, $delete, $sid, $param);
>  
>  	return undef;
>      }});
> diff --git a/src/PVE/HA/Config.pm b/src/PVE/HA/Config.pm
> index ead1ee2..e800154 100644
> --- a/src/PVE/HA/Config.pm
> +++ b/src/PVE/HA/Config.pm
> @@ -125,6 +125,43 @@ sub read_and_check_resources_config {
>      return $conf;
>  }
>  
> +sub update_resources_config {
> +    my ($digest, $delete, $sid, $param) = @_;

Hmm, I do not like the order of arguments, IMO it's better to have
optional and less likely ones at the end, so that they can just
be omitted allowing to write
foo("bar");
vs-
foo(undef, undef, "bar");

I'd propose
my ($sid, $param, $delete, $digest) = @_;

The $digest is only for the API, the CRM does not cares as it just
enforces a state from a locked context and does no "read-change-update"
cycle where other changes from other API users could be overwritten.

$delete is also something the API will rather use, in your in-service
usage of this method you have both set to "0" anyway.

Sorry, this could've been said on v1 already, missed it.

> +
> +    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};
> +		}
> +	    }
> +
> +	    write_resources_config($cfg);
> +	}, "update resources config failed");
> +}
> +
>  sub parse_sid {
>      my ($sid) = @_;
>  
>