From 863615c0c5b899f73f0f98049a14c9c2203d9e0e Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Fri, 22 Sep 2023 13:28:42 +0200 Subject: [PATCH] [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. --- controllers/novacell_controller.go | 7 +++++ controllers/novacompute_controller.go | 3 +++ controllers/novanovncproxy_controller.go | 2 -- templates/nova.conf | 29 +++++++++------------ test/functional/nova_novncproxy_test.go | 1 - test/functional/novacell_controller_test.go | 18 ++++++++++--- 6 files changed, 38 insertions(+), 22 deletions(-) diff --git a/controllers/novacell_controller.go b/controllers/novacell_controller.go index f40de38de..ce4bcaa4e 100644 --- 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 index 57e6a8db6..01343829b 100644 --- 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 index 1cad02faa..cffc90f37 100644 --- 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 index b83d5224c..8d74c19d7 100644 --- 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 index 5d0c9984f..e636304f2 100644 --- 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 index 4fc841087..2836bf7ac 100644 --- 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))