Refactor Feast Helm charts for better end user install experience

[davidheryanto]

What this PR does / why we need it:

1. Use --spring.config.additional-location to specify Spring application config
   so the default application.yaml bundled in the jar can be used, and users
   only need to override required config. This is to prevent chart users being
   overwhelmed by the amount of config to set.

2. Use less templating, if else, and functions in configmap.yaml in Feast
   charts. Instead set a reasonable default and allow users to override
   in application-override.yaml. This makes it easier to manage when
   application.yaml structure changes and documentation for application.yaml
   can be delegated to:
   https://github.com/gojek/feast/blob/master/core/src/main/resources/application.yml
   https://github.com/gojek/feast/blob/master/serving/src/main/resources/application.yml

3. Remove double nesting of charts. Previously, Postgresql and Kafka subcharts
   are bundled in Feast Core suchart. Now all 3 subcharts are sibling charts.
   This reduces coupling between charts and it's easier to configure chart
   values one at a time than one big values yaml. Also, documentation for
   third party subcharts can be delegated to the original authors.

4. Use helm-docs to generate README for Feast charts so the documentation
   is reproducible and easier to keep up to date when values.yaml changes.
   It is also easier to follow best practices when writing values.yaml.
   https://github.com/norwoodj/helm-docs

5. Add prometheus and grafana charts to support out of the box metrics
   collector and visualization when users install Feast with this chart.

6. Add tests for users to verify installation:

7. Add examples to install Feast with 3 different configurations (from simple to complex):

   • online serving only with direct runner
   • online and batch serving with DirectRunner
   • online and batch serving with DataflowRunner

To check how the Chart documentation will look like
https://github.com/davidheryanto/feast/tree/helm-chart-v0.4-refactor/infra/charts/feast

Which issue(s) this PR fixes:

NONE

Does this PR introduce a user-facing change?:

Yes, the format of Helm values passed during installation has changed, from nested to flatter structure. But in general, the accepted values in each chart are the same.

[feast-ci-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidheryanto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [davidheryanto]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[davidheryanto]

/retest

[Yanson]

Some of this is really great, the different configuration locations and documentation around it is a mess.

However, this PR introduces a lot of breaking changes without workarounds.

Use --spring.config.additional-location to specify Spring application config
so the default application.yaml bundled in the jar can be used, and users
only need to override required config.

Additional spring configs are already supported through profiles which can be toggled on and off. This PR forces all the additional files with hard-coded contents are used.

If you want to use the default, you should use something like this instead of spring.config.additional-location:

-        - "--spring.config.location=file:{{ .Values.springConfigMountPath }}/"	
+        - "--spring.config.location=classpath:/application.yaml,file:{{ .Values.springConfigMountPath }}/"	

This will continue to allow profiles and the files to be used. For example, an additional use-case is to provide a custom profile with complete database settings that point to a managed service.

Use less templating, if else, and functions in configmap.yaml in Feast
charts. Instead set a reasonable default and allow users to override
in application-override.yaml.

Setting defaults for non-deployed services is undesirable and leaves confusion over what needs to be overridden and what will take precedence.

Remove double nesting of charts. Previously, Postgresql and Kafka subcharts
are bundled in Feast Core suchart. Now all 3 subcharts are sibling charts.

I believe part of the reason for this is to check wither the dependencies are enabled and get access to their configuration. Using siblings makes assumptions about configuration and does not allow you to check which are enabled.

Other observations:

SPRING_DATASOURCE_PASSWORD is enforced and requires a pre-existing secret. Imagine you want to encrypt your DB password in the values file (e.g. in a profile), this is no longer possible.

jvmOptions moved to javaOpts, removed from command and defined as environmental JAVA_OPTS. The java start command does not make use of the environmental variable and hence options will be ignored.

Server port is hardcoded.

jarPath is no longer supported.

Overall, I think this PR makes an assumption that the Feast chart will deploy all dependant services and tightens the coupling. This is very unlikely to be the case for [shared or managed] services like Kafka and Postgres.

[davidheryanto]

Hi @Yanson thanks for the feedback.

My main motivation for updating the Helm chart is to simplify the installation steps (may be at the expense of less configurability, and a few breaking changes) so it is easier for users to get started. And to consolidate the documentation so there is mostly one source of truth (for example rather than documenting application.yaml in the Helm chart, I refer to the application.yaml in Feast Core resources folder, where it is better documented and is always up-to-date)

This PR forces all the additional files with hard-coded contents are used.

Maybe I'm missing something but I don't see how this PR forces the additional files with hard-coded contents. This PR provides user with one and only way to override config, via application-override.yaml. I'm following the approach used by Kafka chart https://github.com/helm/charts/blob/master/incubator/kafka/values.yaml#L173

However, some of the defaults are modified so it suits Helm installation better. For instance the default spring.datasource.url in https://github.com/gojek/feast/blob/master/core/src/main/resources/application.yml is pointing to Postgresql in localhost:5432. The default is modified to release-name-postgresql:5432/postgres. This is to support first time users who just want to try install Feast with helm install --name myrelease feast.

So this chart modifies the default configuration (which is usually not valid anyway for Feast installation in Kube cluster), so it supports the most minimal installation option. Users can and are advised to override this default via application-override.yaml if they need to.

I understand that this Helm chart now disables the use of providing configuration via Spring profiles. But again my reasoning is to simplify configuration so users only use one application-override.yaml to specify options.

With profiles support, users need to somewhat process how these profiles are eventually gonna end up in the final config. And whether the configuration provided via profiles or those provided via application.yaml are going to be used. This requires users to also have a bit of understanding how Spring profiles work.

Is using (perhaps multiple) profiles compared to a single application yaml is a better practice and preferred approach? If yes, we should add again the profiles support.

Setting defaults for non-deployed services is undesirable and leaves confusion over what needs to be overridden and what will take precedence.

I think this depend on the application. For Feast Core e.g dependencies on Kafka and Postgresql service is required. The default is to faciliate easier installation if user is to install these dependencies using default config. If not, they can set their own setting via application-override.yaml. I think the naming of this field makes it obvious that it takes precedence. And they can check against the default to see whether or not they need to override the value.

I believe part of the reason for using nested charts is to check whether the dependencies are enabled and get access to their configuration. Using siblings makes assumptions about configuration and does not allow you to check which are enabled.

Yes at first, this is what we thought. But after some experiences with using this approach, I feel the subscharts approach, without nesting feels more flexible. For instance, sometimes we want to migrate, backup or update the dependent services e.g. Kafka, Postgres or Redis deployments. With them being a child subcharts of Feast Core and Serving, migrating may involve changes to the parent Feast Core and Serving parent, and may accidentally update Feast Core and Serving (which we do not want esp if they are serving production traffic). With these dependent services being separate subcharts, updates to these are more independent. This seems to be the approach taken by istio too https://github.com/istio/istio/tree/release-1.5/install/kubernetes/helm/istio. Also making these dependent services sibling vs child subcharts, allow us to more easily upgrade their Chart version. With nested approach there may be some dependencies created between the parent and child.

SPRING_DATASOURCE_PASSWORD is enforced and requires a pre-existing secret. Imagine you want to encrypt your DB password in the values file (e.g. in a profile), this is no longer possible.

This is to encourage best practice to prevent the setting of plaintext password in the application.yaml, by enforcing the use of secret for saving passwords. If users encrypt the DB in values file, in profile, I'm sure there are other configs that they will need to provide as well to decrypt this password etc. I'm not sure what common approaches there are but we can always extend this although I think it will make configuration more complex.

jvmOptions moved to javaOpts, removed from command and defined as environmental JAVA_OPTS. The java start command does not make use of the environmental variable and hence options will be ignored.

JAVA_OPTS is a commonly used environment variable used by JVM unless maybe they have changed it?
https://github.com/helm/charts/blob/master/stable/metabase/templates/deployment.yaml#L102
https://github.com/helm/charts/blob/master/stable/jenkins/templates/jenkins-master-deployment.yaml#L182

I changed the name from jvmOptions to javaOpts to make it more consistent with other Helm charts in stable repository.

It is specified in env variable versus command to make the command simpler.

jarPath is no longer supported.

I think this use case is quite unusual. Unless users build custom Feast images, but if they do, they can always build it such that the feast jar is at the correct location. I checked other charts in stable repo. Most does not provide this option too:
https://github.com/helm/charts/blob/master/stable/jenkins/templates/jenkins-master-deployment.yaml#L101
https://github.com/helm/charts/blob/master/stable/spinnaker/templates/statefulsets/halyard.yaml#L35

I feel removing options that are rarely useful and used ease the burden of people configuring the installation.

Overall, I think this PR makes an assumption that the Feast chart will deploy all dependant services and tightens the coupling. This is very unlikely to be the case for [shared or managed] services like Kafka and Postgres.

The assumption is true only if users install this chart with the default enabled options. Users can completely disable Postgres,Redis and Kafka installation in this chart. And assuming they have these installed externally, they can set the config to connect to these external services in application-override.yaml

I'm open to recommendation and fixes. But current PR is biased towards favouring simplicity (in the sense of one single source of truth) over configurability (various ways to provide config in multiple places). But maybe the latter is the preferred one?

[woop]

@Yanson I am reviewing this PR now. I want to make sure we address any concerns you have. I much prefer the simplicity that this PR brings, especially because I am working on a refactor as part of #525. It can be hard for us to keep up with maintaining documentation and configuration, as you might have noticed. We're trying to reduce the surface area of configuration as much as possible to decrease the burden on our engineers, while at the same time hopefully providing escape hatches for you to customize it.

I still have a few open comments on the discussion

Spring Profiles:
I am not 100% sure if it is necessary to support additional Spring profiles. Is there anything that can be done with additional profiles that can't be done with the application-override.yml?

In the ideal case a user of Feast shouldn't know that Spring is being used at the back. Having this exposed at the Helm level seems unnecessary to me, at least at the level of values.yaml

We also have a PR (#493) from @pmjacinto that adds Kube secrets that map to spring profiles. @davidheryanto, how would you propose we handle those?

Would we do something like this?

• application-default.yaml
• application-default-secret.yaml
• application-default-override.yaml

Coupling:
You mentioned that we are increasing coupling with this PR. It seems like we are moving to more modularized component dependencies. Can you explain what you meant by that?

Setting default for services:
I think it makes sense for a values.yaml to contain defaults since this makes installation simpler and it is a known user facing API. They know they have to modify it. My concern would be more in terms of any default configuration set inside the chart itself. I agree with both of you here. For Kafka and Postgres it makes sense to have defaults in place, but we don't want to have magical defaults that are hard for users to see and configure, especially if there is a high likelihood that they would want to change it.

The implementation here for the Feast Core Configmap contains 3 profiles. It's not clear why we need three profiles there. Also I agree that the configuration would ideally not be hardcoded inside the chart. Especially not ports

It seems like the biggest drawback with @davidheryanto's approach is that if the user is tasked with dumping all of their configuration in an override yaml, then they don't benefit from any config generation. A good example is the following snippet that would need to be hardcoded by the users.

bootstrapServers: {{ printf "%s:9092" (printf "%s-kafka" .Release.Name)

It would be highly appreciated if you could enumerate any pains that remain, given @davidheryanto's response.

[Yanson]

There is a lot going on in this MR which makes it difficult to discuss the changes as there are multiple threads. I know it's taken me a week to reply but it is high on my agenda so please can we all be on the same page before moving forward.

Profiles

I don't see how this PR forces the additional files with hard-coded contents

What I am referring to is the list of files specified in spring.config.additional-location; default-postgresql, default-kafka, default-statsd. These files are a. created, and b. specified as configuration, regardless of whether those services are deployed or not.

The profiles approach is incredibly similar to this, only that it lets the non-required files be omitted from the specified configuration. I found it a tidy way to separate concerns without exposing Spring fundamentals it to the user. The additional option to define and use arbitrary profiles is overkill for what I wanted to accomplish.

If the same goal can be accomplished (which it can) with application-override.yml and application-secret.yml, I am all for that 👍

Defaults

One major point of concern here, is there are multiple default values making it difficult to understand what the default is, which needs to be specified and which need to be overrided. 1/ resources/application.yml, 2/ templates/configmap.yaml (some of which are calculated), 3 values.yaml, 4 environment / pre-existing secret.

For example, feast.stream.options.topic is feast-features in resources/application.yml but feast in configmap.yaml. If we are saying that resources/application.yml should be where the defaults live, I might think "I am happy with the topic but I need to change the broker to kafka.mycompany.com". So I go to values.yaml, find the application-override.yml and specify the setting (bootstrapServers). Unbeknownst to me, the topic has been modified by application-default-kafka.yaml, the definition of which is in templates/configmap.yaml and on top of this, it's for a service I didn't deploy.

Pre-existing secrets

Dependency on pre-existing secrets removes some simplicity from the goal of just being able to deploy the chart with defaults. It forces the user into a deployment strategy they might not want (not deploying everything as code).

Specifically on SPRING_DATASOURCE_PASSWORD - this is an override on top of all the other places we might define the value. resources/application.yml defaults to password but falls back to DB_PASSWORD env. Specifying it myself in application-override.yml (or application-secret.yml if we make that) doesn't work. I NEED to make the secret with a given key (postgresql-password) defined in deployment.yaml.

Minor points

jarPath is no longer supported.

We create our own image with different conventions which is why I added support for that. You're right we can put it at the "correct" location, according to Feast. So probably not worth the overhead of supporting configuration for this.

I changed the name from jvmOptions to javaOpts to make it more consistent

I don't mind what it's called as long as the upgrade guide explains that it changed.

JAVA_OPTS is a commonly used environment variable

This variable needs to be read by something and passed to the java command. In the charts you link, they have an entrypoint script which does that. In this chart, there is no script - the raw command is specified in deployment.yaml.

Sorry I don't have time to go into any further depth on any point today, but hopefully I've covered most of the points.

[woop]

it is high on my agenda so please can we all be on the same page before moving forward.
Thanks for taking the time to respond @Yanson. I'm happy to take some time to get this right. As you have rightly pointed out, there are many things going on in this PR which makes it a little bit harder to review in one go.

Let me review all of the comments again and see if I can come up with a list of changes we agree to and ones that are outstanding.

[woop]

this looks like a good way to move forward.

I think the cleanest thing to do is hard code everything in application-override.yml, i.e. that is a property of values.yaml and there is no interpolation of release name (etc). Given this is only for getting started quickly, we can assume the release name, port selection etc.

Great. I think its a safe first start that doesn't prevent us from adding more flexible options in the future.

I didn't know about _JAVA_OPTIONS but having dug a little into it, it seems JAVA_TOOL_OPTIONS is more suited: https://bugs.openjdk.java.net/browse/JDK-4971166

Good catch. Let's use JAVA_TOOL_OPTIONS

[davidheryanto]

The chart has been updated so it will work with the current application.yaml format in master (to be v0.5)

Also these modifications that we discuss:

• Allow users to set application-secret.yaml in Helm values for Feast Core and Serving. This works similar to application-override.yaml but will be created as a Secret and has a lower precedence than application-override.yaml
• javaOpts Helm values will become the environment variable value for JAVA_TOOL_OPTIONS
• Add envOverrides to support arbitrary setting of env variables e.g. when application-override.yaml is not sufficient
• feast-core.existingSecret is now optional. However it is recommended to use this approach so it will work seamlessly with the Postgresql chart. Also as a result, of making this optional, the default installation for getting started is modified from

helm install --name myrelease feast-charts/feast

to

helm install --name myrelease feast-charts/feast \
    --set feast-core.postgresql.existingSecret=feast-postgresql \
    --set postgresql.existingSecret=feast-postgresql

[Yanson]

Thanks, I will see if I can try out this version today or tomorrow to validate it.

[woop]

Thanks @davidheryanto, the changes look good. I especially like the level of detail in the README.md

One thing that stands out though. Previously we wanted to do this:

For Helm: I’d recommend moving all the existing default configurations (from Config Maps or other places) into application-override.yml key, and document the fact that this overrides or merges with resources/application.yml.

This suggestion was so that we didn't have hardcoded application.yml files in the configmaps as well as at the jar path

--spring.config.additional-location=/etc/feast/application-default-postgresql.yaml,/etc/feast/application-default-kafka.yaml,/etc/feast/application-default-statsd.yaml,/etc/secrets/feast/application-secret.yaml,/etc/feast/application-override.yaml

So basically we just need something like (no kafka, postgres, statsd)

--spring.config.additional-location=/etc/secrets/feast/application-secret.yaml,/etc/feast/application-override.yaml

Otherwise the chart is too hardcoded as @Yanson said. I mean it feels like we are stuck between two worlds. Either we define everything in values.yaml and we have to do more work in hardcoding it, or we generate the configuration with templates from variables.

[Yanson]

Hi @davidheryanto, thank you for the changes. It's looking very clean now.

Just a few minor things to note:

Feast Core

Only items apply to both, see further down.

Feast Serving

feast-serving/templates/_helpers.tpl

• bq_store_and_no_job_options no longer used

Both

templates/deployment.yaml

prometheus.io/port: "8080"

Other places in the file use {{ .Values.service.http.targetPort }}

Note, the above value needs to match the Spring Boot setting (which is currently defaulting to 8080). We should document how to change the Spring setting and that they need to be aligned.

The same goes for service.grpc.targetPort

GOOGLE_CLOUD_PROJECT=.Values.gcpProjectId

Missing documentation in values.yaml.

template.metadata.annotations

Checksum annotations are missing to force redeploy on config changes.

src/main/resources/application.yml

• It might be worth removing the env placeholders with fallbacks as that is another point of confusion. e.g. username: ${DB_USERNAME:postgres}. These can be overiddedn with env anyway.
• Perhaps rename to .yaml to match all the other config names.

Third party

Discussed with @woop it would be cleaner to remove the third party charts and document running helm dependency udpate, OR just include the tgz.

Not sure if prometheus-statsd-exporter is packaged anywhere, if not you can use our vanilla-service with these settings (which is what we do):

helm repo add konnekt-public "https://konnekt-public-charts.storage.googleapis.com"

helm install statsd-exporter konnekt-public/vanilla-service --version 0.19.0 -f values.yaml

Where values.yaml is:

image:
  repository: prom/statsd-exporter
  tag: v0.12.2

resources:
  limits:
    cpu: 250m
    memory: 256Mi
  requests:
    cpu: 25m
    memory: 64Mi

container:
  args:
  - --log.format=logger:stdout?json=true
  port: 9125
  portName: statsd
  portProtocol: UDP
  readinessProbeAction: tcpSocket
  livenessProbeAction: tcpSocket

# Enable for a GCP internal load balancer
# ilb:
#  enabled: true

service:
  port: 9125

ingress:
  enabled: false

prometheus:
  enabled: true
  path: /metrics
  port: 9102

[davidheryanto]

Thanks @Yanson for the feedback.

• Removed bq_store_and_no_job_options from feast-serving/templates/_helpers.tpl

• Set prometheus.io/port to {{ .Values.service.http.targetPort }}.

  Regarding documentation on how to change this from the Spring config, I think we should leave it to another issue or PR: to document the application.yml on how to change the HTTP port. Similarly, regarding this part username: ${DB_USERNAME:postgres} and yml vs yaml extensions. So that this PR will not have unattended side effects and
  is mainly about deployment of Feast with Helm charts using the
  existing Spring config.

• For .Values.gcpProjectId actually I was thinking if this value is necessary? Since in order to configure the Dataflow runner, we already require the person configuring Feast deployment to specify the project ID. Similarly, for BigQuery as the Serving store and Deadletter store.

  Given this, it may generate confusion to users regarding which project is actually being used, if we also provide this option .Values.gcpProjectId

• Added the checksums in the deployment template spec so changes in the
  configmap and secret will trigger rolling updates of Feast Core and Serving.

• For 3rd party charts, I actually follow the approach taken by istio https://github.com/istio/istio/tree/1.5.0/install/kubernetes/helm/istio to include the chart source files directly as opposed to include the packaged .tgz file. Mainly because we may want to include additional files and modify slightly the defaults in these 3rd party charts:

  • If Feast is to pre-include Feast specific Grafana dashboard JSON (such as charts showing ingestion lags and feature statistics) and automatically configure it with the prometheus datasource if installed together in this chart, I feel it makes sense to have the 3rd party Chart in the source files format. So that these extra dashboard JSON and datasource configuration can be more easily configured.
  • Some of the defaults in the 3rd party charts can be modifed so it fits Feast defaults better. E.g. the prometheus charts by default will install the push gateway which currently Feast does not use at all. We can disable this default from top-level values.yaml or setting it in the values.yaml in the prometheus chart. The latter allows the top level values.yaml (which is closer to the end-user) to be cleaner because it will only consists of enable: true/false toggle.

  Of course the drawback is upgrading these 3rd party charts will require more effort than just simply increasing the version number in requirements.yaml. But I don't foresee we need to be doing such upgrades frequently.

[Yanson]

Thanks for addressing.

• Happy for docs to be handled in a separate PR.

• gcpProjectId this is definitely needed by us. It shouldn't be and took a while to debug, but I found one of the google SDKs was not correctly selecting the project from the JSON key and using the metadata server (project it's running in). Hence why I added it. It might be difficult to explain where/when it's used.

• Will need a bit of time to verify the new changes.

• I don't agree about the 3rd party charts. It makes it very difficult to understand what is Feast code and what is imported. Regarding changing or adding files, this makes things even worse, as you say for upgrading and comparing what we might have to re-implement with their new version. We should "modify" (customise) through the values options they expose. It's not a show-stopper for me, I just think it inflates the codebase unnecessarily. We'll be disabling them all and deploying separately anyway 😄. I'll let you and @woop decide what to do.

[woop]

@davidheryanto Thanks for being so responsive here.

On the topic of expanded charts. I don't find the current points super convincing.

• Grafana dashboards: I am not sure I follow how having an expanded chart here helps. Can we get away with not including that for now?
• Defaults: If its only for defaults, then I dont see a big need to do this.

It seems to go against the idea of reduced management for the Feast team if we are expanding the charts.

one of the google SDKs was not correctly selecting the project from the JSON key

@Yanson It's funny because I am also running into this problem now.

Could this be the issue?
https://github.com/gojek/feast/blob/v0.4-branch/ingestion/src/main/java/feast/ingestion/utils/StoreUtil.java#L115

Solution: https://stackoverflow.com/questions/45246449/passing-service-account-credentialjson-file-programmatically-for-bigquery-serv

I really want to remove GCP specific configuration from charts. I think we should try to limit that to values.yaml examples for each different deployment type. Can we use envOverrides for this parameter for now. I will try to solve this on master.

[davidheryanto]

Ok I removed the 3rd party charts from the charts folder.
I'm ok either way with the gcpProjectId configuration.

[woop]

Thanks @davidheryanto.

Ok I removed the 3rd party charts from the charts folder.
I'm ok either way with the gcpProjectId configuration.

Think the following is still pending:

• I think we need to add gcpProjectId into the documentation. That is just code gen right?
• Out of date configuration for stores in the examples.

Almost done!

[davidheryanto]

@woop Oh yes, I forgot to update the values-*.yaml example files. It's now updated to Feast v0.5 config structure. I also added Helm values doc for .Values.gcpProjectId

[woop]

@woop Oh yes, I forgot to update the values-*.yaml example files. It's now updated to Feast v0.5 config structure. I also added Helm values doc for .Values.gcpProjectId

Thank you @davidheryanto. Really appreciate the effort on this one. I am happy to merge.

@Yanson We've probably missed some small details here and there since the surface area of these charts is pretty massive. I think we should get this in and create a new issue if we need any more changes.

[woop]

/lgtm