k8s: Provide fallback for EndpointSlices detection if discovery API is not available #11253

tgraf · 2020-04-30T10:40:46Z

Discovery of API groups requires the API services of the apiserver to be healthy. Such API services can depend on the readiness of regular pods which require Cilium to function correctly. By treating failure to discover API groups as fatal, a critial loop can be entered in which Cilium cannot start because the API groups can't be discovered and the API groups will only become discoverable once Cilium is up.

Warn about the lack of discovery ability and fall back to probing the API directly.

errordeveloper

LGTM 👍

nit: could you clarify if Update would get called again? The description comment for this function doesn't necessarily suggest what is the case... Perhaps copying the info from commit message into a comment would be quite helpful also, especially as this is a pretty special case.

coveralls · 2020-04-30T11:09:01Z

Coverage decreased (-0.02%) to 44.428% when pulling 9f48402 on pr/tgraf/server-version-bootstrap into 3ecb413 on master.

tgraf · 2020-04-30T13:45:33Z

nit: could you clarify if Update would get called again?

I'll update the comment. It doesn't really make sense to call Update() again as the capabilities are primarily used during bootstrapping. What could be possible is to keep running to stabilize and then restart a couple of minutes later to detect capabilities again.

tgraf · 2020-04-30T13:47:32Z

test-me-please

00:08:31.763  failed to get destination image "sha256:799bab65f89532154bfa2d34fc49bc036f88c52f9fa63864080714f65d9ac37a": No such image: sha256:799bab65f89532154bfa2d34fc49bc036f88c52f9fa63864080714f65d9ac37a

aanm

Discussed with Thomas offline. This requires a flag that does the discovery on a conditional basis

tgraf · 2020-05-01T09:48:16Z

test-me-please

tgraf · 2020-05-01T15:24:43Z

test-me-please

aanm

To be fair I only noticed the typos because I opened the PR on my IDE.

pkg/k8s/version/version.go

aanm · 2020-05-01T16:38:37Z

pkg/k8s/config/config.go

+// K8sAPIDiscoveryEnabled returns true if API discovery of API groups and
+// resources is enabled
+func (d *DefaultConfiguration) K8sAPIDiscoveryEnabled() bool {
+	return defaults.K8sEnableAPIDiscovery


Is this right? Shouldn't this be the value specified by the user?

Yes, this represents the default configuration when no user options are available. This also clearly documents in which cases that happens instead of just relying on pkg/option which is uninitialized in several cases.

pkg/k8s/version/version.go

joestringer

CLI/docs changes LGTM.

Discovery of API groups requires the API services of the apiserver to be healthy. Such API services can depend on the readiness of regular pods which require Cilium to function correctly. By treating failure to discover API groups as fatal, a critial loop can be entered in which Cilium cannot start because the API groups can't be discovered and th API groups will only become discoverable once Cilium is up. Warn about the lack of discovery ability and fall back to probing the API directly. Signed-off-by: Thomas Graf <thomas@cilium.io>

The discovery request consists of several API calls to the apiserver. It has already been observed that the k8s client can stsrt throttling during startup phase. Disable the use of the discovery API by default and rely on individual API probing but allow users to benefit from it by opting into it. Signed-off-by: Thomas Graf <thomas@cilium.io>

tgraf · 2020-05-03T11:45:10Z

test-me-please

joestringer · 2020-05-05T23:45:00Z

@tgraf @aanm did we intend to mark this for backport to v1.7?

Context:
#11233 (comment)

Not sure if this is the right place or if it is too late to ask, but can we backport #11253 too? Without it #11206 might cause cilium to not start correctly on environments like GKE (where discovery API would fail).

aanm · 2020-05-06T09:45:37Z

@joestringer yes, it should be marked for backport.

vadorovsky · 2020-05-06T15:15:00Z

I will backport it in #11233

tgraf added kind/bug This is a bug in the Cilium logic. pending-review release-note/bug This PR fixes an issue in a previous release of Cilium. labels Apr 30, 2020

tgraf requested a review from a team April 30, 2020 10:40

maintainer-s-little-helper bot added this to In progress in 1.8.0 Apr 30, 2020

errordeveloper approved these changes Apr 30, 2020

View reviewed changes

tgraf force-pushed the pr/tgraf/server-version-bootstrap branch from 67f41e6 to 51b78b6 Compare April 30, 2020 13:47

tgraf requested review from Weil0ng and aanm April 30, 2020 13:47

aanm requested changes Apr 30, 2020

View reviewed changes

tgraf force-pushed the pr/tgraf/server-version-bootstrap branch from 51b78b6 to eb53fe1 Compare May 1, 2020 09:35

tgraf requested a review from a team May 1, 2020 09:35

tgraf requested review from a team as code owners May 1, 2020 09:35

tgraf changed the title ~~k8s: Don't treat failure discover API groups as fatal~~ k8s: Provide fallback for EndpointSlices detection if discovery API is not available May 1, 2020

tgraf force-pushed the pr/tgraf/server-version-bootstrap branch from eb53fe1 to 46403af Compare May 1, 2020 12:46

aanm requested changes May 1, 2020

View reviewed changes

Weil0ng reviewed May 1, 2020

View reviewed changes

pkg/k8s/version/version.go Outdated Show resolved Hide resolved

Weil0ng reviewed May 1, 2020

View reviewed changes

pkg/k8s/version/version.go Outdated Show resolved Hide resolved

joestringer approved these changes May 1, 2020

View reviewed changes

tgraf added 2 commits May 3, 2020 13:38

tgraf force-pushed the pr/tgraf/server-version-bootstrap branch from 46403af to 9f48402 Compare May 3, 2020 11:44

tgraf requested a review from aanm May 3, 2020 11:44

aanm approved these changes May 3, 2020

View reviewed changes

tgraf merged commit 6fcf0cd into master May 4, 2020

1.8.0 automation moved this from In progress to Merged May 4, 2020

tgraf deleted the pr/tgraf/server-version-bootstrap branch May 4, 2020 08:16

Weil0ng mentioned this pull request May 5, 2020

v1.7 backports 2020-04-29 #11233

Merged

aanm added the needs-backport/1.7 label May 6, 2020

maintainer-s-little-helper bot added this to Needs backport from master in 1.7.4 May 6, 2020

vadorovsky added backport-pending/1.7 and removed needs-backport/1.7 labels May 6, 2020

maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.4 May 6, 2020

nebril mentioned this pull request May 7, 2020

GKE tests fail because of a deadlock between Cilium and metrics-server #11262

Closed

aanm added backport-done/1.7 and removed backport-pending/1.7 labels May 13, 2020

maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.4 May 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

k8s: Provide fallback for EndpointSlices detection if discovery API is not available #11253

k8s: Provide fallback for EndpointSlices detection if discovery API is not available #11253

tgraf commented Apr 30, 2020 •

edited

errordeveloper left a comment

coveralls commented Apr 30, 2020 •

edited

tgraf commented Apr 30, 2020

tgraf commented Apr 30, 2020 •

edited

aanm left a comment

tgraf commented May 1, 2020

tgraf commented May 1, 2020

aanm left a comment

aanm May 1, 2020

tgraf May 3, 2020

joestringer left a comment

tgraf commented May 3, 2020

joestringer commented May 5, 2020 •

edited

aanm commented May 6, 2020

vadorovsky commented May 6, 2020

k8s: Provide fallback for EndpointSlices detection if discovery API is not available #11253

k8s: Provide fallback for EndpointSlices detection if discovery API is not available #11253

Conversation

tgraf commented Apr 30, 2020 • edited

errordeveloper left a comment

Choose a reason for hiding this comment

coveralls commented Apr 30, 2020 • edited

tgraf commented Apr 30, 2020

tgraf commented Apr 30, 2020 • edited

aanm left a comment

Choose a reason for hiding this comment

tgraf commented May 1, 2020

tgraf commented May 1, 2020

aanm left a comment

Choose a reason for hiding this comment

aanm May 1, 2020

Choose a reason for hiding this comment

tgraf May 3, 2020

Choose a reason for hiding this comment

joestringer left a comment

Choose a reason for hiding this comment

tgraf commented May 3, 2020

joestringer commented May 5, 2020 • edited

aanm commented May 6, 2020

vadorovsky commented May 6, 2020

tgraf commented Apr 30, 2020 •

edited

coveralls commented Apr 30, 2020 •

edited

tgraf commented Apr 30, 2020 •

edited

joestringer commented May 5, 2020 •

edited