You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@yunikorn.apache.org by GitBox <gi...@apache.org> on 2020/03/10 13:27:36 UTC
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 opened a new pull request
#81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
TaoYang526 opened a new pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81
Initial PR for YUNIKORN-28.
@yangwwei please help to review it. Thanks.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
yangwwei commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-598396970
> This will mean that we cannot change the core config code without updating the shim. When we introduce a new shim which adds some config to the core and we want to deploy the scheduler without the k8shim we cannot.
This I disagree. Configs are core side things, even there are 2 shims. the configs should be consistent in core. If we diverse different configs for different shims, that will diverse the core codebase as well. that's something we don't want to run into. Core should be generic. Note, this approach shim doesn't use the configs directly, it only validates it using configs pkg for the core.
> I am also wondering where the config volume is if you have no scheduler deployed.
This is not a valid case. We certainly don't want the admission-controller running without the scheduler.
> Using a service discovery to find the rest interface is a better solution and allows us to really keep any knowledge that does not belong in the admission controller out of it.
That's something we can do, but apparently it introduces more complexity. Do you think we should invest resources for this? The admission-controller is an optional deployment, if we need some changes in scheduler's deployment for service discovery, that is not always necessary.
> You also need to keep in mind that even if the validate passes the scheduler can still reject the configuration. The config can be valid but the running config might prevent it from being applied, i.e. a leaf queue with running apps cannot be changed into a parent queue, or a placement rule defined in the config might not exist in the code itself.
Here is to place a safe-guard that includes all necessary static checks. IIRC, we do refresh the configs once the load is successful. This is consistent. But if it fails sometime later, that's another thing we need to think about.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on a change in
pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r396998695
##########
File path: deployments/admission-controllers/scheduler/README.md
##########
@@ -56,6 +59,28 @@ kubectl get pod task0 -o yaml
you'll see the `schedulerName` has been injected with value `yunikorn`.
+#### Validations
+
+YuniKorn loads its configuration from a configmap called yunikorn-configs, this admission controller adds a web-hook to
+validate the update and create requests for this configmap.
+It is a safeguard to protect the scheduler not to load invalid configuration, by rejecting such requests.
Review comment:
Make sense, will replace the second section with that.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on a change in
pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r390700267
##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
##########
@@ -167,6 +169,53 @@ func updateLabels(pod *v1.Pod, patch []patchOperation) []patchOperation {
return patch
}
+func (c *admissionController) validateConf(ar *v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {
+ req := ar.Request
+ log.Logger.Info("AdmissionReview",
+ zap.Any("Kind", req.Kind),
+ zap.String("Namespace", req.Namespace),
+ zap.String("UID", string(req.UID)),
+ zap.String("Operation", string(req.Operation)),
+ zap.Any("UserInfo", req.UserInfo))
+
+ if req.Kind.Kind == "ConfigMap" {
+ var configmap v1.ConfigMap
+ if err := json.Unmarshal(req.Object.Raw, &configmap); err != nil {
+ return &v1beta1.AdmissionResponse{
+ Result: &metav1.Status{
+ Message: err.Error(),
+ },
+ }
+ }
+ // validate new/updated config map
+ if err := c.validateConfigMap(&configmap); err != nil {
+ return &v1beta1.AdmissionResponse{
+ Result: &metav1.Status{
+ Message: err.Error(),
Review comment:
Make sense.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 edited a comment on issue
#81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 edited a comment on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-597563887
> I think we need to look in the directions of using REST and directly talk to the core web app.
I can't understand this way, could you please elaborate?
> The config is also already loaded by the webhook as it needs the scheduler name.
I think webhook doesn't load the consistent config with scheduler, it just gets the default scheduler name.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull
request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r393372123
##########
File path: deployments/admission-controllers/scheduler/generate-signed-ca.sh
##########
@@ -24,8 +24,8 @@ if [ ! -f ${CONF_FILE} ]; then
fi
tmpdir="$1"
-service=`cat ${CONF_FILE} | grep ^service | cut -d "=" -f 2`
-namespace=`cat ${CONF_FILE} | grep ^namespace | cut -d "=" -f 2`
+service=$SERVICE
Review comment:
please add a check here in case environment variables are missing here
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull
request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r393370279
##########
File path: helm-charts/yunikorn/templates/deployment.yaml
##########
@@ -36,7 +36,7 @@ spec:
lifecycle:
postStart:
exec:
- command: ["/bin/sh", "/admission_util.sh", "create"]
+ command: ["/bin/sh", "/admission_util.sh", "create", "--env", "NAMESPACE={{ .Release.Namespace }}"]
Review comment:
Have you tested the case when we do helm install without giving a namespace?
The charts will be installed to default namespace, but I am not sure `Release.Namespace` will have the proper value set.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-600958132
Thanks @yangwwei for the review.
Make sense, already updated in the latest commit, please take a look.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull
request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r393439252
##########
File path: helm-charts/yunikorn/templates/deployment.yaml
##########
@@ -36,7 +36,7 @@ spec:
lifecycle:
postStart:
exec:
- command: ["/bin/sh", "/admission_util.sh", "create"]
+ command: ["/bin/sh", "/admission_util.sh", "create", "--env", "NAMESPACE={{ .Release.Namespace }}"]
Review comment:
Cool, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
yangwwei commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-597840599
Hi @TaoYang526
I think @wilfred-s means to create a web-service in yunikorn-core, e.g http://localhost:9080/ws/v1/validateconf. Then the admission-controller here only needs to pass the configs through HTTP request.
However, this increased complexity. We are running admission-controller in a separate pod than the scheduler, which means you will have to figure out how to connect with the scheduler pod in the admission-controller pod.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-597563887
> I think we need to look in the directions of using REST and directly talk to the core web app.
I can't understand this way, could you please elaborate?
> The config is also already loaded by the webhook as it needs the scheduler name.
I think webhook doesn't load the consistent config with scheduler, it just gets the default scheduler name.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on a change in
pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r393416294
##########
File path: deployments/admission-controllers/scheduler/admission_util.sh
##########
@@ -46,6 +46,42 @@ delete_resources() {
return 0
}
+updateEnvVars() {
+ # update environment variables
+ while true; do
+ case $1 in
+ -e|--env)
+ shift
+ if [ -z "$1" ]; then
+ echo "empty argument to -e|--env flag"
+ exit 1
+ fi
+ setEnvCmd=$1
+ case $setEnvCmd in
+ *=*)
+ echo "Update environment variable: $setEnvCmd"
+ eval "$setEnvCmd"
Review comment:
I have tried to export the environment variables in preStart/exec/command but it seems not supported since scheduler pod failed and shown file "export xxxx=..." not exist. Also can't find any places to set the environments.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
yangwwei commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-598925872
Hi @TaoYang526
The changes in shim and proposed changes in core are fine.
My question is how we do the service recovery.
We want our deployment as simple as possible, if you look at the helm charts we have one flag to enable or disable the admission-controller: https://github.com/apache/incubator-yunikorn-k8shim/blob/de94366ce5a7d0c21fd49f41ad991a9a67f75508/helm-charts/yunikorn/values.yaml#L26.
When we add the validation via REST, how we can make this all auto-configured in helm charts? I still expect to do a `helm install` without any manual configuration. So any more info about how we can achieve this?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull
request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r396896255
##########
File path: pkg/plugin/admissioncontrollers/webhook/webhook.go
##########
@@ -38,6 +39,13 @@ const (
tlsDir = `/run/secrets/tls`
tlsCertFile = `cert.pem`
tlsKeyFile = `key.pem`
+ policyGroupEnvVarName = "POLICY_GROUP"
+ schedulerServiceAddressEnvVarName = "SCHEDULER_SERVICE_ADDRESS"
+ validateConfUrlPattern = "http://%s/ws/v1/validate-conf"
Review comment:
check `go fmt` for this please
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-598511284
Thanks @wilfred-s and @yangwwei for the comments. I agree with you according to these comments, and would like to keep working on this.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
yangwwei commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-598485439
I just had some more offline discussion with @wilfred-s . I think @wilfred-s makes a very good point: today, the code only does the static check (syntax, value, etx); but moving on, it's possible we might need to add more in-context checks to validate configs before applying it. In that case, it will have more dependency with core code. So it sounds a good idea to keep that part of code inside of core, and just expose an API (through rest endpoint which we already have today).
@TaoYang526 , could you please investigate how we can do service discovery from admission-controller to the scheduler pod? If that is not too complex, it would be good to have this model.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-598166263
By adding the core scheduler config code into the admission controller introduce a hard coupling between the k8shim code and the core code. This will mean that we cannot change the core config code without updating the shim. When we introduce a new shim which adds some config to the core and we want to deploy the scheduler without the k8shim we cannot. It will be blocked by the admission controller as it will not allow us to make any configuration changes. The new core can thus not be released without releasing a new k8shim and a new admission controller.
We need to keep the flexibility to make configuration changes in the core without the need to release a shim or admission controller. The current deployment of one binary for shim and core is not the end state.
I am also wondering where the config volume is if you have no scheduler deployed. Beside that having an active admission controller in the cluster and no scheduler would also leave your whole cluster stuck. Nothing will be scheduled without a scheduler. I do not think that it is a good enough reason to push the code into the admission controller. The admission controller must never rely on any shim or core version. It should stay as dumb as possible.
Using a service discovery to find the rest interface is a better solution and allows us to really keep any knowledge that does not belong in the admission controller out of it.
You also need to keep in mind that even if the validate passes the scheduler can still reject the configuration. The config can be valid but the running config might prevent it from being applied, i.e. a leaf queue with running apps cannot be changed into a parent queue, or a placement rule defined in the config might not exist in the code itself.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-597438656
@wilfred-s @yangwwei
I thought we can add a new API in api.SchedulerAPI and its implementation in RMProxy:
```
// Request scheduler to validate configuration
ValidateConfiguration(content string) error
```
We also need to get an instance of api.SchedulerAPI in admission-controller, I'm not sure is it acceptable to add an method in entrypoint.go like this:
```
func NewSchedulerAPI() api.SchedulerAPI {
return rmproxy.NewRMProxy()
}
```
In this way we still need to dependent the entrypoint package. Is there a better solution? Hope to hear your thoughts. Thanks.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 edited a comment on issue
#81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 edited a comment on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-598806310
@wilfred-s @yangwwei
I have done the initial update in latest commit which validates configs via /ws/v1/validate-conf REST API (needs to be added later) in yunikorn scheduler, it assumes that we have a service to expose the scheduler REST API, and the service address need to be configured in configs.properties.
Changes in core repo like this:
```
routers.go
// endpoint to validate conf
Route{
"Scheduler",
"POST",
"/ws/v1/validate-conf",
ValidateConf,
},
handlers.go
func ValidateConf(w http.ResponseWriter, r *http.Request) {
writeHeaders(w)
var result dao.ValidateConfResponse
requestBytes, err := ioutil.ReadAll(r.Body)
if err != nil {
result.Error = err.Error()
}
_, err = configs.LoadSchedulerConfigFromByteArray(requestBytes)
if err != nil {
result.Error = err.Error()
}
if result.Error == "" {
result.Allowed = true
}
log.Logger().Debug("Validate conf",
zap.Any("content", string(requestBytes)),
zap.Any("result", result))
if err := json.NewEncoder(w).Encode(result); err != nil {
panic(err)
}
}
```
Could you please take a look at this update and share your thoughts? Thanks.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull
request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r390612743
##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
##########
@@ -167,6 +169,53 @@ func updateLabels(pod *v1.Pod, patch []patchOperation) []patchOperation {
return patch
}
+func (c *admissionController) validateConf(ar *v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {
+ req := ar.Request
+ log.Logger.Info("AdmissionReview",
+ zap.Any("Kind", req.Kind),
+ zap.String("Namespace", req.Namespace),
+ zap.String("UID", string(req.UID)),
+ zap.String("Operation", string(req.Operation)),
+ zap.Any("UserInfo", req.UserInfo))
+
+ if req.Kind.Kind == "ConfigMap" {
+ var configmap v1.ConfigMap
+ if err := json.Unmarshal(req.Object.Raw, &configmap); err != nil {
+ return &v1beta1.AdmissionResponse{
+ Result: &metav1.Status{
+ Message: err.Error(),
+ },
+ }
+ }
+ // validate new/updated config map
+ if err := c.validateConfigMap(&configmap); err != nil {
+ return &v1beta1.AdmissionResponse{
+ Result: &metav1.Status{
+ Message: err.Error(),
Review comment:
I think we should explicitly call the following while rejecting the request:
```
return &v1beta1.AdmissionResponse{
Allowed: false,
Result: &metav1.Status{
Reason: metav1.StatusReasonInvalid,
Message: err.Error(),
},
}
```
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
yangwwei commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-601409572
Two issues while testing this out:
1) It seems the admission-controller service somehow has linked to the yunikorn-scheduler pod. This is not correct. Is this because of the `matchedLabel` used?
<img width="1647" alt="yunikorn-admission-controller-service_-_Kubernetes_Dashboard" src="https://user-images.githubusercontent.com/14214570/77112003-7e8fc500-69e5-11ea-82f8-b412cfb677a2.png">
2) In `admission_controller.go#validateConf()`, it seems the http connection opened but not closed. Could you please make sure that is done as well.
3) When I try this out, I got a warning event:
```
7m25s Warning FailedToCreateEndpoint endpoints/yunikorn-service Failed to create endpoint for service default/yunikorn-service: endpoints "yunikorn-service" already exists
```
did you see this on your deployment?
Please help to take look at these issues. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
yangwwei commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-597448104
Hi @TaoYang526 , @wilfred-s
I think that's over-engineered.
Can we just use the current approach by depending on the configs package to do the validation?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-597419447
Thanks @wilfred-s for the detailed clarification.
I'll try to verify the config via a new API.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-601557543
@yangwwei Thanks for the review, replies as following:
1. Yes, I think so. But it can works as well since it restricts the target port to `webhook-api`. It's better to support role labels, we can correct the selector like this: `app: yunikorn && role: admission-controller`
2. Good catch, I forgot to close it, will be corrected soon.
3. No, have you checked if the scheduler pod works well? and is there any detailed logs?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-599014612
> So the changes needed here is to adhere the namespace value passed in from helm, and populate that into the deployment template.
Got it, I'll update this PR and test helm chart install later. Thanks.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-603128388
Thanks @yangwwei and @wilfred-s for the review.
Formatted code and fixed lint errors in the latest commit.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull
request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r393371996
##########
File path: deployments/admission-controllers/scheduler/admission_util.sh
##########
@@ -46,6 +46,42 @@ delete_resources() {
return 0
}
+updateEnvVars() {
+ # update environment variables
+ while true; do
+ case $1 in
+ -e|--env)
+ shift
+ if [ -z "$1" ]; then
+ echo "empty argument to -e|--env flag"
+ exit 1
+ fi
+ setEnvCmd=$1
+ case $setEnvCmd in
+ *=*)
+ echo "Update environment variable: $setEnvCmd"
+ eval "$setEnvCmd"
Review comment:
why not export the environment variables here?
so we don't need that in `create_resources()`
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
yangwwei commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-603348009
Thanks @TaoYang526 .
Hi @wilfred-s , please help to merge this once you think it is ready.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
yangwwei commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-600940698
Hi @TaoYang526
One more comment, can we use env vars https://kubernetes.io/docs/tasks/inject-data-application/define-environment-variable-container/ to pass in values, e.g the namespace? So the script can directly use that instead of parsing from CLI.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
yangwwei commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-602973946
Hi @TaoYang526
I have tested the changes. It works well. +1 from my side.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull
request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r390607337
##########
File path: deployments/admission-controllers/scheduler/README.md
##########
@@ -56,6 +59,28 @@ kubectl get pod task0 -o yaml
you'll see the `schedulerName` has been injected with value `yunikorn`.
+#### Validations
+
+After the admission controller is started, the config-map named `yunikorn-configs` can be validated
+ before it's created or updated, thus update/creation request with invalid content will be denied immediately and
+ the error cause will be returned to the client.
Review comment:
Can we rephrase this to:
YuniKorn loads its configuration from a configmap called `yunikorn-configs`, this admission controller adds a web-hook to validate the `update` and `create` requests for this configmap. It is a safeguard to protect the scheduler not to load invalid configuration, by rejecting such requests.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-599851060
@yangwwei Thanks for the review and reminder.
> How about the changes for yunikorn-core?
I'll create a new PR in yunikorn-core for that.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 edited a comment on issue
#81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 edited a comment on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-599014612
> So the changes needed here is to adhere the namespace value passed in from helm, and populate that into the deployment template.
Got it, I'll update this PR and test helm chart install later. Thanks.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull
request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r396895524
##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller_test.go
##########
@@ -19,6 +19,9 @@
package main
import (
+ "fmt"
+ "github.com/apache/incubator-yunikorn-k8shim/pkg/common"
+ "github.com/apache/incubator-yunikorn-k8shim/pkg/conf"
Review comment:
This does not comply with the imports formatting. Please run:
```golangci-lint run --new-from-rev=`git rev-parse --short=12 origin/HEAD` ```
That should pull the origin/HEAD tag and check your change against it.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on a change in
pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r397007212
##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller_test.go
##########
@@ -19,6 +19,9 @@
package main
import (
+ "fmt"
+ "github.com/apache/incubator-yunikorn-k8shim/pkg/common"
+ "github.com/apache/incubator-yunikorn-k8shim/pkg/conf"
Review comment:
Thanks for the reminder and telling how to use it. I will execute this tool and check format before committing from now on.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-597547339
Beside the pull in of the core code described above I was overlooking the fact that the admission controller is a separate binary which is build out of just two source files: webhook.go and the mutating_controller.go. This further complicates the design..
The admission controller is a separate binary and must thus use GPRC to communicate with the shim/scheduler. There is no provision for that in the current setup. The webhook would thus become a pretty heavy executable with a lot of exposure.
Second point: looking at the source admission controller you will see that there is no code outside of some really basic config and logging details that are getting pulled in. You thus have no `entrypoint` or even a `RMProxy`. Adding all this would thus be a major change.
Adding all that into the admission controller would have a far bigger impact than I first thought:
1. bloat of the code
2. open up extra ports and adding services to integrate.
I think we need to look in the directions of using REST and directly talk to the core web app. There is already a HTTP server in the webhook so we can leverage that dependency and it keeps it simple. The config is also already loaded by the webhook as it needs the scheduler name.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] wilfred-s closed pull request #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
wilfred-s closed pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull
request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r396896583
##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller_test.go
##########
@@ -211,3 +214,20 @@ func TestUpdateSchedulerName(t *testing.T) {
}
}
+func TestValidateConfigMap(t *testing.T) {
+ configName := fmt.Sprintf("%s.yaml", conf.DefaultPolicyGroup)
+ controller := &admissionController{
+ configName: configName,
+ }
+ configmap := &v1.ConfigMap{
+ ObjectMeta: metav1.ObjectMeta{
+ Name: common.DefaultConfigMapName,
+ },
+ Data: make(map[string]string),
Review comment:
check `go fmt` for this please
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on a change in
pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r390700026
##########
File path: deployments/admission-controllers/scheduler/README.md
##########
@@ -56,6 +59,28 @@ kubectl get pod task0 -o yaml
you'll see the `schedulerName` has been injected with value `yunikorn`.
+#### Validations
+
+After the admission controller is started, the config-map named `yunikorn-configs` can be validated
+ before it's created or updated, thus update/creation request with invalid content will be denied immediately and
+ the error cause will be returned to the client.
Review comment:
Sure, that's more clear. Thanks.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-598986158
@yangwwei Thanks for the comments.
According to the codes for helm charts, I can see that service name is constant and the port is configurable, I think we can only configure the name of service in configs.properties and get the http port of yunikorn service automatically in admission_util.sh via `kubectl get service yunikorn-service -n $NAMESPACE -o jsonpath="{.spec.ports[0].port}"`, then inject the serviceAddress into admission-controller by environment variable.
My doubt is that the namespace in admission-controller seems not related to the helm charts, are we expecting they may be different?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull
request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r396896085
##########
File path: pkg/plugin/admissioncontrollers/webhook/webhook.go
##########
@@ -22,6 +22,7 @@ import (
"context"
"crypto/tls"
"fmt"
+ "github.com/apache/incubator-yunikorn-k8shim/pkg/conf"
Review comment:
See above import format needs fixing
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 edited a comment on issue
#81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 edited a comment on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-598806310
@wilfred-s @yangwwei
I have done the initial update in latest commit which validates configs via /ws/v1/validate-conf REST API (needs to be added later) in yunikorn scheduler, it assumes that we have a service to expose the scheduler REST API, and the service address need to be configured in configs.properties.
Could you please take a look at this update and share your thoughts? Thanks.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on a change in
pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r393416400
##########
File path: deployments/admission-controllers/scheduler/generate-signed-ca.sh
##########
@@ -24,8 +24,8 @@ if [ ! -f ${CONF_FILE} ]; then
fi
tmpdir="$1"
-service=`cat ${CONF_FILE} | grep ^service | cut -d "=" -f 2`
-namespace=`cat ${CONF_FILE} | grep ^namespace | cut -d "=" -f 2`
+service=$SERVICE
Review comment:
OK
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
yangwwei commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-598997292
Hi @TaoYang526
> I think we can only configure the name of service in configs.properties and get the http port of yunikorn service automatically in admission_util.sh via...
Sounds good.
> My doubt is that the namespace in admission-controller seems not related to the helm charts, are we expecting they may be different?
Right now when we do helm chart install, we pass in namespace as a CLI parameter and give value "yunikorn". We should always make scheduler and admission-controller running in same namespace. So the changes needed here is to adhere the namespace value passed in from helm, and populate that into the deployment template.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on a change in
pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r393415259
##########
File path: helm-charts/yunikorn/templates/deployment.yaml
##########
@@ -36,7 +36,7 @@ spec:
lifecycle:
postStart:
exec:
- command: ["/bin/sh", "/admission_util.sh", "create"]
+ command: ["/bin/sh", "/admission_util.sh", "create", "--env", "NAMESPACE={{ .Release.Namespace }}"]
Review comment:
I have tested this via calling command `helm install ./yunikorn --generate-name` , the result seems same as the command with `--namespace default`.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull
request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r390600401
##########
File path: deployments/admission-controllers/scheduler/templates/server.yaml.template
##########
@@ -17,7 +17,7 @@ spec:
spec:
containers:
- name: yunikorn-admission-controller-webhook
- image: yunikorn/yunikorn-scheduler-admission-controller:latest
+ image: reg.docker.alibaba-inc.com/boyuan/yunikorn-scheduler-admission-controller:latest
Review comment:
pls do not include alibaba repo
this line of change can be reverted
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull
request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r396895502
##########
File path: deployments/admission-controllers/scheduler/README.md
##########
@@ -56,6 +59,28 @@ kubectl get pod task0 -o yaml
you'll see the `schedulerName` has been injected with value `yunikorn`.
+#### Validations
+
+YuniKorn loads its configuration from a configmap called yunikorn-configs, this admission controller adds a web-hook to
+validate the update and create requests for this configmap.
+It is a safeguard to protect the scheduler not to load invalid configuration, by rejecting such requests.
Review comment:
Can we rephrase this to clearly explain the limits of what we have now:
This validation rejects invalid configuration which otherwise would have been rejected inside the scheduler update process. Validations are currently limited to a syntax check only. The scheduler could still reject a configuration if it cannot replace the current configuration due to conflicts.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on a change in
pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before
admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on a change in pull request #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#discussion_r390699900
##########
File path: deployments/admission-controllers/scheduler/templates/server.yaml.template
##########
@@ -17,7 +17,7 @@ spec:
spec:
containers:
- name: yunikorn-admission-controller-webhook
- image: yunikorn/yunikorn-scheduler-admission-controller:latest
+ image: reg.docker.alibaba-inc.com/boyuan/yunikorn-scheduler-admission-controller:latest
Review comment:
Sorry for forgetting to revert my testing repo.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-598806310
@wilfred-s @yangwwei
I have done the initial update in latest commit which validates configs via /ws/v1/validate-conf REST API (needs to be added later) in yunikorn scheduler, it assumes that we have a scheduler REST API service and need to be configured in configs.properties.
Could you please take a look at this update and share your thoughts? Thanks.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on issue #81:
[YUNIKORN-28] Support validating yunikorn-configs before admitting it
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #81: [YUNIKORN-28] Support validating yunikorn-configs before admitting it
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/81#issuecomment-597980392
Thanks @yangwwei for the comments.
I think we have discussed this way yesterday, this may not only increases complexity but also decreases reliability, it works well only when admission-controller and scheduler are both available. I prefer to decouple these two since admission-controller doesn't dependent on dynamic data in scheduler for now.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org