Block removing a valid lease in case not owned #11
Conversation
|
/hold |
|
/lgtm |
mshitrit
force-pushed
the
block-deleting-unowned-lease
branch
2 times, most recently
from
779f2f5
to
18d6f67
Compare
Signed-off-by: Michael Shitrit <[email protected]>
Signed-off-by: Michael Shitrit <[email protected]>
mshitrit
force-pushed
the
block-deleting-unowned-lease
branch
from
6f1d9de
to
2d2a950
Compare
pkg/lease/manager.go
Outdated
| @@ -227,6 +227,12 @@ func (l *manager) invalidateLease(ctx context.Context, obj client.Object) error | |||
| } | |||
| return err | |||
| } | |||
| if l.holderIdentity != *lease.Spec.HolderIdentity { | |||
| isLeaseExpired := time.Now().After(lease.Spec.RenewTime.Time.Add(time.Duration(*lease.Spec.LeaseDurationSeconds) * time.Second)) | |||
There was a problem hiding this comment.
I don't think we should invalidate not owned leases, even when they expired.
(and nil checks are missing, and we should reuse isValidLease, but that doesn't matter when we remove this...)
There was a problem hiding this comment.
I don't think we should invalidate not owned leases, even when they expired.
why not ? IIRC RequestLease would work for this use case (i.e we can request a lease owned by a different holder if it is expired)
There was a problem hiding this comment.
- what's the use case for invalidating not owned leases? 🤷🏼♂️
- why adding more code than needed for fixing this bug after code freeze?
There was a problem hiding this comment.
what's the use case for invalidating not owned leases? 🤷🏼♂️
I don't have a particular use case in mind, however I do think it makes sense to align the behavior to that of RequestLease, I think it's quite confusing that we will not allow deleting an expired lease directly (through Invalidate API) but it is fine to do it if we use RequestLease API.
why adding more code than needed for fixing this bug after code freeze?
I do agree that we should minimize code inserted this late before the release, however in this case it's a single "if" statement so IMO the value outweigh the risk.
I think that a better approach to follow this principal would be ATM having a fix only at NHC instead of both repos.
There was a problem hiding this comment.
align the behavior to that of RequestLease
There we have clear usecase.
I think it's quite confusing that we will not allow deleting an expired lease
I think it's more confusing to delete leases you don't own. When I create a lease and it's deleted by someone else, at least I would be surprised.
however in this case it's a single "if" statement
which had two missing nil checks in its 1st iteration...
a better approach to follow this principal would be ATM having a fix only at NHC
which would need more code, and would leave the issue here for the next one...
There was a problem hiding this comment.
ATM looks like we don't agree on whether InvalidateLease should only work on owned leases or not.
In order to get on with the release I've made the change so only owned leases will be Invalidated and we can have further discussion later on.
pkg/lease/manager_test.go
Outdated
| err = manager.RequestLease(context.Background(), node, leaseDuration) | ||
| switch actionOnLease { | ||
| case requestLease: | ||
| { |
There was a problem hiding this comment.
unneeded {} in both case blocks
There was a problem hiding this comment.
Hmm I don't follow, both requestLease and invalidateLease has { }.
Am I missing something ?
There was a problem hiding this comment.
you just don't need them, they are redundant.
Signed-off-by: Michael Shitrit <[email protected]>
Signed-off-by: Michael Shitrit <[email protected]>
Signed-off-by: Michael Shitrit <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clobrano, mshitrit, slintes The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
ECOPROJECT-1589
Block lease removal in case it is still valid and owned by a different holder