From 92f0f20af2865de8e02e1e8b8de05f34d35bf7ba Mon Sep 17 00:00:00 2001 From: Rune Juhl Jacobsen Date: Fri, 24 May 2019 11:44:09 +0200 Subject: [PATCH 1/2] Fix unnecessarily setting gluster volume option repeatedly Checks Gluster volume options against existing values to avoid repeatedly setting values Also adds these types: + `Gluster::VolumeName` + `Gluster::VolumeOption` And these functions: + `gluster::cmd_volume_get_option` + `gluster::onoff` --- functions/cmd_volume_get_option.pp | 34 ++++++++++++++++++++++++++++++ functions/onoff.pp | 9 ++++++++ manifests/volume/option.pp | 33 ++++++++++++++--------------- types/volumename.pp | 1 + types/volumeoption.pp | 1 + 5 files changed, 61 insertions(+), 17 deletions(-) create mode 100644 functions/cmd_volume_get_option.pp create mode 100644 functions/onoff.pp create mode 100644 types/volumename.pp create mode 100644 types/volumeoption.pp diff --git a/functions/cmd_volume_get_option.pp b/functions/cmd_volume_get_option.pp new file mode 100644 index 00000000..f87a3ded --- /dev/null +++ b/functions/cmd_volume_get_option.pp @@ -0,0 +1,34 @@ +# Create a command string to get option `$opt` from gluster volume `$vol`, and +# optionally compare it against `$comparison`. +# +# @param vol [Gluster::VolumeName] Gluster volume name +# @param opt [Gluster::OptionName] Gluster volume option name +# @param comparison [Optional[String]] Optional string to compare the existing +# value against +# @return [String] +# +# @example Usage +# +# ```puppet +# gluster::cmd_volume_get_option('data', 'nfs.disable', String(true)) +# ``` +# +function gluster::cmd_volume_get_option( + Gluster::VolumeName $vol, + Gluster::VolumeOption $opt, + Optional[Any] $comparison = undef, +) { + $_cmd = "${::gluster_binary} volume get ${vol} ${opt}" + + unless $comparison { + return $_cmd + } + + $_comparison = $comparison ? { + Undef => '\(null\)', + Boolean => gluster::onoff($comparison), + default => $comparison, + } + + "${_cmd} | tail -n1 | grep -E '^${opt} +${_comparison} *\$'" +} diff --git a/functions/onoff.pp b/functions/onoff.pp new file mode 100644 index 00000000..4a5e61fd --- /dev/null +++ b/functions/onoff.pp @@ -0,0 +1,9 @@ +function gluster::onoff ( + Boolean $value, +) { + if $value { + 'on' + } else { + 'off' + } +} diff --git a/manifests/volume/option.pp b/manifests/volume/option.pp index 86328de2..f6098efb 100644 --- a/manifests/volume/option.pp +++ b/manifests/volume/option.pp @@ -3,7 +3,8 @@ # @param title # the name of the volume, a colon, and the name of the option # @param value -# the value to set for this option +# the value to set for this option. Boolean values will be coerced to +# 'on'/'off'. # @param ensure # whether to set or remove an option # @@ -30,31 +31,29 @@ Enum['present', 'absent'] $ensure = 'present', ) { - $arr = split( $title, ':' ) - $count = count($arr) + $arr = $title.split(':') # do we have more than one array element? - if $count != 2 { + if count($arr) != 2 { fail("${title} does not parse as volume:option") } - $vol = $arr[0] - $opt = $arr[1] + [$vol, $opt] = $arr + + $_value = $value ? { + Boolean => gluster::onoff($value), + default => String($value), + } $cmd = if $ensure == 'absent' { "reset ${vol} ${opt}" } else { - "set ${vol} ${opt} ${value}" - } - - $_value = $value ? { - Boolean => if $value { - 'on' - } else { - 'off' - }, - default => $value, + "set ${vol} ${opt} ${_value}" } exec { "gluster option ${vol} ${opt} ${_value}": - command => "${facts['gluster_binary']} volume ${cmd}", + path => '/usr/bin:/usr/sbin:/bin', + command => "${::gluster_binary} volume ${cmd}", + unless => unless $ensure == 'absent' { + gluster::cmd_volume_get_option($vol, $opt, $_value) + }, } } diff --git a/types/volumename.pp b/types/volumename.pp new file mode 100644 index 00000000..a2fb03fd --- /dev/null +++ b/types/volumename.pp @@ -0,0 +1 @@ +type Gluster::VolumeName = Pattern[/^[a-zA-Z0-9_-]+$/] diff --git a/types/volumeoption.pp b/types/volumeoption.pp new file mode 100644 index 00000000..d4034ecf --- /dev/null +++ b/types/volumeoption.pp @@ -0,0 +1 @@ +type Gluster::VolumeOption = Pattern[/^[a-z0-9]+\.[a-z0-9-]+$/] From e0fea8eee3290bd289822e6780508f00ce57035d Mon Sep 17 00:00:00 2001 From: Rune Juhl Jacobsen Date: Wed, 29 May 2019 22:28:36 +0200 Subject: [PATCH 2/2] Make code more idiomatic, replace use of `getvar` --- functions/cmd_volume_get_option.pp | 2 +- manifests/peer.pp | 21 +++++++------- manifests/volume.pp | 45 +++++++++++++++--------------- manifests/volume/option.pp | 2 +- 4 files changed, 35 insertions(+), 35 deletions(-) diff --git a/functions/cmd_volume_get_option.pp b/functions/cmd_volume_get_option.pp index f87a3ded..b61778ab 100644 --- a/functions/cmd_volume_get_option.pp +++ b/functions/cmd_volume_get_option.pp @@ -18,7 +18,7 @@ function gluster::cmd_volume_get_option( Gluster::VolumeOption $opt, Optional[Any] $comparison = undef, ) { - $_cmd = "${::gluster_binary} volume get ${vol} ${opt}" + $_cmd = "${facts['gluster_binary']} volume get ${vol} ${opt}" unless $comparison { return $_cmd diff --git a/manifests/peer.pp b/manifests/peer.pp index 14474045..d00a73cf 100644 --- a/manifests/peer.pp +++ b/manifests/peer.pp @@ -39,19 +39,20 @@ $fqdn = $::fqdn, ) { - # we can't do much without the Gluster binary - # but we don't necessarily want the Puppet run to fail if the - # gluster_binary fact is absent! - if getvar('::gluster_binary') { + # we can't do much without the Gluster binary but we don't necessarily want + # the Puppet run to fail if the gluster_binary fact is absent! + $_gluster_binary = $facts.dig('gluster_binary') + if $_gluster_binary { # we can't join to ourselves, so it only makes sense to operate # on other gluster servers in the same pool if $fqdn != $::fqdn { - # and we don't want to attach a server that is already a member - # of the current pool - if getvar('::gluster_peer_list') { - $peers = split($::gluster_peer_list, ',' ) - if ! member($peers, $title) { + # and we don't want to attach a server that is already a member of the + # current pool + $_gluster_peer_list = $facts.dig('gluster_peer_list') + if $_gluster_peer_list { + $peers = split($_gluster_peer_list, ',' ) + if $title in $peers { $already_in_pool = false } else { $already_in_pool = true @@ -61,7 +62,7 @@ } if !$already_in_pool { exec { "gluster peer probe ${title}": - command => "${::gluster_binary} peer probe ${title}", + command => "${_gluster_binary} peer probe ${title}", } } } diff --git a/manifests/volume.pp b/manifests/volume.pp index 4e3d85da..2d3f06cd 100644 --- a/manifests/volume.pp +++ b/manifests/volume.pp @@ -99,20 +99,18 @@ $args = join(delete($cmd_args, ''), ' ') - if getvar('::gluster_binary'){ + $_gluster_binary = $facts.dig('gluster_binary') + if $_gluster_binary { # we need the Gluster binary to do anything! - if getvar('::gluster_volume_list') and member( split( $::gluster_volume_list, ',' ), $title ) { - $already_exists = true - } else { - $already_exists = false - } + $_gluster_volume_list = $facts.dig('gluster_volume_list') + $already_exists = $_gluster_volume_list and $title in $_gluster_volume_list.split(',') - if $already_exists == false { + unless $already_exists { # this volume has not yet been created exec { "gluster create volume ${title}": - command => "${::gluster_binary} volume create ${title} ${args}", + command => "${_gluster_binary} volume create ${title} ${args}", } # if we have volume options, activate them now @@ -144,20 +142,22 @@ before => Exec["gluster start volume ${title}"], } - create_resources(::gluster::volume::option, $hoh, $new_volume_defaults) + create_resources('::gluster::volume::option', $hoh, $new_volume_defaults) } # don't forget to start the new volume! exec { "gluster start volume ${title}": - command => "${::gluster_binary} volume start ${title}", + command => "${_gluster_binary} volume start ${title}", require => Exec["gluster create volume ${title}"], } - } elsif $already_exists { + } else { # this volume exists # our fact lists bricks comma-separated, but we need an array - $vol_bricks = split( getvar( "::gluster_volume_${title}_bricks" ), ',') + $vol_bricks = $facts.dig("gluster_volume_${title}_bricks").then |$bs| { + $bs.split(',') + } if $bricks != $vol_bricks { # this resource's list of bricks does not match the existing # volume's list of bricks @@ -193,12 +193,12 @@ $new_bricks_list = join($new_bricks, ' ') exec { "gluster add bricks to ${title}": - command => "${::gluster_binary} volume add-brick ${title} ${s} ${r} ${new_bricks_list} ${_force}", + command => "${_gluster_binary} volume add-brick ${title} ${s} ${r} ${new_bricks_list} ${_force}", } if $rebalance { exec { "gluster rebalance ${title}": - command => "${::gluster_binary} volume rebalance ${title} start", + command => "${_gluster_binary} volume rebalance ${title} start", require => Exec["gluster add bricks to ${title}"], } } @@ -208,7 +208,7 @@ # the self heal daemon comes back to life. # as such, we sleep 5 here before starting the heal exec { "gluster heal ${title}": - command => "/bin/sleep 5; ${::gluster_binary} volume heal ${title} full", + command => "/bin/sleep 5; ${_gluster_binary} volume heal ${title} full", require => Exec["gluster add bricks to ${title}"], } } @@ -222,15 +222,14 @@ } # did the options change? - $current_options_hash = pick(fact("gluster_volumes.${title}.options"), {}) - $_current = sort(join_keys_to_values($current_options_hash, ': ')) - + $current_options = $facts.dig("gluster_volume_${title}.options").pick({}) + $_current = sort(join_keys_to_values($current_options, ': ')) if $_current != $_options { # # either of $current_options or $_options may be empty. # we need to account for this situation # - if is_array($_current) and is_array($_options) { + if $_current =~ Array and $_options =~ Array { $to_remove = difference($_current, $_options) $to_add = difference($_options, $_current) } else { @@ -242,7 +241,7 @@ $to_add = $_options } } - if ! empty($to_remove) { + unless $to_remove.empty { # we have some options active that are not defined here. Remove them # # the syntax to remove ::gluster::volume::options is a little different @@ -252,18 +251,18 @@ $remove_yaml = join( regsubst( $remove_opts, ': .+$', ":\n ensure: absent", 'G' ), "\n" ) $remove = parseyaml($remove_yaml) if $remove_options { - create_resources( ::gluster::volume::option, $remove ) + create_resources('::gluster::volume::option', $remove ) } else { $remove_str = join( keys($remove), ', ' ) notice("NOT REMOVING the following options for volume ${title}: ${remove_str}.") } } - if ! empty($to_add) { + unless $to_add.empty { # we have some options defined that are not active. Add them $add_opts = prefix( $to_add, "${title}:" ) $add_yaml = join( regsubst( $add_opts, ': ', ":\n value: ", 'G' ), "\n" ) $add = parseyaml($add_yaml) - create_resources( ::gluster::volume::option, $add ) + create_resources('::gluster::volume::option', $add ) } } } diff --git a/manifests/volume/option.pp b/manifests/volume/option.pp index f6098efb..dfbf3038 100644 --- a/manifests/volume/option.pp +++ b/manifests/volume/option.pp @@ -51,7 +51,7 @@ exec { "gluster option ${vol} ${opt} ${_value}": path => '/usr/bin:/usr/sbin:/bin', - command => "${::gluster_binary} volume ${cmd}", + command => "${facts['gluster_binary']} volume ${cmd}", unless => unless $ensure == 'absent' { gluster::cmd_volume_get_option($vol, $opt, $_value) },