You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2020/03/27 13:34:46 UTC

[GitHub] [incubator-yunikorn-k8shim] kingamarton opened a new pull request #90: [YUNIKORN-44] Pass scheduler startup options from environment variables

kingamarton opened a new pull request #90: [YUNIKORN-44] Pass scheduler startup options from environment variables
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/90
 
 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on issue #90: [YUNIKORN-44] Pass scheduler startup options from environment variables

Posted by GitBox <gi...@apache.org>.

yangwwei commented on issue #90: [YUNIKORN-44] Pass scheduler startup options from environment variables
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/90#issuecomment-606239758
 
 
   Hi @kingamarton 
   
   Thanks for the updates, it looks good. I have tested and it works as expected.
   Can I ask one more thing for this PR? Sorry I did not mention this earlier
   
   In `pkg/conf/schedulerconf.go`, there are a lot more configurable options. Ideally, we need them all in the dockerfile, giving a default value. So this gives us enough flexibility not to go back and modify the dockerfile again if we want to tweak some changes in the future. I think we need to add following (just excluded one or two no need for updates)
   
   ```
   ENV SCHEDULER_NAME yunikorn
   ENV POLICY_GROUP queues
   ENV SCHEDULING_INTERVAL 0
   ENV VOLUME_BINDING_TIMEOUT 10s
   ENV KUBE_CLIENT_QPS 1000
   ENV KUBE_CLIENT_BURST 1000
   ENV EVENT_CHANNEL_CAPACITY 1048576
   ENV DISPATCHER_TIMEOUT 300s
   ```
   in the deployment yaml files, we don't need to expose these, your current change is fine. 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-yunikorn-k8shim] yangwwei edited a comment on issue #90: [YUNIKORN-44] Pass scheduler startup options from environment variables

Posted by GitBox <gi...@apache.org>.

yangwwei edited a comment on issue #90: [YUNIKORN-44] Pass scheduler startup options from environment variables
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/90#issuecomment-606239758

Hi @kingamarton

Thanks for the updates, it looks good. I have tested and it works as expected.
Can I ask one more thing for this PR? Sorry I did not mention this earlier

In `pkg/conf/schedulerconf.go`, there are a lot more configurable options. Ideally, we need them all in the dockerfile, giving a default value. So this gives us enough flexibility not to go back and modify the dockerfile again if we want to tweak some changes in the future. I think we need to add following (just excluded one or two no need for updates)

```
ENV CLUSTER_ID "mycluster"
ENV CLUSTER_VERSION "latest"
ENV LOG_ENCODING "console"
ENV LOG_LEVEL "-1"
ENV SCHEDULER_NAME "yunikorn"
ENV POLICY_GROUP "queues"
ENV SCHEDULING_INTERVAL "0"
ENV VOLUME_BINDING_TIMEOUT "10s"
ENV KUBE_CLIENT_QPS "1000"
ENV KUBE_CLIENT_BURST "1000"
ENV EVENT_CHANNEL_CAPACITY "1048576"
ENV DISPATCHER_TIMEOUT "300s"
ENV PREDICATES ""
ENV OPERATOR_PLUGINS "general"
```
in the deployment yaml files, we don't need to expose these, your current change is fine.

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

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

With regards,
Apache Git Services

[GitHub] [incubator-yunikorn-k8shim] yangwwei merged pull request #90: [YUNIKORN-44] Pass scheduler startup options from environment variables

Posted by GitBox <gi...@apache.org>.

yangwwei merged pull request #90: [YUNIKORN-44] Pass scheduler startup options from environment variables
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/90
 
 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-yunikorn-k8shim] kingamarton commented on a change in pull request #90: [YUNIKORN-44] Pass scheduler startup options from environment variables

Posted by GitBox <gi...@apache.org>.

kingamarton commented on a change in pull request #90: [YUNIKORN-44] Pass scheduler startup options from environment variables
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/90#discussion_r400220719
 
 

 ##########
 File path: deployments/image/configmap/Dockerfile
 ##########
 @@ -35,4 +35,3 @@ RUN chmod +x /generate-signed-ca.sh
 ADD k8s_yunikorn_scheduler /k8s_yunikorn_scheduler
 WORKDIR /
 ENTRYPOINT ["/k8s_yunikorn_scheduler"]
-CMD ["-logEncoding=console", "-logLevel=-1", "-clusterId=mycluster", "-clusterVersion=latest"]
 
 Review comment:
   You are right! I checked only if the environment variables are set. Unfortunately the solution proposed by you is not working. In order to pass the variable I had to execute a shell as described here: https://docs.docker.com/engine/reference/builder/#cmd
   
   After this solution I tested it by checking in the logs if the passed values are used(both the ones from the docker file and from the .yaml file as well) instead of the default ones.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #90: [YUNIKORN-44] Pass scheduler startup options from environment variables

Posted by GitBox <gi...@apache.org>.

yangwwei commented on a change in pull request #90: [YUNIKORN-44] Pass scheduler startup options from environment variables
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/90#discussion_r399467036
 
 

 ##########
 File path: deployments/scheduler/scheduler.yaml
 ##########
 @@ -16,6 +16,15 @@ spec:
       containers:
         - name: yunikorn-scheduler-k8s
           image: yunikorn/yunikorn-scheduler-k8s:latest
+          env:
+            - name: logEncoding
+              value: console
+            - name: logLevel
+              value: '1'
 
 Review comment:
   let's use `-1` for now, that is DEBUG level.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #90: [YUNIKORN-44] Pass scheduler startup options from environment variables

Posted by GitBox <gi...@apache.org>.

yangwwei commented on a change in pull request #90: [YUNIKORN-44] Pass scheduler startup options from environment variables
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/90#discussion_r399470074
 
 

 ##########
 File path: deployments/image/configmap/Dockerfile
 ##########
 @@ -35,4 +35,3 @@ RUN chmod +x /generate-signed-ca.sh
 ADD k8s_yunikorn_scheduler /k8s_yunikorn_scheduler
 WORKDIR /
 ENTRYPOINT ["/k8s_yunikorn_scheduler"]
-CMD ["-logEncoding=console", "-logLevel=-1", "-clusterId=mycluster", "-clusterVersion=latest"]
 
 Review comment:
   Will this be enough? I thought we also need to specify ENVs in the docker file?
   If these arguments are removed, how the binary `k8s_yunikorn_scheduler` receive CLI arguments?
   
   Should we do something like
   ```
   ENV LOG_ENCODING "console"
   ENV LOG_LEVEL "-1"
   ENV ...
   
   CMD ["-logEncoding=${LOG_ENCODING}", "-logLevel=${LOG_LEVEL}", "-clusterId=${CLUSRTER_ID}", "-clusterVersion=${CLUSTER_VERSION}"]
   ```
   just a thought, not tested. Found more info: https://stackoverflow.com/questions/40454470/how-can-i-use-a-variable-inside-a-dockerfile-cmd
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-yunikorn-k8shim] kingamarton commented on a change in pull request #90: [YUNIKORN-44] Pass scheduler startup options from environment variables

Posted by GitBox <gi...@apache.org>.

kingamarton commented on a change in pull request #90: [YUNIKORN-44] Pass scheduler startup options from environment variables
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/90#discussion_r400218226
 
 

 ##########
 File path: deployments/scheduler/scheduler.yaml
 ##########
 @@ -16,6 +16,15 @@ spec:
       containers:
         - name: yunikorn-scheduler-k8s
           image: yunikorn/yunikorn-scheduler-k8s:latest
+          env:
+            - name: logEncoding
+              value: console
+            - name: logLevel
+              value: '1'
 
 Review comment:
   Done

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-yunikorn-k8shim] yangwwei edited a comment on issue #90: [YUNIKORN-44] Pass scheduler startup options from environment variables

Posted by GitBox <gi...@apache.org>.

yangwwei edited a comment on issue #90: [YUNIKORN-44] Pass scheduler startup options from environment variables
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/90#issuecomment-606239758

Hi @kingamarton

Thanks for the updates, it looks good. I have tested and it works as expected.
Can I ask one more thing for this PR? Sorry I did not mention this earlier

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

With regards,
Apache Git Services

[GitHub] [incubator-yunikorn-k8shim] yangwwei edited a comment on issue #90: [YUNIKORN-44] Pass scheduler startup options from environment variables

Posted by GitBox <gi...@apache.org>.

yangwwei edited a comment on issue #90: [YUNIKORN-44] Pass scheduler startup options from environment variables
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/90#issuecomment-606239758

Hi @kingamarton

Thanks for the updates, it looks good. I have tested and it works as expected.
Can I ask one more thing for this PR? Sorry I did not mention this earlier

```
ENV CLUSTER_ID "mycluster"
ENV CLUSTER_VERSION "latest"
ENV LOG_ENCODING "console"
ENV LOG_LEVEL "-1"
ENV SCHEDULER_NAME "yunikorn"
ENV POLICY_GROUP "queues"
ENV SCHEDULING_INTERVAL "0"
ENV VOLUME_BINDING_TIMEOUT "10s"
ENV KUBE_CLIENT_QPS "1000"
ENV KUBE_CLIENT_BURST "1000"
ENV EVENT_CHANNEL_CAPACITY "1048576"
ENV DISPATCHER_TIMEOUT "300s"
```
in the deployment yaml files, we don't need to expose these, your current change is fine.

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

With regards,
Apache Git Services