[pve-devel] [RFC cluster v2 01/10] move addnode/delnode from CLI to cluster config API
Thomas Lamprecht
t.lamprecht at proxmox.com
Tue Dec 12 09:25:33 CET 2017
On 12/06/2017 04:01 PM, Fabian Grünbichler wrote:
> high-level: not sure if those operations should really run sync? maybe
> forking a worker just to make sure is a good idea.. also would create a
> task log entry and record potential warnings outside of the journal ;)
>
OK, so after picking this up again and re-thinking this thoughtfully
I decided against this proposal.
The reasons are:
* this may never be a long running operation, we have a 10 second
timeout for the locks we acquire and then just add a config entry
which has no significant time usage (say < ms). If ain't quorate to
begin with we abort directly
* I can pass the current up to date corosync config and authkey directly
as an answer (see 03/10) to the requester, this is really convenient and
would be quite complicated to do so if running in a worker. Granted, I
could add another API call returning this info but then the adding side
would still need to poll if addnode worked or not, to much overhead and
complexity, IMHO.
But I agree still that a out-of journal tracking of this would be nice,
especially for user feedback. To address this, I would do a
PVE::Cluster::log_msg where the addnode is logged and tracked, this shows
up in the WebUIs Cluster Log, which is a fitting place IMO.
Also I still agree that the create (09/10) and add (04/10) (not addnode)
calls should be in a worker, here we do not add such complexity and they
actually may run that long so that a worker is justified.
> On Mon, Dec 04, 2017 at 12:11:08PM +0100, Thomas Lamprecht wrote:
>> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
>> ---
>> data/PVE/API2/ClusterConfig.pm | 210 ++++++++++++++++++++++++++++++++++++++
>> data/PVE/CLI/pvecm.pm | 223 +----------------------------------------
>> 2 files changed, 213 insertions(+), 220 deletions(-)
>>
>> diff --git a/data/PVE/API2/ClusterConfig.pm b/data/PVE/API2/ClusterConfig.pm
>> index 46db3da..fa01022 100644
>> --- a/data/PVE/API2/ClusterConfig.pm
>> +++ b/data/PVE/API2/ClusterConfig.pm
>> @@ -76,6 +76,216 @@ __PACKAGE__->register_method({
>> return PVE::RESTHandler::hash_to_array($nodelist, 'node');
>> }});
>>
>> +# lock method to ensure local and cluster wide atomicity
>> +# if we're a single node cluster just lock locally, we have no other cluster
>> +# node which we could contend with, else also acquire a cluster wide lock
>> +my $config_change_lock = sub {
>> + my ($code) = @_;
>> +
>> + my $local_lock_fn = "/var/lock/pvecm.lock";
>> + PVE::Tools::lock_file($local_lock_fn, 10, sub {
>> + PVE::Cluster::cfs_update(1);
>> + my $members = PVE::Cluster::get_members();
>> + if (scalar(keys %$members) > 1) {
>> + return PVE::Cluster::cfs_lock_file('corosync.conf', 10, $code);
>> + } else {
>> + return $code->();
>> + }
>> + });
>> +};
>> +
>> +
>> +__PACKAGE__->register_method ({
>> + name => 'addnode',
>> + path => 'nodes',
>> + method => 'POST',
>> + protected => 1,
>> + description => "Adds a node to the cluster configuration.",
>> + parameters => {
>> + additionalProperties => 0,
>> + properties => {
>> + node => PVE::JSONSchema::get_standard_option('pve-node'),
>> + nodeid => {
>> + type => 'integer',
>> + description => "Node id for this node.",
>> + minimum => 1,
>> + optional => 1,
>> + },
>> + votes => {
>> + type => 'integer',
>> + description => "Number of votes for this node",
>> + minimum => 0,
>> + optional => 1,
>> + },
>> + force => {
>> + type => 'boolean',
>> + description => "Do not throw error if node already exists.",
>> + optional => 1,
>> + },
>> + ring0_addr => {
>> + type => 'string', format => 'address',
>> + description => "Hostname (or IP) of the corosync ring0 address of this node.".
>> + " Defaults to nodes hostname.",
>> + optional => 1,
>> + },
>> + ring1_addr => {
>> + type => 'string', format => 'address',
>> + description => "Hostname (or IP) of the corosync ring1 address, this".
>> + " needs an valid bindnet1_addr.",
>> + optional => 1,
>> + },
>> + },
>> + },
>> + returns => { type => 'null' },
>> + code => sub {
>> + my ($param) = @_;
>> +
>> + PVE::Cluster::check_cfs_quorum();
>> +
>> + my $code = sub {
>> + my $conf = PVE::Cluster::cfs_read_file("corosync.conf");
>> + my $nodelist = PVE::Corosync::nodelist($conf);
>> + my $totem_cfg = PVE::Corosync::totem_config($conf);
>> +
>> + my $name = $param->{node};
>> +
>> + # ensure we do not reuse an address, that can crash the whole cluster!
>> + my $check_duplicate_addr = sub {
>> + my $addr = shift;
>> + return if !defined($addr);
>> +
>> + while (my ($k, $v) = each %$nodelist) {
>> + next if $k eq $name; # allows re-adding a node if force is set
>> + if ($v->{ring0_addr} eq $addr || ($v->{ring1_addr} && $v->{ring1_addr} eq $addr)) {
>> + die "corosync: address '$addr' already defined by node '$k'\n";
>> + }
>> + }
>> + };
>> +
>> + &$check_duplicate_addr($param->{ring0_addr});
>> + &$check_duplicate_addr($param->{ring1_addr});
>> +
>> + $param->{ring0_addr} = $name if !$param->{ring0_addr};
>> +
>> + die "corosync: using 'ring1_addr' parameter needs a configured ring 1 interface!\n"
>> + if $param->{ring1_addr} && !defined($totem_cfg->{interface}->{1});
>> +
>> + die "corosync: ring 1 interface configured but 'ring1_addr' parameter not defined!\n"
>> + if defined($totem_cfg->{interface}->{1}) && !defined($param->{ring1_addr});
>> +
>> + if (defined(my $res = $nodelist->{$name})) {
>> + $param->{nodeid} = $res->{nodeid} if !$param->{nodeid};
>> + $param->{votes} = $res->{quorum_votes} if !defined($param->{votes});
>> +
>> + if ($res->{quorum_votes} == $param->{votes} &&
>> + $res->{nodeid} == $param->{nodeid} && $param->{force}) {
>> + print "forcing overwrite of configured node '$name'\n";
>> + } else {
>> + die "can't add existing node '$name'\n";
>> + }
>> + } elsif (!$param->{nodeid}) {
>> + my $nodeid = 1;
>> +
>> + while(1) {
>> + my $found = 0;
>> + foreach my $v (values %$nodelist) {
>> + if ($v->{nodeid} eq $nodeid) {
>> + $found = 1;
>> + $nodeid++;
>> + last;
>> + }
>> + }
>> + last if !$found;
>> + };
>> +
>> + $param->{nodeid} = $nodeid;
>> + }
>> +
>> + $param->{votes} = 1 if !defined($param->{votes});
>> +
>> + PVE::Cluster::gen_local_dirs($name);
>> +
>> + eval { PVE::Cluster::ssh_merge_keys(); };
>> + warn $@ if $@;
>> +
>> + $nodelist->{$name} = {
>> + ring0_addr => $param->{ring0_addr},
>> + nodeid => $param->{nodeid},
>> + name => $name,
>> + };
>> + $nodelist->{$name}->{ring1_addr} = $param->{ring1_addr} if $param->{ring1_addr};
>> + $nodelist->{$name}->{quorum_votes} = $param->{votes} if $param->{votes};
>> +
>> + PVE::Corosync::update_nodelist($conf, $nodelist);
>> + };
>> +
>> + $config_change_lock->($code);
>> + die $@ if $@;
>> +
>> + return undef;
>> + }});
>> +
>> +
>> +__PACKAGE__->register_method ({
>> + name => 'delnode',
>> + path => 'nodes',
>> + method => 'DELETE',
>> + protected => 1,
>> + description => "Removes a node from the cluster configuration.",
>> + parameters => {
>> + additionalProperties => 0,
>> + properties => {
>> + node => {
>> + type => 'string',
>> + description => "Hostname or IP of the corosync ring0 address of this node.",
>> + },
>> + },
>> + },
>> + returns => { type => 'null' },
>> + code => sub {
>> + my ($param) = @_;
>> +
>> + my $local_node = PVE::INotify::nodename();
>> + die "Cannot delete myself from cluster!\n" if $param->{node} eq $local_node;
>> +
>> + PVE::Cluster::check_cfs_quorum();
>> +
>> + my $code = sub {
>> + my $conf = PVE::Cluster::cfs_read_file("corosync.conf");
>> + my $nodelist = PVE::Corosync::nodelist($conf);
>> +
>> + my $node;
>> + my $nodeid;
>> +
>> + foreach my $tmp_node (keys %$nodelist) {
>> + my $d = $nodelist->{$tmp_node};
>> + my $ring0_addr = $d->{ring0_addr};
>> + my $ring1_addr = $d->{ring1_addr};
>> + if (($tmp_node eq $param->{node}) ||
>> + (defined($ring0_addr) && ($ring0_addr eq $param->{node})) ||
>> + (defined($ring1_addr) && ($ring1_addr eq $param->{node}))) {
>> + $node = $tmp_node;
>> + $nodeid = $d->{nodeid};
>> + last;
>> + }
>> + }
>> +
>> + die "Node/IP: $param->{node} is not a known host of the cluster.\n"
>> + if !defined($node);
>> +
>> + delete $nodelist->{$node};
>> +
>> + PVE::Corosync::update_nodelist($conf, $nodelist);
>> +
>> + PVE::Tools::run_command(['corosync-cfgtool','-k', $nodeid]) if defined($nodeid);
>> + };
>> +
>> + $config_change_lock->($code);
>> + die $@ if $@;
>> +
>> + return undef;
>> + }});
>> +
>> __PACKAGE__->register_method({
>> name => 'totem',
>> path => 'totem',
>> diff --git a/data/PVE/CLI/pvecm.pm b/data/PVE/CLI/pvecm.pm
>> index 6d0d704..a92f831 100755
>> --- a/data/PVE/CLI/pvecm.pm
>> +++ b/data/PVE/CLI/pvecm.pm
>> @@ -11,6 +11,7 @@ use PVE::Cluster;
>> use PVE::INotify;
>> use PVE::JSONSchema;
>> use PVE::CLIHandler;
>> +use PVE::API2::ClusterConfig;
>> use PVE::Corosync;
>>
>> use base qw(PVE::CLIHandler);
>> @@ -58,23 +59,6 @@ sub backup_database {
>> }
>> }
>>
>> -# lock method to ensure local and cluster wide atomicity
>> -# if we're a single node cluster just lock locally, we have no other cluster
>> -# node which we could contend with, else also acquire a cluster wide lock
>> -my $config_change_lock = sub {
>> - my ($code) = @_;
>> -
>> - my $local_lock_fn = "/var/lock/pvecm.lock";
>> - PVE::Tools::lock_file($local_lock_fn, 10, sub {
>> - PVE::Cluster::cfs_update(1);
>> - my $members = PVE::Cluster::get_members();
>> - if (scalar(keys %$members) > 1) {
>> - return PVE::Cluster::cfs_lock_file('corosync.conf', 10, $code);
>> - } else {
>> - return $code->();
>> - }
>> - });
>> -};
>>
>> __PACKAGE__->register_method ({
>> name => 'keygen',
>> @@ -263,207 +247,6 @@ _EOD
>> }});
>>
>> __PACKAGE__->register_method ({
>> - name => 'addnode',
>> - path => 'addnode',
>> - method => 'PUT',
>> - description => "Adds a node to the cluster configuration.",
>> - parameters => {
>> - additionalProperties => 0,
>> - properties => {
>> - node => PVE::JSONSchema::get_standard_option('pve-node'),
>> - nodeid => {
>> - type => 'integer',
>> - description => "Node id for this node.",
>> - minimum => 1,
>> - optional => 1,
>> - },
>> - votes => {
>> - type => 'integer',
>> - description => "Number of votes for this node",
>> - minimum => 0,
>> - optional => 1,
>> - },
>> - force => {
>> - type => 'boolean',
>> - description => "Do not throw error if node already exists.",
>> - optional => 1,
>> - },
>> - ring0_addr => {
>> - type => 'string', format => 'address',
>> - description => "Hostname (or IP) of the corosync ring0 address of this node.".
>> - " Defaults to nodes hostname.",
>> - optional => 1,
>> - },
>> - ring1_addr => {
>> - type => 'string', format => 'address',
>> - description => "Hostname (or IP) of the corosync ring1 address, this".
>> - " needs an valid bindnet1_addr.",
>> - optional => 1,
>> - },
>> - },
>> - },
>> - returns => { type => 'null' },
>> -
>> - code => sub {
>> - my ($param) = @_;
>> -
>> - if (!$param->{force} && (-t STDIN || -t STDOUT)) {
>> - die "error: `addnode` should not get called interactively!\nUse ".
>> - "`pvecm add <cluster-node>` to add a node to a cluster!\n";
>> - }
>> -
>> - PVE::Cluster::check_cfs_quorum();
>> -
>> - my $code = sub {
>> - my $conf = PVE::Cluster::cfs_read_file("corosync.conf");
>> - my $nodelist = PVE::Corosync::nodelist($conf);
>> - my $totem_cfg = PVE::Corosync::totem_config($conf);
>> -
>> - my $name = $param->{node};
>> -
>> - # ensure we do not reuse an address, that can crash the whole cluster!
>> - my $check_duplicate_addr = sub {
>> - my $addr = shift;
>> - return if !defined($addr);
>> -
>> - while (my ($k, $v) = each %$nodelist) {
>> - next if $k eq $name; # allows re-adding a node if force is set
>> - if ($v->{ring0_addr} eq $addr || ($v->{ring1_addr} && $v->{ring1_addr} eq $addr)) {
>> - die "corosync: address '$addr' already defined by node '$k'\n";
>> - }
>> - }
>> - };
>> -
>> - &$check_duplicate_addr($param->{ring0_addr});
>> - &$check_duplicate_addr($param->{ring1_addr});
>> -
>> - $param->{ring0_addr} = $name if !$param->{ring0_addr};
>> -
>> - die "corosync: using 'ring1_addr' parameter needs a configured ring 1 interface!\n"
>> - if $param->{ring1_addr} && !defined($totem_cfg->{interface}->{1});
>> -
>> - die "corosync: ring 1 interface configured but 'ring1_addr' parameter not defined!\n"
>> - if defined($totem_cfg->{interface}->{1}) && !defined($param->{ring1_addr});
>> -
>> - if (defined(my $res = $nodelist->{$name})) {
>> - $param->{nodeid} = $res->{nodeid} if !$param->{nodeid};
>> - $param->{votes} = $res->{quorum_votes} if !defined($param->{votes});
>> -
>> - if ($res->{quorum_votes} == $param->{votes} &&
>> - $res->{nodeid} == $param->{nodeid}) {
>> - print "node $name already defined\n";
>> - if ($param->{force}) {
>> - exit (0);
>> - } else {
>> - exit (-1);
>> - }
>> - } else {
>> - die "can't add existing node\n";
>> - }
>> - } elsif (!$param->{nodeid}) {
>> - my $nodeid = 1;
>> -
>> - while(1) {
>> - my $found = 0;
>> - foreach my $v (values %$nodelist) {
>> - if ($v->{nodeid} eq $nodeid) {
>> - $found = 1;
>> - $nodeid++;
>> - last;
>> - }
>> - }
>> - last if !$found;
>> - };
>> -
>> - $param->{nodeid} = $nodeid;
>> - }
>> -
>> - $param->{votes} = 1 if !defined($param->{votes});
>> -
>> - PVE::Cluster::gen_local_dirs($name);
>> -
>> - eval { PVE::Cluster::ssh_merge_keys(); };
>> - warn $@ if $@;
>> -
>> - $nodelist->{$name} = {
>> - ring0_addr => $param->{ring0_addr},
>> - nodeid => $param->{nodeid},
>> - name => $name,
>> - };
>> - $nodelist->{$name}->{ring1_addr} = $param->{ring1_addr} if $param->{ring1_addr};
>> - $nodelist->{$name}->{quorum_votes} = $param->{votes} if $param->{votes};
>> -
>> - PVE::Corosync::update_nodelist($conf, $nodelist);
>> - };
>> -
>> - $config_change_lock->($code);
>> - die $@ if $@;
>> -
>> - exit (0);
>> - }});
>> -
>> -
>> -__PACKAGE__->register_method ({
>> - name => 'delnode',
>> - path => 'delnode',
>> - method => 'PUT',
>> - description => "Removes a node to the cluster configuration.",
>> - parameters => {
>> - additionalProperties => 0,
>> - properties => {
>> - node => {
>> - type => 'string',
>> - description => "Hostname or IP of the corosync ring0 address of this node.",
>> - },
>> - },
>> - },
>> - returns => { type => 'null' },
>> -
>> - code => sub {
>> - my ($param) = @_;
>> -
>> - my $local_node = PVE::INotify::nodename();
>> - die "Cannot delete myself from cluster!\n" if $param->{node} eq $local_node;
>> -
>> - PVE::Cluster::check_cfs_quorum();
>> -
>> - my $code = sub {
>> - my $conf = PVE::Cluster::cfs_read_file("corosync.conf");
>> - my $nodelist = PVE::Corosync::nodelist($conf);
>> -
>> - my $node;
>> - my $nodeid;
>> -
>> - foreach my $tmp_node (keys %$nodelist) {
>> - my $d = $nodelist->{$tmp_node};
>> - my $ring0_addr = $d->{ring0_addr};
>> - my $ring1_addr = $d->{ring1_addr};
>> - if (($tmp_node eq $param->{node}) ||
>> - (defined($ring0_addr) && ($ring0_addr eq $param->{node})) ||
>> - (defined($ring1_addr) && ($ring1_addr eq $param->{node}))) {
>> - $node = $tmp_node;
>> - $nodeid = $d->{nodeid};
>> - last;
>> - }
>> - }
>> -
>> - die "Node/IP: $param->{node} is not a known host of the cluster.\n"
>> - if !defined($node);
>> -
>> - delete $nodelist->{$node};
>> -
>> - PVE::Corosync::update_nodelist($conf, $nodelist);
>> -
>> - run_command(['corosync-cfgtool','-k', $nodeid]) if defined($nodeid);
>> - };
>> -
>> - $config_change_lock->($code);
>> - die $@ if $@;
>> -
>> - return undef;
>> - }});
>> -
>> -__PACKAGE__->register_method ({
>> name => 'add',
>> path => 'add',
>> method => 'PUT',
>> @@ -885,8 +668,8 @@ our $cmddef = {
>> keygen => [ __PACKAGE__, 'keygen', ['filename']],
>> create => [ __PACKAGE__, 'create', ['clustername']],
>> add => [ __PACKAGE__, 'add', ['hostname']],
>> - addnode => [ __PACKAGE__, 'addnode', ['node']],
>> - delnode => [ __PACKAGE__, 'delnode', ['node']],
>> + addnode => [ 'PVE::API2::ClusterConfig', 'addnode', ['node']],
>> + delnode => [ 'PVE::API2::ClusterConfig', 'delnode', ['node']],
>> status => [ __PACKAGE__, 'status' ],
>> nodes => [ __PACKAGE__, 'nodes' ],
>> expected => [ __PACKAGE__, 'expected', ['expected']],
>> --
>> 2.11.0
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
More information about the pve-devel
mailing list