bitnami/dataplatform-bp1 addition of observability framework #5602

gkulkarni32 · 2021-02-24T08:10:58Z

Description of the change

Addition of wavefront observability framework for the dataplatform
Addition of the values.schema.json for the form layout in kubeapps

Benefits

we will be able to provide observability framework to the users
we will be able to provide a form layout in kubeapps portal

Possible drawbacks

Applicable issues

fixes #

Additional information

Checklist

Chart version bumped in Chart.yaml according to semver.
Variables are documented in the README.md
Title of the PR starts with chart name (e.g. [bitnami/chart])

⚠️ Keep in mind that if you want to make changes to the kubeapps chart, please implement them in the kubeapps repository. This is only a synchronized mirror.

marcosbc

Thanks for the contribution! It mostly looks good. However, I'm requesting @javsalgar to check a couple of things and also to have another check of this PR in case I missed anything.

bitnami/dataplatform-bp1/values.yaml

miguelaeh

Thank you for the PR!
Please check the comments

bitnami/dataplatform-bp1/Chart.yaml

bitnami/dataplatform-bp1/values.schema.json

bitnami/dataplatform-bp1/values.yaml

carrodher

Thanks for the contribution, I just added some comments mainly related to some generic files like Chart.yaml, deps and README

bitnami/dataplatform-bp1/Chart.yaml

carrodher · 2021-02-24T15:17:45Z

bitnami/dataplatform-bp1/Chart.yaml

+ - https://github.com/bitnami/bitnami-docker-wavefront-proxy
+ - https://github.com/wavefrontHQ/wavefront-collector-for-kubernetes
+ - https://github.com/wavefrontHQ/wavefront-proxy
+version: 1.0.0


Is backward compatibility guaranteed? If the changes in this PR are not going to break the upgrades from the previous version, we only need to bump the chart version in a minor instead of a major

Suggested change

version: 1.0.0

version: 0.2.0

Per the above explanation, this field should be updated using semver, in this case as it is a feature addition and not a backward incompatibility we should bump the version in a minor.

Updated the version as per the comments. Chart version now is 0.2.0 and appVersion is 1.0.0.

carrodher · 2021-02-24T15:26:57Z

bitnami/dataplatform-bp1/Chart.yaml

@@ -1,7 +1,7 @@
 annotations:
 category: Infrastructure
 apiVersion: v2
-appVersion: 1.0.0
+appVersion: 2.0.0


As there is no standalone application behind this chart, let's use the same app version as the one used as the chart version (see below comment)

Suggested change

appVersion: 2.0.0

appVersion: 0.2.0

In the first release, the appVersion was 1.0.0 and the chart version was 0.1.0.
According to us, this is a major release for the data platform chart, hence we went with 2.0.0 and 1.0.0.

The appVersion is not used by Helm to generate the package tarball, it is considered as metadata. This version points to the version of the bundled application in the Helm Chart. For example, in the PostgreSQL Helm chart, this field is appVersion: 11.11.0 which is the version of the PostgreSQL container used by the Helm Chart.

Then we have the version field which is the version of the chart itself (nothing related to the application inside the chart). This version should be modified every time the chart is modified (changes in the values.yaml, notes.txt, k8s manifests, etc). This version follows semver. According to semver, the patch version is bumped when the changes are fixing bugs, the minor version when some feature is added and the major version when the backward compatibility is not guaranteed. +info: https://helm.sh/docs/topics/charts/#charts-and-versioning

The above explanation is how the version works in Helm as a general rule. In the case of this dataplatform chart, there are some exceptions because there is not any main component bundled in the chart to be used to set the appVersion according to it, we can say this chart is not bundling an application so we can consider the chart itself as the product/application, that is why we are suggesting to use the same version in the appVersion and version field.

In the rest of the charts, the appVersion is set according to the upstream maintainers, I mean, if PostgreSQL releases a new version such as 12.4.5 and we update the chart to use it, the appVersion is going to reflect this change.

The version is not metadata, this field is used to package the chart and it is important to follow the semver directives

@miguelaeh maybe you have more context about this topic and I'm missing something, what do you think the relationship between appVersion and version should be?

I have made the changes to the versions, please have a look.

Thanks for the change, now the version was bumped in a minor what is fine, but the appVersion field is still pointing to 1.0.0 that was the original value (probably wrong in the first iteration of the Helm chart).

The topic is: version is going to be updated at the moment in which there is any change in the chart (even a minor change in a comment), but what about appVersion? The purpose of this field is to serve as metadata to show what is the version used by the bundled application, but in this case, there is not a single application. What is your plan to bump appVersion? Where should we look to detect that there is a new version? What we usually do in Bitnami in this kind of chart where there is not a single app is set the same value used in version and bump both values at the same time.

Hi @carrodher Considering we had 1.0.0 as the appversion in the first one, we can let that be.
The idea is to have different sizes of the data platform clusters.
Example: Current appversion 1.0.0 is related to 'Small' data platform cluster. Next major version i.e. appversion 2.0.0 will be when we add the 'Medium'/'Large' data platform cluster.
Hope this clarifies.

Ok, as it is something that you are going to handle due to the lack of application when the chart evolves

bitnami/dataplatform-bp1/README.md

Co-authored-by: Marcos Bjoerkelund <[email protected]>

Co-authored-by: Carlos Rodríguez Hernández <[email protected]>

marcosbc · 2021-02-25T16:05:08Z

Added a request to remove the templating from metrics.enabled keys, as it would not be rendered properly in the subchart.

marcosbc

LGTM! I would also like to get a review from another team member before merging.

bitnami/dataplatform-bp1/README.md

miguelaeh · 2021-03-02T15:46:29Z

bitnami/dataplatform-bp1/values.yaml

+ masterAnnotations:
+ prometheus.io/scrape: 'true'
+ prometheus.io/path: '/metrics/'
+ prometheus.io/port: '{{ .Values.master.webPort }}'


I think this will not be templatized in the subchart so we should use the port value.

Thanks for pointing this out. I updated the port to {{ .values.spark.master.webPort}}

miguelaeh · 2021-03-02T15:46:35Z

bitnami/dataplatform-bp1/values.yaml

+ workerAnnotations:
+ prometheus.io/scrape: 'true'
+ prometheus.io/path: '/metrics/'
+ prometheus.io/port: '{{ .Values.worker.webPort }}'


Thanks for pointing this out. I updated the port to {{ .values.spark.worker.webPort}}

Co-authored-by: Miguel Ángel Cabrera Miñagorri <[email protected]>

miguelaeh

LGTM!

carrodher

Last request on my side, can you run helm dep update and add the Chart.lock file to the PR? Once done, we will be able to merge the PR

gkulkarni32 · 2021-03-03T13:31:24Z

Updated Chart.lock file, Please check from your side.

…#5602) * updated metrics and values schema json * updated chart version * Update bitnami/dataplatform-bp1/values.yaml Co-authored-by: Marcos Bjoerkelund <[email protected]> * Update bitnami/dataplatform-bp1/Chart.yaml Co-authored-by: Carlos Rodríguez Hernández <[email protected]> * Update bitnami/dataplatform-bp1/Chart.yaml Co-authored-by: Carlos Rodríguez Hernández <[email protected]> * fixed typo and added wavefront in README * updated schema json * updated version * updated solr exporter parameter * Update bitnami/dataplatform-bp1/README.md Co-authored-by: Miguel Ángel Cabrera Miñagorri <[email protected]> * updated spark master/worker annotation * updated spark ports * updated Chart.lock file * Remove trailing spaces Co-authored-by: Marcos Bjoerkelund <[email protected]> Co-authored-by: Carlos Rodríguez Hernández <[email protected]> Co-authored-by: Miguel Ángel Cabrera Miñagorri <[email protected]>

gkulkarni32 added 2 commits February 24, 2021 13:15

updated metrics and values schema json

9a2839e

updated chart version

bc15153

marcosbc reviewed Feb 24, 2021

View reviewed changes

bitnami/dataplatform-bp1/values.yaml Outdated Show resolved Hide resolved

bitnami/dataplatform-bp1/values.yaml Show resolved Hide resolved

bitnami/dataplatform-bp1/values.yaml Outdated Show resolved Hide resolved

miguelaeh suggested changes Feb 24, 2021

View reviewed changes

carrodher reviewed Feb 24, 2021

View reviewed changes

gkulkarni32 and others added 5 commits February 25, 2021 11:36

Update bitnami/dataplatform-bp1/values.yaml

7d10be5

Co-authored-by: Marcos Bjoerkelund <[email protected]>

Update bitnami/dataplatform-bp1/Chart.yaml

2345c7a

Co-authored-by: Carlos Rodríguez Hernández <[email protected]>

Update bitnami/dataplatform-bp1/Chart.yaml

2d80c7a

Co-authored-by: Carlos Rodríguez Hernández <[email protected]>

fixed typo and added wavefront in README

9b99238

updated schema json

9a57ae3

gkulkarni32 added 2 commits February 26, 2021 12:04

updated version

bfdfd91

updated solr exporter parameter

2fd4145

marcosbc approved these changes Feb 26, 2021

View reviewed changes

gkulkarni32 requested a review from miguelaeh March 2, 2021 10:43

miguelaeh suggested changes Mar 2, 2021

View reviewed changes

gkulkarni32 and others added 3 commits March 3, 2021 07:45

Update bitnami/dataplatform-bp1/README.md

2b95421

Co-authored-by: Miguel Ángel Cabrera Miñagorri <[email protected]>

updated spark master/worker annotation

4c34fa9

updated spark ports

9c19221

miguelaeh approved these changes Mar 3, 2021

View reviewed changes

carrodher approved these changes Mar 3, 2021

View reviewed changes

updated Chart.lock file

f9597fa

Remove trailing spaces

865b7f8

carrodher merged commit 3bbab7d into bitnami:master Mar 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bitnami/dataplatform-bp1 addition of observability framework #5602

bitnami/dataplatform-bp1 addition of observability framework #5602

gkulkarni32 commented Feb 24, 2021

marcosbc left a comment

miguelaeh left a comment

carrodher left a comment

carrodher Feb 24, 2021

carrodher Feb 25, 2021

gkulkarni32 Feb 26, 2021

carrodher Feb 24, 2021

gkulkarni32 Feb 25, 2021

carrodher Feb 25, 2021

carrodher Feb 28, 2021

gkulkarni32 Mar 1, 2021

carrodher Mar 2, 2021

gkulkarni32 Mar 2, 2021

carrodher Mar 3, 2021

marcosbc commented Feb 25, 2021

marcosbc left a comment

miguelaeh Mar 2, 2021

gkulkarni32 Mar 3, 2021

miguelaeh Mar 2, 2021

gkulkarni32 Mar 3, 2021

miguelaeh left a comment

carrodher left a comment

gkulkarni32 commented Mar 3, 2021

bitnami/dataplatform-bp1 addition of observability framework #5602

bitnami/dataplatform-bp1 addition of observability framework #5602

Conversation

gkulkarni32 commented Feb 24, 2021

marcosbc left a comment

Choose a reason for hiding this comment

miguelaeh left a comment

Choose a reason for hiding this comment

carrodher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcosbc commented Feb 25, 2021

marcosbc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelaeh left a comment

Choose a reason for hiding this comment

carrodher left a comment

Choose a reason for hiding this comment

gkulkarni32 commented Mar 3, 2021