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

SUMO-115354 Add dependencies with overridden values.yaml #132

Merged
merged 52 commits into from
Aug 22, 2019

Conversation

yuting-liu
Copy link
Contributor

@yuting-liu yuting-liu commented Aug 7, 2019

Add following dependencies for our fluentd plugin:

* fluent-bit
* prometheus-operator

For versions defined for these two dependencies in requirements.yaml, we used the latest release version ~2.4.0 for fluent-bit and ~6.2.0 for prometheus-operator. In slack conversation https://sumologic.slack.com/archives/GFJ6U4SCE/p1565109513061400, the version 6.2.0 is explicitly specified for use of --no-crd-hook.

@yuting-liu
Copy link
Contributor Author

yuting-liu commented Aug 7, 2019

For overridden values for fluent-bit and prometheus-operator, now we keep two copies for those values. What we did here in values.yaml is to copy-paste the text from fluent-bit-overrides.yaml and prometheus-operator-overrdes.yaml into values.yaml in following format:

fluent-bit:
  enabled: true
  ## copy-paste fluent-bit-overrides.yaml
  ...
prometheus-operator:
  enabled:true
  ## copy-paste prometheus-overrides.yaml
 ...

Since two copies of overridden values for our dependencies, there will be configuration drift between them. We need to discuss whether this drift is acceptable based on how frequently we are going to change overridden values for these two dependencies.

The reason why we cannot pass in XXX-overrides.yaml with helm intall -f <yaml-file> is that there is a leading key added before the section overridden values and also an extra indent before each line. The dependency's overridden values in values.yaml in helm is not of the same level as those in overrides.yaml file.
For example, here is the fluent-bit-overrides.yaml:

metrics:
  enabled: true
  serviceMonitor:
    enabled: true
    additionalLabels:
      release: prometheus-operator

When we defined values.yaml in helm with same overridden values, we should specify it in following structure:

fluent-bit
  ## condition defined in requirements.yaml
  enabled: true
  ## same as fluent-bit-overrides.yaml, but with an extra indent
  metrics:
  enabled: true
  serviceMonitor:
    enabled: true
    additionalLabels:
      release: prometheus-operator

There are some options we can do to reduce the configuration drift:

  1. Use CI to auto generate overrdes.yaml for fluent-bit and prometheus-operator from values.yaml file we provided in helm
  2. Use CI to append values from overrdes.yaml of our dependencies to values.yaml in helm

@rvmiller89
Copy link
Contributor

This will probably be merged while I'm on PTO, but I'd like to leave my 2 cents:

We should not copy-paste the same config in multiple files. We should try to find a way to avoid this, either through reading the Helm docs, or integrating this into our CI.

@yuting-liu
Copy link
Contributor Author

yuting-liu commented Aug 8, 2019

Will discuss offline about how to maintain the copies of overridden values for dependencies today.

dependencies:
- name: fluent-bit
repository: https://kubernetes-charts.storage.googleapis.com/
version: 2.4.4
Copy link
Contributor

Choose a reason for hiding this comment

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

how did you decide on these versions?

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 now what I decided. This file is autogenerated by running helm install. It get the version 2.4.4 from the version we specified for version: ~2.4.0 in requirements.yaml.
https://stackoverflow.com/questions/53702445/use-of-requirements-lock-file-in-helm-charts
As here it shows that how it works:

  1. After created initial requirements.yaml file. You run helm install and helm creates requirements.lock file as it builds the dependency tree.
  2. On the next helm install, helm will ensure that it uses the same versions identified in the requirements.lock file.
  3. At some later date, you update requirements.yaml file. You run helm install (or helm upgrade) and helm will notice your changes and update the requirements.lock file to reflect them.

Copy link
Contributor

Choose a reason for hiding this comment

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

how did you decide to use version: ~2.4.0 in the yaml file then? and is the auto-generated version 2.4.4 the latest version after version: ~2.4.0 or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

~2.4.0 is a patch-level version match (suggested by helm), which means >=2.4.0 and <2.5.0. And 2.4.4 is auto generated then. Here I used the latest patch level version officially provided by fluent-bit. Maybe we need to ask @samjsong to make sure whether there is some explicit requirements for fluent-bit version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't know of any explicit requirements for fluent bit version, I've just been using latest for now, I'm assuming this is fine and can be upgraded in the future if there are updates we want to include

@maimaisie
Copy link
Contributor

Sorry for the long commit history 😂 but this PR is now ready for review

ci/build.sh Outdated Show resolved Hide resolved
ci/build.sh Show resolved Hide resolved
dependencies:
- name: fluent-bit
repository: https://kubernetes-charts.storage.googleapis.com/
version: 2.4.4
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't know of any explicit requirements for fluent bit version, I've just been using latest for now, I'm assuming this is fine and can be upgraded in the future if there are updates we want to include

@maimaisie maimaisie merged commit 9427d31 into master Aug 22, 2019
@maimaisie maimaisie deleted the yuting-helm-dependency branch August 22, 2019 18:06
psaia pushed a commit to psaia/sumologic-kubernetes-collection that referenced this pull request May 25, 2021
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.

None yet

4 participants