force release of quota when AW is deleted in target namespace #365
Conversation
asm582
changed the title
| if err != nil { | ||
| return nil, fmt.Errorf("consumer %s already allocated on forest %s", consumerID, forestName) | ||
| } else { | ||
| //release quota | ||
| m.DeAllocateForest(forestName, consumerID) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm confused here. If the consumerID is allocated it is removed in the else, but if it's allocated and no AppWrapper is found for the consumerID then it isn't deallocated?
There was a problem hiding this comment.
I'm confused too. The consumer shouldn't exist to begin with. So if it does, then that is an error. I get that from line 393. But then if it doesn't exist in the forest, then it is deallocated from the forest, where it doesn't exist?
| if err != nil { | ||
| return nil, fmt.Errorf("consumer %s already allocated on forest %s", consumerID, forestName) | ||
| } else { | ||
| //release quota | ||
| m.DeAllocateForest(forestName, consumerID) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm confused too. The consumer shouldn't exist to begin with. So if it does, then that is an error. I get that from line 393. But then if it doesn't exist in the forest, then it is deallocated from the forest, where it doesn't exist?
pkg/quotaplugins/quota-forest/quota-manager/quota/quotamanager.go
Outdated
Show resolved
Hide resolved
pkg/quotaplugins/quota-forest/quota-manager/quota/quotamanager.go
Outdated
Show resolved
Hide resolved
pkg/quotaplugins/quota-forest/quota-manager/quota/quotamanager.go
Outdated
Show resolved
Hide resolved
pkg/quotaplugins/quota-forest/quota-manager/quota/quotamanager.go
Outdated
Show resolved
Hide resolved
pkg/quotaplugins/quota-forest/quota-manager/quota/quotamanager.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
In line 404 where we deallocate the consumer if the AppWrapper doesn't exist, we should add a log. That is a really funky behavior that should be recorded somehow. Message could be [AllocateForest] Found consumer %v in quota tree but didn't find its matching AppWrapper. Deleting consumer from the tree. or something like it. I'll let you choose the logging level.
pkg/quotaplugins/quota-forest/quota-manager/quota/quotamanager.go
Outdated
Show resolved
Hide resolved
|
pkg/quotaplugins/quota-forest/quota-manager/quota/quotamanager.go
Outdated
Show resolved
Hide resolved
pkg/quotaplugins/quota-forest/quota-manager/quota/quotamanager.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I think that the logic for checking if the AW is a namespace does not belong to the qutamanger struct. It should be placed (if needed) a lot higher in the call chain: the Fits method call:
quotaFits, preemptAWs, msg := qjm.quotaManager.Fits(qj, aggqj, proposedPreemptions) or inside the method itself.
Good Point, I will move the code higher up |
asm582
changed the title
| @@ -1267,7 +1269,9 @@ func (qjm *XController) ScheduleNext() { | |||
| if qjm.quotaManager != nil { | |||
| var msg string | |||
| var preemptAWs []*arbv1.AppWrapper | |||
| quotaFits, preemptAWs, msg = qjm.quotaManager.Fits(qj, aggqj, proposedPreemptions) | |||
| //quota trees fail to update with AW deletes, release quota before assigning | |||
| qjm.quotaManager.Release(qj) | |||
There was a problem hiding this comment.
I am not sure if this makes any sense here now, it seems superfluous. Have you observed a scenario where having this call here would be beneficial?
|
This PR should by replaced by #398 once completed. |
No description provided.