[pve-devel] [PATCH cluster 1/4] add migration format to datacenter config

[Thomas Lamprecht]

On 10/28/2016 12:09 PM, Fabian Grünbichler wrote:
> On Thu, Oct 27, 2016 at 05:00:10PM +0200, Thomas Lamprecht wrote:
>> This adds a new format for configuring cluster wide migration
>> settings.
>> Those settings include the migration transfer method, secure ssh or
>> unsecure tcp, this deprecates the migration_unsecure parameter which
>> we only keep for backward compatibility and map it to the new
>> property.
>> Further the migration network can be set, this denotes the network
>> used for sending the migration traffic.
>>
>> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
>> ---
>>   data/PVE/Cluster.pm | 42 ++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
>> index fe741cf..893dbe0 100644
>> --- a/data/PVE/Cluster.pm
>> +++ b/data/PVE/Cluster.pm
>> @@ -1291,6 +1291,25 @@ sub ssh_merge_known_hosts {
>>   
>>   }
>>   
>> +my $migration_format = {
>> +    type => {
>> +	default_key => 1,
>> +	type => 'string',
>> +	enum => ['ssh', 'insecure'],
>> +	description => "Migration is secure using SSH tunnel by default. For " .
>> +	  "secure private networks you can disable it to speed up migration.",
>> +	default => 'ssh',
>> +	format_description => 'migration type',
>> +    },
>> +    network => {
>> +	optional => 1,
>> +	type => 'string', format => 'CIDR',
>> +	format_description => 'CIDR',
>> +	description => "Set the network CIDR for the network you want to use " .
>> +	  "for transfering migration data.",
>> +    },
>> +};
>> +
> it might make sense to make the descriptions easier to understand, maybe
> something like:
>
> "Type of migration tunnel. Migration traffic is encrypted using an SSH
> tunnel by default. On secure, completely private networks this can be
> disabled to increase performance."
>
> "CIDR of the (sub) network that is used for migration."

Yes, makes sense.

>>   my $datacenter_schema = {
>>       type => "object",
>>       additionalProperties => 0,
>> @@ -1316,7 +1335,14 @@ my $datacenter_schema = {
>>   	migration_unsecure => {
>>   	    optional => 1,
>>   	    type => 'boolean',
>> -	    description => "Migration is secure using SSH tunnel by default. For secure private networks you can disable it to speed up migration.",
>> +	    description => "Migration is secure using SSH tunnel by default. " .
>> +	      "For secure private networks you can disable it to speed up " .
>> +	      "migration. Deprecated, use the 'migration' property instead!",
>> +	},
>> +	migration => {
>> +	    optional => 1,
>> +	    type => 'string', format => $migration_format,
>> +	    description => "For cluster wide migration settings.",
>>   	},
>>   	console => {
>>   	    optional => 1,
>> @@ -1362,7 +1388,19 @@ sub get_datacenter_schema { return $datacenter_schema };
>>   sub parse_datacenter_config {
>>       my ($filename, $raw) = @_;
>>   
>> -    return PVE::JSONSchema::parse_config($datacenter_schema, $filename, $raw // '');
>> +    my $res = PVE::JSONSchema::parse_config($datacenter_schema, $filename, $raw // '');
>> +
>> +    if (my $migration = $res->{migration}) {
>> +	$res->{migration} = PVE::JSONSchema::parse_property_string($migration_format, $migration);
>> +    }
>> +
>> +    # for backwards compatibility only, new migration property has precedence
>> +    if (my $migration_unsecure = $res->{migration_unsecure} &&
>> +        !defined($res->{migration}->{type})) {
>> +      $res->{migration}->{type} = ($migration_unsecure) ? 'insecure' : 'ssh';
>> +    }
>> +
>> +    return $res;
> wouldn't it make more sense to not allow both options (and error out or
> at least warn)?

Error-ing out on each datacenter config read seems not that ideal to me.
As I can directly map the old setting to the new (i.e. a name change of 
the property only) doing this seems not that bad for me.

How about adding similar check to the write parser so that it gets 
rewritten to the new format if the old is set?

>
>>   }
>>   
>>   sub write_datacenter_config {
>> -- 
>> 2.1.4
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel