Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include unavailable apiservices in discovery response #66932

Merged

Conversation

nilebox
Copy link

@nilebox nilebox commented Aug 2, 2018

What this PR does / why we need it:
Include unavailable apiservices into apis/ discovery endpoint response to fix namespace deletion kubernetes-retired/service-catalog#2254

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes kubernetes-retired/service-catalog#2254

Special notes for your reviewer:

Release note:

kube-apiserver now includes all registered API groups in discovery, including registered extension API group/versions for unavailable extension API servers.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 2, 2018
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 2, 2018
@nilebox
Copy link
Author

nilebox commented Aug 2, 2018

/assign @liggitt

@nilebox
Copy link
Author

nilebox commented Aug 2, 2018

/sig api-machinery

@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Aug 2, 2018
@nilebox
Copy link
Author

nilebox commented Aug 2, 2018

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 2, 2018
@liggitt
Copy link
Member

liggitt commented Aug 2, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 2, 2018
@fedebongio
Copy link
Contributor

/assign @cheftako

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 3, 2018
@nilebox nilebox force-pushed the discovery-include-unavailable branch 2 times, most recently from e3fead4 to 3bc4782 Compare August 3, 2018 06:04
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 3, 2018
@nilebox nilebox force-pushed the discovery-include-unavailable branch 2 times, most recently from b6cfd02 to 9c381cf Compare August 3, 2018 07:10
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 3, 2018
t.Fatal(err)
}
_, err = aggregatorDiscoveryClient.Discovery().ServerResources()
assertWardleUnavailableDiscoveryError(t, err)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wardle APIService was never available, because Service object is missing, which is referenced in the APIService object above:

 			Service: &apiregistrationv1beta1.ServiceReference{
 				Namespace: "kube-wardle",
 				Name:      "api",
},

Kube-aggregator previously silently skipped unavailable API services in the /apis endpoint. With the new change, there is an error with details that are asserted in the assertWardleUnavailableDiscoveryError function (since .Discovery().ServerResources() now invokes /apis/wardle.k8s.io/v1alpha1 endpoint that returns 503 error).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good to verify this error this way to make sure discovery reports it is incomplete in this case. once we verify this way, can we create the service pointing at 127.0.0.1:<wardlePort> and make sure we get a successful discovery?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, I'm not sure we have the network infrastructure here to use services

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heh, your initial suggestion made me wonder if there was some magic trick to make such Service

@nilebox nilebox changed the title [WIP] Include unavailable apiservices in discovery response Include unavailable apiservices in discovery response Aug 3, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 3, 2018
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 6, 2018

// TODO figure out how to turn on enough of services and dns to run more
}

func assertWardleUnavailableDiscoveryError(t *testing.T, err error) {
if err == nil {
Copy link
Member

@liggitt liggitt Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

call t.Helper() at the top of this method to make sure failures report the line number where this was called

if err != nil {
t.Fatal(err)
}
assertWardleUnavailableDiscoveryError(t, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be able to drop this if we create a Service object above

@liggitt
Copy link
Member

liggitt commented Aug 6, 2018

this looks good to me, just the one comment on updating the integration test to test discovery in both failure and success cases, then squash down

@liggitt
Copy link
Member

liggitt commented Aug 7, 2018

this looks good to me as-is. the aggregated e2e test exercises successful end-to-end discovery and aggregation cases more thoroughly, and the integration test change here ensures discovery functions appropriately in failure cases. can you squash this down, then I'll LGTM

@nilebox
Copy link
Author

nilebox commented Aug 7, 2018

@liggitt awesome, will let you know when I squash this PR.

@nilebox nilebox force-pushed the discovery-include-unavailable branch from 780f11e to d4690f4 Compare August 8, 2018 04:27
@nilebox
Copy link
Author

nilebox commented Aug 8, 2018

@liggitt commits squashed, please LGTM.

@liggitt
Copy link
Member

liggitt commented Aug 8, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 8, 2018
@nilebox
Copy link
Author

nilebox commented Aug 8, 2018

@deads2k @lavalamp can you approve this pull request please?

@deads2k
Copy link
Contributor

deads2k commented Aug 8, 2018

surprisingly painless.

/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt, nilebox

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 8, 2018
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 66394, 66888, 66932). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 28d649c into kubernetes:master Aug 8, 2018
@nilebox
Copy link
Author

nilebox commented Aug 8, 2018

@liggitt can you cherry-pick this change to k8s 1.11 branch please? We need it to fix the issue kubernetes-retired/service-catalog#2254

k8s-github-robot pushed a commit that referenced this pull request Aug 15, 2018
Automatic merge from submit-queue (batch tested with PRs 67294, 67320, 67335, 67334, 67325). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Add wait to discovery integration test to fix flakiness

**What this PR does / why we need it**:
Reduce the flakiness of the discovery test that was introduced in #66932  (see cherry-pick PR #67154 for report on flaky failure and suggestions).

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:

**Release note**:

```release-note
NONE
```
@nilebox nilebox deleted the discovery-include-unavailable branch August 20, 2018 00:23
k8s-github-robot pushed a commit that referenced this pull request Aug 21, 2018
…2-upstream-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #66932 #67433: Include unavailable API services in discovery response, allow failed discovery on initial quota controller start

Cherry pick of #66932 on release-1.10.

#66932: Include unavailable API services in discovery response
#67433: allow failed discovery on initial quota controller start

```release-note
kube-apiserver now includes all registered API groups in discovery, including registered extension API group/versions for unavailable extension API servers.
kube-controller-manager can now start the quota controller when discovery results can only be partially determined.
```
k8s-github-robot pushed a commit that referenced this pull request Aug 21, 2018
…2-upstream-release-1.11

Automatic merge from submit-queue.

Automated cherry pick of #66932 #67433: Include unavailable API services in discovery response, allow failed discovery on initial quota controller start

Cherry pick of #66932 #67433 on release-1.11.

#66932: Include unavailable API services in discovery response
#67433: allow failed discovery on initial quota controller start

```release-note
kube-apiserver now includes all registered API groups in discovery, including registered extension API group/versions for unavailable extension API servers.
kube-controller-manager can now start the quota controller when discovery results can only be partially determined.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Orphaned Service Catalog objects after Namespace deletion during Service Catalog API server outage
7 participants