[pve-devel] [PATCH cluster v2 5/7] corosync: add atomic_write_conf and cleanup

[Thomas Lamprecht]

On 09/14/2017 10:18 AM, Fabian Grünbichler wrote:
> On Thu, Sep 14, 2017 at 10:00:39AM +0200, Thomas Lamprecht wrote:
>> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
>> ---
>>   data/PVE/Corosync.pm | 39 ++++++++++++++++-----------------------
>>   1 file changed, 16 insertions(+), 23 deletions(-)
>>
>> diff --git a/data/PVE/Corosync.pm b/data/PVE/Corosync.pm
>> index 1180316..1d58bf0 100644
>> --- a/data/PVE/Corosync.pm
>> +++ b/data/PVE/Corosync.pm
>> @@ -146,20 +146,6 @@ sub write_conf {
>>       return $raw;
>>   }
>>   
>> -sub conf_version {
>> -    my ($conf, $noerr, $new_value) = @_;
>> -
>> -    my $totem = $conf->{main}->{totem};
>> -    if (defined($totem) && defined($totem->{config_version})) {
>> -	$totem->{config_version} = $new_value if $new_value;
>> -	return $totem->{config_version};
>> -    }
>> -
>> -    return undef if $noerr;
>> -
>> -    die "invalid corosync config - unable to read version\n";
>> -}
>> -
>>   # read only - use "rename corosync.conf.new corosync.conf" to write
> 
> minor nitpick: this comment should maybe be updated to refer to the new
> atomic_write_conf?
> 

Yes, makes sense to do so!

>>   PVE::Cluster::cfs_register_file('corosync.conf', \&parse_conf);
>>   # this is read/write
>> @@ -182,17 +168,9 @@ sub check_conf_exists {
>>   sub update_nodelist {
>>       my ($conf, $nodelist) = @_;
>>   
>> -    delete $conf->{digest};
>> -
>> -    my $version = conf_version($conf);
>> -    conf_version($conf, undef, $version + 1);
>> -
>>       $conf->{main}->{nodelist}->{node} = $nodelist;
>>   
>> -    PVE::Cluster::cfs_write_file("corosync.conf.new", $conf);
>> -
>> -    rename("/etc/pve/corosync.conf.new", "/etc/pve/corosync.conf")
>> -	|| die "activate  corosync.conf.new failed - $!\n";
>> +    atomic_write_conf($conf, 1);
>>   }
>>   
>>   sub nodelist {
>> @@ -205,4 +183,19 @@ sub totem_config {
>>       return clone($conf->{main}->{totem});
>>   }
>>   
> 
> maybe add a comment here that you need to hold a cluster lock if you
> want to call this ;)
> 

Yes, for read-modify-write it must be locked, the current users of this
method also do this (not since long, I added it ~two months ago)
Will do so, thanks.

>> +sub atomic_write_conf {
>> +    my ($conf, $increase_version) = @_;
>> +
>> +    if ($increase_version) {
>> +	die "invalid corosync config: unable to read config version\n"
>> +	    if !defined($conf->{main}->{totem}->{config_version});
>> +	$conf->{main}->{totem}->{config_version}++;
>> +    }
>> +
>> +    PVE::Cluster::cfs_write_file("corosync.conf.new", $conf);
>> +
>> +    rename("/etc/pve/corosync.conf.new", "/etc/pve/corosync.conf")
>> +	|| die "activating corosync.conf.new failed - $!\n";
>> +}
>> +
>>   1;
>> -- 
>> 2.11.0
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel