[DNM] Horizon k8s cluster logging #399

mcgonago · 2024-12-17T22:23:31Z

Add support to Horizon operator for k8s cluster logging.

openshift-ci · 2024-12-17T22:23:42Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mcgonago
Once this PR has been reviewed and has the lgtm label, please assign dprince for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bshephar · 2024-12-18T02:14:57Z

pkg/horizon/volumes.go

+func GetLogVolumeMount() corev1.VolumeMount {
+	return corev1.VolumeMount{
+		Name:      logVolume,
+		MountPath: "/var/log/manila",


/var/log/horizon ?

bshephar · 2024-12-18T02:17:55Z

pkg/horizon/const.go

 	// Horizon is the global ServiceType that refers to all the components deployed
 	// by the horizon-operator
 	Horizon storage.PropagationType = "Horizon"
+
+	//LogFile -
+	LogFile = "/var/log/horizon/horizon.log"


Are we also planning to capture Apache logs as part of this? Or do you feel that just the Horizon application logs are sufficient for any support / debugging requirements your team may have?

Current logging for Apache is just to the stdout of the container, so that might be sufficient:
https://github.com/openstack-k8s-operators/horizon-operator/blob/main/templates/horizon/config/httpd.conf#L25-L26

It just means logs will be lost when / if the pod is evicted from a node.

This can probably be a separate PR and topic, but just asking to make sure it has been considered.

The other thing about writing it to a specific log file is that we wont be able to see them simply by oc logs on the Horizon pod. So we will also need to add a sidecar container to the pod which will just run tail -f /var/log/horizon/horizon.log. That architecture is defined under this section:
https://kubernetes.io/docs/concepts/cluster-administration/logging/#sidecar-container-with-logging-agent

We can just add a new container to the pod called horizon-logs or something. That way, users will be able to clearly tell which container they can check to get Horizon Django application logs.

mcgonago · 2024-12-18T13:31:19Z

recheck

bshephar · 2024-12-22T23:46:41Z

pkg/horizon/volumes.go

+func GetLogVolumeMount() corev1.VolumeMount {
+	return corev1.VolumeMount{
+		Name:      logVolume,
+		MountPath: "/var/log/horizon",
+		ReadOnly:  false,
+	}
+}


It would be easier to return a slice of corev1.VolumeLogMount here. Then you can just call it from within the pod definition above without having to append the result to a slice.

Suggested change

func GetLogVolumeMount() corev1.VolumeMount {

return corev1.VolumeMount{

Name: logVolume,

MountPath: "/var/log/horizon",

ReadOnly: false,

}

}

func GetLogVolumeMount() []corev1.VolumeMount {

return []corev1.VolumeMount{

{

Name: logVolume,

MountPath: "/var/log/horizon",

ReadOnly: false,

},

}

}

bshephar · 2024-12-22T23:47:25Z

pkg/horizon/deployment.go

+								RunAsUser: &runAsUser,
+							},
+							Env:          env.MergeEnvs([]corev1.EnvVar{}, envVars),
+							VolumeMounts: []corev1.VolumeMount{GetLogVolumeMount()},


With the change suggested to the GetLogVolumtMount() function below. You can just call the function here:

Suggested change

VolumeMounts: []corev1.VolumeMount{GetLogVolumeMount()},

VolumeMounts: GetLogVolumeMount(),

bshephar · 2024-12-22T23:47:49Z

pkg/horizon/deployment.go

@@ -146,6 +162,7 @@ func Deployment(
 							},
 							Env:            env.MergeEnvs([]corev1.EnvVar{}, envVars),
 							VolumeMounts:   volumeMounts,
+                                                                []corev1.VolumeMount{GetLogVolumeMount()}...),


Same here, you can just call the function once it's changed to return a slice.

bshephar · 2024-12-22T23:55:25Z

pkg/horizon/deployment.go

@@ -158,6 +175,9 @@ func Deployment(
 			},
 		},
 	}
+	deployment.Spec.Template.Spec.Volumes = append(GetVolumes(
+                instance.Name,
+		instance.Spec.ExtraMounts), GetLogVolume())


The suggested change to GetLogVolume() means that this would require merging the two slices at this point though.

The other way you could do it, is keep the function the same and then just append this to the volumeMounts variable defined on line 99 and just have everything using the same mounts.

I would probably opt for the first option though, just to keep the mounts minimal on the log pod. It just means you'll need to merge the slices here, rather than appending since this would now give you a slice of slices.

Actually, sorry, just realised this is the Volume not the Volume mount. The only thing missing here is the HorizonPropagation variable. So it should be:

Suggested change

instance.Spec.ExtraMounts), GetLogVolume())

instance.Spec.ExtraMounts, HorizonPropagation), GetLogVolume())

bshephar · 2024-12-22T23:55:42Z

pkg/horizon/deployment.go

@@ -158,6 +175,9 @@ func Deployment(
 			},
 		},
 	}
+	deployment.Spec.Template.Spec.Volumes = append(GetVolumes(


getVolumes? GetVolumes is undefined.

bshephar · 2024-12-23T06:05:57Z

controllers/horizon_controller.go

+	templateParameters := map[string]interface{}{
+		"LogFile": horizon.LogFile,
+	}


Oh, this will override the entire templateParameters and you will just end up with LogFile in there. So you want to just set the LogFile key:

Suggested change

templateParameters := map[string]interface{}{

"LogFile": horizon.LogFile,

}

templateParameters["LogFile"] = horizon.LogFile

fmount

Other than what @bshephar already suggested, to have a green run we should:

rebase the patch on main as a lot of things changed because of the Topology related patches
add the missing GetLogVolume() and GetLogVolumeMount() to volume and volumeMount of deployment.go

I can see envTests passing locally after the two changes above:

Will run 29 of 29 specs
•••••••••••••••••••••••••••••W0228 14:45:07.689610  507081 reflector.go:462] pkg/mod/k8s.io/client-go@v0.29.14/tools/
cache/reflector.go:229: watch of *v1beta1.Horizon ended with: an error on the server ("unable to decode an event from
 the watch stream: context canceled") has prevented the request from succeeding
W0228 14:45:07.689610  507081 reflector.go:462] pkg/mod/k8s.io/client-go@v0.29.14/tools/cache/reflector.go:229: watch
 of *v1beta1.Topology ended with: an error on the server ("unable to decode an event from the watch stream: context c
anceled") has prevented the request from succeeding


Ran 29 of 29 Specs in 18.622 seconds
SUCCESS! -- 29 Passed | 0 Failed | 0 Pending | 0 Skipped
PASS

fmount · 2025-02-28T13:50:03Z

pkg/horizon/deployment.go

@@ -109,6 +109,22 @@ func Deployment(
 				Spec: corev1.PodSpec{
 					ServiceAccountName: instance.RbacResourceName(),
 					Containers: []corev1.Container{
+						// the first container in a pod is the default selected


I don't know how to comment on different lines other than the modified ones, but L90 and L91 should be:

volumes := append(getVolumes(instance.Name, instance.Spec.ExtraMounts, HorizonPropagation), GetLogVolume()) volumeMounts := append(getVolumeMounts(instance.Spec.ExtraMounts, HorizonPropagation), GetLogVolumeMount())

fmount · 2025-02-28T13:50:43Z

pkg/horizon/deployment.go

@@ -132,6 +148,27 @@ func Deployment(
 			},
 		},
 	}
+	deployment.Spec.Template.Spec.Volumes = getVolumes(instance.Name, instance.Spec.ExtraMounts, HorizonPropagation)
+	/*
+   * +++owen - when looking at how manila did this - we see care taken with a GetVolumes that handles two pods of the same service?


remove the comment from L153 to L160

fmount · 2025-02-28T13:50:59Z

pkg/horizon/deployment.go

+	// If possible two pods of the same service should not
+	// run on the same worker node. If this is not possible
+	// the get still created on the same worker node.
+	deployment.Spec.Template.Spec.Affinity = affinity.DistributePods(


remove this function as Topology already merged

fmount · 2025-02-28T13:52:04Z

pkg/horizon/deployment.go

+						// the first container in a pod is the default selected
+						// by oc log so define the log stream container first.
+						{
+							Name: instance.Name + "-log",


you need to update envTests to point to container[1] when checking horizon, and not the sidecar w/ logs.

--- a/tests/functional/horizon_controller_test.go +++ b/tests/functional/horizon_controller_test.go @@ -580,7 +580,7 @@ var _ = Describe("Horizon controller", func() { th.AssertVolumeExists(CABundleSecretName, d.Spec.Template.Spec.Volumes) th.AssertVolumeExists(InternalCertSecretName, d.Spec.Template.Spec.Volumes) - svcC := d.Spec.Template.Spec.Containers[0] + svcC := d.Spec.Template.Spec.Containers[1] // check TLS volume mounts th.AssertVolumeMountExists(CABundleSecretName, "tls-ca-bundle.pem", svcC.VolumeMounts)

According to [1] looks like it's a good practice introducing a sidecar container for logging purposes. By doing this we can rotate, process and even forward logs elsewhere without affecting the original service pod. [1] https://kubernetes.io/docs/concepts/cluster-administration/logging/ Signed-off-by: Francesco Pantano <fpantano@redhat.com>

openshift-ci · 2025-02-28T22:11:10Z

@mcgonago: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/horizon-operator-build-deploy-kuttl	`9c13c9f`	link	true	`/test horizon-operator-build-deploy-kuttl`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci bot requested review from dprince and viroel December 17, 2024 22:23

bshephar reviewed Dec 18, 2024

View reviewed changes

bshephar reviewed Dec 22, 2024

View reviewed changes

bshephar reviewed Dec 23, 2024

View reviewed changes

openshift-merge-robot added needs-rebase and removed needs-rebase labels Feb 17, 2025

fmount self-requested a review February 25, 2025 15:54

fmount requested changes Feb 28, 2025

View reviewed changes

openshift-ci bot assigned fmount Feb 28, 2025

mcgonago added the do-not-merge/work-in-progress label Feb 28, 2025

mcgonago force-pushed the horizon_k8s_cluster_logging branch from 2a16719 to 57c1d6f Compare February 28, 2025 18:45

openshift-merge-robot added needs-rebase and removed needs-rebase labels Feb 28, 2025

mcgonago force-pushed the horizon_k8s_cluster_logging branch from 4c5ee60 to 8fe2f1e Compare February 28, 2025 19:34

openshift-merge-robot added needs-rebase and removed needs-rebase labels Feb 28, 2025

fmount force-pushed the horizon_k8s_cluster_logging branch 3 times, most recently from 5ba1f56 to 9c13c9f Compare February 28, 2025 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DNM] Horizon k8s cluster logging #399

[DNM] Horizon k8s cluster logging #399

mcgonago commented Dec 17, 2024

openshift-ci bot commented Dec 17, 2024

bshephar Dec 18, 2024

bshephar Dec 18, 2024

bshephar Dec 18, 2024

mcgonago commented Dec 18, 2024

bshephar Dec 22, 2024

bshephar Dec 22, 2024

bshephar Dec 22, 2024

bshephar Dec 22, 2024

bshephar Dec 23, 2024

bshephar Dec 22, 2024

bshephar Dec 23, 2024

fmount left a comment

fmount Feb 28, 2025

fmount Feb 28, 2025

fmount Feb 28, 2025

fmount Feb 28, 2025

openshift-ci bot commented Feb 28, 2025

	VolumeMounts: []corev1.VolumeMount{GetLogVolumeMount()},
	VolumeMounts: GetLogVolumeMount(),

	instance.Spec.ExtraMounts), GetLogVolume())
	instance.Spec.ExtraMounts, HorizonPropagation), GetLogVolume())

[DNM] Horizon k8s cluster logging #399

Are you sure you want to change the base?

[DNM] Horizon k8s cluster logging #399

Conversation

mcgonago commented Dec 17, 2024

openshift-ci bot commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcgonago commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmount left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 28, 2025