Delete expectations of a deleted rc instead of letting them expire #8000
Conversation
| @@ -327,6 +327,7 @@ func (rm *ReplicationManager) syncReplicationController(key string) error { | |||
| obj, exists, err := rm.controllerStore.Store.GetByKey(key) | |||
| if !exists { | |||
| glog.Infof("Replication Controller has been deleted %v", key) | |||
| rm.expectations.DeleteExpectations(key) | |||
There was a problem hiding this comment.
Sorry for delay reviewing; when an rc gets deleted, the proper behavior is for it to sync to the last requested state (if known) before RC manager forgets about it.
There was a problem hiding this comment.
hm. That gets complicated with expectations. Maybe this is better behavior. Ideally I'd like it to make one last attempt to sync; so if you set replicas=0 and then delete the rc, it would in fact delete all the replicas. But if you set replicas=5, delete the RC & add it again with a new name and replicas=17, RC Manager should not keep the old one around to fight with the new one...
There was a problem hiding this comment.
Doing that last sync is a little hard, though I'm sure I can figure out a way if that's needed. It's hard because we insert the key of the rc into the work queue, and by the time we pull it out the other side, the rc has been deleted from the store. I'd have to sidestep the work queue alltogether for deletes.
There was a problem hiding this comment.
OK I thought about it and I think that since we require the client to wait anyway, it's OK if RC manager doesn't do this for the moment.
|
LGTM; Can you rebase? |
bprashanth
force-pushed
the
rc_ttl_cleanup
branch
from
4aceaa5
to
c0a8981
Compare
|
Rebased |
lavalamp
added
the
lgtm
Delete expectations of a deleted rc instead of letting them expire
We currently let the expectations of an rc expire naturally. This PR deletes them explicitly when the rc is deleted. Also adds a unittest for rc deletion, which I apparently didn't have :\