Fix AddUpdateAPIService for openapiv2 #120814

Jefftree · 2023-09-21T16:30:30Z

What type of PR is this?

/kind bug
/kind regression

What this PR does / why we need it:

A couple of issues were discovered and they've been combined into this PR since they all need a backport.

For #120758: Triggered by a race when a GET to /openapi/v2 is called between https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/aggregator.go#L227 and https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/aggregator.go#L230

Fix is to initialize the cache with empty data so aggregation won't have problems.

For #120739: AddUpdateAPIService was adding and also updating the apiservice spec. The apiservice spec update should be done async by the controller.

Fix is to not update the spec in the addupdateapiservice flow during startup. This is handled async by the openapi controller.

I've discovered #120878 while working on the above two issues: The code path when an apiservice exists and needs to be updated was not handled properly. I've refactored the CacheableDownloader to have an new function UpdateHandler() and call that in AddUpdateAPIService() if the apiservice already exists. I've also removed the decorateError function and integrated the better logging directly in the downloader.

I've fixed the AddUpdateAPIService flow and added integration tests to simulate the cases described in #120758 and #120739. Added additional unit tests to catch #120878. Tried to keep the changes self contained since we need to backport.

Which issue(s) this PR fixes:

Fixes #120758
Fixes #120739
Fixes #120878

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix regression with adding aggregated apiservices panicking and affected health check introduced in release v1.28.0

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

jiahuif · 2023-09-21T16:33:57Z

/assign @apelisse

jiahuif · 2023-09-21T16:34:25Z

/triage accepted

dims · 2023-09-22T00:33:22Z

@Jefftree will use this PR to try some CI jobs. hope that's ok :)

dims · 2023-09-22T00:33:44Z

/test pull-kubernetes-e2e-containerd-gce
/test pull-kubernetes-node-e2e-containerd

liggitt · 2023-09-23T16:26:52Z

Please add the exact patch version where the regression was introduced into the release note

staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/controller.go

liggitt · 2023-10-03T19:20:19Z

staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/downloader.go

+ d.mutex.Lock()
+ defer d.mutex.Unlock()
+ d.handler = handler
+}

 func (d *cacheableDownloader) Get() (*spec.Swagger, string, error) {


Get is long-running, right? the mutex addition makes that block UpdateHandler, which now blocks specAggregator#AddUpdateAPIService

That means a hang fetching one service's openapi can block processing of other services, right?

hmm that's true. What if we have the mutex only be required when we fetch the handler instead of the entire get operation?

is the only thing we need to guard the handler, or is the etag / spec also concurrently modifiable? (can Get be called from multiple threads?)

We currently don't call Get() from multiple threads but the defacto cached.Value interface that we follow is supposed to be thread safe (https://github.com/kubernetes/kube-openapi/blob/master/pkg/cached/cache.go#L17-L22).

@apelisse I think this ties into what you mentioned earlier that since we don't evaluate aggregated apiservices lazily, maybe we shouldn't follow cached library interface?

leaving this open before merge just to get @apelisse's feedback

If you don't then you'll have to deal with the etag manually, I'm not sure it would better.

staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/aggregator.go

staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/downloader.go

liggitt · 2023-10-04T17:35:35Z

staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/aggregator.go

@@ -218,16 +219,21 @@ func (s *specAggregator) AddUpdateAPIService(apiService *v1.APIService, handler
 s.mutex.Lock()


if an API Service changes from remote to local (service != nil → service == nil), will the short-circuit return above prevent cleanup from happening? should we call RemoveAPIService in that case?

just noticed this, it's pre-existing in 1.27, so probably best to handle separately in a follow up

ack, will do that in a follow up

thanks, link the issue here when you open it to close the loop

Jefftree · 2023-10-04T22:22:03Z

/retest

liggitt · 2023-10-05T15:16:25Z

lgtm but will defer to @apelisse on #120814 (comment) before merge

apelisse

My comments are nits and don't have to be fixed here since they mostly relate to style and have no behavior impact. LGTM for me, thanks.

apelisse · 2023-10-05T23:43:52Z

staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/downloader.go

+ return spec, etag, err
+}
+
+func (d *cacheableDownloader) getWithHandler(h http.Handler) (*spec.Swagger, string, error) {


is there a get without handler? The name sounds like there would be. But wait, I think I see where this is coming from. You shouldn't have to receive the handler here.

I've updated with the atomic pointer, but kept this function. Separating the two functions lets us decorate the error with the apiservice name. lmk if you have thoughts, thanks!

Sure, I still think it's weird to pass to getWithHandler one of it's own member though.

Maybe just do ...

spec, etag, err := d.get() ... func (d *cacheableDownloader) get() (*spec.Swagger, string, error) { h := *d.handler.Load() ...

staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/downloader.go

apelisse · 2023-10-05T23:48:27Z

staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/downloader.go

+ d.mutex.Lock()
+ defer d.mutex.Unlock()
+ d.handler = handler
+}

 func (d *cacheableDownloader) Get() (*spec.Swagger, string, error) {


If you don't then you'll have to deal with the etag manually, I'm not sure it would better.

staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/aggregator.go

liggitt · 2023-10-06T00:33:32Z

/retest
/approve

k8s-ci-robot · 2023-10-06T00:33:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, Jefftree, liggitt

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:

~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [liggitt]
~~test/integration/apiserver/OWNERS~~ [Jefftree,apelisse,liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

liggitt · 2023-10-06T00:34:00Z

/hold in case you want to incorporate @apelisse's comments here before backport

…nd fix AddUpdateAPIService to update handler

apelisse · 2023-10-06T17:59:48Z

Thanks
/lgtm

k8s-ci-robot · 2023-10-06T17:59:55Z

LGTM label has been added.

Git tree hash: 4720303ed0593197c1a1e5ab658781aacb568cfc

liggitt · 2023-10-06T18:05:53Z

/hold cancel

…0814-upstream-release-1.28 Automated cherry pick of #120814: Fix 120758 - prevent cache Load on uninitialized spec

k8s-ci-robot requested review from deads2k and smarterclayton September 21, 2023 16:31

k8s-ci-robot assigned apelisse Sep 21, 2023

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 21, 2023

Jefftree force-pushed the fix-openapiv2-aggregator-apiservice branch 6 times, most recently from 70c2e14 to eb867b9 Compare September 21, 2023 19:11

k8s-ci-robot requested a review from sttts September 28, 2023 20:05

liggitt reviewed Oct 3, 2023

View reviewed changes

staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/controller.go Show resolved Hide resolved

liggitt reviewed Oct 3, 2023

View reviewed changes

staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/aggregator.go Show resolved Hide resolved

liggitt reviewed Oct 3, 2023

View reviewed changes

staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/downloader.go Outdated Show resolved Hide resolved

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2023

k8s-ci-robot requested a review from apelisse October 4, 2023 17:07

liggitt reviewed Oct 4, 2023

View reviewed changes

apelisse reviewed Oct 5, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 6, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2023

Jefftree force-pushed the fix-openapiv2-aggregator-apiservice branch from 3e23beb to 14e3a3a Compare October 6, 2023 15:39

Jefftree added 2 commits October 6, 2023 12:57

Fix 120878 - Refactor cacheabledownloader so handler can be updated a…

83fb504

…nd fix AddUpdateAPIService to update handler

Unit & integration tests for OpenAPIV2 AddUpdateAPIService

89adbb4

Jefftree force-pushed the fix-openapiv2-aggregator-apiservice branch from 14e3a3a to 89adbb4 Compare October 6, 2023 16:58

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 6, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2023

k8s-ci-robot merged commit e339e03 into kubernetes:master Oct 6, 2023
15 checks passed

k8s-ci-robot added this to the v1.29 milestone Oct 6, 2023

Jefftree mentioned this pull request Oct 6, 2023

Automated cherry pick of #120814: Fix 120758 - prevent cache Load on uninitialized spec #121040

Merged

k8s-ci-robot added a commit that referenced this pull request Oct 12, 2023

Merge pull request #121040 from Jefftree/automated-cherry-pick-of-#12…

e3b5e62

…0814-upstream-release-1.28 Automated cherry pick of #120814: Fix 120758 - prevent cache Load on uninitialized spec

Jefftree deleted the fix-openapiv2-aggregator-apiservice branch January 25, 2024 22:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix AddUpdateAPIService for openapiv2 #120814

Fix AddUpdateAPIService for openapiv2 #120814

Jefftree commented Sep 21, 2023 •

edited

jiahuif commented Sep 21, 2023

jiahuif commented Sep 21, 2023

dims commented Sep 22, 2023

dims commented Sep 22, 2023

liggitt commented Sep 23, 2023

liggitt Oct 3, 2023

Jefftree Oct 3, 2023

liggitt Oct 3, 2023 •

edited

Jefftree Oct 3, 2023

liggitt Oct 5, 2023

apelisse Oct 5, 2023

liggitt Oct 4, 2023

Jefftree Oct 4, 2023

liggitt Oct 5, 2023

Jefftree commented Oct 4, 2023

liggitt commented Oct 5, 2023

apelisse left a comment

apelisse Oct 5, 2023

Jefftree Oct 6, 2023

apelisse Oct 6, 2023

apelisse Oct 6, 2023

Jefftree Oct 6, 2023

apelisse Oct 5, 2023

liggitt commented Oct 6, 2023

k8s-ci-robot commented Oct 6, 2023

liggitt commented Oct 6, 2023

apelisse commented Oct 6, 2023

k8s-ci-robot commented Oct 6, 2023

liggitt commented Oct 6, 2023

		@@ -218,16 +219,21 @@ func (s specAggregator) AddUpdateAPIService(apiService v1.APIService, handler
		s.mutex.Lock()

Fix AddUpdateAPIService for openapiv2 #120814

Fix AddUpdateAPIService for openapiv2 #120814

Conversation

Jefftree commented Sep 21, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

jiahuif commented Sep 21, 2023

jiahuif commented Sep 21, 2023

dims commented Sep 22, 2023

dims commented Sep 22, 2023

liggitt commented Sep 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Oct 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jefftree commented Oct 4, 2023

liggitt commented Oct 5, 2023

apelisse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Oct 6, 2023

k8s-ci-robot commented Oct 6, 2023

liggitt commented Oct 6, 2023

apelisse commented Oct 6, 2023

k8s-ci-robot commented Oct 6, 2023

liggitt commented Oct 6, 2023

Jefftree commented Sep 21, 2023 •

edited

liggitt Oct 3, 2023 •

edited