[VNC]Cleanup configuration

While reviewing other PRs we noticed that there is some unused template parameters for the vnc configurations. So I started cleaning up it. Then I noticed that never filled template parameters in our nova.conf related to vnc. So this PR cleans up the [vnc] section and the related config generation code. To be able to follow our template I split the template to compute and vncproxy parts and made sure that only the relevant configs are applied for each service. Also fixed a small bug that ignored config generation errors.
openstack-k8s-operators · Sep 28, 2023 · 863615c · 863615c
1 parent 1f39d71
commit 863615c
Show file tree

Hide file tree

Showing 6 changed files with 38 additions and 22 deletions.
diff --git a/controllers/novacell_controller.go b/controllers/novacell_controller.go
@@ -770,7 +770,10 @@ func (r *NovaCellReconciler) generateComputeConfigs(
 	// vnc is optional so we only need to configure it for the compute
 	// if the proxy service is deployed in the cell
 	if vncProxyURL != nil {
+		templateParameters["vnc_enabled"] = true
 		templateParameters["novncproxy_base_url"] = *vncProxyURL
+	} else {
+		templateParameters["vnc_enabled"] = false
 	}
 
 	cmLabels := labels.GetLabels(
@@ -783,6 +786,10 @@ func (r *NovaCellReconciler) generateComputeConfigs(
 	err := r.GenerateConfigs(
 		ctx, h, instance, configName, &hashes, templateParameters, map[string]string{}, cmLabels, map[string]string{},
 	)
+	if err != nil {
+		return err
+	}
+
 	// TODO(gibi): can we make it simpler?
 	a := &corev1.EnvVar{}
 	hashes[configName](a)

diff --git a/controllers/novacompute_controller.go b/controllers/novacompute_controller.go
@@ -273,7 +273,10 @@ func (r *NovaComputeReconciler) generateConfigs(
 		"transport_url":          string(secret.Data[TransportURLSelector]),
 		"log_file":               "/var/log/nova/nova-compute.log",
 		"compute_driver":         instance.Spec.ComputeDriver,
+		// Neither the ironic driver nor the fake driver support VNC
+		"vnc_enabled": false,
 	}
+
 	extraData := map[string]string{}
 	if instance.Spec.CustomServiceConfig != "" {
 		extraData["02-nova-override.conf"] = instance.Spec.CustomServiceConfig

diff --git a/controllers/novanovncproxy_controller.go b/controllers/novanovncproxy_controller.go
@@ -285,8 +285,6 @@ func (r *NovaNoVNCProxyReconciler) generateConfigs(
 		"cell_db_password":       string(secret.Data[CellDatabasePasswordSelector]),
 		"cell_db_address":        instance.Spec.CellDatabaseHostname,
 		"cell_db_port":           3306,
-		"api_interface_address":  "",     // fixme
-		"public_protocol":        "http", // fixme
 		"transport_url":          string(secret.Data[TransportURLSelector]),
 		"openstack_cacert":       "",          // fixme
 		"openstack_region_name":  "regionOne", // fixme

diff --git a/templates/nova.conf b/templates/nova.conf
@@ -124,30 +124,27 @@ driver = noop
 notify_on_state_change = vm_and_task_state
 {{ end }}
 
-{{ if eq .service_name "nova-novncproxy" "nova-compute"}}
+{{ if eq .service_name "nova-novncproxy"}}
 [vnc]
-{{ if (index . "compute_driver") }}
-{{if eq .compute_driver "ironic.IronicDriver"}}
-# TODO(ksambor) in tripleo we disable vnc console for ironicdriver
-enabled = False
-{{else}}
 enabled = True
+novncproxy_host = "::0"
+novncproxy_port = 6080
 {{end}}
-{{end}}
-novncproxy_host = {{ if (index . "novncproxy_service_host") }} {{ .novncproxy_service_host }} {{else}}"::0"{{ end }}
-novncproxy_port = {{ if (index . "nova_novncproxy_listen_port") }}{{ .nova_novncproxy_listen_port }} {{else}}6080{{ end }}
-server_listen = {{ if (index . "novncproxy_service_host") }}{{ .novncproxy_service_host }} {{else}}"::0"{{ end }}
-{{ if eq .service_name "nova-compute"}}
+
+{{ if and (eq .service_name "nova-compute") .vnc_enabled }}
+[vnc]
+enabled = True
+novncproxy_base_url = {{ .novncproxy_base_url }}
+server_listen = "::0"
 {{/* https://docs.openstack.org/oslo.config/latest/configuration/format.html#substitution    */}}
 # note we may want to use console_host instead of my_ip however it wont be resolved via
 # dns currently so we need to use my_ip for now.
 # https://docs.openstack.org/nova/latest/configuration/config.html#DEFAULT.console_host
 server_proxyclient_address = "$my_ip"
-{{if (index . "novncproxy_base_url")}}
-novncproxy_base_url = {{ .novncproxy_base_url }}
-{{ end }}
-{{ end }}
-{{ end }}
+{{else}}
+[vnc]
+enabled = False
+{{end}}
 
 [cache]
 # always enable caching

diff --git a/test/functional/nova_novncproxy_test.go b/test/functional/nova_novncproxy_test.go
@@ -162,7 +162,6 @@ var _ = Describe("NovaNoVNCProxy controller", func() {
 				configData := string(configDataMap.Data["01-nova.conf"])
 				Expect(configData).Should(ContainSubstring("novncproxy_host = \"::0\""))
 				Expect(configData).Should(ContainSubstring("novncproxy_port = 6080"))
-				Expect(configData).Should(ContainSubstring("server_listen = \"::0\""))
 				Expect(configData).Should(ContainSubstring("password = service-password"))
 				Expect(configData).Should(ContainSubstring("transport_url=rabbit://cell1/fake"))
 				Expect(configDataMap.Data).Should(HaveKey("02-nova-override.conf"))

diff --git a/test/functional/novacell_controller_test.go b/test/functional/novacell_controller_test.go
@@ -270,6 +270,7 @@ var _ = Describe("NovaCell controller", func() {
 			vncUrlConfig := fmt.Sprintf("novncproxy_base_url = http://%s/vnc_lite.html",
 				fmt.Sprintf("nova-novncproxy-%s-public.%s.svc:6080", cell1.CellName, cell1.CellCRName.Namespace))
 			Expect(configData).To(ContainSubstring(vncUrlConfig))
+			Expect(configData).To(ContainSubstring("[vnc]\nenabled = True"))
 
 			th.ExpectCondition(
 				cell1.CellCRName,
@@ -316,8 +317,9 @@ var _ = Describe("NovaCell controller", func() {
 			configData := string(computeConfigData.Data["01-nova.conf"])
 			Expect(configData).To(ContainSubstring("transport_url=rabbit://cell1/fake"))
 			Expect(configData).To(ContainSubstring("username = nova\npassword = service-password\n"))
-			vncUrlConfig := fmt.Sprintf("novncproxy_base_url = http://foo/vnc_lite.html")
+			vncUrlConfig := "novncproxy_base_url = http://foo/vnc_lite.html"
 			Expect(configData).To(ContainSubstring(vncUrlConfig))
+			Expect(configData).To(ContainSubstring("[vnc]\nenabled = True"))
 
 			th.ExpectCondition(
 				cell1.CellCRName,
@@ -377,6 +379,14 @@ var _ = Describe("NovaCell controller", func() {
 				corev1.ConditionTrue,
 			)
 
+			// updates the compute config and disables VNC there
+			computeConfigData := th.GetSecret(cell1.ComputeConfigSecretName)
+			Expect(computeConfigData).ShouldNot(BeNil())
+			Expect(computeConfigData.Data).Should(HaveKey("01-nova.conf"))
+			configData := string(computeConfigData.Data["01-nova.conf"])
+			Expect(configData).NotTo(ContainSubstring("novncproxy_base_url"))
+			Expect(configData).To(ContainSubstring("[vnc]\nenabled = False"))
+
 		})
 		It("deletes NovaMetadata if it is disabled later", func() {
 			th.SimulateJobSuccess(cell1.DBSyncJobName)
@@ -438,8 +448,9 @@ var _ = Describe("NovaCell controller", func() {
 			Expect(configData).To(ContainSubstring("transport_url=rabbit://cell2/fake"))
 			Expect(configData).To(ContainSubstring("username = nova\npassword = service-password\n"))
 			// There is no VNCProxy created but we still get a compute config just
-			// without any vnc proxy url
-			Expect(configData).NotTo(ContainSubstring("novncproxy_base_url "))
+			// without any vnc proxy url and therefore vnc disabled
+			Expect(configData).NotTo(ContainSubstring("novncproxy_base_url"))
+			Expect(configData).To(ContainSubstring("[vnc]\nenabled = False"))
 
 			th.ExpectCondition(
 				cell2.CellCRName,
@@ -523,6 +534,7 @@ var _ = Describe("NovaCell controller", func() {
 			vncUrlConfig := fmt.Sprintf("novncproxy_base_url = http://%s/vnc_lite.html",
 				fmt.Sprintf("nova-novncproxy-%s-public.%s.svc:6080", cell2.CellName, cell2.CellCRName.Namespace))
 			Expect(configData).To(ContainSubstring(vncUrlConfig))
+			Expect(configData).To(ContainSubstring("[vnc]\nenabled = True"))
 
 			Expect(GetNovaCell(cell2.CellCRName).Status.Hash[cell2.ComputeConfigSecretName.Name]).NotTo(BeNil())
 			Expect(GetNovaCell(cell2.CellCRName).Status.Hash[cell2.ComputeConfigSecretName.Name]).NotTo(Equal(oldComputeConfigHash))