add dynamic rest mapper to the admission plugin initializer #16215
Conversation
openshift-ci-robot
added
the
size/L
openshift-merge-robot
added
the
approved
pkg/cmd/server/origin/master.go
Outdated
| aggregatedAPIServer.GenericAPIServer.AddPostStartHookOrDie("admission.openshift.io-RefreshRESTMapper", func(context apiserver.PostStartHookContext) error { | ||
| c.RESTMapper.Reset() | ||
| go func() { | ||
| wait.PollUntil(10*time.Second, func() (bool, error) { |
There was a problem hiding this comment.
I don't have the upstream issues handy where @caesarxuchao noted the performance characteristics of this refresh, but I seem to recall it not being great. Would it be worth at least making the period configurable?
There was a problem hiding this comment.
I don't have the upstream issues handy where @caesarxuchao noted the performance characteristics of this refresh, but I seem to recall it not being great. Would it be worth at least making the period configurable?
We'll get to see how bad it is. Without rate limiting, I would expect the calls to be quick for discovery.
There was a problem hiding this comment.
how will performance issues here surface? 10 seconds seems aggressive to me. also, does use of this this tolerate partial discovery failures?
There was a problem hiding this comment.
Do you need to catch panics since this is in a different go routine?
There was a problem hiding this comment.
10 seconds seems aggressive to me
And arbitrary.
does use of this this tolerate partial discovery failures?
Since we blindly reset the rest mapper, it would be possible to get into a state where GetAPIGroupResources fails and we have no longer have the old rest mapper.
There was a problem hiding this comment.
how will performance issues here surface? 10 seconds seems aggressive to me. also, does use of this this tolerate partial discovery failures?
Better hope so, since its upstream.
There was a problem hiding this comment.
10 seconds seems aggressive to me. also, does use of this this tolerate partial discovery failures?
Got a preferred time? I don't like waiting, so I tend to prefer tighter times.
And arbitrary.
Any choice would be arbitrary.
There was a problem hiding this comment.
Since we blindly reset the rest mapper, it would be possible to get into a state where GetAPIGroupResources fails and we have no longer have the old rest mapper.
Doesn't look like it to me. @ironcladlou's implementation does a refresh on invalidate. If you can't make contact, you keep using the old results. Individual servers which fail are skipped and old results are used if they are present.
how will performance issues here surface?
On the invalidating gofunc (not the admission gofunc), you get lag as it waits. Old results are used for admission. It all seems pretty reasonable to me.
There was a problem hiding this comment.
Do you need to catch panics since this is in a different go routine?
I was reasonably sure that the wait.* functions protected the callers, but I'm having trouble finding it.
openshift-merge-robot
added
the
needs-rebase
deads2k
force-pushed
the
gc-01-restmapper
branch
from
87ea4ec
to
9d40c04
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/assign liggitt I think this blocking @spadgett |
|
Yeah, this is blocking users of catalog at the moment, so if we could get this in, that would be great |
deads2k
force-pushed
the
gc-01-restmapper
branch
from
9d40c04
to
2ffa593
Compare
| go func() { | ||
| wait.Until(func() { | ||
| c.RESTMapper.Reset() | ||
| }, 10*time.Second, context.StopCh) |
There was a problem hiding this comment.
So it fails well, doesn't crash the process, doesn't affect apiserver response time, and handles partials as well as can be expected. Anyone want to come out with an "I prefer to wait longer, so please change it to value X"?
works for me /lgtm |
|
The part that is confusing is the unconditional It makes it appear that /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, enj, liggitt The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
Automatic merge from submit-queue |
fixes #16141
picks the dynamic memcache discovery for the RESTMapper and wires it for admission. There's a post-start hook that starts it refreshing.