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

[Improvement-11505][k8s] Use configmap to mount the configuration files of each server #11508

Closed
wants to merge 1 commit into from

Conversation

rickchengx
Copy link
Contributor

@rickchengx rickchengx commented Aug 16, 2022

Purpose of the pull request

close: #11505

Use configmap to mount the configuration files of each server so that user can easily modify the configuration of each server when deploying Dolphinscheduler in the k8s cluster through Helm.

Ref:
https://kubernetes.io/docs/concepts/configuration/configmap/
https://helm.sh/docs/chart_template_guide/accessing_files/

For example, if the user wants to change the configuration (E.g., the root directory of zookeeper in application.yaml, or the log.base in logback-spring.yaml, or resource.storage.type in common.properties), he can directly do the modification in deploy/kubernetes/dolphinscheduler/conf/<server>/<configuration file>.

Take master-server as an example, k8s will automatically mount the configmap (4 configuration files in deploy/kubernetes/dolphinscheduler/conf/master-server) to /opt/dolphinscheduler/conf in the container.

Brief change log

Verify this pull request

Manually verified the change by testing locally.

/opt/dolphinscheduler/conf in the container is mounted.

image

@kezhenxu94
Copy link
Member

Hi @rickchengx , for those application.yaml configuration files, please refer to #11402 , I've enable the Spring Cloud Kubernetes configmap and it can watch the configmap in realtime and update the configurations, can you remove the related part in this PR to only contains those that are not included in #11402 ? i.e. common.properties

@sonarcloud
Copy link

sonarcloud bot commented Aug 16, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@rickchengx
Copy link
Contributor Author

Hi @rickchengx , for those application.yaml configuration files, please refer to #11402 , I've enable the Spring Cloud Kubernetes configmap and it can watch the configmap in realtime and update the configurations, can you remove the related part in this PR to only contains those that are not included in #11402 ? i.e. common.properties

Hi @kezhenxu94 , thanks for the reply. #11402 seems to be related to submitting a kubernetes-type task, but the scenario of this pr is to use Helm to deploy DS in a k8s cluster.

The purpose of this pr is to allow users to easily change the configuration files of each server before deploying with Helm, and mount the user-defined configuration files to the directory /opt/dolphinscheduler/conf in each server's container through configmap.

Please correct me if my understanding is wrong.

@kezhenxu94
Copy link
Member

Hi @kezhenxu94 , thanks for the reply. #11402 seems to be related to submitting a kubernetes-type task,

No. 11402 is nothing related to task.

but the scenario of this pr is to use Helm to deploy DS in a k8s cluster.

This scenario is the same as 11402

The purpose of this pr is to allow users to easily change the configuration files of each server before deploying with Helm, and mount the user-defined configuration files to the directory /opt/dolphinscheduler/conf in each server's container through configmap.

Please correct me if my understanding is wrong.

11402 not only allows users to modify the configs before starting DS components, but also allows users to modify the configs without restarting the components and the configs can be reloaded at runtime.

@aspexdaniel
Copy link

this solution will not use helm chart's values.yaml to change the properties file, which is a breaking change since it is available for v2.0.6, better to keep the same behavior. In v2.0.6 a script is used to sed all ENV variables into properties file.

@SbloodyS SbloodyS added improvement make more easy to user or prompt friendly backend labels Aug 21, 2022
@SbloodyS SbloodyS added this to the 3.0.1 milestone Aug 21, 2022
@rickchengx
Copy link
Contributor Author

11402 not only allows users to modify the configs before starting DS components, but also allows users to modify the configs without restarting the components and the configs can be reloaded at runtime.

@kezhenxu94 Sorry for the late response. I understand that 11402 could allow users to modify the configs without restarting the components and the configs can be reloaded at runtime.

But my point is that if users use Helm to deploy DS in a k8s cluster and he wants to modify some configs (E.g., the root directory of zookeeper in application.yaml) before deploying the DS.

In this case, DS cannot apply some configuration modifications at startup unless he modifies the configuration file in the docker image (Some configurations cannot be specified via values.yaml). E.g., he modifies the application.yaml and rebuilds the docker image with ADD application.yaml to /opt/dolphinscheduler/conf in Dockerfile.

This PR mainly mounts the configuration directory through configmap so that users can avoid rebuilding the docker image if he wants to modify the configs before deploying DS to k8s through Helm. As for 11402, users can still modify the configs without restarting the components and the configs can be reloaded at runtime.

@kezhenxu94
Copy link
Member

@kezhenxu94 Sorry for the late response. I understand that 11402 could allow users to modify the configs without restarting the components and the configs can be reloaded at runtime.

Hi, do you also know that the configs also works at startup phase, not only at runtime? Meaning when you create a config map with the desired configurations, the components will pick them up when starting

@rickchengx
Copy link
Contributor Author

@kezhenxu94 Sorry for the late response. I understand that 11402 could allow users to modify the configs without restarting the components and the configs can be reloaded at runtime.

Hi, do you also know that the configs also works at startup phase, not only at runtime? Meaning when you create a config map with the desired configurations, the components will pick them up when starting

Hi, @kezhenxu94 , thanks for the response. I know that the configs work at startup phase, and this is exactly the purpose of this PR. This PR mainly mounts the configuration directory through configmap so that users can avoid rebuilding the docker image if he wants to modify the configs before deploying DS to k8s through Helm.

Maybe I should explain my point more clearly:

  1. If users use Helm to deploy DS in a k8s cluster, each container will start the DS component with the configuration files in /opt/dolphinscheduler/conf
  2. DS cannot apply some configuration modifications at startup unless he modifies the configuration file in the docker image (Some configurations cannot be specified via values.yaml). E.g., he modifies the application.yaml and rebuilds the docker image with ADD application.yaml to /opt/dolphinscheduler/conf in Dockerfile.

But with this PR, if user wants to modify some cofigurations, he can just modify the configuration files in deploy/kubernetes/dolphinscheduler/conf/<specific-server>/ and these configurations will automatically be mounted to /opt/dolphinscheduler/conf in each container without rebuilding the docker image

@kezhenxu94
Copy link
Member

@rickchengx now, in the master, if the user wants to modify some configurations, they can just kubectl create configmap master-server --from-file=/path/to/the/master/application.yaml or kubectl create configmap worker-server --from-file=/path/to/the/worker/application.yaml, etc.. Does this solve your problem?

ALso, the benefit of 👆 compared to your changes, is that, users can just provide a patch config without copying the whole application.yaml, this is convenient when upgrading to a newer version so users don't need to compare the 2 config files and cherrypick the new config section.

@rickchengx
Copy link
Contributor Author

@rickchengx now, in the master, if the user wants to modify some configurations, they can just kubectl create configmap master-server --from-file=/path/to/the/master/application.yaml or kubectl create configmap worker-server --from-file=/path/to/the/worker/application.yaml, etc.. Does this solve your problem?

ALso, the benefit of 👆 compared to your changes, is that, users can just provide a patch config without copying the whole application.yaml, this is convenient when upgrading to a newer version so users don't need to compare the 2 config files and cherrypick the new config section.

Thanks for the reply. I've got your point.

@rickchengx rickchengx closed this Sep 5, 2022
@aspexdaniel
Copy link

@rickchengx now, in the master, if the user wants to modify some configurations, they can just kubectl create configmap master-server --from-file=/path/to/the/master/application.yaml or kubectl create configmap worker-server --from-file=/path/to/the/worker/application.yaml, etc.. Does this solve your problem?

ALso, the benefit of 👆 compared to your changes, is that, users can just provide a patch config without copying the whole application.yaml, this is convenient when upgrading to a newer version so users don't need to compare the 2 config files and cherrypick the new config section.

@kezhenxu94 you only created these configmaps, but not mounting them into deployment/statefulset, which will not be seen by DS pods, right?

I agree with your point users do not need to specify all configs within values.yaml, can we refer back to what 2.x version did:

  1. passing ENV variables at chart values.yaml (which are not picked up directly by DS programs)
  2. in all DS docker conainter entrypoint, start.sh refers to startup-init-conf.sh, which replaces all ENV variables into conf files, see https://github.com/apache/dolphinscheduler/blob/2.0.6-release/docker/build/startup-init-conf.sh#L133
  3. so, if user changes any single ENV value in values.yaml ( e.g. helm install xxx) the startup-init-conf.sh will pickup the ENV and replace into actual conf files, which are then used by DS programs after startup

@kezhenxu94
Copy link
Member

@rickchengx now, in the master, if the user wants to modify some configurations, they can just kubectl create configmap master-server --from-file=/path/to/the/master/application.yaml or kubectl create configmap worker-server --from-file=/path/to/the/worker/application.yaml, etc.. Does this solve your problem?
ALso, the benefit of 👆 compared to your changes, is that, users can just provide a patch config without copying the whole application.yaml, this is convenient when upgrading to a newer version so users don't need to compare the 2 config files and cherrypick the new config section.

@kezhenxu94 you only created these configmaps, but not mounting them into deployment/statefulset, which will not be seen by DS pods, right?

Please learn how Spring Cloud works, DS components will automatically pick up the config map, please also read #11730 (comment)

@aspexdaniel
Copy link

@kezhenxu94 thx for the link, its useful. Here's some caveat, correct me if I'm wrong:

  1. Yes spring-cloud config reload is useful, but the config map name is defined by spring.application.name, which will not be shared by all components, so how can we change some 'common' properties?

  2. Creating config maps is not a default Helm installation process, we need better documentation about this, seems to me we should use Kustomize instead of Helm?

  3. Interacting with K8S API in spring-cloud requires certain permissions, if RBAC is enabled in K8S, we will need to create service account and role-bindings for all the deployments/statefulset, which I believe is missing in current helm chart, see https://github.com/spring-cloud/spring-cloud-kubernetes/tree/main/spring-cloud-kubernetes-examples/kubernetes-reload-example/src/k8s

@kezhenxu94
Copy link
Member

  1. Yes spring-cloud config reload is useful, but the config map name is defined by spring.application.name, which will not be shared by all components, so how can we change some 'common' properties?

I don't see there is a scenario to share configs between different DS components. Every component has its own configurations. Also, sharing configs between components might be error-prone because you need to manually merge the common config and the component-specific config to see the final effective config when debugging. There is a method to do that in Spring but I don't see a scenario yet.

  1. Creating config maps is not a default Helm installation process, we need better documentation about this, seems to me we should use Kustomize instead of Helm?

if you want, you are welcome to migrate from helm to kustomize. But currently polishing the documentation is our better choice
because we don't have contributor to migrate from helm to kustomize,

  1. Interacting with K8S API in spring-cloud requires certain permissions, if RBAC is enabled in K8S, we will need to create service account and role-bindings for all the deployments/statefulset, which I believe is missing in current helm chart, see https://github.com/spring-cloud/spring-cloud-kubernetes/tree/main/spring-cloud-kubernetes-examples/kubernetes-reload-example/src/k8s

Yes we missed that. I will manage to add them

@aspexdaniel
Copy link

  1. Yes spring-cloud config reload is useful, but the config map name is defined by spring.application.name, which will not be shared by all components, so how can we change some 'common' properties?

I don't see there is a scenario to share configs between different DS components. Every component has its own configurations. Also, sharing configs between components might be error-prone because you need to manually merge the common config and the component-specific config to see the final effective config when debugging. There is a method to do that in Spring but I don't see a scenario yet.

  1. Creating config maps is not a default Helm installation process, we need better documentation about this, seems to me we should use Kustomize instead of Helm?

if you want, you are welcome to migrate from helm to kustomize. But currently polishing the documentation is our better choice because we don't have contributor to migrate from helm to kustomize,

  1. Interacting with K8S API in spring-cloud requires certain permissions, if RBAC is enabled in K8S, we will need to create service account and role-bindings for all the deployments/statefulset, which I believe is missing in current helm chart, see https://github.com/spring-cloud/spring-cloud-kubernetes/tree/main/spring-cloud-kubernetes-examples/kubernetes-reload-example/src/k8s

Yes we missed that. I will manage to add them

cool, I will test it out afterwards, may try to convert to Kustomize if I have time, seems more intuitive, thx.

@zhuangchong zhuangchong removed this from the 3.0.1 milestone Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend improvement make more easy to user or prompt friendly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement][k8s] Use configmap to mount the configuration files of each server
5 participants