Description: Fix VNC console in Debian
 In Debian, SPICE is the default. Unfortunately, puppet-openstack fails to
 set [spice]/enabled = false when VNC is selected, and therefore, both VNC
 and SPICE ends up being enabled in nova.conf.
 .
 Also, Debian has a unique package nova-consoleproxy handling SPICE, VNC
 and the XenVNC console, with /etc/default/nova-consoleproxy being used to
 select what daemon to start. As puppet-openstack doesn't set it before
 starting the VNC console service, it stays with spicehtml5 as default
 value, and therefore, nova-novncproxy cannot start.
 .
 This patch fixes both issues.
Author: Thomas Goirand <zigo@debian.org>
Date: Wed, 25 Apr 2018 21:29:54 +0200
Change-Id: Ia40805f27e8833fa01576432ae792e1becedd729
Forwarded: https://review.openstack.org/#/c/564328/
Last-Update: 2018-06-16

diff --git a/manifests/compute.pp b/manifests/compute.pp
index a23372a..cb6771e 100644
--- a/manifests/compute.pp
+++ b/manifests/compute.pp
@@ -24,6 +24,11 @@
 #   (optional) Whether to use a VNC proxy
 #   Defaults to true
 #
+# [*spice_enabled*]
+#   (optional) Whether to use a SPICE html5 proxy
+#   Mutually exclusive with vnc_enabled.
+#   Defaults to false.
+#
 # [*vncserver_proxyclient_address*]
 #   (optional) The IP address of the server running the VNC proxy client
 #   Defaults to '127.0.0.1'
@@ -164,6 +169,7 @@ class nova::compute (
   $manage_service                              = true,
   $ensure_package                              = 'present',
   $vnc_enabled                                 = true,
+  $spice_enabled                               = false,
   $vncserver_proxyclient_address               = '127.0.0.1',
   $vncproxy_host                               = false,
   $vncproxy_protocol                           = 'http',
@@ -210,6 +216,10 @@ class nova::compute (
     $keymgr_backend_real = $keymgr_backend
   }
 
+  if ($vnc_enabled and $spice_enabled) {
+    fail('vnc_enabled and spice_enabled is mutually exclusive')
+  }
+
   # cryptsetup is required when Barbican is encrypting volumes
   if $keymgr_backend_real =~ /barbican/ {
     ensure_packages('cryptsetup', {
@@ -262,8 +272,10 @@ class nova::compute (
       'vnc/keymap':                        ensure => absent;
     }
   }
+
   nova_config {
-    'vnc/enabled': value => $vnc_enabled;
+    'vnc/enabled':   value => $vnc_enabled;
+    'spice/enabled': value => $spice_enabled;
   }
 
   if $neutron_enabled != true and $install_bridge_utils {
diff --git a/manifests/compute/spice.pp b/manifests/compute/spice.pp
index cdfc739..892465a 100644
--- a/manifests/compute/spice.pp
+++ b/manifests/compute/spice.pp
@@ -58,7 +58,6 @@ class nova::compute::spice(
   }
 
   nova_config {
-    'spice/enabled':                    value => true;
     'spice/agent_enabled':              value => $agent_enabled;
     'spice/server_listen':              value => $server_listen;
     'spice/server_proxyclient_address': value => $server_proxyclient_address;
diff --git a/manifests/spicehtml5proxy.pp b/manifests/spicehtml5proxy.pp
index 0156e5d..a44bf05 100644
--- a/manifests/spicehtml5proxy.pp
+++ b/manifests/spicehtml5proxy.pp
@@ -39,13 +39,35 @@ class nova::spicehtml5proxy(
   include ::nova::deps
   include ::nova::params
 
+  # Nodes running spicehtml5proxy do *not* need (and in fact, don't care)
+  # about [spice]/enable to be set. This setting is for compute nodes,
+  # where we must select VNC or SPICE so that it can be passed on to
+  # libvirt which passes it as parameter when starting VMs with KVM.
+  # Therefore, this setting is set within compute.pp only.
   nova_config {
-    'spice/enabled':         value => $enabled;
     'spice/agent_enabled':   value => $enabled;
     'spice/html5proxy_host': value => $host;
     'spice/html5proxy_port': value => $port;
   }
 
+  # The Debian package needs some scheduling:
+  # 1/ Install the packagin
+  # 2/ Fix /etc/default/nova-consoleproxy
+  # 3/ Start the service
+  # Other OS don't need this scheduling and can use
+  # the standard nova::generic_service
+  if $::os_package_type == 'debian' {
+    if $enabled {
+      file_line { '/etc/default/nova-consoleproxy:NOVA_CONSOLE_PROXY_TYPE':
+        path    => '/etc/default/nova-consoleproxy',
+        match   => '^NOVA_CONSOLE_PROXY_TYPE=(.*)$',
+        line    => 'NOVA_CONSOLE_PROXY_TYPE=spicehtml5',
+        tag     => 'nova-consoleproxy',
+        require => Anchor['nova::config::begin'],
+        notify  => Anchor['nova::config::end'],
+      }
+    }
+  }
   nova::generic_service { 'spicehtml5proxy':
     enabled        => $enabled,
     manage_service => $manage_service,
@@ -54,4 +76,3 @@ class nova::spicehtml5proxy(
     ensure_package => $ensure_package,
   }
 }
-
diff --git a/manifests/vncproxy.pp b/manifests/vncproxy.pp
index d16beb7..3c0b6d8 100644
--- a/manifests/vncproxy.pp
+++ b/manifests/vncproxy.pp
@@ -103,12 +103,35 @@ class nova::vncproxy(
     $auth_schemes = 'none'
   }
 
+  # Nodes running novncproxy do *not* need (and in fact, don't care)
+  # about [vnc]/enable to be set. This setting is for compute nodes,
+  # where we must select VNC or SPICE so that it can be passed on to
+  # libvirt which passes it as parameter when starting VMs with KVM.
+  # Therefore, this setting is set within compute.pp only.
   nova_config {
     'vnc/novncproxy_host': value => $host;
     'vnc/novncproxy_port': value => $port;
     'vnc/auth_schemes':    value => $auth_schemes;
   }
 
+  # The Debian package needs some scheduling:
+  # 1/ Install the packagin
+  # 2/ Fix /etc/default/nova-consoleproxy
+  # 3/ Start the service
+  # Other OS don't need this scheduling and can use
+  # the standard nova::generic_service
+  if $::os_package_type == 'debian' {
+    if $enabled {
+      file_line { '/etc/default/nova-consoleproxy:NOVA_CONSOLE_PROXY_TYPE':
+        path    => '/etc/default/nova-consoleproxy',
+        match   => '^NOVA_CONSOLE_PROXY_TYPE=(.*)$',
+        line    => 'NOVA_CONSOLE_PROXY_TYPE=novnc',
+        tag     => 'nova-consoleproxy',
+        require => Anchor['nova::config::begin'],
+        notify  => Anchor['nova::config::end'],
+      }
+    }
+  }
   nova::generic_service { 'vncproxy':
     enabled        => $enabled,
     manage_service => $manage_service,
diff --git a/releasenotes/notes/spice-enabled-78f1bf8f333928aa.yaml b/releasenotes/notes/spice-enabled-78f1bf8f333928aa.yaml
new file mode 100644
index 0000000..78f58d8
--- /dev/null
+++ b/releasenotes/notes/spice-enabled-78f1bf8f333928aa.yaml
@@ -0,0 +1,13 @@
+---
+features:
+  - |
+    Puppet-openstack now manages both [vnc]/enabled and [spice]/enabled. This
+    was needed because Debian has [spice]/enabled set to True by default, and
+    one cannot have both enabled in a compute node. Therefore, it was mandatory
+    to have [spice]/enabled set to Flase if we're using VNC.
+upgrade:
+  - |
+    Users of puppet-openstack will have to set ::nova::compute::spice_enabled to
+    True to make a meaningful decisioin to use Spice and not use VNC, and not
+    rely on ::nova::spicehtml5proxy or ::nova::compute::spice to set it as
+    enabled by default.
diff --git a/spec/classes/nova_compute_spec.rb b/spec/classes/nova_compute_spec.rb
index c0ea5c2..ffb6c4f 100644
--- a/spec/classes/nova_compute_spec.rb
+++ b/spec/classes/nova_compute_spec.rb
@@ -122,6 +122,10 @@ describe 'nova::compute' do
         )
       end
 
+      it 'should have spice disabled' do
+        is_expected.to contain_nova_config('spice/enabled').with_value(false)
+      end
+
       it { is_expected.to contain_nova_config('DEFAULT/heal_instance_info_cache_interval').with_value('120') }
 
       it { is_expected.to contain_nova_config('DEFAULT/force_raw_images').with(:value => false) }
@@ -221,9 +225,10 @@ describe 'nova::compute' do
 
     end
 
-    context 'with vnc_enabled set to false' do
+    context 'with vnc_enabled set to false and spice_enabled set to true' do
       let :params do
-        { :vnc_enabled => false }
+        { :vnc_enabled => false,
+          :spice_enabled => true, }
       end
 
       it 'disables vnc in nova.conf' do
@@ -232,6 +237,19 @@ describe 'nova::compute' do
         is_expected.to contain_nova_config('vnc/keymap').with_ensure('absent')
         is_expected.to_not contain_nova_config('vnc/novncproxy_base_url')
       end
+
+      it 'enables spice' do
+        is_expected.to contain_nova_config('spice/enabled').with_value(true)
+      end
+    end
+
+    context 'with vnc_enabled and spice_enabled set to true' do
+      let :params do
+        { :vnc_enabled   => true,
+          :spice_enabled => true, }
+      end
+
+      it { is_expected.to raise_error Puppet::Error, /vnc_enabled and spice_enabled is mutually exclusive/ }
     end
 
     context 'with force_config_drive parameter set to true' do
diff --git a/spec/classes/nova_compute_spice_spec.rb b/spec/classes/nova_compute_spice_spec.rb
index b010027..a0f64ff 100644
--- a/spec/classes/nova_compute_spice_spec.rb
+++ b/spec/classes/nova_compute_spice_spec.rb
@@ -1,7 +1,6 @@
 require 'spec_helper'
 describe 'nova::compute::spice' do
 
-  it { is_expected.to contain_nova_config('spice/enabled').with_value('true')}
   it { is_expected.to contain_nova_config('spice/agent_enabled').with_value('true')}
   it { is_expected.to contain_nova_config('spice/server_proxyclient_address').with_value('127.0.0.1')}
   it { is_expected.to_not contain_nova_config('spice/html5proxy_base_url')}
diff --git a/spec/classes/nova_spicehtml5proxy_spec.rb b/spec/classes/nova_spicehtml5proxy_spec.rb
index dd81201..5ad851f 100644
--- a/spec/classes/nova_spicehtml5proxy_spec.rb
+++ b/spec/classes/nova_spicehtml5proxy_spec.rb
@@ -6,22 +6,36 @@ describe 'nova::spicehtml5proxy' do
     'include nova'
   end
 
-  shared_examples 'nova-spicehtml5proxy' do
+  shared_examples 'nova-spicehtml5proxy debian package' do
+    let :params do
+      { :enabled => true }
+    end
+
+    it { is_expected.to contain_file_line('/etc/default/nova-consoleproxy:NOVA_CONSOLE_PROXY_TYPE').with(
+        :path    => '/etc/default/nova-consoleproxy',
+        :match   => '^NOVA_CONSOLE_PROXY_TYPE=(.*)$',
+        :line    => 'NOVA_CONSOLE_PROXY_TYPE=spicehtml5',
+        :tag     => 'nova-consoleproxy',
+        :require => 'Anchor[nova::config::begin]',
+        :notify  => 'Anchor[nova::config::end]',
+    )}
+  end
 
+  shared_examples 'nova-spicehtml5proxy' do
     it 'configures nova.conf' do
       is_expected.to contain_nova_config('spice/html5proxy_host').with(:value => '0.0.0.0')
       is_expected.to contain_nova_config('spice/html5proxy_port').with(:value => '6082')
     end
 
     it { is_expected.to contain_package('nova-spicehtml5proxy').with(
-      :name   => platform_params[:spicehtml5proxy_package_name],
-      :ensure => 'present'
+      :ensure => 'present',
+      :name   => platform_params[:spicehtml5proxy_package_name]
     ) }
 
     it { is_expected.to contain_service('nova-spicehtml5proxy').with(
+      :ensure    => 'running',
       :name      => platform_params[:spicehtml5proxy_service_name],
-      :hasstatus => 'true',
-      :ensure    => 'running'
+      :hasstatus => true
     )}
 
     context 'with manage_service as false' do
@@ -30,6 +44,7 @@ describe 'nova::spicehtml5proxy' do
           :manage_service => false
         }
       end
+
       it { is_expected.to contain_service('nova-spicehtml5proxy').without_ensure }
     end
 
@@ -39,7 +54,8 @@ describe 'nova::spicehtml5proxy' do
       end
 
       it { is_expected.to contain_package('nova-spicehtml5proxy').with(
-        :ensure => params[:ensure_package]
+        :ensure => params[:ensure_package],
+        :name   => platform_params[:spicehtml5proxy_package_name],
       )}
     end
   end
@@ -49,8 +65,8 @@ describe 'nova::spicehtml5proxy' do
       @default_facts.merge({
         :osfamily        => 'Debian',
         :operatingsystem => 'Ubuntu',
-        :os_package_type => 'ubuntu'
-       })
+        :os_package_type => 'ubuntu',
+      })
     end
 
     let :platform_params do
@@ -76,6 +92,7 @@ describe 'nova::spicehtml5proxy' do
         :spicehtml5proxy_service_name => 'nova-spicehtml5proxy' }
     end
 
+    it_configures 'nova-spicehtml5proxy debian package'
     it_configures 'nova-spicehtml5proxy'
   end
 
@@ -93,6 +110,7 @@ describe 'nova::spicehtml5proxy' do
         :spicehtml5proxy_service_name => 'nova-spicehtml5proxy' }
     end
 
+    it_configures 'nova-spicehtml5proxy debian package'
     it_configures 'nova-spicehtml5proxy'
   end
 
diff --git a/spec/classes/nova_vnc_proxy_spec.rb b/spec/classes/nova_vnc_proxy_spec.rb
index b95de42..27ee489 100644
--- a/spec/classes/nova_vnc_proxy_spec.rb
+++ b/spec/classes/nova_vnc_proxy_spec.rb
@@ -8,6 +8,10 @@ describe 'nova::vncproxy' do
       'include nova'
     end
 
+    let :params do
+      { :enabled => true }
+    end
+
     context 'with default parameters' do
 
       it { is_expected.to contain_nova_config('vnc/novncproxy_host').with(:value => '0.0.0.0') }
@@ -22,7 +26,7 @@ describe 'nova::vncproxy' do
         :name      => platform_params[:nova_vncproxy_service],
         :hasstatus => true,
         :ensure    => 'running'
-      )}
+      ) }
 
       describe 'with manage_service as false' do
         let :params do
@@ -30,16 +34,20 @@ describe 'nova::vncproxy' do
             :manage_service => false
           }
         end
+
         it { is_expected.to contain_service('nova-vncproxy').without_ensure }
       end
 
       describe 'with package version' do
         let :params do
-          {:ensure_package => '2012.1-2'}
+          { :ensure_package => '2012.1-2' }
         end
+
         it { is_expected.to contain_package('nova-vncproxy').with(
-          'ensure' => '2012.1-2'
-        )}
+            :ensure => '2012.1-2',
+            :name   => platform_params[:nova_vncproxy_package],
+          )
+        }
       end
     end
 
@@ -122,6 +130,25 @@ describe 'nova::vncproxy' do
 
   end
 
+  shared_examples 'nova_vnc_proxy debian package' do
+    let :pre_condition do
+      'include nova'
+    end
+
+    before do
+      facts.merge!( :os_package_type => 'debian' )
+    end
+
+    it { is_expected.to contain_file_line('/etc/default/nova-consoleproxy:NOVA_CONSOLE_PROXY_TYPE').with(
+      :path    => '/etc/default/nova-consoleproxy',
+      :match   => '^NOVA_CONSOLE_PROXY_TYPE=(.*)$',
+      :line    => 'NOVA_CONSOLE_PROXY_TYPE=novnc',
+      :tag     => 'nova-consoleproxy',
+      :require => 'Anchor[nova::config::begin]',
+      :notify  => 'Anchor[nova::config::end]',
+    )}
+  end
+
   on_supported_os({
     :supported_os => OSDefaults.get_supported_os
   }).each do |os,facts|
@@ -133,8 +160,13 @@ describe 'nova::vncproxy' do
       let (:platform_params) do
         case facts[:osfamily]
         when 'Debian'
-          { :nova_vncproxy_package => 'nova-novncproxy',
-            :nova_vncproxy_service => 'nova-novncproxy' }
+          if facts[:os_package_type] == 'debian'
+            { :nova_vncproxy_package => 'nova-consoleproxy',
+              :nova_vncproxy_service => 'nova-novncproxy' }
+          else
+            { :nova_vncproxy_package => 'nova-novncproxy',
+              :nova_vncproxy_service => 'nova-novncproxy' }
+          end
         when 'RedHat'
           { :nova_vncproxy_package => 'openstack-nova-novncproxy',
             :nova_vncproxy_service => 'openstack-nova-novncproxy' }
@@ -143,6 +175,10 @@ describe 'nova::vncproxy' do
 
       it_behaves_like 'nova_vnc_proxy'
 
+      if facts[:os_package_type] == 'debian'
+        it_behaves_like 'nova_vnc_proxy debian package'
+      end
+
     end
   end
 
-- 
cgit v1.1