Finalizing objects are mutable #77135
Comments
thockin
added
kind/bug
|
/assign @lavalamp |
|
/cc |
|
I can see why you'd expect that, but it can be valid for system components to mutate the object (yes, even spec) as part of the shutdown process. So, it is risky to blanket-ban these modifications. You can opt-in to a ban by e.g. explicitly setting deletion timestamp to nil in a patch or PUT; this will always fail validation if the object already has a deletion timestamp. |
|
Humm..This may be problematic for service LB finalizer (kubernetes/enhancements#992) --- service controller might be cleaning up wrong resources. One specific case:
|
|
I'm mostly looking for guidance to implementors. This is effectively
a new state of being that they might want to consider. E.g. I would
like Service to reject changes while finalizing so I don't try to spin
up LBs or change ports.
…On Mon, Apr 29, 2019 at 2:32 PM Daniel Smith ***@***.***> wrote:
I can see why you'd expect that, but it can be valid for system components to mutate the object (yes, even spec) as part of the shutdown process. So, it is risky to blanket-ban these modifications.
You can opt-in to a ban by e.g. explicitly setting deletion timestamp to nil in a patch or PUT; this will always fail validation if the object already has a deletion timestamp.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
So can we validate that deletionTimestamp is not already set on an update,
and if it is fail the update?
…On Mon, Apr 29, 2019 at 2:54 PM Zihong Zheng ***@***.***> wrote:
Humm..This may be problematic for service LB finalizer (
kubernetes/enhancements#992
<kubernetes/enhancements#992>) --- service
controller might be cleaning up wrong resources. One specific case:
- Delete LB type service with externalTrafficPolicy=Local.
- (Before service controller gets a chance to process the deletion)
Change service to externalTrafficPolicy=Cluster.
- Service controller tries to cleanup resource based on name. But our
naming scheme depends on how externalTrafficPolicy is configured.
Hence it does cleanup on the wrong ones.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#77135 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABKWAVG4ZOXMRM44BHYCOSLPS5VBJANCNFSM4HIYJEAQ>
.
|
|
...for spec
…On Mon, Apr 29, 2019 at 3:06 PM Tim Hockin ***@***.***> wrote:
So can we validate that deletionTimestamp is not already set on an update,
and if it is fail the update?
On Mon, Apr 29, 2019 at 2:54 PM Zihong Zheng ***@***.***>
wrote:
> Humm..This may be problematic for service LB finalizer (
> kubernetes/enhancements#992
> <kubernetes/enhancements#992>) --- service
> controller might be cleaning up wrong resources. One specific case:
>
> - Delete LB type service with externalTrafficPolicy=Local.
> - (Before service controller gets a chance to process the deletion)
> Change service to externalTrafficPolicy=Cluster.
> - Service controller tries to cleanup resource based on name. But our
> naming scheme depends on how externalTrafficPolicy is configured.
> Hence it does cleanup on the wrong ones.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#77135 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABKWAVG4ZOXMRM44BHYCOSLPS5VBJANCNFSM4HIYJEAQ>
> .
>
|
|
Yes, if you're 100% sure no update will ever need to change that to finish
deleting the service. The penalty for being wrong is an object that is
wedged and can't be removed without manual intervention. So, if you want to
do this, don't be wrong :)
I think it's OK to do this on a case-by-case basis. I don't think it's
great to have a blanket rule, something will end up wedged if we do that.
…On Mon, Apr 29, 2019 at 3:09 PM Tim Hockin ***@***.***> wrote:
...for spec
On Mon, Apr 29, 2019 at 3:06 PM Tim Hockin ***@***.***> wrote:
> So can we validate that deletionTimestamp is not already set on an
update,
> and if it is fail the update?
>
> On Mon, Apr 29, 2019 at 2:54 PM Zihong Zheng ***@***.***>
> wrote:
>
>> Humm..This may be problematic for service LB finalizer (
>> kubernetes/enhancements#992
>> <kubernetes/enhancements#992>) --- service
>> controller might be cleaning up wrong resources. One specific case:
>>
>> - Delete LB type service with externalTrafficPolicy=Local.
>> - (Before service controller gets a chance to process the deletion)
>> Change service to externalTrafficPolicy=Cluster.
>> - Service controller tries to cleanup resource based on name. But our
>> naming scheme depends on how externalTrafficPolicy is configured.
>> Hence it does cleanup on the wrong ones.
>>
>> —
>> You are receiving this because you authored the thread.
>> Reply to this email directly, view it on GitHub
>> <
#77135 (comment)
>,
>> or mute the thread
>> <
https://github.com/notifications/unsubscribe-auth/ABKWAVG4ZOXMRM44BHYCOSLPS5VBJANCNFSM4HIYJEAQ
>
>> .
>>
>
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#77135 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAE6BFRDQNH55XSVUI2SPHTPS5WYTANCNFSM4HIYJEAQ>
.
|
|
What you wrote is already good guidance.
On Mon, Apr 29, 2019 at 3:28 PM Daniel Smith <[email protected]>
wrote:
… Yes, if you're 100% sure no update will ever need to change that to finish
deleting the service. The penalty for being wrong is an object that is
wedged and can't be removed without manual intervention. So, if you want to
do this, don't be wrong :)
I think it's OK to do this on a case-by-case basis. I don't think it's
great to have a blanket rule, something will end up wedged if we do that.
On Mon, Apr 29, 2019 at 3:09 PM Tim Hockin ***@***.***>
wrote:
> ...for spec
>
> On Mon, Apr 29, 2019 at 3:06 PM Tim Hockin ***@***.***> wrote:
>
> > So can we validate that deletionTimestamp is not already set on an
> update,
> > and if it is fail the update?
> >
> > On Mon, Apr 29, 2019 at 2:54 PM Zihong Zheng ***@***.***
>
> > wrote:
> >
> >> Humm..This may be problematic for service LB finalizer (
> >> kubernetes/enhancements#992
> >> <kubernetes/enhancements#992>) --- service
> >> controller might be cleaning up wrong resources. One specific case:
> >>
> >> - Delete LB type service with externalTrafficPolicy=Local.
> >> - (Before service controller gets a chance to process the deletion)
> >> Change service to externalTrafficPolicy=Cluster.
> >> - Service controller tries to cleanup resource based on name. But our
> >> naming scheme depends on how externalTrafficPolicy is configured.
> >> Hence it does cleanup on the wrong ones.
> >>
> >> —
> >> You are receiving this because you authored the thread.
> >> Reply to this email directly, view it on GitHub
> >> <
>
#77135 (comment)
> >,
> >> or mute the thread
> >> <
>
https://github.com/notifications/unsubscribe-auth/ABKWAVG4ZOXMRM44BHYCOSLPS5VBJANCNFSM4HIYJEAQ
> >
> >> .
> >>
> >
>
> —
> You are receiving this because you were assigned.
> Reply to this email directly, view it on GitHub
> <
#77135 (comment)
>,
> or mute the thread
> <
https://github.com/notifications/unsubscribe-auth/AAE6BFRDQNH55XSVUI2SPHTPS5WYTANCNFSM4HIYJEAQ
>
> .
>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#77135 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABKWAVF4WJQ2WGXONVQSN73PS5Y75ANCNFSM4HIYJEAQ>
.
|
|
/triage unresolved Comment 🤖 I am a bot run by @vllry. 👩🔬 |
k8s-ci-robot
added
the
triage/unresolved
|
@lavalamp If you aren't able to handle this issue, consider unassigning yourself and/or adding the 🤖 I am a bot run by vllry. 👩🔬 |
|
I worry about creating resource instances in "limbo" state such that they can't be fixed, as with Allocs in Borgmaster. The Service LB cleanup process sounds unsound. Changing the type and then immediately deleting would cause the same problem, no? Why not create a finalizer specific to the types of resources being cleaned up? Or actually enumerate the resources somewhere? Or create a separate CR to represent the resources? |
|
Finalizer is what is happening now (for Service), but as I explored the
semantics, I found this topic and I thought it was worth discussing to see
if there was a larger impact.
…On Tue, May 7, 2019 at 8:23 AM Brian Grant ***@***.***> wrote:
I worry about creating resource instances in "limbo" state such that they
can't be fixed, as with Allocs in Borgmaster.
The Service LB cleanup process sounds unsound. Changing the type and then
immediately deleting would cause the same problem, no?
Why not create a finalizer specific to the types of resources being
cleaned up? Or actually enumerate the resources somewhere? Or create a
separate CR to represent the resources?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#77135 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABKWAVGJBZPRRJ23W6WUSXTPUGNHZANCNFSM4HIYJEAQ>
.
|
Good point.. I guess we haven't experienced such problem that much because service controller caches service's previous state. But with finalizer we plan to remove the cache layer, and will have to consider this case.
Sounds viable, though can't easily think of a non-cloud-provider specific solution out of this --- I guess the simplest version would be annotating service with the created resources (#70159) and make sure cloud controller does the cleanup and remove annotations? |
|
Removing sig-arch tag, please re-tag us when there is a proposal or KEP /remove-sig architecture |
k8s-ci-robot
removed
the
sig/architecture
|
@lavalamp If you aren't able to handle this issue, consider unassigning yourself and/or adding the 🤖 I am a bot run by vllry. 👩🔬 |
|
cc @bowei |
What happened:
Delete an object (Service in this case) with a finalizer. Mutate the Service.
What you expected to happen:
I expected an error.
Anything else we need to know?:
It's debatable whether this should be a blanket ban on mutability. Status seems important to mutate. Metadata too, probably. Maybe it should be resource-by-resource? E.g. chaning the
portsortypeof a Service should not be allowed when being deleted.At the very minimum we should have guidance for resource implementors to follow on when to reject mutations and what error messages to deliver, or (if we think mutability is good) guidance that they must explicitly allow it.
The text was updated successfully, but these errors were encountered: