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

[K8S][HELM] Implement new configuration approach #6521

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dnskr
Copy link
Contributor

@dnskr dnskr commented Jul 2, 2024

🔍 Description

Issue References 🔗

This pull request changes Helm chart configuration approach as discussed in #6123

Describe Your Solution 🔧

Suggested implementation makes chart configuration more general and more flexible. It allows to configure Kyuubi (and its engines) by setting configuration file contents to chart properties or by providing configuration files through existing ConfigMaps and Secrets. Also users are not limited by predefined number of files and can put any files to configuration directories.

Types of changes 🔖

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

1. Test suite "Kyuubi configuration"

Property file values-kyuubi-files.yaml

kyuubiConf:
  dir: /opt/kyuubi/conf
  files:
    'kyuubi-env.sh': |
      #!/usr/bin/env bash
      export KYUUBI_TEST=true
    'kyuubi-custom.properties': |
      kyuubi.custom=true
  filesFrom:
    - configMap:
        name: kyuubi-configs

ConfigMap kyuubi-configs from configmap-kyuubi-configs.yaml

apiVersion: v1
kind: ConfigMap
metadata:
  name: kyuubi-configs
data:
  'kyuubi-test.properties': |
    kyuubi.config.test=true

Rendered templates are correct

$ helm template charts/kyuubi -f values-kyuubi-files.yaml -s templates/kyuubi-configmap.yaml -s templates/kyuubi-statefulset.yaml

Configuration files are in place

$ kubectl create -f configmap-kyuubi-configs.yaml
$ helm install kyuubi charts/kyuubi -f values-kyuubi-files.yaml
$ kubectl exec kyuubi-0 -- ls conf
kyuubi-custom.properties
kyuubi-env.sh
kyuubi-test.properties

2. Test suite "Spark configuration"

Property file values-spark-files.yaml

sparkConf:
  dir: /opt/spark/conf
  files:
    'spark-env.sh': |
      #!/usr/bin/env bash
      export SPARK_TEST=true
    'spark-custom.properties': |
      spark.custom=true
  filesFrom:
    - configMap:
        name: spark-configs

ConfigMap spark-configs from configmap-spark-configs.yaml

apiVersion: v1
kind: ConfigMap
metadata:
  name: spark-configs
data:
  'spark-test.properties': |
    spark.config.test=true

Rendered templates are correct

$ helm template charts/kyuubi -f values-spark-files.yaml -s templates/kyuubi-spark-configmap.yaml -s templates/kyuubi-statefulset.yaml

Configuration files are in place

$ kubectl create -f configmap-spark-configs.yaml
$ helm install kyuubi charts/kyuubi -f values-spark-files.yaml
$ kubectl exec kyuubi-0 -- ls ../spark/conf
spark-custom.properties
spark-env.sh
spark-test.properties
  1. Test suite "Custom kyuubi-defaults.conf from existing ConfigMap overwrites kyuubi-defaults.conf from values"

Property file values-kyuubi-defaults.yaml

kyuubiConf:
  dir: /opt/kyuubi/conf
  files:
    'kyuubi-defaults.conf': |
      custom.from.values=true
  filesFrom:
    - configMap:
        name: kyuubi-defaults-config

ConfigMap kyuubi-defaults-config from configmap-kyuubi-defaults.yaml

apiVersion: v1
kind: ConfigMap
metadata:
  name: kyuubi-defaults-config
data:
  'kyuubi-defaults.conf': |
    custom.from.configmap=true

Rendered templates are correct

$ helm template charts/kyuubi -f values-kyuubi-defaults.yaml -s templates/kyuubi-configmap.yaml -s templates/kyuubi-statefulset.yaml

Content of kyuubi-defaults.conf comes from ConfigMap

$ kubectl create -f configmap-kyuubi-defaults.yaml
$ helm install kyuubi charts/kyuubi -f values-kyuubi-defaults.yaml
$ kubectl exec kyuubi-0 -- ls conf
kyuubi-defaults.conf
$ kubectl exec kyuubi-0 -- cat conf/kyuubi-defaults.conf
custom.from.configmap=true

Checklist 📝

Be nice. Be informative.

@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (ad53eef) to head (452dca3).

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #6521   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files         687     687           
  Lines       42442   42442           
  Branches     5793    5793           
======================================
  Misses      42442   42442           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pan3793
Copy link
Member

pan3793 commented Jul 4, 2024

It allows to configure Kyuubi (and its engines) by setting configuration file contents to chart properties or by providing configuration files through existing ConfigMaps and Secrets. Also users are not limited by predefined number of files and can put any files to configuration directories.

That sounds amazing. Sorry a little busy these days, will take a detailed look later (hopefully next week)

@pan3793
Copy link
Member

pan3793 commented Jul 4, 2024

Also cc @zwangsheng and @zhaohehuhu who also deploy Kyuubi on K8s using Helm chart.

@dnskr dnskr force-pushed the implement-new-helm-chart-configuration-approach branch from 671c64a to 513cb9a Compare July 24, 2024 14:10
@dnskr dnskr force-pushed the implement-new-helm-chart-configuration-approach branch from 513cb9a to 0cfbed9 Compare August 6, 2024 13:54
@dnskr dnskr force-pushed the implement-new-helm-chart-configuration-approach branch from 0cfbed9 to 14829f3 Compare November 13, 2024 17:08
@dnskr dnskr marked this pull request as ready for review November 13, 2024 20:59
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.

3 participants