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/08/18 07:28:02 UTC
[GitHub] [incubator-yunikorn-core] kingamarton opened a new pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
kingamarton opened a new pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198
This is still a wip PR. I have to add the following things:
- [ ] unit tests
- [ ] revert the configmap change if saving the config fails
- [ ] return if there is already one update in progress
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] sunilgovind commented on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
sunilgovind commented on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-682597245
I think this makes sense to have either one of the option to be enabled in YK.
1. Via configmap hot refresh
2. Via UpdateConf REST API
At the time when we are expecting to have the config update from rest api, its always gonna be a mess and complex to watch for configmap (some one could edit via kube apis). And as a system, we will be patching holes all around.
I am not seeing a major reason to support both together. So +1 to this approach.
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-689202630
All look good to me now. +1
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-685439631
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=h1) Report
> Merging [#198](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/1083b40438cfc21de6d67deee27cf262be13c147?el=desc) will **decrease** coverage by `0.10%`.
> The diff coverage is `47.12%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #198 +/- ##
==========================================
- Coverage 60.88% 60.77% -0.11%
==========================================
Files 67 67
Lines 6537 6616 +79
==========================================
+ Hits 3980 4021 +41
- Misses 2419 2453 +34
- Partials 138 142 +4
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/cluster\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NsdXN0ZXJfaW5mby5nbw==) | `2.26% <0.00%> (-0.13%)` | :arrow_down: |
| [pkg/cache/partition\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BhcnRpdGlvbl9pbmZvLmdv) | `72.56% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `13.51% <ø> (ø)` | |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `49.00% <67.39%> (+4.12%)` | :arrow_up: |
| [pkg/cache/initializer.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2luaXRpYWxpemVyLmdv) | `81.25% <100.00%> (+0.60%)` | :arrow_up: |
| [pkg/plugins/plugins.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbnMvcGx1Z2lucy5nbw==) | `75.00% <100.00%> (+5.30%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=footer). Last update [1083b40...542903e](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] kingamarton commented on a change in pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#discussion_r484290449
##########
File path: pkg/webservice/handlers.go
##########
@@ -339,3 +341,67 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
http.Error(w, err.Error(), http.StatusInternalServerError)
}
}
+
+func updateConfig(w http.ResponseWriter, r *http.Request) {
+ lock.Lock()
+ defer lock.Unlock()
+ writeHeaders(w)
+ requestBytes, err := ioutil.ReadAll(r.Body)
+ if err != nil {
+ buildUpdateResponse(false, err.Error(), w)
+ return
+ }
+ // validation is already called when loading the config
+ schedulerConf, err := configs.LoadSchedulerConfigFromByteArray(requestBytes)
+ if err != nil {
+ buildUpdateResponse(false, err.Error(), w)
+ return
+ }
+ oldConf, err := updateConfiguration(string(requestBytes))
+ if err != nil {
+ buildUpdateResponse(false, err.Error(), w)
+ return
+ }
+ err = gClusterInfo.UpdateSchedulerConfig(schedulerConf)
+ if err != nil {
+ // revert configmap changes
+ _, err := updateConfiguration(oldConf)
+ if err != nil {
+ log.Logger().Error("Configuration rollback failed")
+ }
+ buildUpdateResponse(false, err.Error(), w)
Review comment:
Good catch! I was just focusing to fix the lint issue :). Fixed 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
[GitHub] [incubator-yunikorn-core] kingamarton commented on a change in pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#discussion_r477394549
##########
File path: pkg/webservice/handlers.go
##########
@@ -339,3 +342,55 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
http.Error(w, err.Error(), http.StatusInternalServerError)
}
}
+
+var mutex sync.Mutex
+func updateConfig(w http.ResponseWriter, r *http.Request) {
+ mutex.Lock()
+ defer mutex.Unlock()
+ writeHeaders(w)
+ requestBytes, err := ioutil.ReadAll(r.Body)
+
+ if err == nil {
Review comment:
I added a log message when creating the response. Also handled this case as well.
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] yangwwei edited a comment on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
yangwwei edited a comment on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-682337194
> @yangwwei, I already checked that part and I am afraid that a dup refresh is not possible(or it not straightforward how to do it) to prevent in every case because the config reloader is triggered at the moment when the configmap is changed. However I will have to find out a solution for this issue, because I debugged the actual workflow and I can see that after the configuration is refreshed in the scheduler, the configwatcher will revert the changes, because it is triggered, but since there is a delay in observing the changes in the pod, it detects the difference between the pod(still having the old config) and the scheduler (having already the new config), and will change the configuration in the scheduler.
Got it, this is exactly what I was concerned about.
I think there is an easier solution to fix this. On the shim side, we can add a CLI option, e.g `enableConfigHotRefresh`. If this value is `false`, we can bypass https://github.com/apache/incubator-yunikorn-k8shim/blob/ac345b38432b416aede47e3d51d5e120011c56fd/pkg/cache/context.go#L283-L293. This code triggers the config refresh.
We should only allow only ONE updateConf mechanism at runtime, either
1. Via configmap hot refresh
2. Via UpdateConf REST API
not both. When the hot refresh is enabled. We should deny the UpdateConf calls from the REST 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
[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#discussion_r483826590
##########
File path: pkg/webservice/handlers.go
##########
@@ -339,3 +341,67 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
http.Error(w, err.Error(), http.StatusInternalServerError)
}
}
+
+func updateConfig(w http.ResponseWriter, r *http.Request) {
+ lock.Lock()
+ defer lock.Unlock()
+ writeHeaders(w)
+ requestBytes, err := ioutil.ReadAll(r.Body)
+ if err != nil {
+ buildUpdateResponse(false, err.Error(), w)
+ return
+ }
+ // validation is already called when loading the config
+ schedulerConf, err := configs.LoadSchedulerConfigFromByteArray(requestBytes)
+ if err != nil {
+ buildUpdateResponse(false, err.Error(), w)
+ return
+ }
+ oldConf, err := updateConfiguration(string(requestBytes))
+ if err != nil {
+ buildUpdateResponse(false, err.Error(), w)
+ return
+ }
+ err = gClusterInfo.UpdateSchedulerConfig(schedulerConf)
+ if err != nil {
+ // revert configmap changes
+ _, err := updateConfiguration(oldConf)
+ if err != nil {
+ log.Logger().Error("Configuration rollback failed")
+ }
+ buildUpdateResponse(false, err.Error(), w)
Review comment:
these lines seem to be problematic.
line 368 has overwritten the value of `err`, and line 372 is out of the `err != nil` check.
that means if `err == nil`, line 372 will give an NPE 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
[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-685439631
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=h1) Report
> Merging [#198](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/4b8da89cbe010331e54d223fc3bfd0771c1d00c6?el=desc) will **increase** coverage by `0.38%`.
> The diff coverage is `65.11%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #198 +/- ##
==========================================
+ Coverage 60.88% 61.26% +0.38%
==========================================
Files 67 67
Lines 6537 6615 +78
==========================================
+ Hits 3980 4053 +73
+ Misses 2419 2417 -2
- Partials 138 145 +7
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/partition\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BhcnRpdGlvbl9pbmZvLmdv) | `72.56% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `13.51% <ø> (ø)` | |
| [pkg/cache/cluster\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NsdXN0ZXJfaW5mby5nbw==) | `9.75% <51.61%> (+7.35%)` | :arrow_up: |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `48.80% <66.66%> (+3.92%)` | :arrow_up: |
| [pkg/cache/initializer.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2luaXRpYWxpemVyLmdv) | `81.25% <100.00%> (+0.60%)` | :arrow_up: |
| [pkg/plugins/plugins.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbnMvcGx1Z2lucy5nbw==) | `75.00% <100.00%> (+5.30%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=footer). Last update [4b8da89...f43bb78](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-685439631
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=h1) Report
> Merging [#198](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/1083b40438cfc21de6d67deee27cf262be13c147?el=desc) will **decrease** coverage by `0.10%`.
> The diff coverage is `47.12%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #198 +/- ##
==========================================
- Coverage 60.88% 60.77% -0.11%
==========================================
Files 67 67
Lines 6537 6616 +79
==========================================
+ Hits 3980 4021 +41
- Misses 2419 2453 +34
- Partials 138 142 +4
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/cluster\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NsdXN0ZXJfaW5mby5nbw==) | `2.26% <0.00%> (-0.13%)` | :arrow_down: |
| [pkg/cache/partition\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BhcnRpdGlvbl9pbmZvLmdv) | `72.56% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `13.51% <ø> (ø)` | |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `49.00% <67.39%> (+4.12%)` | :arrow_up: |
| [pkg/cache/initializer.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2luaXRpYWxpemVyLmdv) | `81.25% <100.00%> (+0.60%)` | :arrow_up: |
| [pkg/plugins/plugins.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbnMvcGx1Z2lucy5nbw==) | `75.00% <100.00%> (+5.30%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=footer). Last update [1083b40...dcd73a1](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] TravisBuddy commented on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-687250570
Hey @kingamarton,
Your changes look good to me!
<a href="https://travis-ci.org/apache/incubator-yunikorn-core/builds/724226958">View build log</a>
###### TravisBuddy Request Identifier: fd67e370-eeca-11ea-9705-b7c481b3600a
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] TravisBuddy commented on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-686473471
## Travis tests have failed
Hey @kingamarton,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.
### 1st Build
<a href="https://travis-ci.org/apache/incubator-yunikorn-core/jobs/723760539">View build log</a>
<details>
<summary>
<strong>
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.22.2
</strong>
</summary>
```
golangci/golangci-lint info checking GitHub for tag 'v1.22.2'
golangci/golangci-lint info found version: 1.22.2 for v1.22.2/linux/amd64
golangci/golangci-lint info installed /home/travis/gopath/bin/golangci-lint
```
</details>
<details>
<summary>
<strong>
make lint
</strong>
</summary>
```
running golangci-lint
pkg/webservice/handlers_test.go:454: unnecessary trailing newline (whitespace)
}
pkg/cache/cluster_info.go:51: File is not `goimports`-ed with -local github.com/apache/incubator-yunikorn (goimports)
EventHandlers handler.EventHandlers
Makefile:45: recipe for target 'lint' failed
make: *** [lint] Error 1
```
</details>
###### TravisBuddy Request Identifier: a27b6170-ede5-11ea-a128-157d03751755
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-685439631
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=h1) Report
> Merging [#198](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/57d663e73cb10674873b119ae1eaf61ad43fdcd9?el=desc) will **decrease** coverage by `0.22%`.
> The diff coverage is `38.88%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #198 +/- ##
==========================================
- Coverage 60.61% 60.39% -0.23%
==========================================
Files 67 67
Lines 6533 6615 +82
==========================================
+ Hits 3960 3995 +35
- Misses 2427 2469 +42
- Partials 146 151 +5
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/cluster\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NsdXN0ZXJfaW5mby5nbw==) | `2.23% <0.00%> (-0.16%)` | :arrow_down: |
| [pkg/cache/partition\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BhcnRpdGlvbl9pbmZvLmdv) | `72.56% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `13.51% <ø> (ø)` | |
| [pkg/plugins/plugins.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbnMvcGx1Z2lucy5nbw==) | `67.50% <57.14%> (-2.20%)` | :arrow_down: |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `48.38% <65.11%> (+3.50%)` | :arrow_up: |
| [pkg/cache/initializer.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2luaXRpYWxpemVyLmdv) | `81.25% <100.00%> (+0.60%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=footer). Last update [57d663e...aedb13c](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] kingamarton commented on a change in pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#discussion_r482941347
##########
File path: pkg/cache/cluster_info.go
##########
@@ -483,12 +484,33 @@ func (m *ClusterInfo) processRMRegistrationEvent(event *commonevents.RegisterRME
// Updated and deleted partitions can not fail on the scheduler side.
// Locking occurs by the methods that are called, this must be lock free.
func (m *ClusterInfo) processRMConfigUpdateEvent(event *commonevents.ConfigUpdateRMEvent) {
+ if m.skipConfigUpdate {
Review comment:
removed it
##########
File path: pkg/webservice/handlers.go
##########
@@ -339,3 +341,63 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
http.Error(w, err.Error(), http.StatusInternalServerError)
}
}
+
+func updateConfig(w http.ResponseWriter, r *http.Request) {
+ lock.Lock()
+ defer lock.Unlock()
+ writeHeaders(w)
+ requestBytes, err := ioutil.ReadAll(r.Body)
+ if err != nil {
+ buildUpdateResponse(false, err.Error(), w)
+ return
+ }
+ // validation is already called when loading the config
+ schedulerConf, err := configs.LoadSchedulerConfigFromByteArray(requestBytes)
+ if err != nil {
+ buildUpdateResponse(false, err.Error(), w)
+ return
+ }
+ oldConf, err := updateConfiguration(string(requestBytes))
+ if err != nil {
+ buildUpdateResponse(false, err.Error(), w)
+ return
+ }
+ err = gClusterInfo.UpdateSchedulerConfig(schedulerConf)
+ if err != nil {
+ // revert configmap changes
+ _, err := updateConfiguration(oldConf)
+ if err != nil {
+ log.Logger().Error("Configuration rollback failed")
+ }
+ buildUpdateResponse(false, err.Error(), w)
+ return
+ }
+ buildUpdateResponse(true, "", w)
+}
+
+func buildUpdateResponse(success bool, reason string, w http.ResponseWriter) {
Review comment:
Changed 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
[GitHub] [incubator-yunikorn-core] kingamarton commented on a change in pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#discussion_r477394849
##########
File path: pkg/webservice/handlers.go
##########
@@ -339,3 +342,55 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
http.Error(w, err.Error(), http.StatusInternalServerError)
}
}
+
+var mutex sync.Mutex
+func updateConfig(w http.ResponseWriter, r *http.Request) {
+ mutex.Lock()
+ defer mutex.Unlock()
+ writeHeaders(w)
+ requestBytes, err := ioutil.ReadAll(r.Body)
+
+ if err == nil {
+ // validation is already called when loading the config
+ schedulerConf, err2 := configs.LoadSchedulerConfigFromByteArray(requestBytes)
+ if err2 != nil {
+ buildUpdateResponse(false, err2.Error(), w)
+ return
+ }
+ oldConf, err3 := saveConfigmap(string(requestBytes))
+ if err3 != nil {
+ buildUpdateResponse(false, err3.Error(), w)
+ return
+ }
+ err4 := gClusterInfo.UpdateSchedulerConfig(schedulerConf)
+ if err4 != nil {
+ // revert configmap changes
+ saveConfigmap(oldConf)
+ buildUpdateResponse(false, err4.Error(), w)
+ return
+ }
+ buildUpdateResponse(true, "", w)
+ }
+}
+
+func buildUpdateResponse(success bool, reason string, w http.ResponseWriter) {
+ var result dao.UpdateConfResponse
+ result.Success = success
+ result.Reason = reason
+ if err := json.NewEncoder(w).Encode(result); err != nil {
+ http.Error(w, err.Error(), http.StatusInternalServerError)
+ }
+}
+func saveConfigmap(conf string) (string, error) {
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
[GitHub] [incubator-yunikorn-core] yangwwei merged pull request #198: [YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
yangwwei merged pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] kingamarton commented on a change in pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#discussion_r477394148
##########
File path: pkg/plugins/types.go
##########
@@ -62,3 +63,7 @@ type ContainerSchedulingStateUpdater interface {
// the shim side implementation must be thread safe
Update(request *si.UpdateContainerSchedulingStateRequest)
}
+
+type ConfigMapPlugin interface {
+ UpdateConfigMap(args *si.ConfigMapArgs) (string, error)
Review comment:
Renamed
##########
File path: pkg/webservice/handlers.go
##########
@@ -339,3 +342,55 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
http.Error(w, err.Error(), http.StatusInternalServerError)
}
}
+
+var mutex sync.Mutex
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
[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-682959071
Agree with @sunilgovind . We should be more restrictive about this. I don't think we need to support both options in a deployment.
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] kingamarton commented on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
kingamarton commented on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-682578819
> > @yangwwei, I already checked that part and I am afraid that a dup refresh is not possible(or it not straightforward how to do it) to prevent in every case because the config reloader is triggered at the moment when the configmap is changed. However I will have to find out a solution for this issue, because I debugged the actual workflow and I can see that after the configuration is refreshed in the scheduler, the configwatcher will revert the changes, because it is triggered, but since there is a delay in observing the changes in the pod, it detects the difference between the pod(still having the old config) and the scheduler (having already the new config), and will change the configuration in the scheduler.
>
> Got it, this is exactly what I was concerned about.
> I think there is an easier solution to fix this. On the shim side, we can add a CLI option, e.g `enableConfigHotRefresh`. If this value is `false`, we can bypass https://github.com/apache/incubator-yunikorn-k8shim/blob/ac345b38432b416aede47e3d51d5e120011c56fd/pkg/cache/context.go#L283-L293. This code triggers the config refresh.
>
> We should only allow ONE updateConf mechanism at runtime, either
>
> 1. Via configmap hot refresh
> 2. Via UpdateConf REST API
>
> not both. When the hot refresh is enabled. We should deny the UpdateConf calls from the REST API.
@yangwwei, by limiting the mechanisms might solve some issues, but for me is strange to limit this, once both of them are available. What would be the default one? I can imagine use cases when editing directly the configmap is easier, and some other cases when the REST API can be used. How will the user change what he/she want's to use?
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] TravisBuddy commented on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-686484752
Hey @kingamarton,
Your changes look good to me!
<a href="https://travis-ci.org/apache/incubator-yunikorn-core/builds/723779647">View build log</a>
###### TravisBuddy Request Identifier: 05c8c220-ede8-11ea-a128-157d03751755
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] TravisBuddy commented on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-689184847
Hey @kingamarton,
Your changes look good to me!
<a href="https://travis-ci.org/apache/incubator-yunikorn-core/builds/725392573">View build log</a>
###### TravisBuddy Request Identifier: 36761270-f229-11ea-ba41-b1d3a642fad8
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-685439631
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=h1) Report
> Merging [#198](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/4b8da89cbe010331e54d223fc3bfd0771c1d00c6?el=desc) will **increase** coverage by `0.35%`.
> The diff coverage is `62.92%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #198 +/- ##
==========================================
+ Coverage 60.88% 61.24% +0.35%
==========================================
Files 67 67
Lines 6537 6618 +81
==========================================
+ Hits 3980 4053 +73
- Misses 2419 2420 +1
- Partials 138 145 +7
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/partition\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BhcnRpdGlvbl9pbmZvLmdv) | `72.56% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `13.51% <ø> (ø)` | |
| [pkg/cache/cluster\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NsdXN0ZXJfaW5mby5nbw==) | `9.75% <51.61%> (+7.35%)` | :arrow_up: |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `48.22% <62.50%> (+3.34%)` | :arrow_up: |
| [pkg/cache/initializer.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2luaXRpYWxpemVyLmdv) | `81.25% <100.00%> (+0.60%)` | :arrow_up: |
| [pkg/plugins/plugins.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbnMvcGx1Z2lucy5nbw==) | `75.00% <100.00%> (+5.30%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=footer). Last update [4b8da89...f43bb78](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] kingamarton commented on a change in pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#discussion_r477395473
##########
File path: pkg/cache/cluster_info.go
##########
@@ -711,3 +720,29 @@ func (m *ClusterInfo) processRemovedApplication(event *cacheevent.RemovedApplica
fmt.Sprintf("Application %s Removed", event.ApplicationID))
}
}
+
+func (m *ClusterInfo) UpdateSchedulerConfig(conf *configs.SchedulerConfig) error {
+ rmID := ""
+ for _, pi := range m.partitions {
+ rmID = pi.RmID
+ break
+ }
+ updatedPartitions, deletedPartitions, err := m.applyConfigChanges(conf, rmID)
+ if err != nil {
+ return err
+ }
+ if len(updatedPartitions) > 0 {
+ result := m.sendUpdatedPartitionsToScheduler(updatedPartitions)
+ if !result.Succeeded {
+ return fmt.Errorf(result.Reason)
+ }
+ }
+ if len(deletedPartitions) > 0 {
+ result2 := m.sendDeletedPartitionsToScheduler(deletedPartitions)
+ if !result2.Succeeded {
+ return fmt.Errorf(result2.Reason)
+ }
+ }
+ return nil
+}
+
Review comment:
Yes, this can be an issue. I suggest to think about it in a follow up task, because this is how it works the auto-refresh as well.
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] kingamarton commented on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
kingamarton commented on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-680954715
> hi @kingamarton
>
> Thanks for the PR. Except for the changes in this PR, could you please check the logic here:
>
> https://github.com/apache/incubator-yunikorn-core/blob/742e21611d15da8e3c8d5441f8d3b42d8532634c/pkg/common/configs/configwatcher.go#L93-L97
>
> , see if this is enough to prevent a dup refresh?
@yangwwei, I already checked that part and I am afraid that a dup refresh is not possible(or it not straightforward how to do it) to prevent in every case because the config reloader is triggered at the moment when the configmap is changed. However I will have to find out a solution for this issue, because I debugged the actual workflow and I can see that after the configuration is refreshed in the scheduler, the configwatcher will revert the changes, because it is triggered, but since there is a delay in observing the changes in the pod, it detects the difference between the pod(still having the old config) and the scheduler (having already the new config), and will change the configuration in the scheduler.
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-685439631
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=h1) Report
> Merging [#198](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/1083b40438cfc21de6d67deee27cf262be13c147?el=desc) will **decrease** coverage by `0.49%`.
> The diff coverage is `38.88%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #198 +/- ##
==========================================
- Coverage 60.88% 60.39% -0.50%
==========================================
Files 67 67
Lines 6537 6615 +78
==========================================
+ Hits 3980 3995 +15
- Misses 2419 2469 +50
- Partials 138 151 +13
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/cluster\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NsdXN0ZXJfaW5mby5nbw==) | `2.23% <0.00%> (-0.16%)` | :arrow_down: |
| [pkg/cache/partition\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BhcnRpdGlvbl9pbmZvLmdv) | `72.56% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `13.51% <ø> (ø)` | |
| [pkg/plugins/plugins.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbnMvcGx1Z2lucy5nbw==) | `67.50% <57.14%> (-2.20%)` | :arrow_down: |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `48.38% <65.11%> (+3.50%)` | :arrow_up: |
| [pkg/cache/initializer.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2luaXRpYWxpemVyLmdv) | `81.25% <100.00%> (+0.60%)` | :arrow_up: |
| [pkg/scheduler/placement/fixed\_rule.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wbGFjZW1lbnQvZml4ZWRfcnVsZS5nbw==) | `63.26% <0.00%> (-12.74%)` | :arrow_down: |
| [pkg/scheduler/placement/user\_rule.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wbGFjZW1lbnQvdXNlcl9ydWxlLmdv) | `68.29% <0.00%> (-10.28%)` | :arrow_down: |
| [pkg/scheduler/placement/provided\_rule.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wbGFjZW1lbnQvcHJvdmlkZWRfcnVsZS5nbw==) | `70.45% <0.00%> (-9.55%)` | :arrow_down: |
| [pkg/scheduler/placement/tag\_rule.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wbGFjZW1lbnQvdGFnX3J1bGUuZ28=) | `75.51% <0.00%> (-4.49%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=footer). Last update [1083b40...542903e](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] TravisBuddy commented on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-685426596
## Travis tests have failed
Hey @kingamarton,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.
### 1st Build
<a href="https://travis-ci.org/apache/incubator-yunikorn-core/jobs/723358750">View build log</a>
<details>
<summary>
<strong>
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.22.2
</strong>
</summary>
```
golangci/golangci-lint info checking GitHub for tag 'v1.22.2'
golangci/golangci-lint info found version: 1.22.2 for v1.22.2/linux/amd64
golangci/golangci-lint info installed /home/travis/gopath/bin/golangci-lint
```
</details>
<details>
<summary>
<strong>
make lint
</strong>
</summary>
```
running golangci-lint
pkg/webservice/dao/config_info.go:27: File is not `gofmt`-ed with `-s` (gofmt)
Success bool `json:"success"`
Reason string `json:"reason"`
pkg/plugins/types.go:32: File is not `gofmt`-ed with `-s` (gofmt)
configPlugin ConfigurationPlugin
pkg/cache/cluster_info.go:513: File is not `gofmt`-ed with `-s` (gofmt)
func(m *ClusterInfo) sendUpdatedPartitionsToScheduler(updatedPartitions []*PartitionInfo) *commonevents.Result {
pkg/webservice/webservice.go:37: File is not `gofmt`-ed with `-s` (gofmt)
var lock sync.RWMutex
pkg/webservice/handlers_test.go:376: File is not `gofmt`-ed with `-s` (gofmt)
func (f fakeConfigPlugin) UpdateConfiguration (args *si.UpdateConfigurationRequest) *si.UpdateConfigurationResponse {
pkg/webservice/handlers.go:23: File is not `goimports`-ed with -local github.com/apache/incubator-yunikorn (goimports)
"go.uber.org/zap"
"gopkg.in/yaml.v2"
pkg/webservice/handlers.go:398: unnecessary trailing newline (whitespace)
}
pkg/webservice/handlers.go:367:22: Error return value of `updateConfiguration` is not checked (errcheck)
updateConfiguration(oldConf)
^
pkg/webservice/handlers_test.go:474:9: Error return value of `http.NewRequest` is not checked (errcheck)
req, _ := http.NewRequest("PUT", "", strings.NewReader(tc.newConf))
^
pkg/webservice/handlers.go:395:10: `if` block ends with a `return` statement, so drop this `else` and outdent its block (golint)
} else {
^
Makefile:45: recipe for target 'lint' failed
make: *** [lint] Error 1
```
</details>
###### TravisBuddy Request Identifier: 7d593320-ecf2-11ea-b6ba-27fe84eba218
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] TravisBuddy commented on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-687221201
## Travis tests have failed
Hey @kingamarton,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.
### 1st Build
<a href="https://travis-ci.org/apache/incubator-yunikorn-core/jobs/724209889">View build log</a>
<details>
<summary>
<strong>
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.22.2
</strong>
</summary>
```
golangci/golangci-lint info checking GitHub for tag 'v1.22.2'
golangci/golangci-lint info found version: 1.22.2 for v1.22.2/linux/amd64
golangci/golangci-lint info installed /home/travis/gopath/bin/golangci-lint
```
</details>
<details>
<summary>
<strong>
make lint
</strong>
</summary>
```
running golangci-lint
pkg/webservice/handlers.go:387:10: Error return value of `w.Write` is not checked (errcheck)
w.Write([]byte("Configuration updates successfully"))
^
pkg/common/testUtils/event_handler_mock.go:19:1: don't use MixedCaps in package name; testUtils should be testutils (golint)
package testUtils
^
Makefile:45: recipe for target 'lint' failed
make: *** [lint] Error 1
```
</details>
###### TravisBuddy Request Identifier: 8596fe00-eec3-11ea-9705-b7c481b3600a
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-685439631
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=h1) Report
> Merging [#198](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/1083b40438cfc21de6d67deee27cf262be13c147?el=desc) will **decrease** coverage by `0.10%`.
> The diff coverage is `47.12%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #198 +/- ##
==========================================
- Coverage 60.88% 60.77% -0.11%
==========================================
Files 67 67
Lines 6537 6616 +79
==========================================
+ Hits 3980 4021 +41
- Misses 2419 2453 +34
- Partials 138 142 +4
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/cluster\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NsdXN0ZXJfaW5mby5nbw==) | `2.26% <0.00%> (-0.13%)` | :arrow_down: |
| [pkg/cache/partition\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BhcnRpdGlvbl9pbmZvLmdv) | `72.56% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `13.51% <ø> (ø)` | |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `49.00% <67.39%> (+4.12%)` | :arrow_up: |
| [pkg/cache/initializer.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2luaXRpYWxpemVyLmdv) | `81.25% <100.00%> (+0.60%)` | :arrow_up: |
| [pkg/plugins/plugins.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbnMvcGx1Z2lucy5nbw==) | `75.00% <100.00%> (+5.30%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=footer). Last update [1083b40...408adb8](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] kingamarton commented on a change in pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#discussion_r483690598
##########
File path: pkg/webservice/handlers.go
##########
@@ -339,3 +341,63 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
http.Error(w, err.Error(), http.StatusInternalServerError)
}
}
+
+func updateConfig(w http.ResponseWriter, r *http.Request) {
+ lock.Lock()
+ defer lock.Unlock()
+ writeHeaders(w)
+ requestBytes, err := ioutil.ReadAll(r.Body)
+ if err != nil {
+ buildUpdateResponse(false, err.Error(), w)
+ return
+ }
+ // validation is already called when loading the config
+ schedulerConf, err := configs.LoadSchedulerConfigFromByteArray(requestBytes)
+ if err != nil {
+ buildUpdateResponse(false, err.Error(), w)
+ return
+ }
+ oldConf, err := updateConfiguration(string(requestBytes))
+ if err != nil {
+ buildUpdateResponse(false, err.Error(), w)
+ return
+ }
+ err = gClusterInfo.UpdateSchedulerConfig(schedulerConf)
+ if err != nil {
+ // revert configmap changes
+ _, err := updateConfiguration(oldConf)
+ if err != nil {
+ log.Logger().Error("Configuration rollback failed")
+ }
+ buildUpdateResponse(false, err.Error(), w)
+ return
+ }
+ buildUpdateResponse(true, "", w)
+}
+
+func buildUpdateResponse(success bool, reason string, w http.ResponseWriter) {
Review comment:
@yangwwei , If we return StatusConflict in case of failure, I think there is no need to have an UpdateConfResponse, because we can set the status code to 200-OK and return a message that the config update was successful, so I removed the UpdateConfResponse and set 200-OK in case of success.
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#discussion_r482335349
##########
File path: pkg/cache/cluster_info.go
##########
@@ -47,8 +48,8 @@ type ClusterInfo struct {
pendingSchedulerEvents chan interface{}
// RM Event Handler
- EventHandlers handler.EventHandlers
-
+ EventHandlers handler.EventHandlers
+ skipConfigUpdate bool
Review comment:
based on the discussion, I don't think we need this flag anymore.
Let's keep the core side change as simple as possible
##########
File path: pkg/webservice/handlers.go
##########
@@ -339,3 +341,63 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
http.Error(w, err.Error(), http.StatusInternalServerError)
}
}
+
+func updateConfig(w http.ResponseWriter, r *http.Request) {
+ lock.Lock()
+ defer lock.Unlock()
+ writeHeaders(w)
+ requestBytes, err := ioutil.ReadAll(r.Body)
+ if err != nil {
+ buildUpdateResponse(false, err.Error(), w)
+ return
+ }
+ // validation is already called when loading the config
+ schedulerConf, err := configs.LoadSchedulerConfigFromByteArray(requestBytes)
+ if err != nil {
+ buildUpdateResponse(false, err.Error(), w)
+ return
+ }
+ oldConf, err := updateConfiguration(string(requestBytes))
+ if err != nil {
+ buildUpdateResponse(false, err.Error(), w)
+ return
+ }
+ err = gClusterInfo.UpdateSchedulerConfig(schedulerConf)
+ if err != nil {
+ // revert configmap changes
+ _, err := updateConfiguration(oldConf)
+ if err != nil {
+ log.Logger().Error("Configuration rollback failed")
+ }
+ buildUpdateResponse(false, err.Error(), w)
+ return
+ }
+ buildUpdateResponse(true, "", w)
+}
+
+func buildUpdateResponse(success bool, reason string, w http.ResponseWriter) {
Review comment:
for the REST API response. If update was successful, we should do
```
json.NewEncoder(w).Encode(result)
```
however, if the update was not successful, I think we should return an error code to the client
```
http.Error(w, err.Error(), http.StatusConflict))
```
where the StatusConflict means: The request could not be completed due to a conflict with the current state of the target resource. This code is used in situations where the user might be able to resolve the conflict and resubmit the request.
##########
File path: pkg/cache/cluster_info.go
##########
@@ -483,12 +484,33 @@ func (m *ClusterInfo) processRMRegistrationEvent(event *commonevents.RegisterRME
// Updated and deleted partitions can not fail on the scheduler side.
// Locking occurs by the methods that are called, this must be lock free.
func (m *ClusterInfo) processRMConfigUpdateEvent(event *commonevents.ConfigUpdateRMEvent) {
+ if m.skipConfigUpdate {
Review comment:
remove this flag
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] TravisBuddy commented on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-685439502
Hey @kingamarton,
Your changes look good to me!
<a href="https://travis-ci.org/apache/incubator-yunikorn-core/builds/723365577">View build log</a>
###### TravisBuddy Request Identifier: b20583f0-ecf5-11ea-b6ba-27fe84eba218
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] yangwwei edited a comment on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
yangwwei edited a comment on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-682337194
> @yangwwei, I already checked that part and I am afraid that a dup refresh is not possible(or it not straightforward how to do it) to prevent in every case because the config reloader is triggered at the moment when the configmap is changed. However I will have to find out a solution for this issue, because I debugged the actual workflow and I can see that after the configuration is refreshed in the scheduler, the configwatcher will revert the changes, because it is triggered, but since there is a delay in observing the changes in the pod, it detects the difference between the pod(still having the old config) and the scheduler (having already the new config), and will change the configuration in the scheduler.
Got it, this is exactly what I was concerned about.
I think there is an easier solution to fix this. On the shim side, we can add a CLI option, e.g `enableConfigHotRefresh`. If this value is `false`, we can bypass https://github.com/apache/incubator-yunikorn-k8shim/blob/ac345b38432b416aede47e3d51d5e120011c56fd/pkg/cache/context.go#L283-L293. This code triggers the config refresh.
We should only allow ONE updateConf mechanism at runtime, either
1. Via configmap hot refresh
2. Via UpdateConf REST API
not both. When the hot refresh is enabled. We should deny the UpdateConf calls from the REST 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
[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-685439631
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=h1) Report
> Merging [#198](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/1083b40438cfc21de6d67deee27cf262be13c147?el=desc) will **increase** coverage by `0.38%`.
> The diff coverage is `65.11%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #198 +/- ##
==========================================
+ Coverage 60.88% 61.26% +0.38%
==========================================
Files 67 67
Lines 6537 6615 +78
==========================================
+ Hits 3980 4053 +73
+ Misses 2419 2417 -2
- Partials 138 145 +7
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/partition\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BhcnRpdGlvbl9pbmZvLmdv) | `72.56% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `13.51% <ø> (ø)` | |
| [pkg/cache/cluster\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NsdXN0ZXJfaW5mby5nbw==) | `9.75% <51.61%> (+7.35%)` | :arrow_up: |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `48.80% <66.66%> (+3.92%)` | :arrow_up: |
| [pkg/cache/initializer.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2luaXRpYWxpemVyLmdv) | `81.25% <100.00%> (+0.60%)` | :arrow_up: |
| [pkg/plugins/plugins.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbnMvcGx1Z2lucy5nbw==) | `75.00% <100.00%> (+5.30%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=footer). Last update [1083b40...dcd73a1](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] kingamarton commented on a change in pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#discussion_r477394730
##########
File path: pkg/webservice/handlers.go
##########
@@ -339,3 +342,55 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
http.Error(w, err.Error(), http.StatusInternalServerError)
}
}
+
+var mutex sync.Mutex
+func updateConfig(w http.ResponseWriter, r *http.Request) {
+ mutex.Lock()
+ defer mutex.Unlock()
+ writeHeaders(w)
+ requestBytes, err := ioutil.ReadAll(r.Body)
+
+ if err == nil {
+ // validation is already called when loading the config
+ schedulerConf, err2 := configs.LoadSchedulerConfigFromByteArray(requestBytes)
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
[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-682337194
> @yangwwei, I already checked that part and I am afraid that a dup refresh is not possible(or it not straightforward how to do it) to prevent in every case because the config reloader is triggered at the moment when the configmap is changed. However I will have to find out a solution for this issue, because I debugged the actual workflow and I can see that after the configuration is refreshed in the scheduler, the configwatcher will revert the changes, because it is triggered, but since there is a delay in observing the changes in the pod, it detects the difference between the pod(still having the old config) and the scheduler (having already the new config), and will change the configuration in the scheduler.
Got it, this is exactly what I was concerned about.
I think there is an easier solution to fix this. On the shim side, we can add a CLI option, e.g `enableConfigHotRefresh`. If this value is `false`, we can bypass https://github.com/apache/incubator-yunikorn-k8shim/blob/ac345b38432b416aede47e3d51d5e120011c56fd/pkg/cache/context.go#L283-L293. This code triggers the config refresh.
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] kingamarton commented on a change in pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#discussion_r483688905
##########
File path: pkg/cache/cluster_info.go
##########
@@ -47,8 +48,8 @@ type ClusterInfo struct {
pendingSchedulerEvents chan interface{}
// RM Event Handler
- EventHandlers handler.EventHandlers
-
+ EventHandlers handler.EventHandlers
+ skipConfigUpdate bool
Review comment:
You are right, I removed 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
[GitHub] [incubator-yunikorn-core] codecov[bot] commented on pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#issuecomment-685439631
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=h1) Report
> Merging [#198](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/57d663e73cb10674873b119ae1eaf61ad43fdcd9?el=desc) will **decrease** coverage by `0.22%`.
> The diff coverage is `38.88%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #198 +/- ##
==========================================
- Coverage 60.61% 60.39% -0.23%
==========================================
Files 67 67
Lines 6533 6615 +82
==========================================
+ Hits 3960 3995 +35
- Misses 2427 2469 +42
- Partials 146 151 +5
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/cluster\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NsdXN0ZXJfaW5mby5nbw==) | `2.23% <0.00%> (-0.16%)` | :arrow_down: |
| [pkg/cache/partition\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BhcnRpdGlvbl9pbmZvLmdv) | `72.56% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `13.51% <ø> (ø)` | |
| [pkg/plugins/plugins.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbnMvcGx1Z2lucy5nbw==) | `67.50% <57.14%> (-2.20%)` | :arrow_down: |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `48.38% <65.11%> (+3.50%)` | :arrow_up: |
| [pkg/cache/initializer.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2luaXRpYWxpemVyLmdv) | `81.25% <100.00%> (+0.60%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=footer). Last update [57d663e...f6cf8db](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/198?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #198: [WIP][YUNIKORN-366] Add rest API to update queue configuration
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #198:
URL: https://github.com/apache/incubator-yunikorn-core/pull/198#discussion_r477056978
##########
File path: pkg/webservice/handlers.go
##########
@@ -339,3 +342,55 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
http.Error(w, err.Error(), http.StatusInternalServerError)
}
}
+
+var mutex sync.Mutex
Review comment:
can we use the RWMutex defined here: https://github.com/apache/incubator-yunikorn-core/blob/742e21611d15da8e3c8d5441f8d3b42d8532634c/pkg/webservice/webservice.go#L41. But looks like we need to move it out of the constructor.
##########
File path: pkg/plugins/types.go
##########
@@ -62,3 +63,7 @@ type ContainerSchedulingStateUpdater interface {
// the shim side implementation must be thread safe
Update(request *si.UpdateContainerSchedulingStateRequest)
}
+
+type ConfigMapPlugin interface {
+ UpdateConfigMap(args *si.ConfigMapArgs) (string, error)
Review comment:
Can we rename the API to `UpdateConfiguration`?
I think we should not assume the configuration must be in a configmap, because configmap is just a form of persistent storage. Similarly, we should rename `ConfigMapArgs` to `UpdateConfigurationRequest`. And the return type is better to be `(UpdateConfigurationResponse, error)`.
##########
File path: pkg/webservice/handlers.go
##########
@@ -339,3 +342,55 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
http.Error(w, err.Error(), http.StatusInternalServerError)
}
}
+
+var mutex sync.Mutex
+func updateConfig(w http.ResponseWriter, r *http.Request) {
+ mutex.Lock()
+ defer mutex.Unlock()
+ writeHeaders(w)
+ requestBytes, err := ioutil.ReadAll(r.Body)
+
+ if err == nil {
Review comment:
when `err != nil` we should log an error in the server-side and response the client with an error too.,
##########
File path: pkg/webservice/handlers.go
##########
@@ -339,3 +342,55 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
http.Error(w, err.Error(), http.StatusInternalServerError)
}
}
+
+var mutex sync.Mutex
+func updateConfig(w http.ResponseWriter, r *http.Request) {
+ mutex.Lock()
+ defer mutex.Unlock()
+ writeHeaders(w)
+ requestBytes, err := ioutil.ReadAll(r.Body)
+
+ if err == nil {
+ // validation is already called when loading the config
+ schedulerConf, err2 := configs.LoadSchedulerConfigFromByteArray(requestBytes)
+ if err2 != nil {
+ buildUpdateResponse(false, err2.Error(), w)
+ return
+ }
+ oldConf, err3 := saveConfigmap(string(requestBytes))
+ if err3 != nil {
+ buildUpdateResponse(false, err3.Error(), w)
+ return
+ }
+ err4 := gClusterInfo.UpdateSchedulerConfig(schedulerConf)
+ if err4 != nil {
+ // revert configmap changes
+ saveConfigmap(oldConf)
+ buildUpdateResponse(false, err4.Error(), w)
+ return
+ }
+ buildUpdateResponse(true, "", w)
+ }
+}
+
+func buildUpdateResponse(success bool, reason string, w http.ResponseWriter) {
+ var result dao.UpdateConfResponse
+ result.Success = success
+ result.Reason = reason
+ if err := json.NewEncoder(w).Encode(result); err != nil {
+ http.Error(w, err.Error(), http.StatusInternalServerError)
+ }
+}
+func saveConfigmap(conf string) (string, error) {
Review comment:
Rename `saveConfigmap` to `updateConfiguration`
##########
File path: pkg/cache/cluster_info.go
##########
@@ -711,3 +720,29 @@ func (m *ClusterInfo) processRemovedApplication(event *cacheevent.RemovedApplica
fmt.Sprintf("Application %s Removed", event.ApplicationID))
}
}
+
+func (m *ClusterInfo) UpdateSchedulerConfig(conf *configs.SchedulerConfig) error {
+ rmID := ""
+ for _, pi := range m.partitions {
+ rmID = pi.RmID
+ break
+ }
+ updatedPartitions, deletedPartitions, err := m.applyConfigChanges(conf, rmID)
+ if err != nil {
+ return err
+ }
+ if len(updatedPartitions) > 0 {
+ result := m.sendUpdatedPartitionsToScheduler(updatedPartitions)
+ if !result.Succeeded {
+ return fmt.Errorf(result.Reason)
+ }
+ }
+ if len(deletedPartitions) > 0 {
+ result2 := m.sendDeletedPartitionsToScheduler(deletedPartitions)
+ if !result2.Succeeded {
+ return fmt.Errorf(result2.Reason)
+ }
+ }
+ return nil
+}
+
Review comment:
It is still possible that failure might happen while trying to delete some partitions.
The `updatedPartition` is already processed, but `deletedPartition` is not, the config becomes inconsistent.
I am not sure if we can have a more robust way to handle this based on the current structure, maybe we can leave this as the future work. As the auto-refresh code is exactly the same.
##########
File path: pkg/webservice/handlers.go
##########
@@ -339,3 +342,55 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
http.Error(w, err.Error(), http.StatusInternalServerError)
}
}
+
+var mutex sync.Mutex
+func updateConfig(w http.ResponseWriter, r *http.Request) {
+ mutex.Lock()
+ defer mutex.Unlock()
+ writeHeaders(w)
+ requestBytes, err := ioutil.ReadAll(r.Body)
+
+ if err == nil {
+ // validation is already called when loading the config
+ schedulerConf, err2 := configs.LoadSchedulerConfigFromByteArray(requestBytes)
Review comment:
Can we re-use err instead of having multiple ones err2, err3, and err4?
----------------------------------------------------------------
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