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.

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

chart: add -scraper suffix to pod names #429

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

roobre
Copy link
Contributor

@roobre roobre commented May 13, 2022

As previously discussed, the overly simplified names were causing some confusion among users. The -scraper suffix should hopefully help to make more obvious that these pods are clients, rather than servers.

@roobre roobre requested a review from a team May 13, 2022 12:26
Copy link
Contributor

@kang-makes kang-makes left a comment

Choose a reason for hiding this comment

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

Take a look at the container's name because this is a cool opportunity to change that too.

{{- end -}}

{{- define "nriKubernetes.ksm.fullname.agent" -}}
{{- include "newrelic.common.naming.truncateToDNSWithSuffix" (dict "name" (include "nriKubernetes.naming.fullname" .) "suffix" "agent-ksm") -}}
{{- include "newrelic.common.naming.truncateToDNSWithSuffix" (dict "name" (include "nriKubernetes.naming.fullname" .) "suffix" "ksm-agent") -}}
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: I would prefer kubelet-node-scraper because the v2 of this integration named these pods as node.

Suggested change
{{- include "newrelic.common.naming.truncateToDNSWithSuffix" (dict "name" (include "nriKubernetes.naming.fullname" .) "suffix" "ksm-agent") -}}
{{- include "newrelic.common.naming.truncateToDNSWithSuffix" (dict "name" (include "nriKubernetes.naming.fullname" .) "suffix" "kubelet-node-scraper") -}}

@kang-makes
Copy link
Contributor

Solves #420

@roobre
Copy link
Contributor Author

roobre commented May 24, 2022

Pods with the new name look fine and deploy like this:
DeepinScreenshot_select-area_20220524105013

(Note the 3 in newr3lic is intentional as helm does not allow to have two releases with the same name in the same cluster)

Copy link
Contributor

@gsanchezgavier gsanchezgavier left a comment

Choose a reason for hiding this comment

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

LGTM

@paologallinaharbur
Copy link
Member

@roobre can we merge this?

@roobre
Copy link
Contributor Author

roobre commented Jun 23, 2022

@paologallinaharbur Virtuoso is making tests with their updated recipe right now, I'll merge if they confirm everything works as expected :)

@htroisi
Copy link
Contributor

htroisi commented Jan 26, 2023

The ksm pod naming also confused me. @roobre I think PR is a great idea!

@htroisi
Copy link
Contributor

htroisi commented Jan 26, 2023

Chatted with @roobre and the status of this PR is that it was blocked by the new virtuoso (guided install cli) recipe. The recipe has a “success check” that, after running helm install, checks for the existence of certain pods to verify the installation. And so we may need to coordinate the updating of these pod names there.

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.

5 participants