[pve-devel] [PATCH manager 1/4] ceph tools: set_pools: filter settings for erasure code pools

[pve-devel] [PATCH manager 1/4] ceph tools: set_pools: filter settings for erasure code pools

[Aaron Lauterer]

Erasure code pools cannot change certain settings after creation.
Trying to set them will cause errors on Cephs side.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 PVE/Ceph/Tools.pm | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index 7bd3eedc..65589820 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -205,7 +205,7 @@ sub check_ceph_enabled {
 }
 
 my $set_pool_setting = sub {
-    my ($pool, $setting, $value) = @_;
+    my ($pool, $setting, $value, $rados) = @_;
 
     my $command;
     if ($setting eq 'application') {
@@ -224,7 +224,7 @@ my $set_pool_setting = sub {
 	};
     }
 
-    my $rados = PVE::RADOS->new();
+    $rados = PVE::RADOS->new() if !$rados;
     eval { $rados->mon_command($command); };
     return $@ ? $@ : undef;
 };
@@ -232,6 +232,16 @@ my $set_pool_setting = sub {
 sub set_pool {
     my ($pool, $param) = @_;
 
+    my $rados = PVE::RADOS->new();
+
+    if (get_pool_type($pool, $rados) eq 'erasure') {
+	#remove parameters that cannot be changed for erasure coded pools
+	my $ignore_params = ['size', 'crush_rule'];
+	for my $setting (@$ignore_params) {
+	    print "cannot set '${setting}' for erasure coded pool\n";
+	    delete $param->{$setting};
+	}
+    }
     # by default, pool size always resets min_size, so set it as first item
     # https://tracker.ceph.com/issues/44862
     my $keys = [ grep { $_ ne 'size' } sort keys %$param ];
@@ -241,7 +251,7 @@ sub set_pool {
 	my $value = $param->{$setting};
 
 	print "pool $pool: applying $setting = $value\n";
-	if (my $err = $set_pool_setting->($pool, $setting, $value)) {
+	if (my $err = $set_pool_setting->($pool, $setting, $value, $rados)) {
 	    print "$err";
 	} else {
 	    delete $param->{$setting};
@@ -267,6 +277,12 @@ sub get_pool_properties {
     return $rados->mon_command($command);
 }
 
+sub get_pool_type {
+    my ($pool, $rados) = @_;
+    return 'erasure' if get_pool_properties($pool, $rados)->{erasure_code_profile};
+    return 'replicated';
+}
+
 sub create_pool {
     my ($pool, $param, $rados) = @_;
     $rados = PVE::RADOS->new() if !defined($rados);
-- 
2.30.2

[pve-devel] [PATCH manager 2/4] api: ceph pools: add type to returned properties

[Aaron Lauterer]

The osd dump already contains the pool type in numerical format.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 PVE/API2/Ceph/Pools.pm | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/PVE/API2/Ceph/Pools.pm b/PVE/API2/Ceph/Pools.pm
index efdee360..b93f87e4 100644
--- a/PVE/API2/Ceph/Pools.pm
+++ b/PVE/API2/Ceph/Pools.pm
@@ -66,6 +66,11 @@ __PACKAGE__->register_method ({
 		    type => 'integer',
 		    title => 'Size',
 		},
+		type => {
+		    type => 'string',
+		    title => 'Type',
+		    enum => ['replicated', 'erasure', 'unknown'],
+		},
 		min_size => {
 		    type => 'integer',
 		    title => 'Min Size',
@@ -185,6 +190,15 @@ __PACKAGE__->register_method ({
 		$d->{bytes_used} = $s->{bytes_used};
 		$d->{percent_used} = $s->{percent_used};
 	    }
+
+	    if ($e->{type} == 1) {
+		$d->{type} = 'replicated';
+	    } elsif ($e->{type} == 3) {
+		$d->{type} = 'erasure';
+	    } else {
+		# we should never get here, but better be safe
+		$d->{type} = 'unknown';
+	    }
 	    push @$data, $d;
 	}
 
-- 
2.30.2

Re: [pve-devel] [PATCH manager 2/4] api: ceph pools: add type to returned properties

[Dominik Csapak]

one nit inline

On 4/29/22 16:16, Aaron Lauterer wrote:
> The osd dump already contains the pool type in numerical format.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
>   PVE/API2/Ceph/Pools.pm | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/PVE/API2/Ceph/Pools.pm b/PVE/API2/Ceph/Pools.pm
> index efdee360..b93f87e4 100644
> --- a/PVE/API2/Ceph/Pools.pm
> +++ b/PVE/API2/Ceph/Pools.pm
> @@ -66,6 +66,11 @@ __PACKAGE__->register_method ({
>   		    type => 'integer',
>   		    title => 'Size',
>   		},
> +		type => {
> +		    type => 'string',
> +		    title => 'Type',
> +		    enum => ['replicated', 'erasure', 'unknown'],
> +		},
>   		min_size => {
>   		    type => 'integer',
>   		    title => 'Min Size',
> @@ -185,6 +190,15 @@ __PACKAGE__->register_method ({
>   		$d->{bytes_used} = $s->{bytes_used};
>   		$d->{percent_used} = $s->{percent_used};
>   	    }
> +
> +	    if ($e->{type} == 1) {
> +		$d->{type} = 'replicated';
> +	    } elsif ($e->{type} == 3) {
> +		$d->{type} = 'erasure';
> +	    } else {
> +		# we should never get here, but better be safe
> +		$d->{type} = 'unknown';
> +	    }

i remember we talked off-list about those '1' / '3' values,
but is there any reference we can use to document these?
(as comment or commit message)

even if it's only a link to some github or refence to a file in
some commit hash of ceph, that way we can easily look it up
if it changes

also what would be value '2' ?

>   	    push @$data, $d;
>   	}
>   

Re: [pve-devel] [PATCH manager 2/4] api: ceph pools: add type to returned properties

[Aaron Lauterer]

On 5/2/22 09:11, Dominik Csapak wrote:
> one nit inline
> 
> On 4/29/22 16:16, Aaron Lauterer wrote:
>> The osd dump already contains the pool type in numerical format.
>>
>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>> ---
>>   PVE/API2/Ceph/Pools.pm | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/PVE/API2/Ceph/Pools.pm b/PVE/API2/Ceph/Pools.pm
>> index efdee360..b93f87e4 100644
>> --- a/PVE/API2/Ceph/Pools.pm
>> +++ b/PVE/API2/Ceph/Pools.pm
>> @@ -66,6 +66,11 @@ __PACKAGE__->register_method ({
>>               type => 'integer',
>>               title => 'Size',
>>           },
>> +        type => {
>> +            type => 'string',
>> +            title => 'Type',
>> +            enum => ['replicated', 'erasure', 'unknown'],
>> +        },
>>           min_size => {
>>               type => 'integer',
>>               title => 'Min Size',
>> @@ -185,6 +190,15 @@ __PACKAGE__->register_method ({
>>           $d->{bytes_used} = $s->{bytes_used};
>>           $d->{percent_used} = $s->{percent_used};
>>           }
>> +
>> +        if ($e->{type} == 1) {
>> +        $d->{type} = 'replicated';
>> +        } elsif ($e->{type} == 3) {
>> +        $d->{type} = 'erasure';
>> +        } else {
>> +        # we should never get here, but better be safe
>> +        $d->{type} = 'unknown';
>> +        }
> 
> i remember we talked off-list about those '1' / '3' values,
> but is there any reference we can use to document these?
> (as comment or commit message)
> 
> even if it's only a link to some github or refence to a file in
> some commit hash of ceph, that way we can easily look it up
> if it changes

sure
> 
> also what would be value '2' ?

beats me, didn't find anything ;)

> 
>>           push @$data, $d;
>>       }
> 
> 

[pve-devel] [PATCH manager 3/4] ui: ceph pool edit: disable size and crush rule for erasure pools

[Aaron Lauterer]

They cannot be changed after pool creation for erasure coded pools

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 www/manager6/ceph/Pool.js | 44 ++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/www/manager6/ceph/Pool.js b/www/manager6/ceph/Pool.js
index 41df216e..16b903ce 100644
--- a/www/manager6/ceph/Pool.js
+++ b/www/manager6/ceph/Pool.js
@@ -19,21 +19,28 @@ Ext.define('PVE.CephPoolInputPanel', {
 	    allowBlank: false,
 	},
 	{
-	    xtype: 'proxmoxintegerfield',
+	    xtype: 'pmxDisplayEditField',
+	    cbind: {
+		editable: '{!isErasure}',
+	    },
 	    fieldLabel: gettext('Size'),
 	    name: 'size',
-	    value: 3,
-	    minValue: 2,
-	    maxValue: 7,
-	    allowBlank: false,
-	    listeners: {
-		change: function(field, val) {
-		    let size = Math.round(val / 2);
-		    if (size > 1) {
-			field.up('inputpanel').down('field[name=min_size]').setValue(size);
-		    }
+	    editConfig: {
+		xtype: 'proxmoxintegerfield',
+		value: 3,
+		minValue: 2,
+		maxValue: 7,
+		allowBlank: false,
+		listeners: {
+		    change: function(field, val) {
+			let size = Math.round(val / 2);
+			if (size > 1) {
+			    field.up('inputpanel').down('field[name=min_size]').setValue(size);
+			}
+		    },
 		},
 	    },
+
 	},
     ],
     column2: [
@@ -101,14 +108,18 @@ Ext.define('PVE.CephPoolInputPanel', {
 	    hidden: true,
 	},
 	{
-	    xtype: 'pveCephRuleSelector',
-	    fieldLabel: 'Crush Rule', // do not localize
-	    name: 'crush_rule',
+	    xtype: 'pmxDisplayEditField',
 	    cbind: {
+		editable: '{!isErasure}',
 		nodename: '{nodename}',
 		isCreate: '{isCreate}',
 	    },
-	    allowBlank: false,
+	    fieldLabel: 'Crush Rule', // do not localize
+	    name: 'crush_rule',
+	    editConfig: {
+		xtype: 'pveCephRuleSelector',
+		allowBlank: false,
+	    },
 	},
 	{
 	    xtype: 'proxmoxintegerfield',
@@ -203,6 +214,7 @@ Ext.define('PVE.Ceph.PoolEdit', {
 	cbind: {
 	    nodename: '{nodename}',
 	    pool_name: '{pool_name}',
+	    isErasure: '{isErasure}',
 	    isCreate: '{isCreate}',
 	},
     }],
@@ -354,6 +366,7 @@ Ext.define('PVE.node.Ceph.PoolList', {
 		title: gettext('Edit') + ': Ceph Pool',
 		nodename: nodename,
 		pool_name: rec.data.pool_name,
+		isErasure: rec.data.type === 'erasure',
 		autoShow: true,
 		listeners: {
 		    destroy: () => rstore.load(),
@@ -371,6 +384,7 @@ Ext.define('PVE.node.Ceph.PoolList', {
 			Ext.create('PVE.Ceph.PoolEdit', {
 			    title: gettext('Create') + ': Ceph Pool',
 			    isCreate: true,
+			    isErasure: false,
 			    nodename: nodename,
 			    autoShow: true,
 			    listeners: {
-- 
2.30.2

[pve-devel] [PATCH manager 4/4] ui: ceph pools: add pool type column

[Aaron Lauterer]

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 www/manager6/ceph/Pool.js | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/www/manager6/ceph/Pool.js b/www/manager6/ceph/Pool.js
index 16b903ce..a1f008d1 100644
--- a/www/manager6/ceph/Pool.js
+++ b/www/manager6/ceph/Pool.js
@@ -240,6 +240,13 @@ Ext.define('PVE.node.Ceph.PoolList', {
 	    sortable: true,
 	    dataIndex: 'pool_name',
 	},
+	{
+	    text: gettext('Type'),
+	    minWidth: 100,
+	    flex: 1,
+	    dataIndex: 'type',
+	    hidden: true,
+	},
 	{
 	    text: gettext('Size') + '/min',
 	    minWidth: 100,
-- 
2.30.2

Re: [pve-devel] [PATCH manager 1/4] ceph tools: set_pools: filter settings for erasure code pools

[Dominik Csapak]

one minor nit inline

On 4/29/22 16:16, Aaron Lauterer wrote:
> Erasure code pools cannot change certain settings after creation.
> Trying to set them will cause errors on Cephs side.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
>   PVE/Ceph/Tools.pm | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
> index 7bd3eedc..65589820 100644
> --- a/PVE/Ceph/Tools.pm
> +++ b/PVE/Ceph/Tools.pm
> @@ -205,7 +205,7 @@ sub check_ceph_enabled {
>   }
>   
>   my $set_pool_setting = sub {
> -    my ($pool, $setting, $value) = @_;
> +    my ($pool, $setting, $value, $rados) = @_;
>   
>       my $command;
>       if ($setting eq 'application') {
> @@ -224,7 +224,7 @@ my $set_pool_setting = sub {
>   	};
>       }
>   
> -    my $rados = PVE::RADOS->new();
> +    $rados = PVE::RADOS->new() if !$rados;
>       eval { $rados->mon_command($command); };
>       return $@ ? $@ : undef;
>   };
> @@ -232,6 +232,16 @@ my $set_pool_setting = sub {
>   sub set_pool {
>       my ($pool, $param) = @_;
>   
> +    my $rados = PVE::RADOS->new();
> +
> +    if (get_pool_type($pool, $rados) eq 'erasure') {
> +	#remove parameters that cannot be changed for erasure coded pools
> +	my $ignore_params = ['size', 'crush_rule'];
> +	for my $setting (@$ignore_params) {
> +	    print "cannot set '${setting}' for erasure coded pool\n";

i'd only print that line if there was actually a value there, else
every one gets those warnings even if they did not attempt to set e.g.
the size

> +	    delete $param->{$setting};
> +	}
> +    }
>       # by default, pool size always resets min_size, so set it as first item
>       # https://tracker.ceph.com/issues/44862
>       my $keys = [ grep { $_ ne 'size' } sort keys %$param ];
> @@ -241,7 +251,7 @@ sub set_pool {
>   	my $value = $param->{$setting};
>   
>   	print "pool $pool: applying $setting = $value\n";
> -	if (my $err = $set_pool_setting->($pool, $setting, $value)) {
> +	if (my $err = $set_pool_setting->($pool, $setting, $value, $rados)) {
>   	    print "$err";
>   	} else {
>   	    delete $param->{$setting};
> @@ -267,6 +277,12 @@ sub get_pool_properties {
>       return $rados->mon_command($command);
>   }
>   
> +sub get_pool_type {
> +    my ($pool, $rados) = @_;
> +    return 'erasure' if get_pool_properties($pool, $rados)->{erasure_code_profile};
> +    return 'replicated';
> +}
> +
>   sub create_pool {
>       my ($pool, $param, $rados) = @_;
>       $rados = PVE::RADOS->new() if !defined($rados);

Re: [pve-devel] [PATCH manager 1/4] ceph tools: set_pools: filter settings for erasure code pools

[Dominik Csapak]

aside from the 2 nits, consider this

Reviewed-By: Dominik Csapak <d.csapak@proxmox.com>
Tested-By: Dominik Csapak <d.csapak@proxmox.com>