Fix OpenAPI aggregation cleanup #120108

tnqn · 2023-08-22T15:52:56Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

There were four issues in OpenAPI aggregation cleanup:

When removing an APIService, openAPIAggregationController was called twice while openAPIV3AggregationController was never called, leading to OpenAPI v3 for the APIService not cleaned up.
When removing a local APIService, v2 specAggregator should not return ErrAPIServiceNotFound when it doesn't find the APIService because local APIServices were never added to its cache, otherwise confusing error logs would be generated. Besides, the method's comment indicates that the desired behavior is that no error is returned if the APIService does not exist.
When removing an APIService, v3 specProxier should update openapiv2converter's cache, like when updating an APIService, otherwise the API would not be removed from "/openapi/v3".
When v3 AggregationController reconciles an APIService, it should stop requeueing it if it fails with ErrAPIServiceNotFound as the APIService has been removed, like what v2 AggregationController does, otherwise it would keep reconciling the APIService forever.

Which issue(s) this PR fixes:

Fixes #120107

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix OpenAPI v3 not being cleaned up after deleting APIServices

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

Jefftree

Thanks for reporting and fixing this! A couple small nits

test/e2e/apimachinery/openapiv3.go

Jefftree · 2023-08-22T18:11:07Z

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

@@ -236,7 +236,7 @@ func (s *specAggregator) RemoveAPIService(apiServiceName string) error {
 defer s.mutex.Unlock()

 if _, exists := s.specsByAPIServiceName[apiServiceName]; !exists {
- return ErrAPIServiceNotFound
+ return nil


This function cannot return an error anymore right? Would it make sense to to change the function signature so it's more clear?

Sure, changed the signature.

alexzielenski · 2023-08-22T20:04:22Z

/triage accepted
/assign @Jefftree

tnqn

@Jefftree thanks for your review and comments.

tnqn · 2023-08-23T07:39:02Z

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

@@ -236,7 +236,7 @@ func (s *specAggregator) RemoveAPIService(apiServiceName string) error {
 defer s.mutex.Unlock()

 if _, exists := s.specsByAPIServiceName[apiServiceName]; !exists {
- return ErrAPIServiceNotFound
+ return nil


Sure, changed the signature.

test/e2e/apimachinery/openapiv3.go

Jefftree · 2023-08-23T15:47:23Z

/lgtm
/assign @jpbetz
/cc @apelisse

I think we should backport this at least to 1.27

k8s-ci-robot · 2023-08-23T15:47:45Z

LGTM label has been added.

Git tree hash: a3860487bb874ef97b06556bedf588beefd8ed23

sttts · 2023-08-30T07:41:39Z

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

@@ -45,7 +45,7 @@ var ErrAPIServiceNotFound = errors.New("resource not found")
 type SpecAggregator interface {
 AddUpdateAPIService(apiService *v1.APIService, handler http.Handler) error
 UpdateAPIServiceSpec(apiServiceName string) error
- RemoveAPIService(apiServiceName string) error


you introduced a special error. This is becoming part of the interface and needs documentation. Or better: get rid of the error and use found bool, err error

just a note that adding a return parameter will probably make backporting slightly more involved

made a compromise here: documented the specific error ErrAPIServiceNotFound to avoid affecting v2 code and facilitate backporting.

tnqn · 2023-08-30T08:32:23Z

Thanks @sttts for your review. The code was mainly to be consistent with OpenAPI v2, please let me know if we should change them together or focus on bugfix in the PR.

sttts · 2023-08-30T14:20:11Z

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

- RemoveAPIService(apiServiceName string) error
+ // UpdateAPIServiceSpec updates the APIService. It returns a bool which indicates whether the APIService exists and
+ // the actual error.
+ UpdateAPIServiceSpec(apiServiceName string) (bool, error)


this is not what I meant. If you make "not found" explicit, it is not an error anymore. So we would return false, nil.

There were four issues in OpenAPI aggregation cleanup: 1. When removing an APIService, openAPIAggregationController was called twice while openAPIV3AggregationController was never called, leading to OpenAPI v3 for the APIService not cleaned up. 2. When removing a local APIService, v2 specAggregator should not return ErrAPIServiceNotFound when it doesn't find the APIService because local APIServices were never added to its cache, otherwise confusing error logs would be generated. Besides, the method's comment indicates that the desired behavior is that no error is returned if the APIService does not exist. 3. When removing an APIService, v3 specProxier should update openapiv2converter's cache, like when updating an APIService, otherwise the API would not be removed from "/openapi/v3". 4. When v3 AggregationController reconciles an APIService, it should stop requeueing it if it fails with ErrAPIServiceNotFound as the APIService has been removed, like what v2 AggregationController does, otherwise it would keep reconciling the APIService forever. Signed-off-by: Quan Tian <[email protected]>

sttts · 2023-08-31T06:47:07Z

/approve

Leaving final lgtm to @Jefftree

Jefftree · 2023-08-31T14:45:52Z

/approve
/lgtm

k8s-ci-robot · 2023-08-31T14:45:58Z

LGTM label has been added.

Git tree hash: a371c959f6684053431a451213d161f6b11213f2

k8s-ci-robot · 2023-08-31T14:46:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jefftree, sttts, tnqn

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~~ [sttts]
~~test/e2e/apimachinery/OWNERS~~ [sttts]

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

tnqn · 2023-08-31T16:38:41Z

/retest

tnqn · 2023-09-01T15:08:36Z

I think we should backport this at least to 1.27

@Jefftree I found there are some conflicts when backporting it to 1.27 due to the change: #118212, just want to check with you if it's still worth to backport in this situation. If yes, I would need to add ErrAPIServiceNotFound to 1.27.

There is no conflict with 1.28.

Jefftree · 2023-09-01T15:12:38Z

@tnqn: OpenAPIV3 went GA in 1.27 so I think it'd be worth it. Let me know if you need help with it.

tnqn · 2023-09-01T15:47:48Z

@tnqn: OpenAPIV3 went GA in 1.27 so I think it'd be worth it. Let me know if you need help with it.

Got it, created #120362 for 1.27, removed the changes in v2 as the interface can return error in that release.

…-upstream-release-1.27 Automated cherry pick of #120108: Fix OpenAPI aggregation cleanup

…-upstream-release-1.28 Automated cherry pick of #120108: Fix OpenAPI aggregation cleanup

k8s-ci-robot requested review from smarterclayton and wojtek-t August 22, 2023 15:53

Jefftree reviewed Aug 22, 2023

View reviewed changes

k8s-ci-robot assigned Jefftree Aug 22, 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 Aug 22, 2023

tnqn commented Aug 23, 2023

View reviewed changes

tnqn force-pushed the fix-appgregator branch from ecb169e to 7e156c6 Compare August 23, 2023 07:48

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 23, 2023

tnqn commented Aug 23, 2023

View reviewed changes

test/e2e/apimachinery/openapiv3.go Show resolved Hide resolved

tnqn force-pushed the fix-appgregator branch from 7e156c6 to 7812b6a Compare August 23, 2023 15:36

k8s-ci-robot assigned jpbetz Aug 23, 2023

k8s-ci-robot requested a review from apelisse August 23, 2023 15:47

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

tnqn force-pushed the fix-appgregator branch from 7812b6a to 9c49195 Compare August 23, 2023 16:45

sttts reviewed Aug 30, 2023

View reviewed changes

tnqn force-pushed the fix-appgregator branch from 35133b6 to 9b374c8 Compare August 30, 2023 09:39

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

k8s-ci-robot requested a review from sttts August 30, 2023 09:39

sttts reviewed Aug 30, 2023

View reviewed changes

tnqn force-pushed the fix-appgregator branch 2 times, most recently from f0862ed to 1a753b9 Compare August 30, 2023 15:26

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 30, 2023

tnqn force-pushed the fix-appgregator branch from 1a753b9 to bf41b04 Compare August 30, 2023 16:02

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

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

k8s-ci-robot merged commit dd0c2d4 into kubernetes:master Aug 31, 2023
15 checks passed

k8s-ci-robot added this to the v1.29 milestone Aug 31, 2023

tnqn deleted the fix-appgregator branch September 1, 2023 02:25

tnqn mentioned this pull request Sep 1, 2023

Automated cherry pick of #120108: Fix OpenAPI aggregation cleanup #120358

Merged

tnqn mentioned this pull request Sep 1, 2023

Automated cherry pick of #120108: Fix OpenAPI aggregation cleanup #120362

Merged

k8s-ci-robot added a commit that referenced this pull request Sep 7, 2023

Merge pull request #120362 from tnqn/automated-cherry-pick-of-#120108…

ca7e6fd

…-upstream-release-1.27 Automated cherry pick of #120108: Fix OpenAPI aggregation cleanup

k8s-ci-robot added a commit that referenced this pull request Sep 7, 2023

Merge pull request #120358 from tnqn/automated-cherry-pick-of-#120108…

ca33dac

…-upstream-release-1.28 Automated cherry pick of #120108: Fix OpenAPI aggregation cleanup

brandond mentioned this pull request Sep 18, 2023

poststarthook/apiservice-registration-controller check blocks apiserver readiness for 1 minute if discovery cannot be completed #120739

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix OpenAPI aggregation cleanup #120108

Fix OpenAPI aggregation cleanup #120108

tnqn commented Aug 22, 2023 •

edited

Jefftree left a comment

Jefftree Aug 22, 2023

tnqn Aug 23, 2023

alexzielenski commented Aug 22, 2023

tnqn left a comment

tnqn Aug 23, 2023

Jefftree commented Aug 23, 2023

k8s-ci-robot commented Aug 23, 2023

sttts Aug 30, 2023

tnqn Aug 30, 2023

Jefftree Aug 30, 2023

tnqn Aug 30, 2023

tnqn commented Aug 30, 2023

sttts Aug 30, 2023

sttts commented Aug 31, 2023

Jefftree commented Aug 31, 2023

k8s-ci-robot commented Aug 31, 2023

k8s-ci-robot commented Aug 31, 2023

tnqn commented Aug 31, 2023

tnqn commented Sep 1, 2023

Jefftree commented Sep 1, 2023

tnqn commented Sep 1, 2023

Fix OpenAPI aggregation cleanup #120108

Fix OpenAPI aggregation cleanup #120108

Conversation

tnqn commented Aug 22, 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.:

Jefftree left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexzielenski commented Aug 22, 2023

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jefftree commented Aug 23, 2023

k8s-ci-robot commented Aug 23, 2023

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

tnqn commented Aug 30, 2023

Choose a reason for hiding this comment

sttts commented Aug 31, 2023

Jefftree commented Aug 31, 2023

k8s-ci-robot commented Aug 31, 2023

k8s-ci-robot commented Aug 31, 2023

tnqn commented Aug 31, 2023

tnqn commented Sep 1, 2023

Jefftree commented Sep 1, 2023

tnqn commented Sep 1, 2023

tnqn commented Aug 22, 2023 •

edited