Skip to content

storage: netapp-monitoring chart#11869

Closed
RockSolidScripts wants to merge 21 commits into
masterfrom
feat/storage-netapp-monitoring-chart
Closed

storage: netapp-monitoring chart#11869
RockSolidScripts wants to merge 21 commits into
masterfrom
feat/storage-netapp-monitoring-chart

Conversation

@RockSolidScripts

@RockSolidScripts RockSolidScripts commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Helm for netapp-monitoring harvest
This chart includes service dicsovery from netbox - netappsd

Test Results in our internal qa-de-1 cluster with service discovery for SCI devices:
image

@RockSolidScripts RockSolidScripts requested a review from a team as a code owner June 4, 2026 20:59
@RockSolidScripts RockSolidScripts changed the title storage: netapp monitoring chart storage: netapp-monitoring chart Jun 4, 2026

@viennaa viennaa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for tackling this topic. Couple of nits are inline which need to be addressed for this first review.

Most important one is the location of this chart, we might need to move it to greenhouse-extensions but I would like to clarify this. If needed, we can take this offline.

- host: {{ .Host }}
- DC: {{ .Facility }}
- Service: {{ .Service }}
- lob: `}}{{ .Values.netappsd.lob }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

declare in values.yaml, give it a default

@RockSolidScripts RockSolidScripts Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These labels come from the service discovery 'worker' container. The worker container takes this as a template and render it with the data from the service discovery logic.

ref:

        - name: worker
          image: "{{ .Values.netappsd.image.repository }}:{{ .Values.netappsd.image.tag }}"
          imagePullPolicy: {{ .Values.netappsd.image.pullPolicy | default "IfNotPresent" }}
          command: ["/netappsd", "worker"]
          args:
            - --master-url
            - http://{{ include "netapp-monitoring.fullname" . }}-{{ $appName }}-master.{{ .Release.Namespace }}.svc:{{ .Values.netappsd.ports.metrics }}
            - --listen-addr
            - :8080
            - --template-file
            - /app/harvest.yaml.tpl
            - --output-file
            - /app/shared/harvest.yaml

defaults are set for the values referenced via helm chart.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

mh I still can't spot .Values.netappsd.lob

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed.

Comment thread prometheus-exporters/netapp-monitoring/templates/harvest-netappsd-configmap.yaml Outdated
Comment thread prometheus-exporters/netapp-monitoring/templates/harvest-netappsd-configmap.yaml Outdated
Comment thread prometheus-exporters/netapp-monitoring/values.yaml Outdated
@RockSolidScripts RockSolidScripts requested a review from viennaa June 8, 2026 04:54

@viennaa viennaa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Next round :)

namespace: {{ .Release.Namespace }}
type: Opaque
data:
{{ .Values.netappsd.credentials_secret }}.yml: {{ include "netapp-monitoring.credentials-entry" . | b64enc }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add required

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated.

{{- range $.Values.finalizers }}
- {{ . | quote }}
{{- end }}
name: {{ .Values.netappsd.credentials_secret }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add required to not let it fail

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Comment on lines +68 to +69
mountPath: {{ .Values.netappsd.credentials_file }}
subPath: {{ .Values.netappsd.credentials_secret }}.yml

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add required

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Comment on lines +17 to +18
port: 13000
targetPort: 13000

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

use variable instead

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Create the name of the service account to use
*/}}
{{- define "netapp-monitoring.serviceAccountName" -}}
{{- if .Values.serviceAccount.create }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should this be .Values.netappsd.serviceAccount? Still the create option is missing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch. thanks
fixed.

@RockSolidScripts RockSolidScripts requested a review from viennaa June 10, 2026 17:47
@RockSolidScripts

Copy link
Copy Markdown
Contributor Author

Closing this PR as it is moved - cloudoperators/greenhouse-extensions#1728

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants