Improve CRD liveness/readiness probe #2717

jasiu001 · 2019-10-02T09:47:31Z

This PR is a

Feature Implementation
Bug Fix
Documentation

What this PR does / why we need it:

add missing labels svcat: "true" to CRDs
Delays launch probe (readiness/liveness) which checks if CRDs for ServiceCatalog exists. The probe for now cheks CRD asking Apiserver in every 10 sec which puts a heavy load on the server.
Thanks to delaying the probe inside probe code not at the chart, charts settings (periodSeconds parameter) do not include for another probe (there could be many probes for each readiness and liveness health check).
The second improvement inside probe code is requested for list only CRD belonging to ServiceCatalog not all of them. Thank that response takes up less of resources.

Please leave this checklist in the PR comment so that maintainers can ensure a good PR.

Merge Checklist:

New feature
- Tests
- Documentation
SVCat CLI flag
Server Flag for config
- Chart changes
- removing a flag by marking deprecated and hiding to avoid
  breaking the chart release and existing clients who provide a
  flag that will get an error when they try to update

Related issue:

Provide Migration Path for Apiserver -> CRDs #2634

k8s-ci-robot · 2019-10-02T09:47:45Z

Hi @jasiu001. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mszostok · 2019-10-02T10:37:32Z

/ok-to-test

mszostok · 2019-10-02T10:47:55Z

/test pull-build-all-images-for-arm

mszostok · 2019-10-02T10:56:55Z

pkg/probe/crd_probe.go

+	// DelayCRDProbe - readiness/liveness probe is run each period time defined in `periodSeconds` parameter in chart
+	// if the value is to short then `delay` can postpone checking CRDs which is resource-intensive
+	// period seconds for CRDProbe will be `periodSeconds` * delay
+	DelayCRDProbe = 60


maybe we can name it a little bit different? delay for me is quite connected with time and here we have contract which is saying that we should perform check action on X execution

mszostok · 2019-10-02T10:58:14Z

pkg/probe/crd_probe.go

-func (r *ReadinessCRD) check() (bool, error) {
-	list, err := r.client.ApiextensionsV1beta1().CustomResourceDefinitions().List(v1.ListOptions{})
+func (r *CRDProbe) check() (bool, error) {
+	list, err := r.client.ApiextensionsV1beta1().CustomResourceDefinitions().List(v1.ListOptions{LabelSelector: "svcat=true"})


please replace with

labels.SelectorFromSet(labels.Set{ "svcat": "true", }).String()

I do not want to hardcode the = sign here. It is an implementation details, so better will be to use helpers:)

mszostok · 2019-10-02T10:59:10Z

pkg/probe/crd_probe.go

+func (r *CRDProbe) Check(_ *http.Request) error {
+	if r.counter < r.delay {
+		r.counter++
+		klog.V(4).Infof("CRDProbe %s skiped. Ckeck for %d iteration(s)", r.Name(), r.delay-r.counter)


maybe sth like

Will be executed in %d iteration

@klaudiagrz please check that also:)

Suggested change

klog.V(4).Infof("CRDProbe %s skiped. Ckeck for %d iteration(s)", r.Name(), r.delay-r.counter)

klog.V(4).Infof("%s CRDProbe skipped. Will be executed in %d iteration", r.Name(), r.delay-r.counter)

mszostok · 2019-10-02T10:59:53Z

pkg/probe/crd_probe.go

@@ -45,6 +47,11 @@ const (
 	ServiceInstance = "serviceinstances.servicecatalog.k8s.io"
 	// ServiceBinding define the name of the ServiceBinding CRD
 	ServiceBinding = "servicebindings.servicecatalog.k8s.io"
+
+	// DelayCRDProbe - readiness/liveness probe is run each period time defined in `periodSeconds` parameter in chart


@klaudiagrz please check that description :)

klaudiagrz · 2019-10-02T12:32:19Z

pkg/probe/crd_probe.go

@@ -45,6 +47,11 @@ const (
 	ServiceInstance = "serviceinstances.servicecatalog.k8s.io"
 	// ServiceBinding define the name of the ServiceBinding CRD
 	ServiceBinding = "servicebindings.servicecatalog.k8s.io"
+
+	// DelayCRDProbe - readiness/liveness probe is run each period time defined in `periodSeconds` parameter in chart


Suggested change

// DelayCRDProbe - readiness/liveness probe is run each period time defined in `periodSeconds` parameter in chart

// CRDProbeIterationGap - the number of iterations after which the CRD probe action is performed

klaudiagrz · 2019-10-02T12:38:25Z

pkg/probe/crd_probe.go

@@ -45,6 +47,11 @@ const (
 	ServiceInstance = "serviceinstances.servicecatalog.k8s.io"
 	// ServiceBinding define the name of the ServiceBinding CRD
 	ServiceBinding = "servicebindings.servicecatalog.k8s.io"
+
+	// DelayCRDProbe - readiness/liveness probe is run each period time defined in `periodSeconds` parameter in chart
+	// if the value is to short then `delay` can postpone checking CRDs which is resource-intensive


Suggested change

// if the value is to short then `delay` can postpone checking CRDs which is resource-intensive

// The CRD probe is run after the time period defined in the `periodSeconds` parameter in the chart

klaudiagrz · 2019-10-02T12:39:13Z

pkg/probe/crd_probe.go

+
+	// DelayCRDProbe - readiness/liveness probe is run each period time defined in `periodSeconds` parameter in chart
+	// if the value is to short then `delay` can postpone checking CRDs which is resource-intensive
+	// period seconds for CRDProbe will be `periodSeconds` * delay


Suggested change

// period seconds for CRDProbe will be `periodSeconds` * delay

// Time needed for the CRD Probe to complete is `periodSeconds` * CRDProbeIterationGap

klaudiagrz · 2019-10-02T12:41:51Z

pkg/probe/crd_probe.go

+func (r *CRDProbe) Check(_ *http.Request) error {
+	if r.counter < r.delay {
+		r.counter++
+		klog.V(4).Infof("CRDProbe %s skiped. Ckeck for %d iteration(s)", r.Name(), r.delay-r.counter)


Suggested change

klog.V(4).Infof("CRDProbe %s skiped. Ckeck for %d iteration(s)", r.Name(), r.delay-r.counter)

klog.V(4).Infof("%s CRDProbe skipped. Will be executed in %d iteration", r.Name(), r.delay-r.counter)

jberkhahn · 2019-10-02T18:25:16Z

/test pull-service-catalog-test-integration

jasiu001 · 2019-10-04T05:41:39Z

/test pull-build-all-images-for-s390x

mszostok · 2019-10-04T07:39:02Z

/lgtm
/approve

k8s-ci-robot · 2019-10-04T07:39:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jasiu001, mszostok

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:

~~OWNERS~~ [mszostok]

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

mszostok · 2019-10-04T07:45:38Z

/hold cancel

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

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 2, 2019

k8s-ci-robot requested review from jberkhahn and mszostok October 2, 2019 09:47

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 2, 2019

mszostok reviewed Oct 2, 2019

View reviewed changes

klaudiagrz reviewed Oct 2, 2019

View reviewed changes

jasiu001 force-pushed the readiness-probe branch from d18ff89 to 7eaced9 Compare October 3, 2019 09:05

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 3, 2019

jasiu001 force-pushed the readiness-probe branch 4 times, most recently from 733b89b to d18ff89 Compare October 3, 2019 16:12

Jasiak added 2 commits October 4, 2019 01:55

Improve CRD liveness/readiness probe

a1f93ab

Fix after CR#1

6d9ef6b

mszostok force-pushed the readiness-probe branch from d18ff89 to 6d9ef6b Compare October 3, 2019 23:56

k8s-ci-robot assigned mszostok Oct 4, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2019

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

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 4, 2019

k8s-ci-robot merged commit 943b96d into kubernetes-retired:master Oct 4, 2019

mszostok mentioned this pull request Oct 4, 2019

Bump Service Catalog release to 0.3.0-beta.1 #2722

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve CRD liveness/readiness probe #2717

Improve CRD liveness/readiness probe #2717

jasiu001 commented Oct 2, 2019 •

edited by mszostok

Loading

k8s-ci-robot commented Oct 2, 2019

mszostok commented Oct 2, 2019

mszostok commented Oct 2, 2019

mszostok Oct 2, 2019

mszostok Oct 2, 2019

mszostok Oct 2, 2019

klaudiagrz Oct 2, 2019

mszostok Oct 2, 2019

klaudiagrz Oct 2, 2019

klaudiagrz Oct 2, 2019

klaudiagrz Oct 2, 2019

klaudiagrz Oct 2, 2019

jberkhahn commented Oct 2, 2019

jasiu001 commented Oct 4, 2019

mszostok commented Oct 4, 2019

k8s-ci-robot commented Oct 4, 2019

mszostok commented Oct 4, 2019

	klog.V(4).Infof("CRDProbe %s skiped. Ckeck for %d iteration(s)", r.Name(), r.delay-r.counter)
	klog.V(4).Infof("%s CRDProbe skipped. Will be executed in %d iteration", r.Name(), r.delay-r.counter)

	// DelayCRDProbe - readiness/liveness probe is run each period time defined in `periodSeconds` parameter in chart
	// CRDProbeIterationGap - the number of iterations after which the CRD probe action is performed

	// if the value is to short then `delay` can postpone checking CRDs which is resource-intensive
	// The CRD probe is run after the time period defined in the `periodSeconds` parameter in the chart

	// period seconds for CRDProbe will be `periodSeconds` * delay
	// Time needed for the CRD Probe to complete is `periodSeconds` * CRDProbeIterationGap

Improve CRD liveness/readiness probe #2717

Improve CRD liveness/readiness probe #2717

Conversation

jasiu001 commented Oct 2, 2019 • edited by mszostok Loading

k8s-ci-robot commented Oct 2, 2019

mszostok commented Oct 2, 2019

mszostok commented Oct 2, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jberkhahn commented Oct 2, 2019

jasiu001 commented Oct 4, 2019

mszostok commented Oct 4, 2019

k8s-ci-robot commented Oct 4, 2019

mszostok commented Oct 4, 2019

jasiu001 commented Oct 2, 2019 •

edited by mszostok

Loading