yangwwei commented on a change in pull request #324:
URL:
https://github.com/apache/incubator-yunikorn-k8shim/pull/324#discussion_r737696783
##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
##########
@@ -311,10 +328,14 @@ func (c *admissionController) serve(w
http.ResponseWriter, r *http.Request) {
resp, err := json.Marshal(admissionReview)
if err != nil {
- http.Error(w, fmt.Sprintf("could not encode response: %v",
err), http.StatusInternalServerError)
+ errMessage := fmt.Sprintf("could not encode response: %v", err)
+ log.Logger().Error(errMessage)
Review comment:
you can pass the err object to log.Logger().Error directly, that will
print the stacktrace as well.
##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
##########
@@ -311,10 +328,14 @@ func (c *admissionController) serve(w
http.ResponseWriter, r *http.Request) {
resp, err := json.Marshal(admissionReview)
if err != nil {
- http.Error(w, fmt.Sprintf("could not encode response: %v",
err), http.StatusInternalServerError)
+ errMessage := fmt.Sprintf("could not encode response: %v", err)
+ log.Logger().Error(errMessage)
+ http.Error(w, errMessage, http.StatusInternalServerError)
}
if _, err = w.Write(resp); err != nil {
- http.Error(w, fmt.Sprintf("could not write response: %v", err),
http.StatusInternalServerError)
+ errMessage := fmt.Sprintf("could not write response: %v", err)
+ log.Logger().Error(errMessage)
Review comment:
same as above
##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
##########
@@ -101,10 +107,16 @@ func (c *admissionController) mutate(ar
*v1beta1.AdmissionReview) *v1beta1.Admis
patch = updateSchedulerName(patch)
patch = updateLabels(namespace, &pod, patch)
+ log.Logger().Info(fmt.Sprintf("generated patch for pod %s\n",
pod.Name),
+ zap.Any("patch", patch))
+ } else {
+ log.Logger().Info(fmt.Sprintf("request kind (%s) is not pod,
but %s. Ignored.\n",
+ uid, requestKind))
Review comment:
better format needed
##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
##########
@@ -295,10 +309,13 @@ func (c *admissionController) serve(w
http.ResponseWriter, r *http.Request) {
Message: err.Error(),
},
}
- } else if r.URL.Path == mutateURL {
+ } else if urlPath == mutateURL {
admissionResponse = c.mutate(&ar)
- } else if r.URL.Path == validateConfURL {
+ } else if urlPath == validateConfURL {
admissionResponse = c.validateConf(&ar)
+ } else {
+ log.Logger().Warn(fmt.Sprintf("Request is neither mutation nor
validation: %s\n",
+ urlPath))
Review comment:
format the log
##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
##########
@@ -229,6 +239,8 @@ func (c *admissionController) validateConf(ar
*v1beta1.AdmissionReview) *v1beta1
},
}
}
+ } else {
+ log.Logger().Warn(fmt.Sprintf("request kind is not configmap,
but %s. Ignored.\n", requestKind))
Review comment:
format the log
##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
##########
@@ -101,10 +107,16 @@ func (c *admissionController) mutate(ar
*v1beta1.AdmissionReview) *v1beta1.Admis
patch = updateSchedulerName(patch)
patch = updateLabels(namespace, &pod, patch)
+ log.Logger().Info(fmt.Sprintf("generated patch for pod %s\n",
pod.Name),
+ zap.Any("patch", patch))
Review comment:
our logging package provides a easier way of logging, fmt package is not
needed here, see an example:
```
log.Logger().Info("AdmissionReview",
zap.Any("Kind", req.Kind),
zap.String("Namespace", namespace),
zap.String("UID", string(req.UID)),
zap.String("Operation", string(req.Operation)),
zap.Any("UserInfo", req.UserInfo))
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
