mbp-1128: Add Network Observability with NooBaa-backed Loki#165
mbp-1128: Add Network Observability with NooBaa-backed Loki#165pmatouse wants to merge 4 commits into
Conversation
Install Loki and Network Observability operators via a storage-dependent feature fragment, and deploy LokiStack plus FlowCollector using NooBaa storage. Signed-off-by: Petr Matousek <pmatouse@redhat.com>
mlorenzofr
left a comment
mlorenzofr
left a comment
There was a problem hiding this comment.
overall it's very good, but some small changes need to be made
| namespace: netobserv-loki | ||
| name: loki | ||
| size: 1x.extra-small | ||
| storageClassName: gp3-csi |
There was a problem hiding this comment.
This is AWS platform-specific. We should leave it empty and use the default StorageClass
| secret: | ||
| name: {{ .Values.loki.storageSecretName }} | ||
| type: s3 | ||
| storageClassName: {{ .Values.loki.storageClassName }} |
There was a problem hiding this comment.
We should look for a way to automatically detect the default storageClass, or fail if the storageClass is null, thus forcing the user to define it manually
| } | ||
|
|
||
| echo "Waiting for OBC Secret and ConfigMap ${OBC_NAME}..." | ||
| for i in $(seq 1 60); do |
There was a problem hiding this comment.
I'm missing error handling if resources are unavailable after 5 minutes (60 attempts). With the current logic, the script would continue executing, but the oc get secret requests would fail.
| - | | ||
| # NooBaa OBC creates a Secret (keys) + ConfigMap (bucket/host/port) | ||
| # with the same name as the claim — not all keys live in the Secret. | ||
| set -e |
There was a problem hiding this comment.
we already run bash with the -e argument, we don't need set it again within the script
There was a problem hiding this comment.
You've committed the changes to enable the netobserv functionality
The original file from the main branch should be restored, and the configuration added within comments
There was a problem hiding this comment.
that was an error from e2e testing facepalm
abcf24c to
091415e
Compare
| {{- $sc -}} | ||
| {{- else -}} | ||
| {{- $default := "" -}} | ||
| {{- range (lookup "storage.k8s.io/v1" "StorageClass" "" "").items | default list -}} |
There was a problem hiding this comment.
I've tested this in OCP 4.22 and using Helm lookup to find the default StorageClass works with helm install but fails in ArgoCD because ArgoCD's Helm renderer doesn't support lookup. I had to set loki.storageClassName explicitly via an ArgoCD helm parameter override.
If I'm not wrong, when storageClassName is omitted from the LokiStack CR, the Loki Operator uses the cluster's default StorageClass automatically (https://docs.redhat.com/en/documentation/red_hat_openshift_logging/6.5/html/installing_logging/verifying-cluster-prerequisites).
You can update the lokistack.yaml to make storageClassName conditional (see my next comment).
| secret: | ||
| name: {{ .Values.loki.storageSecretName }} | ||
| type: s3 | ||
| storageClassName: {{ include "netobserv.lokiStorageClassName" . }} |
There was a problem hiding this comment.
You can make it conditional to address my earlier comment:
{{- with (include "netobserv.lokiStorageClassName" .) }}
storageClassName: {{ . }}
{{- end }}
p-rog
left a comment
p-rog
left a comment
There was a problem hiding this comment.
Overall LGTM
I left one suggestion which I found when I was testing it at OCP 4.22.
Additionally, it would be great to add network policies to the Network Observability feature, to follow ZT methodology. You can test my network policy architect skill :)
3 participants
feat: add Network Observability with NooBaa-backed Loki
Install Loki and Network Observability operators via a storage-dependent
feature fragment, and deploy LokiStack plus FlowCollector using NooBaa
storage.
Signed-off-by: Petr Matousek pmatouse@redhat.com