OVS2.6 changes for DPDK

Configuration of DPDK parameters pmd-cpu-mask, dpdk-lcore-mask,
dpdk-socket-mem are addressed as per OVS2.6, while maintaining
backward compatibility with OVS 2.5

Implements: blueprint ovs-2-6-dpdk

Change-Id: I3227189691df85f265cf84bd4115d8d4c9f979f3
Signed-off-by: karthik s <ksundara@redhat.com>

Rakefile

@@ -1 +1,2 @@
 require 'puppet-openstack_spec_helper/rake_tasks'
+PuppetLint.configuration.send('disable_quoted_booleans')

lib/puppet/provider/vs_config/ovs.rb

@@ -4,8 +4,6 @@ Puppet::Type.type(:vs_config).provide(:ovs) do
 
   commands :vsctl => 'ovs-vsctl'
 
-  mk_resource_methods
-
   def self.munge_array_value(value)
     "[#{value[1..-2].split(',').map(&:strip).sort.join(",")}]"
   end
@@ -81,7 +79,12 @@ Puppet::Type.type(:vs_config).provide(:ovs) do
   end
 
   def exists?
-    @property_hash[:ensure] == :present
+    # if skip_if_version matches ovs_version(), then skip the configuration by faking exists
+    if @resource[:skip_if_version].eql? ovs_version()
+      return true
+    else
+      @property_hash[:ensure] == :present
+    end
   end
 
   def destroy
@@ -94,14 +97,39 @@ Puppet::Type.type(:vs_config).provide(:ovs) do
   end
 
   def _set
-    vsctl("set", "Open_vSwitch", ".", "#{@resource[:name]}=#{@resource[:value]}")
+    if @resource[:wait] == :false
+      vsctl("--no-wait", "set", "Open_vSwitch", ".", "#{@resource[:name]}=#{@resource[:value]}")
+    else
+      vsctl("set", "Open_vSwitch", ".", "#{@resource[:name]}=#{@resource[:value]}")
+    end
   end
 
   def create
-    _set
+    if @resource[:value].nil? or @resource[:value].empty?
+      destroy
+    else
+      _set
+    end
+  end
+
+  def value
+    # if skip_if_version matches ovs_version(), then skip the configuration by returning the same value
+    if @resource[:skip_if_version].eql? ovs_version()
+      @resource[:value]
+    else
+      @property_hash[:value]
+    end
+  end
+
+  def ovs_version
+    vsctl("--version")[/.*ovs-vsctl\s+\(Open\s+vSwitch\)\s+(\d+\.\d+)/][/(\d+\.\d+)/].chomp()
   end
 
   def value=(value)
-    _set
+    if  @resource[:value].nil? or @resource[:value].empty?
+      destroy
+    else
+      _set
+    end
   end
 end

lib/puppet/type/vs_config.rb

@@ -15,6 +15,22 @@ Puppet::Type.newtype(:vs_config) do
     end
   end
 
+  newparam(:skip_if_version) do
+    desc 'Skip setting the value when ovs version matches'
+    validate do |value|
+      unless value.is_a?(String) and value =~ /^\d+.\d+$/
+        raise ArgumentError, "Invalid format for  #{value}. Requires a String with format \d+.\d+, not a #{value.class}"
+      end
+    end
+  end
+
+  newparam(:wait) do
+    desc 'Should it wait for ovs-vswitchd to reconfigure itself before it exits'
+
+    newvalues(:true, :false)
+    defaultto :true
+  end
+
   newproperty(:value) do
     desc 'Configuration value for the paramter'
 

manifests/dpdk.pp

@@ -4,15 +4,17 @@
 # === Parameters
 #
 # [*memory_channels*]
-#   (required) The number of memory channels to use as an integer
+#   (optional) The number of memory channels to use as an integer.
 #
 # [*driver_type*]
 #   (Optional) The DPDK Driver type
 #   Defaults to 'vfio-pci'
+#   This parameter is required only for OVS versions <= 2.5.
 #
 # [*host_core_list*]
-#   (optional) The list of host cores to be used by the DPDK Poll Mode Driver
+#   (optional) The list of cores to be used by the lcore threads.
-#   The host_core_list is a string with format as <c1>[-c2][,c3[-c4],...] where c1, c2, etc are core indexes between 0 and 128
+#   The host_core_list is a string with format as <c1>[-c2][,c3[-c4],...]
+#   where c1, c2, etc are core indexes between 0 and 128.
 #   For example, to configure 3 cores the value should be "0-2"
 #
 # [*package_ensure*]
@@ -20,18 +22,20 @@
 #   Defaults to 'present'.
 #
 # [*pmd_core_list*]
-#   (optional) The list of cores to be used by the OVS Agent
+#   (optional) The list of cores to be used by the DPDK PMD threads.
-#   The pmd_core_list is a string with format as <c1>[-c2][,c3[-c4],...] where c1, c2, etc are core indexes between 0 and 128
+#   The pmd_core_list is a string with format as <c1>[-c2][,c3[-c4],...] where
+#   c1, c2, etc are core indexes between 0 and 128
 #   For example, to configure 3 cores the value should be "0-2"
 #
 # [*socket_mem*]
 #   (Optional) Set the memory to be allocated on each socket
-#   The socket_mem is a string with comma separated memory list in MB in the order of socket numbers.
+#   The socket_mem is a string with comma separated memory list in MB in the
-#   For example, to allocate memory of 1GB for socket 1 and no allocation for socket 0, the value should be "0,1024"
+#   order of socket numbers. For example, to allocate memory of 1GB for
+#   socket 1 and no allocation for socket 0, the value should be "0,1024"
 #   Defaults to undef.
 #
 class vswitch::dpdk (
-  $memory_channels,
+  $memory_channels    = undef,
   $driver_type        = 'vfio-pci',
   $host_core_list     = undef,
   $package_ensure     = 'present',
@@ -40,58 +44,90 @@ class vswitch::dpdk (
 ) {
 
   include ::vswitch::params
-
   kmod::load { 'vfio-pci': }
 
+  if $::osfamily != 'Redhat' {
+    fail( "${::osfamily} not yet supported for dpdk installation by puppet-vswitch")
+  }
+
   package { $::vswitch::params::ovs_dpdk_package_name:
     ensure => $package_ensure,
     before => Service['openvswitch'],
     tag    => 'openvswitch',
   }
 
-  # Set DPDK_OPTIONS to openvswitch
+  # DEPRECATED support for OVS 2.5
+  # DPDK_OPTIONS is no longer used in ovs 2.6, since it was a distribution
+  # specific hack to the ovs-ctl scripts. Instead dpdk information is
+  # pulled from the ovsdb.
   if $socket_mem {
     $socket_string = "--socket-mem ${socket_mem}"
   }
-
+  else {
+    $socket_string = undef
+  }
   if $driver_type {
     $pci_list = inline_template('<%= Facter.value("pci_address_driver_#@driver_type") %>')
-    unless empty($pci_list) {
+    if empty($pci_list) {
+      $white_list = undef
+    }
+    else {
       $white_list = inline_template('-w <%= @pci_list.gsub(",", " -w ") %>')
     }
   }
-  if !$host_core_list {
-    fail('host_core_list must be set for ovs agent when DPDK is enabled')
-  }
   $options = "DPDK_OPTIONS = \"-l ${host_core_list} -n ${memory_channels} ${socket_string} ${white_list}\""
-  if $pmd_core_list {
+  file_line { '/etc/sysconfig/openvswitch':
+    path    => '/etc/sysconfig/openvswitch',
+    match   => '^DPDK_OPTIONS.*',
+    line    => $options,
+    require => Package[$::vswitch::params::ovs_dpdk_package_name],
+    before  => Service['openvswitch'],
+  }
+
+  if $pmd_core_list and !empty($pmd_core_list){
     $pmd_core_list_updated = inline_template('<%= @pmd_core_list.split(",").map{|c| c.include?("-")?(c.split("-").map(&:to_i)[0]..c.split("-").map(&:to_i)[1]).to_a.join(","):c}.join(",") %>')
     $pmd_core_mask = inline_template('<%= @pmd_core_list_updated.split(",").map{|c| 1<<c.to_i}.inject(0,:|).to_s(16)  %>')
-    vs_config { 'other_config:pmd-cpu-mask':
-      value   => $pmd_core_mask,
-      require => Service['openvswitch'],
-    }
   }
-  case $::osfamily {
+  else {
-    'Redhat': {
+    $pmd_core_mask = undef
-      file_line { '/etc/sysconfig/openvswitch':
+  }
-        notify  => Service['openvswitch'],
-        path    => '/etc/sysconfig/openvswitch',
-        match   => '^DPDK_OPTIONS.*',
-        line    => $options,
-        require => Package[$::vswitch::params::ovs_dpdk_package_name],
-        before  => Service['openvswitch'],
-      }
 
-      service { 'openvswitch':
+
-        ensure => true,
+  if $host_core_list and !empty($host_core_list) {
-        enable => true,
+    $host_core_list_updated = inline_template('<%= @host_core_list.split(",").map{|c| c.include?("-")?(c.split("-").map(&:to_i)[0]..c.split("-").map(&:to_i)[1]).to_a.join(","):c}.join(",") %>')
-        name   => $::vswitch::params::ovs_service_name,
+    $dpdk_lcore_mask = inline_template('<%= @host_core_list_updated.split(",").map{|c| 1<<c.to_i}.inject(0,:|).to_s(16)  %>')
-      }
-    }
-    default: {
-      fail( "${::osfamily} not yet supported for dpdk installation by puppet-vswitch")
-    }
   }
+  else {
+    $dpdk_lcore_mask = undef
+  }
+
+  if $memory_channels and !empty($memory_channels) {
+    $memory_channels_conf = "-n ${memory_channels}"
+  }
+  else {
+    $memory_channels_conf = undef
+  }
+
+  $dpdk_configs = {
+    'other_config:dpdk-extra'      => { value => $memory_channels_conf, skip_if_version => '2.5'},
+    'other_config:dpdk-init'       => { value => 'true', skip_if_version => '2.5'},
+    'other_config:dpdk-socket-mem' => { value => $socket_mem, skip_if_version => '2.5'},
+    'other_config:dpdk-lcore-mask' => { value => $dpdk_lcore_mask, skip_if_version => '2.5'},
+    'other_config:pmd-cpu-mask'    => { value => $pmd_core_mask},
+  }
+
+  $dpdk_dependencies = {
+    wait    => false,
+    require => Service['openvswitch'],
+  }
+
+  service { 'openvswitch':
+    ensure => true,
+    enable => true,
+    name   => $::vswitch::params::ovs_service_name,
+  }
+
+  create_resources ('vs_config', $dpdk_configs, $dpdk_dependencies)
 
 }
+

manifests/params.pp

@@ -17,6 +17,8 @@ class vswitch::params {
       $ovs_dpdk_package_name = 'openvswitch'
       $ovs_dkms_package_name = undef
       $ovs_service_name      = 'openvswitch'
+      $ovs_service_hasstatus = undef
+      $ovs_status            = undef
       $provider              = 'ovs_redhat'
     }
     'Debian': {

releasenotes/notes/ovs2.6_support-cb3b2b5e75fde8ab.yaml

@@ -0,0 +1,9 @@
+upgrade:
+  - Support for OVS-DPDK is being extended for OvS 2.6 while retaining the
+    backward compatibility with OvS 2.5. In OvS 2.5 the DPDK parameters are
+    written as DPDK_OPTIONS to a sysconfig file, while in OvS 2.6 the DPDK
+    parameters are configured as ovsdb entries
+  - Parameter 'memory_channels' is required for OvS 2.5, while in OvS 2.6 its
+    optional
+  - parameter 'driver_type' is required only for OvS 2.5. Shall be deprecated
+    when OvS 2.5 support is discontinued

spec/classes/vswitch_dpdk_spec.rb

@@ -4,7 +4,6 @@ describe 'vswitch::dpdk' do
 
   let :default_params do {
     :package_ensure   => 'present',
-    :memory_channels  => '2',
   }
   end
 
@@ -21,15 +20,7 @@ describe 'vswitch::dpdk' do
 
   shared_examples_for 'vswitch::dpdk on RedHat' do
     let(:params) { default_params }
-    context 'should raise error when not passing host_core_list' do
+    context 'shall  write DPDK_OPTIONS as well as ovsdb params' do
-      it_raises 'a Puppet::Error', /host_core_list must be set for ovs agent when DPDK is enabled/
-    end
-
-    context 'basic parameters' do
-      before :each do
-        params.merge!(:host_core_list => '1,2')
-      end
-
       it 'include the class' do
         is_expected.to contain_class('vswitch::dpdk')
       end
@@ -43,8 +34,6 @@ describe 'vswitch::dpdk' do
           :ensure     => true,
           :enable     => true,
           :name       => platform_params[:ovs_service_name],
-          :hasstatus  => platform_params[:service_hasstatus],
-          :status     => platform_params[:service_status],
         )
       end
 
@@ -59,36 +48,106 @@ describe 'vswitch::dpdk' do
       it 'should have dpdk driver modules file' do
         is_expected.to contain_kmod__load('vfio-pci')
       end
+      it 'configures dpdk options with socket memory' do
+        is_expected.to contain_file_line('/etc/sysconfig/openvswitch')
 
-      it 'configures dpdk options for ovs' do
+        is_expected.to contain_vs_config('other_config:dpdk-init').with(
-        is_expected.to contain_file_line('/etc/sysconfig/openvswitch').with(
+          :value  => 'true', :wait => false, :skip_if_version => "2.5",
-          :path   => '/etc/sysconfig/openvswitch',
+        )
-          :match  => '^DPDK_OPTIONS.*',
+        is_expected.to contain_vs_config('other_config:pmd-cpu-mask').with(
-          :line   => 'DPDK_OPTIONS = "-l 1,2 -n 2  "',
+          :value  => nil, :wait => false,
-          :before => 'Service[openvswitch]',
+        )
+        is_expected.to contain_vs_config('other_config:dpdk-socket-mem').with(
+          :value => nil, :wait => false, :skip_if_version => "2.5",
+        )
+        is_expected.to contain_vs_config('other_config:dpdk-lcore-mask').with(
+          :value => nil, :wait => false, :skip_if_version => "2.5",
+        )
+        is_expected.to contain_vs_config('other_config:dpdk-extra').with(
+          :value => nil, :wait => false, :skip_if_version => "2.5",
         )
       end
     end
-    context 'when passing socket mem' do
+
+    context 'when passing all empty params' do
       before :each do
-        params.merge!(:host_core_list => '1,2')
+        params.merge!(:host_core_list  => '')
-        params.merge!(:socket_mem => '1024')
+        params.merge!(:socket_mem      => '')
+        params.merge!(:memory_channels => '' )
+        params.merge!(:pmd_core_list => '')
       end
-      it 'configures dpdk options with socket memory' do
+      it 'configures dpdk options' do
+        is_expected.to contain_file_line('/etc/sysconfig/openvswitch').with(
+          :path   => '/etc/sysconfig/openvswitch',
+          :match  => '^DPDK_OPTIONS.*',
+          :before => 'Service[openvswitch]',
+        )
+        is_expected.to contain_vs_config('other_config:dpdk-init').with(
+          :value  => 'true', :wait => false, :skip_if_version => "2.5",
+        )
+        is_expected.to contain_vs_config('other_config:pmd-cpu-mask').with(
+          :value  => nil, :wait => false,
+        )
+        is_expected.to contain_vs_config('other_config:dpdk-socket-mem').with(
+          :value => '', :wait => false, :skip_if_version => "2.5",
+        )
+        is_expected.to contain_vs_config('other_config:dpdk-lcore-mask').with(
+          :value => nil, :wait => false, :skip_if_version => "2.5",
+        )
+        is_expected.to contain_vs_config('other_config:dpdk-extra').with(
+          :value => nil, :wait => false, :skip_if_version => "2.5",
+        )
+
+      end
+    end
+
+
+    context 'when passing all params' do
+      before :each do
+        params.merge!(:host_core_list  => '1,2')
+        params.merge!(:socket_mem      => '1024')
+        params.merge!(:memory_channels => 2)
+        params.merge!(:pmd_core_list => '22,23,24,25,66,67,68,69')
+      end
+      it 'configures dpdk options' do
         is_expected.to contain_file_line('/etc/sysconfig/openvswitch').with(
           :path   => '/etc/sysconfig/openvswitch',
           :match  => '^DPDK_OPTIONS.*',
           :line   => 'DPDK_OPTIONS = "-l 1,2 -n 2 --socket-mem 1024 "',
           :before => 'Service[openvswitch]',
         )
+        is_expected.to contain_vs_config('other_config:dpdk-init').with(
+          :value  => 'true', :wait => false, :skip_if_version => "2.5",
+        )
+        is_expected.to contain_vs_config('other_config:pmd-cpu-mask').with(
+          :value  => '3c0000000003c00000', :wait => false,
+        )
+        is_expected.to contain_vs_config('other_config:dpdk-socket-mem').with(
+          :value => '1024', :wait => false, :skip_if_version => "2.5",
+        )
+        is_expected.to contain_vs_config('other_config:dpdk-lcore-mask').with(
+          :value => '6', :wait => false, :skip_if_version => "2.5",
+        )
+        is_expected.to contain_vs_config('other_config:dpdk-extra').with(
+          :value => '-n 2', :wait => false, :skip_if_version => "2.5",
+        )
       end
     end
 
     context 'when providing valid driver type facts' do
+      let :facts do
+        OSDefaults.get_facts({
+          :osfamily                => 'Redhat',
+          :operatingsystem         => 'RedHat',
+          :ovs_version             => '2.5.1',
+          :pci_address_driver_test => '0000:00:05.0,0000:00:05.1'
+        })
+      end
+
       before :each do
-        params.merge!(:host_core_list => '1,2')
+        params.merge!(:host_core_list  => '1,2')
-        params.merge!(:driver_type => 'test')
+        params.merge!(:driver_type     => 'test')
-        facts.merge!({ :pci_address_driver_test => '0000:00:05.0,0000:00:05.1' })
+        params.merge!(:memory_channels => 2)
       end
       it 'configures dpdk options with pci address for driver test' do
         is_expected.to contain_file_line('/etc/sysconfig/openvswitch').with(
@@ -97,37 +156,19 @@ describe 'vswitch::dpdk' do
           :line   => 'DPDK_OPTIONS = "-l 1,2 -n 2  -w 0000:00:05.0 -w 0000:00:05.1"',
           :before => 'Service[openvswitch]',
         )
-      end
+        is_expected.to contain_vs_config('other_config:dpdk-init').with(
-    end
+          :value  => 'true', :wait => false, :skip_if_version => "2.5",
+        )
+        is_expected.to contain_vs_config('other_config:pmd-cpu-mask').with(
+          :value  => nil, :wait => false,
+        )
+        is_expected.to contain_vs_config('other_config:dpdk-socket-mem').with(
+          :value => nil, :wait => false, :skip_if_version => "2.5",
+        )
+        is_expected.to contain_vs_config('other_config:dpdk-lcore-mask').with(
+          :value => '6', :wait => false, :skip_if_version => "2.5",
+        )
 
-    context 'when passing pmd core list with comma seperator for ovs' do
-      before :each do
-        params.merge!(:host_core_list => '1,2')
-        params.merge!(:pmd_core_list => '22,23,24,25,66,67,68,69')
-      end
-      it 'configures pmd core mask for ovs' do
-        is_expected.to contain_vs_config('other_config:pmd-cpu-mask').with(
-          :value  => '3c0000000003c00000',
-        )
-      end
-    end
-    context 'when passing pmd core list with range for ovs' do
-      before :each do
-        params.merge!(:host_core_list => '1,2')
-        params.merge!(:pmd_core_list => '22-25,66,67,68,69')
-      end
-      it 'configures pmd core mask for ovs' do
-        is_expected.to contain_vs_config('other_config:pmd-cpu-mask').with(
-          :value  => '3c0000000003c00000',
-        )
-      end
-    end
-    context 'when not passing pmd core list for ovs' do
-      before :each do
-        params.merge!(:host_core_list => '1,2')
-      end
-      it 'should not configure pmd core mask for ovs' do
-        is_expected.to_not contain_vs_config('other_config:pmd-cpu-mask')
       end
     end
   end
@@ -137,9 +178,8 @@ describe 'vswitch::dpdk' do
   }).each do |os,facts|
     context "on #{os}" do
       let (:facts) do
-        facts.merge!(OSDefaults.get_facts({ :ovs_version => '1.4.2' }))
+        facts.merge!(OSDefaults.get_facts({ :ovs_version => '2.6.1' }))
       end
-
       let (:platform_params) do
         case facts[:osfamily]
         when 'Debian'
@@ -151,10 +191,10 @@ describe 'vswitch::dpdk' do
             :ovs_dpdk_package_name => 'openvswitch',
             :ovs_service_name      => 'openvswitch',
             :provider              => 'ovs_redhat',
+            :ovsdb_service_name    => 'ovsdb-server',
           }
         end
       end
-
       it_behaves_like "vswitch::dpdk on #{facts[:osfamily]}"
     end
   end

spec/unit/puppet/lib/type/vs_config_spec.rb

@@ -13,6 +13,36 @@ describe Puppet::Type.type(:vs_config) do
     end.to_not raise_error
   end
 
+  it "wait property should accept boolean values" do
+    expect do
+      described_class.new({:name => "foo", :value => "[2, 1, 3, 0]", :ensure => :present, :wait => true})
+    end.to_not raise_error
+  end
+
+  it "wait property should throw error for non boolean values" do
+    expect do
+      described_class.new({:name => "foo", :value => "123", :ensure => :present, :wait => "abc"})
+    end.to raise_error(Puppet::Error)
+  end
+
+  it "skip_if_version param should accept string values of format \d+.\d+" do
+    expect do
+      described_class.new({:name => "foo", :value => "[2, 1, 3, 0]", :ensure => :present, :skip_if_version => '2.5'})
+    end.to_not raise_error
+    expect do
+      described_class.new({:name => "foo", :value => "[2, 1, 3, 0]", :ensure => :present, :skip_if_version => 'a2.5'})
+    end.to raise_error(Puppet::Error)
+    expect do
+      described_class.new({:name => "foo", :value => "[2, 1, 3, 0]", :ensure => :present, :skip_if_version => '2.5a'})
+    end.to raise_error(Puppet::Error)
+  end
+
+  it "skip_if_version param should not accept non string values" do
+    expect do
+      described_class.new({:name => "foo", :value => "[2, 1, 3, 0]", :ensure => :present, :skip_if_version => 2.5})
+    end.to raise_error(Puppet::Error)
+  end
+
   it "should have a :value parameter" do
     expect(described_class.attrtype(:value)).to eq(:property)
   end