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 2022/03/16 19:46:42 UTC
[GitHub] [incubator-yunikorn-k8shim] craigcondit opened a new pull request #389: [YUNIKORN-1118] Make admission controller config validation soft-succeed if scheduler is unreachable
craigcondit opened a new pull request #389:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/389
### What is this PR for?
This PR makes config validation more reliable as it will allow a configmap update even if the scheduler is unreachable or down for some reason. In other words, we will only fail to update the configmap if it is known to be invalid.
### What type of PR is it?
* [ ] - Bug Fix
* [x] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-1118
### How should this be tested?
Unit tests updated to exercise new failure cases.
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] craigcondit commented on a change in pull request #389: [YUNIKORN-1118] Make admission controller config validation soft-succeed if scheduler is unreachable
Posted by GitBox <gi...@apache.org>.
craigcondit commented on a change in pull request #389:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/389#discussion_r828618595
##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
##########
@@ -363,29 +364,44 @@ func (c *admissionController) shouldLabelNamespace(namespace string) bool {
func (c *admissionController) validateConfigMap(cm *v1.ConfigMap) error {
if cm.Name == constants.DefaultConfigMapName {
- log.Logger().Info("validating yunikorn configs")
if content, ok := cm.Data[c.configName]; ok {
+ checksum := fmt.Sprintf("%X", sha256.Sum256([]byte(content)))
+ log.Logger().Info("Validating YuniKorn configuration", zap.String("checksum", checksum))
+ log.Logger().Debug("Configmap data", zap.ByteString("content", []byte(content)))
response, err := http.Post(c.schedulerValidateConfURL, "application/json", bytes.NewBuffer([]byte(content)))
if err != nil {
- return err
+ log.Logger().Error("YuniKorn scheduler is unreachable, assuming configmap is valid", zap.Error(err))
+ return nil
}
defer response.Body.Close()
+ if response.StatusCode < 200 || response.StatusCode > 299 {
+ log.Logger().Error("YuniKorn scheduler responded with unexpected status, assuming configmap is valid",
+ zap.Int("status", response.StatusCode))
+ return nil
+ }
responseBytes, err := io.ReadAll(response.Body)
if err != nil {
- return err
+ log.Logger().Error("Unable to read response from YuniKorn scheduler, assuming configmap is valid", zap.Error(err))
+ return nil
}
var responseData ValidateConfResponse
if err := json.Unmarshal(responseBytes, &responseData); err != nil {
Review comment:
We have to return nil r else the result would be validation failure. We do log the err.
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] wilfred-s closed pull request #389: [YUNIKORN-1118] Make admission controller config validation soft-succeed if scheduler is unreachable
Posted by GitBox <gi...@apache.org>.
wilfred-s closed pull request #389:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/389
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull request #389: [YUNIKORN-1118] Make admission controller config validation soft-succeed if scheduler is unreachable
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #389:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/389#discussion_r828616092
##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
##########
@@ -363,29 +364,44 @@ func (c *admissionController) shouldLabelNamespace(namespace string) bool {
func (c *admissionController) validateConfigMap(cm *v1.ConfigMap) error {
if cm.Name == constants.DefaultConfigMapName {
- log.Logger().Info("validating yunikorn configs")
if content, ok := cm.Data[c.configName]; ok {
+ checksum := fmt.Sprintf("%X", sha256.Sum256([]byte(content)))
+ log.Logger().Info("Validating YuniKorn configuration", zap.String("checksum", checksum))
+ log.Logger().Debug("Configmap data", zap.ByteString("content", []byte(content)))
response, err := http.Post(c.schedulerValidateConfURL, "application/json", bytes.NewBuffer([]byte(content)))
if err != nil {
- return err
+ log.Logger().Error("YuniKorn scheduler is unreachable, assuming configmap is valid", zap.Error(err))
+ return nil
}
defer response.Body.Close()
+ if response.StatusCode < 200 || response.StatusCode > 299 {
+ log.Logger().Error("YuniKorn scheduler responded with unexpected status, assuming configmap is valid",
+ zap.Int("status", response.StatusCode))
+ return nil
+ }
responseBytes, err := io.ReadAll(response.Body)
if err != nil {
- return err
+ log.Logger().Error("Unable to read response from YuniKorn scheduler, assuming configmap is valid", zap.Error(err))
+ return nil
}
var responseData ValidateConfResponse
if err := json.Unmarshal(responseBytes, &responseData); err != nil {
Review comment:
NIT: don't shadow err
##########
File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
##########
@@ -363,29 +364,44 @@ func (c *admissionController) shouldLabelNamespace(namespace string) bool {
func (c *admissionController) validateConfigMap(cm *v1.ConfigMap) error {
if cm.Name == constants.DefaultConfigMapName {
- log.Logger().Info("validating yunikorn configs")
if content, ok := cm.Data[c.configName]; ok {
+ checksum := fmt.Sprintf("%X", sha256.Sum256([]byte(content)))
+ log.Logger().Info("Validating YuniKorn configuration", zap.String("checksum", checksum))
+ log.Logger().Debug("Configmap data", zap.ByteString("content", []byte(content)))
response, err := http.Post(c.schedulerValidateConfURL, "application/json", bytes.NewBuffer([]byte(content)))
if err != nil {
- return err
+ log.Logger().Error("YuniKorn scheduler is unreachable, assuming configmap is valid", zap.Error(err))
+ return nil
}
defer response.Body.Close()
+ if response.StatusCode < 200 || response.StatusCode > 299 {
+ log.Logger().Error("YuniKorn scheduler responded with unexpected status, assuming configmap is valid",
+ zap.Int("status", response.StatusCode))
+ return nil
+ }
responseBytes, err := io.ReadAll(response.Body)
if err != nil {
- return err
+ log.Logger().Error("Unable to read response from YuniKorn scheduler, assuming configmap is valid", zap.Error(err))
+ return nil
}
var responseData ValidateConfResponse
if err := json.Unmarshal(responseBytes, &responseData); err != nil {
- return err
+ log.Logger().Error("Unable to parse response from YuniKorn scheduler, assuming configmap is valid", zap.Error(err))
+ return nil
}
if !responseData.Allowed {
- return fmt.Errorf(responseData.Reason)
+ err = fmt.Errorf(responseData.Reason)
+ log.Logger().Error("Configmap validation failed, aborting", zap.Error(err))
+ return err
}
} else {
- return fmt.Errorf("required config '%s' not found in this configmap", c.configName)
+ err := fmt.Errorf("required config '%s' not found in this configmap", c.configName)
Review comment:
NIT: don't shadow err
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] codecov[bot] commented on pull request #389: [YUNIKORN-1118] Make admission controller config validation soft-succeed if scheduler is unreachable
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #389:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/389#issuecomment-1069557939
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/389?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#389](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/389?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1bb36c6) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/ab7f4bcd3704ffee84365f42bd1925445d43af1d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ab7f4bc) will **increase** coverage by `0.05%`.
> The diff coverage is `77.27%`.
```diff
@@ Coverage Diff @@
## master #389 +/- ##
==========================================
+ Coverage 65.76% 65.81% +0.05%
==========================================
Files 40 40
Lines 6215 6230 +15
==========================================
+ Hits 4087 4100 +13
- Misses 1973 1975 +2
Partials 155 155
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/389?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/389/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `64.01% <77.27%> (+1.04%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/389?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/389?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ab7f4bc...1bb36c6](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/389?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org