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

Initial release for Jaeger Helm Chart #2078

Merged
merged 11 commits into from Sep 28, 2017

Conversation

Projects
None yet
3 participants
@dvonthenen
Collaborator

dvonthenen commented Sep 8, 2017

This is the initial release for the Jaeger Helm Chart.

helm lint: Pass
helm install .: Works with the minimum required resources
data persistence: Done in the cassandra subchart
README.md: Included
NOTES.txt: Included
Best practices labels and values: Yes, with exception. Some of the values override values in the cassandra chart. Those keys do not follow the lowercase/camelcase convention.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Sep 8, 2017

Hi @dvonthenen. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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/test-infra repository. I understand the commands that are listed here.

@unguiculus

This comment has been minimized.

Member

unguiculus commented Sep 10, 2017

Thanks for the PR. Please check our best practices for standard labels, especially regarding the app label.

https://github.com/kubernetes/helm/blob/master/docs/chart_best_practices/labels.md

@unguiculus unguiculus self-assigned this Sep 10, 2017

@unguiculus unguiculus self-requested a review Sep 10, 2017

@dvonthenen

This comment has been minimized.

Collaborator

dvonthenen commented Sep 11, 2017

@unguiculus Will check it out. Thanks!

@dvonthenen

This comment has been minimized.

Collaborator

dvonthenen commented Sep 14, 2017

Hi @unguiculus I updated the app label to match best practices and verified that the other labels comply as well.

FYI my response might be delayed as I am attending Open Source Summit NA and MesosCon NA this week.

@unguiculus

I made a few comments. Please apply them throughout the whole chart. I usually only specified them once.

Please add resources and nodeSelector.

Here's a good example: https://github.com/kubernetes/charts/tree/master/stable/nginx-ingress
You may take this also as reference for grouping values.

apiVersion: v1
description: A Jaeger Helm chart for Kubernetes
name: jaeger
version: 0.1.0

This comment has been minimized.

@unguiculus

unguiculus Sep 15, 2017

Member

Add appVersion

This comment has been minimized.

@dvonthenen

dvonthenen Sep 18, 2017

Collaborator

Added.

{{/*
Expand the name of the chart.
*/}}
{{- define "jaeger" -}}

This comment has been minimized.

@unguiculus

unguiculus Sep 15, 2017

Member

Call it jaeger.name

This comment has been minimized.

@dvonthenen

dvonthenen Sep 18, 2017

Collaborator

Changed to jaeger.name

app: "{{ template "jaeger" . }}"
jaeger-infra: agent-daemonset
chart: {{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}
component: "{{ template "jaeger.fullname" . }}-agent"

This comment has been minimized.

@unguiculus

unguiculus Sep 15, 2017

Member

Don't add the fullname to the component name. Should just be agent.

This comment has been minimized.

@dvonthenen

dvonthenen Sep 18, 2017

Collaborator

I mirrored the component names based on what is recommended from the Jaeger repo for Kubernetes. Their best practice is to have jaeger-agent as seen here:
https://github.com/jaegertracing/jaeger-kubernetes/blob/master/production/jaeger-production-template.yml

I can make the component name static meaning replace "{{ template "jaeger.fullname" . }}-agent" to "jaeger-agent".

This comment has been minimized.

@dvonthenen

dvonthenen Sep 18, 2017

Collaborator

Changed to static names.

protocol: UDP
targetPort: {{ .Values.service.agent.binaryPort }}
clusterIP: None
selector:

This comment has been minimized.

@unguiculus

unguiculus Sep 15, 2017

Member

Selector should have app, component, and release labels.

This comment has been minimized.

@dvonthenen

dvonthenen Sep 18, 2017

Collaborator

Added app, component, and release to selector.

template:
metadata:
labels:
jaeger-infra: collector-pod

This comment has been minimized.

@unguiculus

unguiculus Sep 15, 2017

Member

app, component, and release labels should go here.

This comment has been minimized.

@dvonthenen

dvonthenen Sep 18, 2017

Collaborator

Added app, component, and release.

# Default values for jaeger.
# This is a YAML-formatted file.
# Declare variables to be passed into your templates.

This comment has been minimized.

@unguiculus

unguiculus Sep 15, 2017

Member

We usually group by component instead of resource type. I think that would make more sense.

This comment has been minimized.

@dvonthenen

dvonthenen Sep 18, 2017

Collaborator

Looking to refactor for this now.

This comment has been minimized.

@dvonthenen

dvonthenen Sep 18, 2017

Collaborator

Refactored grouping by component. This is completed.

persistence:
enabled: false
config:
cluster_name: jaeger

This comment has been minimized.

@unguiculus

unguiculus Sep 15, 2017

Member

Camel case, please.

This comment has been minimized.

@dvonthenen

dvonthenen Sep 18, 2017

Collaborator

I am overwriting values in the Cassandra chart and that is how they appear. What's the best way to resolve this?

This comment has been minimized.

@dvonthenen

dvonthenen Sep 18, 2017

Collaborator

@unguiculus requesting feedback for the above question

This comment has been minimized.

@dvonthenen

dvonthenen Sep 19, 2017

Collaborator

I can always submit a PR to fix their camel case issue.

@dvonthenen

This comment has been minimized.

Collaborator

dvonthenen commented Sep 19, 2017

@unguiculus Added in resources and nodeSelector for the various components.

Also updated the various files based on the review at: https://github.com/kubernetes/charts/pull/2078/files/eccf2e7f224a3519e128a70242613ba6c0535e8a

The last outstanding item about the Cassandra subchart not using camel case. Looking for guidance on how to proceed.

@dvonthenen

This comment has been minimized.

Collaborator

dvonthenen commented Sep 19, 2017

Made a small correction to the jaeger-agent pointing to the correct FQDN of the jaeger-collector. Tested and working.

Just waiting for feedback on the Cassandra helm chart not using camel case.

@unguiculus

This comment has been minimized.

Member

unguiculus commented Sep 22, 2017

I overlooked that fact that the camel case is for Cassandra. It's fine then. you have no choice.

@@ -0,0 +1,45 @@
apiVersion: extensions/v1beta1
kind: DaemonSet
metadata:

This comment has been minimized.

@unguiculus

unguiculus Sep 22, 2017

Member

I guess I confused you. Maybe I was not clear enough. Sorry. I did not want you to change the names, only the component labels. Do it like so throughout the chart:

name: "{{ template "jaeger.fullname" . }}-agent"

component: agent

This comment has been minimized.

@dvonthenen

dvonthenen Sep 22, 2017

Collaborator

Ahhh I see. Will provide an update shortly.

This comment has been minimized.

@dvonthenen

dvonthenen Sep 27, 2017

Collaborator

This has been addressed.

@unguiculus

/ok-to-test

release: "{{ .Release.Name }}"
jaeger-infra: agent-instance
spec:
nodeSelector:
{{ toYaml .Values.agent.nodeSelector | indent 8 }}
containers:
- name: "jaeger-agent"
- name: "{{ template "jaeger.fullname" . }}-agent"

This comment has been minimized.

@unguiculus

unguiculus Sep 27, 2017

Member

You could have left the container names as they were. Feel free to change those back if you like.

This comment has been minimized.

@dvonthenen

dvonthenen Sep 28, 2017

Collaborator

I would like to keep them with the jaeger.fullname so that if there is a situation where you want to deploy multiple independent instances, this gives you that flexibility for unique names.

@unguiculus

This comment has been minimized.

Member

unguiculus commented Sep 27, 2017

/ok-to-test

@dvonthenen

This comment has been minimized.

Collaborator

dvonthenen commented Sep 27, 2017

@unguiculus I believe that fixes all outstanding issues. Please provide feedback if you see any other issues. I have tested this as well and all seems to function as expected.

@unguiculus

This comment has been minimized.

Member

unguiculus commented Sep 27, 2017

Only my question regarding the container names is still open (#2078 (comment)). If you like to keep them as they are now, I'll merge the PR.

@dvonthenen

This comment has been minimized.

Collaborator

dvonthenen commented Sep 28, 2017

@unguiculus I replied to your comment above. Would like to keep the jaeger.fullname. Thanks for all your help in this! I really appreciate it!

@unguiculus

This comment has been minimized.

Member

unguiculus commented Sep 28, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 28, 2017

@unguiculus unguiculus merged commit 5632862 into helm:master Sep 28, 2017

2 checks passed

cla/linuxfoundation dvonthenen authorized
Details
pull-charts-e2e Jenkins job succeeded.
Details

@dvonthenen dvonthenen deleted the dvonthenen:feature/jaeger branch Oct 20, 2017

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