Include unavailable apiservices in discovery response #66932

nilebox · 2018-08-02T19:44:00Z

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.

nilebox · 2018-08-02T19:45:06Z

/assign @liggitt

nilebox · 2018-08-02T19:45:55Z

/sig api-machinery

nilebox · 2018-08-02T19:46:29Z

/kind bug

liggitt · 2018-08-02T19:47:46Z

/ok-to-test

fedebongio · 2018-08-02T20:12:29Z

/assign @cheftako

nilebox · 2018-08-03T08:54:22Z

test/integration/examples/apiserver_test.go

-		t.Fatal(err)
-	}
+	_, err = aggregatorDiscoveryClient.Discovery().ServerResources()
+	assertWardleUnavailableDiscoveryError(t, err)


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).

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?

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

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

liggitt · 2018-08-06T14:09:43Z

test/integration/examples/apiserver_test.go


 	// 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 {


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

liggitt · 2018-08-06T14:10:58Z

test/integration/examples/apiserver_test.go

-	if err != nil {
-		t.Fatal(err)
-	}
+	assertWardleUnavailableDiscoveryError(t, err)


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

liggitt · 2018-08-06T14:12:37Z

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 · 2018-08-07T15:02:01Z

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 · 2018-08-07T16:54:48Z

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

nilebox · 2018-08-08T04:28:40Z

@liggitt commits squashed, please LGTM.

liggitt · 2018-08-08T04:31:02Z

/lgtm

nilebox · 2018-08-08T07:36:44Z

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

deads2k · 2018-08-08T11:36:52Z

surprisingly painless.

/lgtm

k8s-ci-robot · 2018-08-08T11:36:58Z

[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:

~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [deads2k]
~~test/OWNERS~~ [deads2k,liggitt]

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

k8s-github-robot · 2018-08-08T14:00:10Z

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.

nilebox · 2018-08-08T15:34:11Z

@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

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 ```

…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. ```

…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. ```

k8s-ci-robot requested review from cheftako and sttts August 2, 2018 19:44

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 2, 2018

k8s-ci-robot assigned liggitt Aug 2, 2018

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

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

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

k8s-ci-robot assigned cheftako Aug 2, 2018

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 force-pushed the discovery-include-unavailable branch 2 times, most recently from e3fead4 to 3bc4782 Compare August 3, 2018 06:04

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 force-pushed the discovery-include-unavailable branch 2 times, most recently from b6cfd02 to 9c381cf Compare August 3, 2018 07:10

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

nilebox commented Aug 3, 2018

View reviewed changes

nilebox changed the title ~~[WIP] Include unavailable apiservices in discovery response~~ Include unavailable apiservices in discovery response Aug 3, 2018

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

nilebox mentioned this pull request Aug 4, 2018

upstream k8s reuse "Service catalog follows namespace lifecycle" (create namespace if one does not exist by denying resource creation if the namespace does not exist) kubernetes-retired/service-catalog#586

Closed

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

liggitt reviewed Aug 6, 2018

View reviewed changes

Include unavailable API services in discovery response

d4690f4

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

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

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 merged commit 28d649c into kubernetes:master Aug 8, 2018

nilebox mentioned this pull request Aug 13, 2018

Add wait to discovery integration test to fix flakiness #67325

Merged

nilebox deleted the discovery-include-unavailable branch August 20, 2018 00:23

nilebox mentioned this pull request Oct 17, 2018

Service Catalog controller-manager fails to start when unrelated API server is unavailable kubernetes-retired/service-catalog#2418

Closed

alexmavr mentioned this pull request Dec 4, 2018

Increase timeout of install.IsRunning docker/compose-on-kubernetes#6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include unavailable apiservices in discovery response #66932

Include unavailable apiservices in discovery response #66932

nilebox commented Aug 2, 2018 •

edited by liggitt

Loading

nilebox commented Aug 2, 2018

nilebox commented Aug 2, 2018

nilebox commented Aug 2, 2018

liggitt commented Aug 2, 2018

fedebongio commented Aug 2, 2018

nilebox Aug 3, 2018

liggitt Aug 6, 2018

liggitt Aug 7, 2018

nilebox Aug 7, 2018

liggitt Aug 6, 2018 •

edited

Loading

liggitt Aug 6, 2018

liggitt commented Aug 6, 2018

liggitt commented Aug 7, 2018 •

edited

Loading

nilebox commented Aug 7, 2018 •

edited

Loading

nilebox commented Aug 8, 2018

liggitt commented Aug 8, 2018

nilebox commented Aug 8, 2018

deads2k commented Aug 8, 2018

k8s-ci-robot commented Aug 8, 2018

k8s-github-robot commented Aug 8, 2018

nilebox commented Aug 8, 2018

Include unavailable apiservices in discovery response #66932

Include unavailable apiservices in discovery response #66932

Conversation

nilebox commented Aug 2, 2018 • edited by liggitt Loading

nilebox commented Aug 2, 2018

nilebox commented Aug 2, 2018

nilebox commented Aug 2, 2018

liggitt commented Aug 2, 2018

fedebongio commented Aug 2, 2018

nilebox Aug 3, 2018

Choose a reason for hiding this comment

liggitt Aug 6, 2018

Choose a reason for hiding this comment

liggitt Aug 7, 2018

Choose a reason for hiding this comment

nilebox Aug 7, 2018

Choose a reason for hiding this comment

liggitt Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

liggitt Aug 6, 2018

Choose a reason for hiding this comment

liggitt commented Aug 6, 2018

liggitt commented Aug 7, 2018 • edited Loading

nilebox commented Aug 7, 2018 • edited Loading

nilebox commented Aug 8, 2018

liggitt commented Aug 8, 2018

nilebox commented Aug 8, 2018

deads2k commented Aug 8, 2018

k8s-ci-robot commented Aug 8, 2018

k8s-github-robot commented Aug 8, 2018

nilebox commented Aug 8, 2018

nilebox commented Aug 2, 2018 •

edited by liggitt

Loading

liggitt Aug 6, 2018 •

edited

Loading

liggitt commented Aug 7, 2018 •

edited

Loading

nilebox commented Aug 7, 2018 •

edited

Loading