Move NamespaceLifecycle to use shared informers #29926
Conversation
|
@hodovska - figured I would take this the last mile since you were looking at the controllers. |
| namespaceInformer framework.SharedIndexInformer | ||
| // forceLiveLookupCache holds a list of entries for namespaces that we have a strong reason to believe are stale in our local cache. | ||
| // if a namespace is in this cache, then we will ignore our local state and always fetch latest from api server. | ||
| forceLiveLookupCache *lru.Cache |
There was a problem hiding this comment.
You sure you want to use an LRU instead of a layer that marks the deletion alongside a resourceversion and uses a versionercomparator to know when it expires? This is vulnerable to mass deletion paging you out against a stale cache.
There was a problem hiding this comment.
Ideally, I would like a layer, but thought that was a good follow-up with what @hodovska is doing.
In this case, I was less concerned about resource version comparison since all I really care about is that the version in my local cache is now in terminating phase which is the check we have later in this file. Good point on the mass deletion paging out the LRU. Not sure how concerned I am on that scenario if we will swap out with the layered store.
There was a problem hiding this comment.
the history of the original behavior to modify the underlying store was done in the earlier PR to address flakes with single API server. It's understood that absent this change, in HA environments, there is still a race potential where API server 1 rejects a request (updated cache), and API server 2 accepts a request (stale cache), and the current background controller will hopefully observe and delete the late added resources as part of namespace clean-up.
so given the original scope of the intended fix, I think a limited size LRU is hopefully sufficient.
There was a problem hiding this comment.
This is a deletedNamespaceCache that you're using to make a livelookup choice. Perhaps a rename?
There was a problem hiding this comment.
It's not necessary deleted. It could either be terminating or deleted. I think I will just keep force live lookups for now.
There was a problem hiding this comment.
It's not necessary deleted. It could either be terminating or deleted. I think I will just keep force live lookups for now.
Somehow I've missed that. It looked like you only called .Add during a Delete. can you link me to where else you're adding it?
There was a problem hiding this comment.
it takes two calls to delete a namespace. delete 1 -> active to terminating, delete 2 -> gone from store. my point being that after the first delete, its not gone, just terminating.
k8s-github-robot
added
size/L
|
#29978 this PR should cover tasks mentioned here: #26709 (comment) |
derekwaynecarr
added
release-note-none
| ) | ||
| reflector.Run() | ||
| func NewLifecycle(c clientset.Interface, immortalNamespaces sets.String) (admission.Interface, error) { | ||
| forceLiveLookupCache, err := lru.New(100) |
There was a problem hiding this comment.
the error only happens if you pass a value less than 1. You can safely panic on the err instead.
|
minor comments, lgtm otherwise. |
derekwaynecarr
force-pushed
the
ns_lifecycle_informer
branch
from
45862e7 to
4c37a81
Compare
|
nits addressed. |
| 5*time.Minute, | ||
| ) | ||
| reflector.Run() | ||
| func NewLifecycle(c clientset.Interface, immortalNamespaces sets.String) (admission.Interface, error) { |
There was a problem hiding this comment.
This function never returns an error.
|
lgtm, fix the signature if you get a chance or we can get it next time we're in here. |
deads2k
added
the
lgtm
|
GCE e2e build/test passed for commit 4c37a81. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit 4c37a81. |
|
Automatic merge from submit-queue |
This was a follow-up to #29634
Moves the
NamespaceLifecycleplug-in to a shared infomer cache./cc @kubernetes/rh-cluster-infra @deads2k @hodovska