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

Pre-install hook for helm #176

Merged
merged 10 commits into from
Sep 6, 2019
Merged

Pre-install hook for helm #176

merged 10 commits into from
Sep 6, 2019

Conversation

maimaisie
Copy link
Contributor

@maimaisie maimaisie commented Sep 3, 2019

Description

Adds a helm pre-install hook that runs the setup script as a job. Currently the container is built from a separate image than our fluentd one. I will look into ways to use the same

Testing performed
  • ci/build.sh
  • Redeploy fluentd and fluentd-events pods
  • Confirm events, logs, and metrics are coming in
  • Package new helm chart locally and deploy to make sure the hook is being run

deploy/docker/setup/setup.sh Outdated Show resolved Hide resolved
## Setup

# If enabled, a pre-install hook will create Collector and Sources in Sumo Logic
setupEnabled: true
Copy link
Contributor

Choose a reason for hiding this comment

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

should we make the default false since we comment out the values below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are commenting out access id, key and endpoint because they are required values with no defaults. When we reference them here , we use the required function so that if user does not override this value on helm install it will error out. For example:

$ helm install maisie-charts/sumologic --name collection --namespace sumologic --no-crd-hook
Error: render error in "sumologic/templates/setup-job.yaml": template: sumologic/templates/setup-job.yaml:28:14: executing "sumologic/templates/setup-job.yaml" at <required "A valid .Values.sumologic.endpoint entry required!" .Values.sumologic.endpoint>: error calling required: A valid .Values.sumologic.endpoint entry required!

The collector name is commented out because the script has the default value, so we don't need to specify it again here

@@ -253,7 +273,7 @@ prometheus-operator:
prometheusSpec:
externalLabels:
# Set this to a value to distinguish between different k8s clusters
cluster: kubernetes
cluster: *cluster_name
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this mean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is called an anchor used to reference a value in YAMLs. so *cluster_name will look for &cluster_name and take whatever value &cluster_name has

Copy link
Contributor Author

@maimaisie maimaisie Sep 4, 2019

Choose a reason for hiding this comment

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

found out this won't work if it's overriden in helm install, it still gets the default value kubernetes instead of the user-supplied value... I have remove the anchor from values.yaml. If user needs to use a different cluster name, they need to set two things

--set sumologic.clusterName="test-cluster" 
--set prometheus-operator.prometheus.prometheusSpec.externalLabels.cluster="test-cluster"

this is not ideal, but @frankreno do you think it's acceptable for GA?

deploy/helm/sumologic/values.yaml Outdated Show resolved Hide resolved
deploy/docker/setup/Dockerfile Outdated Show resolved Hide resolved
deploy/docker/setup/Dockerfile Outdated Show resolved Hide resolved
deploy/helm/sumologic/values.yaml Outdated Show resolved Hide resolved
@maimaisie
Copy link
Contributor Author

I think the CI has a bug, it's thinking I have manual changes in some generated files

Aborting due to manual changes detected in the following generated files: deploy/helm/prometheus-overrides.yaml
deploy/kubernetes/fluentd-sumologic.yaml.tmpl

{{- end }}
"-d", "false",
"-y", "false",
'{{ required "A valid .Values.sumologic.endpoint entry required!" .Values.sumologic.endpoint }}',
Copy link
Contributor

Choose a reason for hiding this comment

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

What are your thoughts on instead of using the full URL, we let the customer specify the deployment name (e.g. US2)? We then do a conditional block rendering the right URL based on what they selected and can fail if they make an invalid choice. This makes it easier for people, other various open-source Sumo projects work with this way. We have to occasionally update that as we add new deployments, but that seems minor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be more maintenance work and I think we generally try to avoid it because it's easily forgotten. What are your thoughts on this @lei-sumo @samjsong @rvmiller89 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with documenting it in our steps for now.

Copy link
Contributor

@samjsong samjsong left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Maisie!

&& apt-get install -y wget

# Install kubectl
ADD https://storage.googleapis.com/kubernetes-release/release/v1.6.12/bin/linux/amd64/kubectl /usr/local/bin/kubectl
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this version too low?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to 1.15.0 according to their doc page

@maimaisie maimaisie merged commit a781604 into master Sep 6, 2019
@maimaisie maimaisie deleted the maisie-pre-install branch September 6, 2019 18:31
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