✨ Use aggregated discovery if available #2901
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
alvaroaleman
force-pushed
the
aggregated
branch
from
3ffd95c
to
b0abe43
Compare
| Kind: "Driver", | ||
| } | ||
| for _, aggregatedDiscovery := range []bool{true, false} { | ||
| t.Run("aggregatedDiscovery="+strconv.FormatBool(aggregatedDiscovery), func(t *testing.T) { |
There was a problem hiding this comment.
Definitely fine for now, but we're going to lose coverage once we bump hack/check-everything.sh to a version where the feature gate doesn't function anymore (so maybe we should use a different Kubernetes version long-term (after #2810))
WDYT about opening an issue to track that?
alvaroaleman
force-pushed
the
aggregated
branch
from
b0abe43
to
6fd96e3
Compare
|
@sbueringer one thing probably worth pointing out: Originally I thought this is always better in terms of api calls as it is one api call for everything. I then learned when implementing this that its actually two api calls for everything: One to /api, one to /apis. That means its not as obviously better as I originally thought: If one GV is used by the binary its worse. If two are used, its the same. Three or more, its better (but I would assume the majority of controllers uses two or less). The other reason why I think it would be nice is that it brings us closer to the upstream restmapper if aggrgated discovery is supported, as it means we have all available info in the cache and issues like #2496 wouldn't happen. But I don't know if that is a super strong argument. |
|
Just to avoid misunderstandings
With "GV used" you basically mean that the controller will ask the restmapper for a specific GV. I.e. "two are used" means that the restmapper would be used for at least two different GVs. I think it's safe to assume that most controllers will use at least 2 GVs. At least 1 for their CRD and then in a lot of cases either another one for other CRDs or builtin resources. If I got it correctly the restmapper should be used for every informer / get / list / write / ... calls. I think a lot of controllers are not restricted to just one GV |
alvaroaleman
force-pushed
the
aggregated
branch
from
6fd96e3
to
78038b3
Compare
alvaroaleman
force-pushed
the
aggregated
branch
from
78038b3
to
076d14e
Compare
alvaroaleman
force-pushed
the
aggregated
branch
3 times, most recently
from
352b901
to
9fb44a4
Compare
|
/assign @vincepri |
alvaroaleman
force-pushed
the
aggregated
branch
from
9fb44a4
to
93059d2
Compare
This change makes the `RESTMapper` use [AggredatedDiscovery][0] if available. `AggregatedDiscovery` is beta and thus enabled by default since Kube 1.28. What this means in particular is that during startup, we will always do a request to `/api` and `/apis`. If `AggregatedDiscovery` is enabled, that is all we have to do. If not, we have to do a third request. The behavior for reloading remains the same: Do one request unless the request didn't include a version. In that case we have to find the group. If the group is unknown, we will keep on doing a request to `/api` and one to `/apis` to find it. If it is found we are again done in the `AggregatedDiscovery` case and have to do a third request otherwise. All in all this means that with `AggregatedDiscovery` enabled, the only case where this is worse is if the client interacts with only one `GroupVersion`, as we end up doing one additional api request. If it is disabled, we effectively waste two api requests. [0]: kubernetes/enhancements#3352
alvaroaleman
force-pushed
the
aggregated
branch
from
93059d2
to
3eb8c96
Compare
|
Thank you very much! :) /lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: c70bca7c69ff2f2555178d3625f143dcf99d27b8
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, sbueringer 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 |
This change makes the
RESTMapperuse AggredatedDiscovery if available.AggregatedDiscoveryis beta and thus enabled by default since Kube 1.27.What this means in particular is that during startup, we will always do a request to
/apiand/apis. IfAggregatedDiscoveryis enabled, that is all we have to do. If not, we have to do a third request.The behavior for reloading remains the same: Do one request unless the request didn't include a version. In that case we have to find the group. If the group is unknown, we will keep on doing a request to
/apiand one to/apisto find it. If it is found we are again done in theAggregatedDiscoverycase and have to do a third request otherwise.All in all this means that with
AggregatedDiscoveryenabled, the only case where this is worse is if the client interacts with only oneGroupVersion, as we end up doing one additional api request. If it is disabled, we effectively waste two api requests.Fixes #2897
Note: Disable whitespace when reviewing, that significantly reduces the size of the diff.