[pve-devel] [PATCH firewall 1/7] configs: add locking helpers

[Thomas Lamprecht]

On 4/29/20 10:52 AM, Fabian Grünbichler wrote:
> to allow some level of safe concurrent config modification, instead of
> the current free for all.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> ---
> 
> Notes:
>     require pve-cluster that provides cfs_lock_firewall, or switching to
>     cfs_lock_domain as mentioned in pve-cluster#1
>     
>     lock_hostfw_conf could also use a node-local lock, but IMHO it's easier to have
>     the same locking semantics/interface across all three levels (especially if we
>     do the second patch in pve-cluster).
>     
>     it's easy enough to switch out though.

Hmm, are you aware that cfs_locks only protect from other nodes, and not
intra-node? I.e., if one process of the node has the lock any other can
aquire it too, just no other node can.

That's the reason for my a bit special corosync config change lock:
https://git.proxmox.com/?p=pve-cluster.git;a=blob;f=data/PVE/API2/ClusterConfig.pm;h=7a4bce07169c15cfe982ea34f2c238ad6ee4abf7;hb=HEAD#l188

> 
>  src/PVE/Firewall.pm | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index d22b15a..eda39eb 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -3053,6 +3053,8 @@ sub generic_fw_config_parser {
>      return $res;
>  }
>  
> +# this is only used to prevent concurrent runs of rule compilation/application
> +# see lock_*_conf for cfs locks protectiong config modification
>  sub run_locked {
>      my ($code, @param) = @_;
>  
> @@ -3101,6 +3103,18 @@ sub read_local_vm_config {
>      return $vmdata;
>  };
>  
> +sub lock_vmfw_conf {
> +    my ($vmid, $timeout, $code, @param) = @_;
> +
> +    die "can't lock VM firewall config for undefined VMID\n"
> +	if !defined($vmid);
> +
> +    my $res = PVE::Cluster::cfs_lock_firewall("vm-$vmid", $timeout, $code, @param);
> +    die $@ if $@;
> +
> +    return $res;
> +}
> +
>  sub load_vmfw_conf {
>      my ($cluster_conf, $rule_env, $vmid, $dir) = @_;
>  
> @@ -3448,6 +3462,15 @@ my $set_global_log_ratelimit = sub {
>      }
>  };
>  
> +sub lock_clusterfw_conf {
> +    my ($timeout, $code, @param) = @_;
> +
> +    my $res = PVE::Cluster::cfs_lock_firewall("cluster", $timeout, $code, @param);
> +    die $@ if $@;
> +
> +    return $res;
> +}
> +
>  sub load_clusterfw_conf {
>      my ($filename) = @_;
>  
> @@ -3511,6 +3534,15 @@ sub save_clusterfw_conf {
>      }
>  }
>  
> +sub lock_hostfw_conf {
> +    my ($timeout, $code, @param) = @_;
> +
> +    my $res = PVE::Cluster::cfs_lock_firewall("host-$nodename", $timeout, $code, @param);
> +    die $@ if $@;
> +
> +    return $res;
> +}
> +
>  sub load_hostfw_conf {
>      my ($cluster_conf, $filename) = @_;
>  
>