Re: [pve-devel] [PATCH common 3/4] section config: allow separated property lists for plugins

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Tue, Nov 14, 2023 at 11:33:38AM +0100, Dominik Csapak wrote:
> when using 'init(1)'. This saves the property lists per type instead of
> a big one, and using create/updateSchema creates a new schema with the
> options as 'oneOf' and/or 'instance-types' (depending if the schemas
> match).
> 
> for that to work there are a few changes in how to use the 'options' hash:
> * when wanting to use a property that is not defined in either the local
>   list or the global one, the 'type' property has to be given to specify
>   which plugin type this should be taken from
> * if it's desired to always have some global properties in the schema,
>   they have to be given in the 'global' options section now (the perl
>   package where the base plugin is defined)
> 
> for now, we then save the global options in the section '__global',
> which is now a forbidden plugin type, but that should be fine as we
> don't use that
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/SectionConfig.pm | 256 ++++++++++++++++++++++++++++++---------
>  1 file changed, 198 insertions(+), 58 deletions(-)
> 
> diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
> index 5690476..1e44c3d 100644
> --- a/src/PVE/SectionConfig.pm
> +++ b/src/PVE/SectionConfig.pm
> @@ -30,6 +30,9 @@ sub register {
>      die "duplicate plugin registration (type = $type)"
>  	if defined($pdata->{plugins}->{$type});
>  
> +    die "invalid plugin type (type = '__global')"
> +	if $type eq '__global';
> +
>      my $plugindata = $class->plugindata();
>      $pdata->{plugindata}->{$type} = $plugindata;
>      $pdata->{plugins}->{$type} = $class;
> @@ -51,6 +54,67 @@ sub plugindata {
>      return {};
>  }
>  
> +my sub cmp_property {
> +    my ($src, $tgt, $skip_opts) = @_;

"compare" vs "source/target", I'd argue 'a' and 'b' would be a better
choice for the parameter names.

> +
> +    my $merged = {$src->%*, $tgt->%*};
> +    for my $opt ($skip_opts->@*) {
> +	delete $merged->{$opt};
> +    }
> +    for my $opt (keys $merged->%*) {
> +	return 0 if !defined($src->{$opt}) || !defined($tgt->{$opt}) || $tgt->{$opt} ne $src->{$opt};

^ might be worth checking that they aren't `ref()`s, as you don't want
to `ne` a potential hash here (eg. an `items` in an array schema?)
I'm not sure we have such types in the section config currently, but
IIRC you wanted to add that at some point?
But apart from array/object types we also have `requires => [array]` -
that one in particular should be compared recursively if we use this to
optimize the `instance-types` arrays.

> +    }
> +
> +    return 1;
> +}
> +
> +my sub copy_property {

^ This thing again. 😒

At the very least make its body just:

    return { $_[0]->%* };

Or just kill it.

> +    my ($src) = @_;
> +
> +    my $res = {};
> +    foreach my $k (keys %$src) {
> +	$res->{$k} = $src->{$k};
> +    }
> +
> +    return $res;
> +}
> +
> +
> +my sub add_property {
> +    my ($props, $key, $prop, $type) = @_;
> +
> +    if (!defined($props->{$key})) {
> +	$props->{$key} = $prop;
> +	return;
> +    }
> +
> +    if (!defined($props->{$key}->{oneOf})) {
> +	if (cmp_property($props->{$key}, $prop, ['instance-types'])) {
> +	    push $props->{$key}->{'instance-types'}->@*, $type;
> +	} else {
> +	    my $new_prop = delete $props->{$key};
> +	    delete $new_prop->{'type-property'};
> +	    delete $prop->{'type-property'};
> +	    $props->{$key} = {
> +		'type-property' => 'type',
> +		oneOf => [
> +		    $new_prop,
> +		    $prop,
> +		],
> +	    };
> +	}
> +    } else {
> +	for my $existing_prop ($props->{$key}->{oneOf}->@*) {
> +	    if (cmp_property($existing_prop, $prop, ['instance-types', 'type-property'])) {
> +		push $existing_prop->{'instance-types'}->@*, $type;
> +		return;
> +	    }
> +	}
> +
> +	push $props->{$key}->{oneOf}->@*, $prop;
> +    }
> +}
> +
>  sub createSchema {
>      my ($class, $skip_type, $base) = @_;
>  
> @@ -58,44 +122,55 @@ sub createSchema {
>      my $propertyList = $pdata->{propertyList};
>      my $plugins = $pdata->{plugins};
>  
> -    my $props = $base || {};
> -
> -    my $copy_property = sub {
> -	my ($src) = @_;
> +    my $global_options = $pdata->{options}->{__global};
> +    my $propertyListSeparated = $pdata->{propertyListSeparated};
>  
> -	my $res = {};
> -	foreach my $k (keys %$src) {
> -	    $res->{$k} = $src->{$k};
> -	}
> +    my $props = $base || {};
>  
> -	return $res;
> -    };
> +    if (!defined($global_options)) {
> +	foreach my $p (keys %$propertyList) {
> +	    next if $skip_type && $p eq 'type';
>  
> -    foreach my $p (keys %$propertyList) {
> -	next if $skip_type && $p eq 'type';
> +	    if (!$propertyList->{$p}->{optional}) {
> +		$props->{$p} = $propertyList->{$p};
> +		next;
> +	    }
>  
> -	if (!$propertyList->{$p}->{optional}) {
> -	    $props->{$p} = $propertyList->{$p};
> -	    next;
> -	}
> +	    my $required = 1;
>  
> -	my $required = 1;
> +	    my $copts = $class->options();
> +	    $required = 0 if defined($copts->{$p}) && $copts->{$p}->{optional};
>  
> -	my $copts = $class->options();
> -	$required = 0 if defined($copts->{$p}) && $copts->{$p}->{optional};
> +	    foreach my $t (keys %$plugins) {
> +		my $opts = $pdata->{options}->{$t} || {};
> +		$required = 0 if !defined($opts->{$p}) || $opts->{$p}->{optional};
> +	    }
>  
> -	foreach my $t (keys %$plugins) {
> -	    my $opts = $pdata->{options}->{$t} || {};
> -	    $required = 0 if !defined($opts->{$p}) || $opts->{$p}->{optional};
> +	    if ($required) {
> +		# make a copy, because we modify the optional property
> +		my $res = copy_property($propertyList->{$p});
> +		$res->{optional} = 0;
> +		$props->{$p} = $res;
> +	    } else {
> +		$props->{$p} = $propertyList->{$p};
> +	    }
>  	}
> -
> -	if ($required) {
> -	    # make a copy, because we modify the optional property
> -	    my $res = &$copy_property($propertyList->{$p});
> -	    $res->{optional} = 0;
> -	    $props->{$p} = $res;
> -	} else {
> -	    $props->{$p} = $propertyList->{$p};
> +    } else {
> +	for my $type (sort keys %$plugins) {
> +	    my $opts = $pdata->{options}->{$type} || {};
> +	    for my $key (sort keys $opts->%*) {
> +		my $schema = $class->get_property_schema($type, $key);
> +		my $prop = copy_property($schema);
> +		$prop->{'instance-types'} = [$type];
> +		$prop->{'type-property'} = 'type';
> +		$prop->{optional} = 1 if $opts->{$key}->{optional};
> +
> +		add_property($props, $key, $prop, $type);
> +	    }
> +	}
> +	for my $opt (keys $global_options->%*) {
> +	    $props->{$opt} = copy_property($propertyList->{$opt});
> +	    $props->{$opt}->{optional} = 1 if $global_options->{$opt}->{optional};
>  	}
>      }
>  
> @@ -113,34 +188,61 @@ sub updateSchema {
>      my $propertyList = $pdata->{propertyList};
>      my $plugins = $pdata->{plugins};
>  
> +    my $global_options = $pdata->{options}->{__global};
> +    my $propertyListSeparated = $pdata->{propertyListSeparated};
> +
>      my $props = $base || {};
>  
>      my $filter_type = $single_class ? $class->type() : undef;
>  
> -    foreach my $p (keys %$propertyList) {
> -	next if $p eq 'type';
> +    if (!defined($global_options)) {
> +	foreach my $p (keys %$propertyList) {
> +	    next if $p eq 'type';
>  
> -	my $copts = $class->options();
> +	    my $copts = $class->options();
>  
> -	next if defined($filter_type) && !defined($copts->{$p});
> +	    next if defined($filter_type) && !defined($copts->{$p});
>  
> -	if (!$propertyList->{$p}->{optional}) {
> -	    $props->{$p} = $propertyList->{$p};
> -	    next;
> -	}
> +	    if (!$propertyList->{$p}->{optional}) {
> +		$props->{$p} = $propertyList->{$p};
> +		next;
> +	    }
>  
> -	my $modifyable = 0;
> +	    my $modifyable = 0;
>  
> -	$modifyable = 1 if defined($copts->{$p}) && !$copts->{$p}->{fixed};
> +	    $modifyable = 1 if defined($copts->{$p}) && !$copts->{$p}->{fixed};
> +
> +	    foreach my $t (keys %$plugins) {
> +		my $opts = $pdata->{options}->{$t} || {};
> +		next if !defined($opts->{$p});
> +		$modifyable = 1 if !$opts->{$p}->{fixed};
> +	    }
> +	    next if !$modifyable;
>  
> -	foreach my $t (keys %$plugins) {
> -	    my $opts = $pdata->{options}->{$t} || {};
> -	    next if !defined($opts->{$p});
> -	    $modifyable = 1 if !$opts->{$p}->{fixed};
> +	    $props->{$p} = $propertyList->{$p};
> +	}
> +    } else {
> +	for my $type (sort keys %$plugins) {
> +	    my $opts = $pdata->{options}->{$type} || {};
> +	    for my $key (sort keys $opts->%*) {
> +		next if $opts->{$key}->{fixed};
> +
> +		my $schema = $class->get_property_schema($type, $key);
> +		my $prop = copy_property($schema);
> +		$prop->{'instance-types'} = [$type];
> +		$prop->{'type-property'} = 'type';
> +		$prop->{optional} = 1;
> +
> +		add_property($props, $key, $prop, $type);
> +	    }
>  	}
> -	next if !$modifyable;
>  
> -	$props->{$p} = $propertyList->{$p};
> +	for my $opt (keys $global_options->%*) {
> +	    next if $global_options->{$opt}->{fixed};
> +
> +	    $props->{$opt} = copy_property($propertyList->{$opt});
> +	    $props->{$opt}->{optional} = 1 if $global_options->{$opt}->{optional};
> +	}
>      }
>  
>      $props->{digest} = get_standard_option('pve-config-digest');
> @@ -160,22 +262,33 @@ sub updateSchema {
>  }
>  
>  sub init {
> -    my ($class) = @_;
> +    my ($class, $separate_properties) = @_;
>  
>      my $pdata = $class->private();
>  
> -    foreach my $k (qw(options plugins plugindata propertyList)) {
> +    foreach my $k (qw(options plugins plugindata propertyList propertyListSeparated)) {
>  	$pdata->{$k} = {} if !$pdata->{$k};
>      }
>  
> +    if ($separate_properties) {
> +	$pdata->{options}->{__global} = delete $pdata->{options};
> +    }
> +
>      my $plugins = $pdata->{plugins};
>      my $propertyList = $pdata->{propertyList};
> +    my $propertyListSeparated = $pdata->{propertyListSeparated};
>  
>      foreach my $type (keys %$plugins) {
>  	my $props = $plugins->{$type}->properties();
>  	foreach my $p (keys %$props) {
> -	    die "duplicate property '$p'" if defined($propertyList->{$p});
> -	    my $res = $propertyList->{$p} = {};
> +	    my $res;
> +	    if ($separate_properties) {
> +		die "duplicate property '$p'" if defined($propertyListSeparated->{$type}->{$p});

^ Can this even happen? $props is a hash per $type after all, and we're
now namespaced by $type
Should we instead check it against the global options?

> +		$res = $propertyListSeparated->{$type}->{$p} = {};
> +	    } else {
> +		die "duplicate property '$p'" if defined($propertyList->{$p});
> +		$res = $propertyList->{$p} = {};
> +	    }
>  	    my $data = $props->{$p};
>  	    for my $a (keys %$data) {
>  		$res->{$a} = $data->{$a};
> @@ -187,7 +300,13 @@ sub init {
>      foreach my $type (keys %$plugins) {
>  	my $opts = $plugins->{$type}->options();
>  	foreach my $p (keys %$opts) {
> -	    die "undefined property '$p'" if !$propertyList->{$p};
> +	    my $prop;
> +	    if ($separate_properties) {
> +		my $opt_type = $opts->{$p}->{type} // $type;

^ Similarly, we're coming from a fixed $type and query the `options`
from that very type, does it make sense for there to be a `type`
property present?

> +		$prop = $propertyListSeparated->{$opt_type}->{$p};
> +	    }
> +	    $prop //= $propertyList->{$p};
> +	    die "undefined property '$p'" if !$prop;
>  	}
>  	$pdata->{options}->{$type} = $opts;
>      }
> @@ -237,11 +356,13 @@ sub check_value {
>      return $value if $key eq 'type' && $type eq $value;
>  
>      my $opts = $pdata->{options}->{$type};
> +    my $global_opts = $pdata->{options}->{__global};
> +
>      die "unknown section type '$type'\n" if !$opts;
>  
> -    die "unexpected property '$key'\n" if !defined($opts->{$key});
> +    die "unexpected property '$key'\n" if !defined($opts->{$key}) && !defined($global_opts->{$key});
>  
> -    my $schema = $pdata->{propertyList}->{$key};
> +    my $schema = $class->get_property_schema($type, $key);
>      die "unknown property type\n" if !$schema;
>  
>      my $ct = $schema->{type};
> @@ -295,6 +416,23 @@ sub format_section_header {
>      return "$type: $sectionId\n";
>  }
>  
> +sub get_property_schema {
> +    my ($class, $type, $key) = @_;
> +
> +    my $pdata = $class->private();
> +    my $opts = $pdata->{options}->{$type};
> +
> +    my $separated = defined($pdata->{options}->{__global});

^ would it make sense to have this condition as a
`$class->is_separated()` sub?

> +
> +    my $schema;
> +    if ($separated) {
> +	my $opt_type = $opts->{$key}->{type} // $type;
> +	$schema = $pdata->{propertyListSeparated}->{$opt_type}->{$key};
> +    }
> +    $schema //= $pdata->{propertyList}->{$key};
> +
> +    return $schema;
> +}
>  
>  sub parse_config {
>      my ($class, $filename, $raw, $allow_unknown) = @_;
> @@ -322,7 +460,7 @@ sub parse_config {
>      my $is_array = sub {
>  	my ($type, $key) = @_;
>  
> -	my $schema = $pdata->{propertyList}->{$key};
> +	my $schema = $class->get_property_schema($type, $key);
>  	die "unknown property type\n" if !$schema;
>  
>  	return $schema->{type} eq 'array';
> @@ -494,7 +632,6 @@ sub write_config {
>      my ($class, $filename, $cfg, $allow_unknown) = @_;
>  
>      my $pdata = $class->private();
> -    my $propertyList = $pdata->{propertyList};
>  
>      my $out = '';
>  
> @@ -545,7 +682,8 @@ sub write_config {
>  	if ($scfg->{comment} && !$done_hash->{comment}) {
>  	    my $k = 'comment';
>  	    my $v = $class->encode_value($type, $k, $scfg->{$k});
> -	    $data .= &$format_config_line($propertyList->{$k}, $k, $v);
> +	    my $prop = $class->get_property_schema($type, $k);
> +	    $data .= &$format_config_line($prop, $k, $v);
>  	}
>  
>  	$data .= "\tdisable\n" if $scfg->{disable} && !$done_hash->{disable};
> @@ -562,7 +700,8 @@ sub write_config {
>  	    die "section '$sectionId' - missing value for required option '$k'\n"
>  		if !defined ($v);
>  	    $v = $class->encode_value($type, $k, $v);
> -	    $data .= &$format_config_line($propertyList->{$k}, $k, $v);
> +	    my $prop = $class->get_property_schema($type, $k);
> +	    $data .= &$format_config_line($prop, $k, $v);
>  	}
>  
>  	foreach my $k (@option_keys) {
> @@ -570,7 +709,8 @@ sub write_config {
>  	    my $v = $scfg->{$k};
>  	    next if !defined($v);
>  	    $v = $class->encode_value($type, $k, $v);
> -	    $data .= &$format_config_line($propertyList->{$k}, $k, $v);
> +	    my $prop = $class->get_property_schema($type, $k);
> +	    $data .= &$format_config_line($prop, $k, $v);
>  	}
>  
>  	$out .= "$data\n";
> -- 
> 2.30.2