Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DNM] Horizon k8s cluster logging #399

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+63 −0
Open

[DNM] Horizon k8s cluster logging #399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8adb32b
Initial changes
mcgonago Dec 17, 2024
e636fed
Fixed typo
mcgonago Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions controllers/horizon_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -907,6 +907,10 @@ func (r *HorizonReconciler) generateServiceConfigMaps(
templateParameters["SSLCertificateKeyFile"] = fmt.Sprintf("/etc/pki/tls/private/%s.key", horizon.ServiceName)
}

templateParameters := map[string]interface{}{
"LogFile": horizon.LogFile,
}
Comment on lines +910 to +912
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


cms := []util.Template{
// ConfigMap
{
Expand Down
18 changes: 18 additions & 0 deletions pkg/horizon/const.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,27 @@ const (
// HorizonExtraVolTypeUndefined can be used to label an extraMount which is
// not associated to anything in particular
HorizonExtraVolTypeUndefined storage.ExtraVolType = "Undefined"

// 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"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// logVolume -
logVolume = "logs"

DefaultsConfigFileName = "00-config.conf"
// ServiceConfigFileName - Represents service config generated in the operator
ServiceConfigFileName = "01-config.conf"
// CustomConfigFileName - Config snippets inherited by the top-level CR
CustomConfigFileName = "02-config.conf"
// CustomServiceConfigFileName - Config snippets defined for the sub CR
CustomServiceConfigFileName = "03-config.conf"
// CustomServiceConfigSecretsFileName - Snippet generated by Secrets passed
// to the sub CR
CustomServiceConfigSecretsFileName = "04-config.conf"
)

// HorizonPropagation is the definition of the Horizon propagation service
Expand Down
20 changes: 20 additions & 0 deletions pkg/horizon/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,22 @@ func Deployment(
Spec: corev1.PodSpec{
ServiceAccountName: instance.RbacResourceName(),
Containers: []corev1.Container{
// the first container in a pod is the default selected
// by oc log so define the log stream container first.
{
Name: instance.Name + "-log",
Command: []string{
"/bin/bash",
},
Args: []string{"-c", "tail -n+1 -F " + LogFile},
Image: instance.Spec.ContainerImage,
SecurityContext: &corev1.SecurityContext{
RunAsUser: &runAsUser,
},
Env: env.MergeEnvs([]corev1.EnvVar{}, envVars),
VolumeMounts: []corev1.VolumeMount{GetLogVolumeMount()},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Resources: instance.Spec.Resources,
},
{
Name: ServiceName,
Command: []string{
Expand All @@ -146,6 +162,7 @@ func Deployment(
},
Env: env.MergeEnvs([]corev1.EnvVar{}, envVars),
VolumeMounts: volumeMounts,
[]corev1.VolumeMount{GetLogVolumeMount()}...),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Resources: instance.Spec.Resources,
ReadinessProbe: readinessProbe,
LivenessProbe: livenessProbe,
Expand All @@ -158,6 +175,9 @@ func Deployment(
},
},
}
deployment.Spec.Template.Spec.Volumes = append(GetVolumes(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getVolumes? GetVolumes is undefined.

instance.Name,
instance.Spec.ExtraMounts), GetLogVolume())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())

deployment.Spec.Template.Spec.Affinity = affinity.DistributePods(
common.AppSelector,
[]string{
Expand Down
19 changes: 19 additions & 0 deletions pkg/horizon/volumes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,22 @@ func getVolumeMounts(
}
return vm
}

// GetLogVolumeMount - Horizon API LogVolumeMount
func GetLogVolumeMount() corev1.VolumeMount {
return corev1.VolumeMount{
Name: logVolume,
MountPath: "/var/log/horizon",
ReadOnly: false,
}
}
Comment on lines +101 to +107
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
},
}
}


// GetLogVolume - Horizon API LogVolume
func GetLogVolume() corev1.Volume {
return corev1.Volume{
Name: logVolume,
VolumeSource: corev1.VolumeSource{
EmptyDir: &corev1.EmptyDirVolumeSource{Medium: ""},
},
}
}
2 changes: 2 additions & 0 deletions templates/horizon/config/01-config.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[DEFAULT]
log_file = {{ .LogFile }}
Loading