You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by GitBox <gi...@apache.org> on 2022/08/16 10:03:24 UTC

[GitHub] [dolphinscheduler] rickchengx opened a new pull request, #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server

rickchengx opened a new pull request, #11508:
URL: https://github.com/apache/dolphinscheduler/pull/11508

   
   
   <!--Thanks very much for contributing to Apache DolphinScheduler. Please review https://dolphinscheduler.apache.org/en-us/community/development/pull-request.html before opening a pull request.-->
   
   
   ## Purpose of the pull request
   
   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 `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
   
   <!--*(for example:)*
     - *Add maven-checkstyle-plugin to root pom.xml*
   -->
   ## Verify this pull request
   
   <!--*(Please pick either of the following options)*-->
   
   Manually verified the change by testing locally.
   
   `/opt/dolphinscheduler/conf` in the container is mounted.
   
   <img width="687" alt="image" src="https://user-images.githubusercontent.com/38122586/184853253-d9c3ddbf-aeb8-4b46-a327-09296628f8e3.png">
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [dolphinscheduler] rickchengx commented on pull request #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server

Posted by GitBox <gi...@apache.org>.
rickchengx commented on PR #11508:
URL: https://github.com/apache/dolphinscheduler/pull/11508#issuecomment-1236498038

   > @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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [dolphinscheduler] aspexdaniel commented on pull request #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server

Posted by GitBox <gi...@apache.org>.
aspexdaniel commented on PR #11508:
URL: https://github.com/apache/dolphinscheduler/pull/11508#issuecomment-1217601835

   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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [dolphinscheduler] aspexdaniel commented on pull request #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server

Posted by GitBox <gi...@apache.org>.
aspexdaniel commented on PR #11508:
URL: https://github.com/apache/dolphinscheduler/pull/11508#issuecomment-1236514900

   > > 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.
   > 
   > > 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?
   > 
   > 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,
   > 
   > > 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
   > 
   > 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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [dolphinscheduler] kezhenxu94 commented on pull request #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #11508:
URL: https://github.com/apache/dolphinscheduler/pull/11508#issuecomment-1233749733

   > @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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [dolphinscheduler] rickchengx closed pull request #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server

Posted by GitBox <gi...@apache.org>.
rickchengx closed pull request #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server
URL: https://github.com/apache/dolphinscheduler/pull/11508


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [dolphinscheduler] kezhenxu94 commented on pull request #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #11508:
URL: https://github.com/apache/dolphinscheduler/pull/11508#issuecomment-1236495427

   @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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [dolphinscheduler] kezhenxu94 commented on pull request #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #11508:
URL: https://github.com/apache/dolphinscheduler/pull/11508#issuecomment-1217383877

   
   > 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. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [dolphinscheduler] sonarcloud[bot] commented on pull request #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #11508:
URL: https://github.com/apache/dolphinscheduler/pull/11508#issuecomment-1216666961

   Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=11508)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=CODE_SMELL)
   
   [![No Coverage information](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/NoCoverageInfo-16px.png 'No Coverage information')](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11508&metric=coverage&view=list) No Coverage information  
   [![No Duplication information](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/NoDuplicationInfo-16px.png 'No Duplication information')](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11508&metric=duplicated_lines_density&view=list) No Duplication information
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [dolphinscheduler] kezhenxu94 commented on pull request #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #11508:
URL: https://github.com/apache/dolphinscheduler/pull/11508#issuecomment-1236505138

   > > @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 https://github.com/apache/dolphinscheduler/pull/11730#issue-1358375669


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [dolphinscheduler] aspexdaniel commented on pull request #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server

Posted by GitBox <gi...@apache.org>.
aspexdaniel commented on PR #11508:
URL: https://github.com/apache/dolphinscheduler/pull/11508#issuecomment-1236509892

   @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
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [dolphinscheduler] kezhenxu94 commented on pull request #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #11508:
URL: https://github.com/apache/dolphinscheduler/pull/11508#issuecomment-1216567761

   Hi @rickchengx , for those `application.yaml` configuration files, please refer to https://github.com/apache/dolphinscheduler/pull/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 https://github.com/apache/dolphinscheduler/pull/11402 ? i.e. `common.properties`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [dolphinscheduler] rickchengx commented on pull request #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server

Posted by GitBox <gi...@apache.org>.
rickchengx commented on PR #11508:
URL: https://github.com/apache/dolphinscheduler/pull/11508#issuecomment-1221711238

   > 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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [dolphinscheduler] kezhenxu94 commented on pull request #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #11508:
URL: https://github.com/apache/dolphinscheduler/pull/11508#issuecomment-1236513165

   > 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. 
   
   > 
   > 
   > 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?
   
   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, 
   
   > 
   > 
   > 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
   > 
   
   Yes we missed that. I will manage to add them


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [dolphinscheduler] sonarcloud[bot] commented on pull request #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #11508:
URL: https://github.com/apache/dolphinscheduler/pull/11508#issuecomment-1216664871

   Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=11508)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=11508&resolved=false&types=CODE_SMELL)
   
   [![No Coverage information](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/NoCoverageInfo-16px.png 'No Coverage information')](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11508&metric=coverage&view=list) No Coverage information  
   [![No Duplication information](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/NoDuplicationInfo-16px.png 'No Duplication information')](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=11508&metric=duplicated_lines_density&view=list) No Duplication information
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [dolphinscheduler] rickchengx commented on pull request #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server

Posted by GitBox <gi...@apache.org>.
rickchengx commented on PR #11508:
URL: https://github.com/apache/dolphinscheduler/pull/11508#issuecomment-1236488501

   > > @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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [dolphinscheduler] aspexdaniel commented on pull request #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server

Posted by GitBox <gi...@apache.org>.
aspexdaniel commented on PR #11508:
URL: https://github.com/apache/dolphinscheduler/pull/11508#issuecomment-1236499734

   > @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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [dolphinscheduler] rickchengx commented on pull request #11508: [Improvement-11505][k8s] Use configmap to mount the configuration files of each server

Posted by GitBox <gi...@apache.org>.
rickchengx commented on PR #11508:
URL: https://github.com/apache/dolphinscheduler/pull/11508#issuecomment-1217381393

   > 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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org