Re: [RFC pve-network 1/1] sdn: apply changes on all nodes in parallel

[Daniel Herzig <d.herzig@proxmox.com>]

Thanks for working on this!

I've tested the patch in 2 scenarios on a three-node cluster:

(1) Worst case (by using VLAN tag 1 on a VNet
backed by a VLAN aware management bridge with default settings)
(2) Good case (doing the same, but using VLAN tag 3 on the same bridge)

Apart from the (expected) instant breaking of the network in case (1)
the patch leads to a more consistent state as previously. The earlier
sequential approach lead to inconsistent configuration states across the
clusternodes (depending on which node was reached first, or  -- to which
node access to the management-IP was lost first). So I'd see the
parallel approach as an enhancement in the worst-case setup. 

Case (2) just worked nicely, and makes application quite somewhat faster.

I haven't gone through further-reaching implications yet, but my first
impression of the patch is quite good by empirical testing of the above
situations.


Stefan Hanreich <s.hanreich@proxmox.com> writes:

> Utilize the API client to spawn tasks on each node and then monitor
> the progress of the tasks via the API as well. This allows for
> starting the reload tasks in parallel. It is particularly useful for
> large setups, where applying the SDN configuration can take a
> considerable amount of time.
>
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>
> Notes:
>     Sending this as an RFC, as this requires careful testing imo.
>     
>     An example where this change could backfire has been encountered
>     recently. A user applied the SDN configuration, which broke the
>     cluster / management network of the node that was carrying out the
>     reload network task. Since it was executed sequentially, only one node
>     was affected. In this scenario, with this patch applied, the whole
>     cluster would have been down.
>     
>     We could potentially provide an option that limits the amount of nodes
>     where the reload task runs in parallel and expose that in the UI?
>     
>     Also, currently the task could never fail and errors were shown in the
>     respective reload tasks executed on the nodes, I've kept that behavior
>     the same, but introduced warnings if there are any issues with API
>     calls.
>
>  src/PVE/API2/Network/SDN.pm | 109 ++++++++++++++++++++++++++----------
>  1 file changed, 78 insertions(+), 31 deletions(-)
>
> diff --git a/src/PVE/API2/Network/SDN.pm b/src/PVE/API2/Network/SDN.pm
> index e3c8d9dd..dd2cf05a 100644
> --- a/src/PVE/API2/Network/SDN.pm
> +++ b/src/PVE/API2/Network/SDN.pm
> @@ -10,6 +10,7 @@ use PVE::Cluster qw(cfs_lock_file cfs_read_file cfs_write_file);
>  use PVE::Exception qw(raise_param_exc);
>  use PVE::JSONSchema qw(get_standard_option);
>  use PVE::RESTHandler;
> +use PVE::RESTEnvironment qw(log_warn);
>  use PVE::RPCEnvironment;
>  use PVE::SafeSyslog;
>  use PVE::Tools qw(run_command extract_param);
> @@ -107,30 +108,6 @@ __PACKAGE__->register_method({
>      },
>  });
>  
> -my $create_reload_network_worker = sub {
> -    my ($nodename, $regenerate_frr) = @_;
> -
> -    my @command = ('pvesh', 'set', "/nodes/$nodename/network");
> -    push(@command, '--regenerate-frr', $regenerate_frr);
> -
> -    # FIXME: how to proxy to final node ?
> -    my $upid;
> -    print "$nodename: reloading network config\n";
> -    run_command(
> -        \@command,
> -        outfunc => sub {
> -            my $line = shift;
> -            if ($line =~ /["']?(UPID:[^\s"']+)["']?$/) {
> -                $upid = $1;
> -            }
> -        },
> -    );
> -    #my $upid = PVE::API2::Network->reload_network_config({ node => $nodename });
> -    my $res = PVE::Tools::upid_decode($upid);
> -
> -    return $res->{pid};
> -};
> -
>  __PACKAGE__->register_method({
>      name => 'lock',
>      protected => 1,
> @@ -282,6 +259,41 @@ __PACKAGE__->register_method({
>      },
>  });
>  
> +sub create_api_client {
> +    my ($request_timeout) = @_;
> +
> +    my $rpcenv = PVE::RPCEnvironment::get();
> +    my $authuser = $rpcenv->get_user();
> +    my $credentials = $rpcenv->get_credentials();
> +
> +    my $api_token = $credentials->{api_token};
> +    my $ticket = $credentials->{ticket};
> +    my $csrf_token = $credentials->{token};
> +
> +    my $node = PVE::INotify::nodename();
> +    my $fingerprint = PVE::Cluster::get_node_fingerprint($node);
> +
> +    my $conn_args = {
> +        protocol => 'https',
> +        host => 'localhost', # always call the api locally, let pveproxy handle the proxying
> +        port => 8006,
> +        username => $authuser,
> +        ticket => $ticket,
> +        apitoken => $api_token,
> +        timeout => $request_timeout // 25, # default slightly shorter than the proxy->daemon timeout
> +        cached_fingerprints => {
> +            $fingerprint => 1,
> +        },
> +    };
> +
> +    my $api_client = PVE::APIClient::LWP->new($conn_args->%*);
> +    if (defined($csrf_token)) {
> +        $api_client->update_csrftoken($csrf_token);
> +    }
> +
> +    return $api_client;
> +}
> +
>  __PACKAGE__->register_method({
>      name => 'reload',
>      protected => 1,
> @@ -291,6 +303,7 @@ __PACKAGE__->register_method({
>      permissions => {
>          check => ['perm', '/sdn', ['SDN.Allocate']],
>      },
> +    expose_credentials => 1,
>      parameters => {
>          additionalProperties => 0,
>          properties => {
> @@ -336,22 +349,56 @@ __PACKAGE__->register_method({
>          my $regenerate_frr = ($previous_config_has_frr || $new_config_has_frr) ? 1 : 0;
>  
>          my $code = sub {
> -            $rpcenv->{type} = 'priv'; # to start tasks in background
>              PVE::Cluster::check_cfs_quorum();
> +
> +            my $api_client = create_api_client();
>              my $nodelist = PVE::Cluster::get_nodelist();
> +
> +            my $tasks = {};
> +
>              for my $node (@$nodelist) {
> -                my $pid = eval { $create_reload_network_worker->($node, $regenerate_frr) };
> -                warn $@ if $@;
> +                print "$node: reloading network config\n";
> +
> +                my $upid = eval {
> +                    $api_client->put(
> +                        "/nodes/$node/network",
> +                        {
> +                            'regenerate-frr' => $regenerate_frr,
> +                        },
> +                    );
> +                };
> +
> +                if ($@) {
> +                    log_warn("$node: could not reload network configuration: $@\n");
> +                    next;
> +                }
> +
> +                $tasks->{$upid} = $node;
>              }
>  
> -            # FIXME: use libpve-apiclient (like in cluster join) to create
> -            # tasks and moitor the tasks.
> +            print "waiting for reload tasks to finish\n";
>  
> -            return;
> +            while ($tasks->%*) {
> +                for my $upid (keys $tasks->%*) {
> +                    my $node = $tasks->{$upid};
> +                    my $task = eval { $api_client->get("/nodes/$node/tasks/$upid/status") };
> +
> +                    if ($@) {
> +                        log_warn("could not get status of reload task: $@\n");
> +                        delete $tasks->{$upid};
> +                        next;
> +                    }
> +
> +                    next if $task->{status} eq 'running';
> +                    print "$node: reload task finished\n";
> +                    delete $tasks->{$upid};
> +                }
> +
> +                sleep(1);
> +            }
>          };
>  
>          return $rpcenv->fork_worker('reloadnetworkall', undef, $authuser, $code);
> -
>      },
>  });