[helm]support config override in starrocksFESpec/starrocksBeSpec/starrocksCnSpec

[dengliu]

Description

#494

Test

1) Test config spec setting

with values1.yaml, run

helm template --debug kube-starrocks --values kube-starrocks/values.yaml --values kube-starrocks/values-1.yaml

# spec for component be, provide storage and compute function.
starrocks:
  starrocksFESpec:
    configyaml:
      query_port: 9031
      foo: FEAdditionalValue
      additonal: values
  starrocksBeSpec:
    configyaml:
      be_port: 9061
      foo: BEAdditionalValue
  starrocksCluster:
    enabledCn: true
  starrocksCnSpec:
    configyaml:
      heartbeat_service_port: 9051
      foo: CNAdditionalValue

Got

2) Test config hash

with template below, a minor change in the configyaml values generate totally different hash value

fe-config-hash: "{{template "starrockscluster.fe.config.hash" . }}"
be-config-hash: "{{template "starrockscluster.be.config.hash" . }}"
cn-config-hash: "{{template "starrockscluster.cn.config.hash" . }}"

3) Test with quote in values

  starrocksFESpec:
    configyaml:
      LOG_DIR : ${STARROCKS_HOME}/log
      DATE : '"$(date +%Y%m%d-%H%M%S)"'
      JAVA_OPTS: '"-Dlog4j2.formatMsgNoLookups=true -Xmx8192m -XX:+UseG1GC -Xlog:gc*:${LOG_DIR}/fe.gc.log.$DATE:time"'
      mysql_service_nio_enabled: false

4) Test with configyaml not setting

Verified that it uses the string value in config field

5) Verified default value setting from configyaml

Related Issue(s)

N/A

Checklist

For operator, please complete the following checklist:

• run make generate to generate the code.
• run golangci-lint run to check the code style.
• run make test to run UT.
• run make manifests to update the yaml files of CRD.

For helm chart, please complete the following checklist:

• make sure you have updated the values.yaml
  file of starrocks chart.
• In scripts directory, run bash create-parent-chart-values.sh to update the values.yaml file of the parent
  chart( kube-starrocks chart).

[yandongxiao]

‌‌‌‌‌‌‌I think there is a backward compatibility issue here. That is, if the user has set a custom config by config and then upgrades to use the latest helm chart, because the value of configyaml is always not empty, it will cause the user to use the default content of configyaml as the configuration for FE/BE/CN.

[yandongxiao]

Looking at your code, you directly modified helm-charts/charts/kube-starrocks/values.yaml, should modify helm-charts/charts/kube-starrocks/charts/starrocks/values.yaml.

In scripts directory, run bash create-parent-chart-values.sh to update the values.yaml file of the parent
chart( kube-starrocks chart).

[yandongxiao]

‌Can we merge the contents of config and configyaml to generate the final result? This way, there will only be one set of configurations in values.yaml. At the same time, allow users to add incremental configurations through configyaml.

[dengliu]

‌‌‌‌‌‌‌I think there is a backward compatibility issue here. That is, if the user has set a custom config by config and then upgrades to use the latest helm chart, because the value of configyaml is always not empty, it will cause the user to use the default content of configyaml as the configuration for FE/BE/CN.

I can set configyaml to {} to address this issue. would this solution OK with you?

[dengliu]

‌Can we merge the contents of config and configyaml to generate the final result? This way, there will only be one set of configurations in values.yaml. At the same time, allow users to add incremental configurations through configyaml.

if both config and configyaml has the same key, which value shall be overriden?
I am afraid this hybrid mode will cause more confusion, and increase the chance of making mistakes

[yandongxiao]

‌‌‌‌‌‌‌I think there is a backward compatibility issue here. That is, if the user has set a custom config by config and then upgrades to use the latest helm chart, because the value of configyaml is always not empty, it will cause the user to use the default content of configyaml as the configuration for FE/BE/CN.

I can set configyaml to {} to address this issue. would this solution OK with you?

This is LGTM. The method I mentioned seems quite difficult to implement, and splitting the configuration into two blocks doesn't seem like a good choice.

[yandongxiao]

‌Can we merge the contents of config and configyaml to generate the final result? This way, there will only be one set of configurations in values.yaml. At the same time, allow users to add incremental configurations through configyaml.

if both config and configyaml has the same key, which value shall be overriden? I am afraid this hybrid mode will cause more confusion, and increase the chance of making mistakes

Agree with you