You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "advancedxy (via GitHub)" <gi...@apache.org> on 2023/01/31 11:46:46 UTC
[GitHub] [incubator-uniffle] advancedxy opened a new pull request, #531: [ISSUE-524][operator] Upgrading rss could also be deleted
advancedxy opened a new pull request, #531:
URL: https://github.com/apache/incubator-uniffle/pull/531
### What changes were proposed in this pull request?
1. soft the constraint that upgrading rss object cannot be deleted
### Why are the changes needed?
more flexibility.
This fixes #524
### Does this PR introduce _any_ user-facing change?
For RSS cluster admin, they can delete a upgrading rss cluster.
### How was this patch tested?
Manually verified. Will add more uts.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #531: [ISSUE-524][operator] Upgrading rss could also be deleted
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #531:
URL: https://github.com/apache/incubator-uniffle/pull/531#discussion_r1092809817
##########
deploy/kubernetes/operator/pkg/webhook/inspector/rss.go:
##########
@@ -35,19 +35,19 @@ import (
// validateRSS validates the create and update operation towards rss objects.
func (i *inspector) validateRSS(ar *admissionv1.AdmissionReview) *admissionv1.AdmissionReview {
if !i.ignoreRSS && ar.Request.Operation == admissionv1.Update {
- oldRSS := &unifflev1alpha1.RemoteShuffleService{}
+ oldRSS := &uniffleAPI.RemoteShuffleService{}
if err := json.Unmarshal(ar.Request.OldObject.Raw, oldRSS); err != nil {
klog.Errorf("unmarshal old object of rss (%v) failed: %v",
string(ar.Request.OldObject.Raw), err)
return util.AdmissionReviewFailed(ar, err)
}
- // for security purposes, we forbid updating rss objects when they are in upgrading phase.
- if oldRSS.Status.Phase == unifflev1alpha1.RSSUpgrading {
- message := "can not update upgrading rss object: " + utils.UniqueName(oldRSS)
+ // for security purposes, we forbid updating rss objects when they are in upgrading/terminating phase.
+ if oldRSS.Status.Phase == uniffleAPI.RSSUpgrading || oldRSS.Status.Phase == uniffleAPI.RSSTerminating {
Review Comment:
> Sorry, webhook is not currently intercepting the subresource status, so updatestatus is not affected.
Yeah, so we don't have to allow `old.Upgrading && new.Terminating`.
> But after Terminating, there will be an action to delete finalizer. Is this also not blocked in your test environment ?
When I'm testing, denied update request when being terminating is not added. But I think you raised a good point.
If a deletion operation is issued, the object is already marked terminating by setting `deletionTimestamp`, does k8S manager itself handles update protection automatically? or does crd controller needed to enforce it?
if K8S itself already handles, then this line of code is unnecessary?
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #531: [ISSUE-524][operator] Upgrading rss could also be deleted
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #531:
URL: https://github.com/apache/incubator-uniffle/pull/531#discussion_r1092831683
##########
deploy/kubernetes/operator/pkg/webhook/inspector/rss.go:
##########
@@ -35,19 +35,19 @@ import (
// validateRSS validates the create and update operation towards rss objects.
func (i *inspector) validateRSS(ar *admissionv1.AdmissionReview) *admissionv1.AdmissionReview {
if !i.ignoreRSS && ar.Request.Operation == admissionv1.Update {
- oldRSS := &unifflev1alpha1.RemoteShuffleService{}
+ oldRSS := &uniffleAPI.RemoteShuffleService{}
if err := json.Unmarshal(ar.Request.OldObject.Raw, oldRSS); err != nil {
klog.Errorf("unmarshal old object of rss (%v) failed: %v",
string(ar.Request.OldObject.Raw), err)
return util.AdmissionReviewFailed(ar, err)
}
- // for security purposes, we forbid updating rss objects when they are in upgrading phase.
- if oldRSS.Status.Phase == unifflev1alpha1.RSSUpgrading {
- message := "can not update upgrading rss object: " + utils.UniqueName(oldRSS)
+ // for security purposes, we forbid updating rss objects when they are in upgrading/terminating phase.
+ if oldRSS.Status.Phase == uniffleAPI.RSSUpgrading || oldRSS.Status.Phase == uniffleAPI.RSSTerminating {
Review Comment:
discussed offline, current code doesn't handle deny update during terminating correct. The finalizers should be removed(update) by the controller. It will bring some complex logic to do it correctly.
However there's no need to deny update when terminating as the rss controller doesn't refer any other fields rather than finalizer/deletionTimestamp when terminating.
Conclusion: revert changes in webhook side.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] wangao1236 commented on a diff in pull request #531: [ISSUE-524][operator] Upgrading rss could also be deleted
Posted by "wangao1236 (via GitHub)" <gi...@apache.org>.
wangao1236 commented on code in PR #531:
URL: https://github.com/apache/incubator-uniffle/pull/531#discussion_r1092813634
##########
deploy/kubernetes/operator/pkg/webhook/inspector/rss.go:
##########
@@ -35,19 +35,19 @@ import (
// validateRSS validates the create and update operation towards rss objects.
func (i *inspector) validateRSS(ar *admissionv1.AdmissionReview) *admissionv1.AdmissionReview {
if !i.ignoreRSS && ar.Request.Operation == admissionv1.Update {
- oldRSS := &unifflev1alpha1.RemoteShuffleService{}
+ oldRSS := &uniffleAPI.RemoteShuffleService{}
if err := json.Unmarshal(ar.Request.OldObject.Raw, oldRSS); err != nil {
klog.Errorf("unmarshal old object of rss (%v) failed: %v",
string(ar.Request.OldObject.Raw), err)
return util.AdmissionReviewFailed(ar, err)
}
- // for security purposes, we forbid updating rss objects when they are in upgrading phase.
- if oldRSS.Status.Phase == unifflev1alpha1.RSSUpgrading {
- message := "can not update upgrading rss object: " + utils.UniqueName(oldRSS)
+ // for security purposes, we forbid updating rss objects when they are in upgrading/terminating phase.
+ if oldRSS.Status.Phase == uniffleAPI.RSSUpgrading || oldRSS.Status.Phase == uniffleAPI.RSSTerminating {
Review Comment:
The only protection mechanism is finalizer. This is not automatically added by the k8s, but by the controller. Controller needs to add and remove finalizers.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] wangao1236 commented on a diff in pull request #531: [ISSUE-524][operator] Upgrading rss could also be deleted
Posted by "wangao1236 (via GitHub)" <gi...@apache.org>.
wangao1236 commented on code in PR #531:
URL: https://github.com/apache/incubator-uniffle/pull/531#discussion_r1092783282
##########
deploy/kubernetes/operator/pkg/webhook/inspector/rss.go:
##########
@@ -35,19 +35,19 @@ import (
// validateRSS validates the create and update operation towards rss objects.
func (i *inspector) validateRSS(ar *admissionv1.AdmissionReview) *admissionv1.AdmissionReview {
if !i.ignoreRSS && ar.Request.Operation == admissionv1.Update {
- oldRSS := &unifflev1alpha1.RemoteShuffleService{}
+ oldRSS := &uniffleAPI.RemoteShuffleService{}
if err := json.Unmarshal(ar.Request.OldObject.Raw, oldRSS); err != nil {
klog.Errorf("unmarshal old object of rss (%v) failed: %v",
string(ar.Request.OldObject.Raw), err)
return util.AdmissionReviewFailed(ar, err)
}
- // for security purposes, we forbid updating rss objects when they are in upgrading phase.
- if oldRSS.Status.Phase == unifflev1alpha1.RSSUpgrading {
- message := "can not update upgrading rss object: " + utils.UniqueName(oldRSS)
+ // for security purposes, we forbid updating rss objects when they are in upgrading/terminating phase.
+ if oldRSS.Status.Phase == uniffleAPI.RSSUpgrading || oldRSS.Status.Phase == uniffleAPI.RSSTerminating {
Review Comment:
In a very early version, validatingwebhookconfiguration object contains "remoteshuffleservices/status" (deploy/kubernetes/operator/PKG/webhook/syncer/syncer.go: 244), I have forgotten 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.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] wangao1236 commented on a diff in pull request #531: [ISSUE-524][operator] Upgrading rss could also be deleted
Posted by "wangao1236 (via GitHub)" <gi...@apache.org>.
wangao1236 commented on code in PR #531:
URL: https://github.com/apache/incubator-uniffle/pull/531#discussion_r1092733798
##########
deploy/kubernetes/operator/pkg/webhook/inspector/rss.go:
##########
@@ -35,19 +35,19 @@ import (
// validateRSS validates the create and update operation towards rss objects.
func (i *inspector) validateRSS(ar *admissionv1.AdmissionReview) *admissionv1.AdmissionReview {
if !i.ignoreRSS && ar.Request.Operation == admissionv1.Update {
- oldRSS := &unifflev1alpha1.RemoteShuffleService{}
+ oldRSS := &uniffleAPI.RemoteShuffleService{}
if err := json.Unmarshal(ar.Request.OldObject.Raw, oldRSS); err != nil {
klog.Errorf("unmarshal old object of rss (%v) failed: %v",
string(ar.Request.OldObject.Raw), err)
return util.AdmissionReviewFailed(ar, err)
}
- // for security purposes, we forbid updating rss objects when they are in upgrading phase.
- if oldRSS.Status.Phase == unifflev1alpha1.RSSUpgrading {
- message := "can not update upgrading rss object: " + utils.UniqueName(oldRSS)
+ // for security purposes, we forbid updating rss objects when they are in upgrading/terminating phase.
+ if oldRSS.Status.Phase == uniffleAPI.RSSUpgrading || oldRSS.Status.Phase == uniffleAPI.RSSTerminating {
Review Comment:
I think the following should be allowed:
oldRSS.Status.Phase == uniffleAPI.RSSUpgrading && newRSS.status == uniffleAPI.RSSTerminating
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] codecov-commenter commented on pull request #531: [ISSUE-524][operator] Upgrading rss could also be deleted
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #531:
URL: https://github.com/apache/incubator-uniffle/pull/531#issuecomment-1411611153
# [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/531?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 [#531](https://codecov.io/gh/apache/incubator-uniffle/pull/531?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a7263f1) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/ebbe2db238cc6a611aea479fab988e23c636c952?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ebbe2db) will **decrease** coverage by `0.34%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #531 +/- ##
============================================
- Coverage 60.21% 59.87% -0.34%
+ Complexity 1784 1565 -219
============================================
Files 205 183 -22
Lines 11557 9725 -1832
Branches 1042 883 -159
============================================
- Hits 6959 5823 -1136
+ Misses 4190 3538 -652
+ Partials 408 364 -44
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/531?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...bernetes/operator/pkg/controller/controller/rss.go](https://codecov.io/gh/apache/incubator-uniffle/pull/531?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGVwbG95L2t1YmVybmV0ZXMvb3BlcmF0b3IvcGtnL2NvbnRyb2xsZXIvY29udHJvbGxlci9yc3MuZ28=) | `33.22% <0.00%> (+0.74%)` | :arrow_up: |
| [...y/kubernetes/operator/pkg/webhook/inspector/rss.go](https://codecov.io/gh/apache/incubator-uniffle/pull/531?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGVwbG95L2t1YmVybmV0ZXMvb3BlcmF0b3IvcGtnL3dlYmhvb2svaW5zcGVjdG9yL3Jzcy5nbw==) | `47.70% <ø> (ø)` | |
| [...storage/handler/impl/DataSkippableReadHandler.java](https://codecov.io/gh/apache/incubator-uniffle/pull/531?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmFnZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvdW5pZmZsZS9zdG9yYWdlL2hhbmRsZXIvaW1wbC9EYXRhU2tpcHBhYmxlUmVhZEhhbmRsZXIuamF2YQ==) | `83.78% <0.00%> (-2.71%)` | :arrow_down: |
| [...org/apache/spark/shuffle/RssSparkShuffleUtils.java](https://codecov.io/gh/apache/incubator-uniffle/pull/531?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS9Sc3NTcGFya1NodWZmbGVVdGlscy5qYXZh) | | |
| [...org/apache/spark/shuffle/writer/AddBlockEvent.java](https://codecov.io/gh/apache/incubator-uniffle/pull/531?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS93cml0ZXIvQWRkQmxvY2tFdmVudC5qYXZh) | | |
| [.../java/org/apache/spark/shuffle/RssSparkConfig.java](https://codecov.io/gh/apache/incubator-uniffle/pull/531?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS9Sc3NTcGFya0NvbmZpZy5qYXZh) | | |
| [.../java/org/apache/hadoop/mapreduce/RssMRConfig.java](https://codecov.io/gh/apache/incubator-uniffle/pull/531?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL1Jzc01SQ29uZmlnLmphdmE=) | | |
| [...k/shuffle/writer/WrappedByteArrayOutputStream.java](https://codecov.io/gh/apache/incubator-uniffle/pull/531?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS93cml0ZXIvV3JhcHBlZEJ5dGVBcnJheU91dHB1dFN0cmVhbS5qYXZh) | | |
| [...java/org/apache/hadoop/mapred/SortWriteBuffer.java](https://codecov.io/gh/apache/incubator-uniffle/pull/531?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkL1NvcnRXcml0ZUJ1ZmZlci5qYXZh) | | |
| [...n/java/org/apache/hadoop/mapreduce/RssMRUtils.java](https://codecov.io/gh/apache/incubator-uniffle/pull/531?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL1Jzc01SVXRpbHMuamF2YQ==) | | |
| ... and [16 more](https://codecov.io/gh/apache/incubator-uniffle/pull/531?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on pull request #531: [ISSUE-524][operator] Upgrading rss could also be deleted
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #531:
URL: https://github.com/apache/incubator-uniffle/pull/531#issuecomment-1411590774
@wangao1236 please take a look. This should be ready for merge.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on pull request #531: [ISSUE-524][operator] Upgrading rss could also be deleted
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #531:
URL: https://github.com/apache/incubator-uniffle/pull/531#issuecomment-1411355630
ping @wangao1236
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] wangao1236 commented on a diff in pull request #531: [ISSUE-524][operator] Upgrading rss could also be deleted
Posted by "wangao1236 (via GitHub)" <gi...@apache.org>.
wangao1236 commented on code in PR #531:
URL: https://github.com/apache/incubator-uniffle/pull/531#discussion_r1092778603
##########
deploy/kubernetes/operator/pkg/webhook/inspector/rss.go:
##########
@@ -35,19 +35,19 @@ import (
// validateRSS validates the create and update operation towards rss objects.
func (i *inspector) validateRSS(ar *admissionv1.AdmissionReview) *admissionv1.AdmissionReview {
if !i.ignoreRSS && ar.Request.Operation == admissionv1.Update {
- oldRSS := &unifflev1alpha1.RemoteShuffleService{}
+ oldRSS := &uniffleAPI.RemoteShuffleService{}
if err := json.Unmarshal(ar.Request.OldObject.Raw, oldRSS); err != nil {
klog.Errorf("unmarshal old object of rss (%v) failed: %v",
string(ar.Request.OldObject.Raw), err)
return util.AdmissionReviewFailed(ar, err)
}
- // for security purposes, we forbid updating rss objects when they are in upgrading phase.
- if oldRSS.Status.Phase == unifflev1alpha1.RSSUpgrading {
- message := "can not update upgrading rss object: " + utils.UniqueName(oldRSS)
+ // for security purposes, we forbid updating rss objects when they are in upgrading/terminating phase.
+ if oldRSS.Status.Phase == uniffleAPI.RSSUpgrading || oldRSS.Status.Phase == uniffleAPI.RSSTerminating {
Review Comment:
> Let me confirm one thing first, when controller updates rss' upgrading to terminating, is that update request also validated by this code path?
>
> In my local test, this line was kept intact, I only changed controller/rss.go:L357, and found upgrading rss object could be deleted normally.
Sorry, webhook is not currently intercepting the subresource status, so updatestatus is not affected. But after Terminating, there will be an action to delete finalizer. Is this also not blocked in your test environment ?
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #531: [ISSUE-524][operator] Upgrading rss could also be deleted
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #531:
URL: https://github.com/apache/incubator-uniffle/pull/531#discussion_r1092740065
##########
deploy/kubernetes/operator/pkg/webhook/inspector/rss.go:
##########
@@ -35,19 +35,19 @@ import (
// validateRSS validates the create and update operation towards rss objects.
func (i *inspector) validateRSS(ar *admissionv1.AdmissionReview) *admissionv1.AdmissionReview {
if !i.ignoreRSS && ar.Request.Operation == admissionv1.Update {
- oldRSS := &unifflev1alpha1.RemoteShuffleService{}
+ oldRSS := &uniffleAPI.RemoteShuffleService{}
if err := json.Unmarshal(ar.Request.OldObject.Raw, oldRSS); err != nil {
klog.Errorf("unmarshal old object of rss (%v) failed: %v",
string(ar.Request.OldObject.Raw), err)
return util.AdmissionReviewFailed(ar, err)
}
- // for security purposes, we forbid updating rss objects when they are in upgrading phase.
- if oldRSS.Status.Phase == unifflev1alpha1.RSSUpgrading {
- message := "can not update upgrading rss object: " + utils.UniqueName(oldRSS)
+ // for security purposes, we forbid updating rss objects when they are in upgrading/terminating phase.
+ if oldRSS.Status.Phase == uniffleAPI.RSSUpgrading || oldRSS.Status.Phase == uniffleAPI.RSSTerminating {
Review Comment:
Let me confirm one thing first, when controller updates rss' upgrading to terminating, is that update request also validated by this code path?
In my local test, this line was kept intact, I only changed controller/rss.go:L357, and found upgrading rss object could be deleted normally.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy merged pull request #531: [ISSUE-524][operator] Upgrading rss could also be deleted
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy merged PR #531:
URL: https://github.com/apache/incubator-uniffle/pull/531
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org