Add retries and fallback to actual API calls to status updates. by markusthoemmes · Pull Request #6162 · knative/serving

markusthoemmes · 2019-12-05T18:35:46Z

Proposed Changes

See title.

Release Note

Reduced update conflicts.

knative-prow-robot

@markusthoemmes: 0 warnings.

Details

In response to this:

Proposed Changes

See title.

Release Note
Reduced update conflicts.

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.

dprotaso · 2019-12-05T19:04:57Z

if we're reconciling an 'out of date' object should we be updating the status of the latest version of the object?

markusthoemmes · 2019-12-05T19:09:30Z

@dprotaso I thought along the same lines, this is mostly asking Dr. Empirical if this does anything. It indeed cuts down the conflicts from ~300 to

      3 fulfilled on services.serving.knative.dev
     26 fulfilled on ingresses.networking.internal.knative.dev
     27 fulfilled on podautoscalers.autoscaling.internal.knative.dev
     46 fulfilled on configurations.serving.knative.dev

I'll work on adding more debugging next week to try to find out if there's a pattern of what gets in the way here, especially as this seems to be particularly bad downstream on Openshift.

mattmoor · 2019-12-05T19:12:09Z

@dprotaso I think it's fine so long at the .status.observedGeneration reflects the version of the spec that we actually reconciled.

vagababov · 2019-12-05T19:21:53Z

So should we at this point compare generation as well, since it might have changed?

dprotaso · 2019-12-06T16:01:46Z

Poking at some K8s controllers it seems like they attempt to update the status and if it fails they fetch and retry one time (jobs controller retries 3 times 🤷‍♀ )

mattmoor · 2019-12-06T16:06:01Z

@vagababov the point is that if the generation changes, that's fine. The status we are posting is only stating that we've reconciled a previous generation.

@dprotaso I assume the fetch is via the client?

dprotaso · 2019-12-06T16:23:37Z

@dprotaso I assume the fetch is via the client?

yup

vagababov · 2019-12-06T17:31:46Z

Well makes sense, since it's double-checking-lock pattern in essence :)

markusthoemmes · 2019-12-09T11:55:20Z

At the latest state, this PR now produces significantly less churn

      6 fulfilled on ingresses.networking.internal.knative.dev
     12 fulfilled on podautoscalers.autoscaling.internal.knative.dev
     20 fulfilled on configurations.serving.knative.dev

dprotaso · 2019-12-09T15:16:07Z

/test pull-knative-serving-go-coverage

dprotaso · 2019-12-09T15:25:56Z

-	existing := pa.DeepCopy()
-	existing.Status = desired.Status
+func (c *Base) UpdateStatus(desired *pav1alpha1.PodAutoscaler) error {
+	return reconciler.RetryUpdateConflicts(func(i int) error {


can i be something more descriptive like attempt

dprotaso · 2019-12-09T15:36:04Z

+		}
+
+		// If there's nothing to update, just return.
+		if reflect.DeepEqual(existing.Status, desired.Status) {


equality.Semantic.DeepEqual

I realize this wasn't in the prior code - feel free to leave this as a separate PR or I can do it

Yeah, let's leave it separate. Was wondering the same while doing it though.

kmp.Diff ? :)

As mentioned, won't fix in this PR. I don't want to inadvertedly and subtly break things in this PR which is already kinda invasive. Let's make that a separate change please.

+1 to doing separately.

dprotaso · 2019-12-09T15:49:19Z

+			return nil
+		}
+
+		becomesReady := desired.Status.IsReady() && !existing.Status.IsReady()


This makes me wonder - what's causing the conflicts? (ie. is it the labeller or gc'er)

knative-metrics-robot · 2019-12-10T09:59:14Z

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/certificate/certificate.go	88.9%	86.7%	-2.2
pkg/reconciler/configuration/configuration.go	87.0%	85.9%	-1.1
pkg/reconciler/ingress/ingress.go	84.9%	84.0%	-0.9
pkg/reconciler/metric/metric.go	92.3%	87.8%	-4.5
pkg/reconciler/retry.go	Do not exist	100.0%
pkg/reconciler/revision/revision.go	86.7%	84.6%	-2.1
pkg/reconciler/route/reconcile_resources.go	84.2%	83.1%	-1.1
pkg/reconciler/serverlessservice/serverlessservice.go	95.2%	94.0%	-1.1
pkg/reconciler/service/service.go	89.4%	88.3%	-1.1

knative-test-reporter-robot · 2019-12-10T10:02:08Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-unit-tests	pull-knative-serving-unit-tests	1/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-unit-tests

markusthoemmes · 2019-12-10T10:02:27Z

/test pull-knative-serving-unit-tests

markusthoemmes · 2019-12-10T12:26:11Z

Values from the last run

      2 fulfilled on certificates.certmanager.k8s.io
      4 fulfilled on ingresses.networking.internal.knative.dev
      4 fulfilled on podautoscalers.autoscaling.internal.knative.dev
     18 fulfilled on configurations.serving.knative.dev

Configurations failures are all from the labeler. I'd like to attack that in a separate PR though to keep this only about status updates.

mattmoor · 2019-12-10T15:22:42Z

/hold
In case there are other comments.

/lgtm

We should make a similar change to knative/sample-* and knative/eventing

cc @n3wscott @vaikas

vagababov

/lgtm

vaikas · 2019-12-10T18:40:43Z

Should we move the pkg/reconciler stuff into knative.dev/pkg/reconciler? I can do that in a follow up, but since we're doing this in all the repos, seems like it could / should be hoisted?

knative-prow-robot · 2019-12-10T20:18:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, markusthoemmes

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/reconciler/OWNERS~~ [dprotaso,markusthoemmes]

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

vaikas · 2019-12-10T20:29:23Z

removing the hold label since everybody that participated has lgtm'd it.

markusthoemmes · 2019-12-11T07:01:59Z

/test pull-knative-serving-upgrade-tests

markusthoemmes · 2019-12-11T16:00:51Z

/test pull-knative-serving-upgrade-tests

nlopezgi · 2020-01-28T17:58:57Z

Should we move the pkg/reconciler stuff into knative.dev/pkg/reconciler? I can do that in a follow up, but since we're doing this in all the repos, seems like it could / should be hoisted?

Did this in knative/pkg#1008 (as part of google/knative-gcp#508). Also created #6657 to use the retry.go from knative.dev/pkg/reconciler and add more tests.

We should make a similar change to knative/sample-* and knative/eventing

cc @n3wscott @vaikas

I'm planning on taking on doing this change in knative/sample-* and knative/eventing if no one else has plans on doing this atm.

nlopezgi · 2020-01-29T15:51:50Z

We should make a similar change to knative/sample-* and knative/eventing

cc @n3wscott @vaikas

Created knative/eventing#2465 for knative/eventing

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Dec 5, 2019

knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 5, 2019

knative-prow-robot reviewed Dec 5, 2019

View reviewed changes

knative-prow-robot added area/API API objects and controllers area/autoscale area/networking labels Dec 5, 2019

knative-prow-robot requested review from dprotaso, taragu, tcnghia and yanweiguo December 5, 2019 18:36

markusthoemmes added 2 commits December 9, 2019 11:29

Fetch objects via API before updating status to reduce conflicts.

d2ff02e

Implement retries on status updates and fallback to client.

fdf8519

markusthoemmes force-pushed the reduce-update-conflicts branch from 1b5ab78 to fdf8519 Compare December 9, 2019 10:30

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 9, 2019

Use common retry function.

81e13ab

mattmoor reviewed Dec 9, 2019

View reviewed changes

markusthoemmes changed the title ~~Fetch objects via API before updating status to reduce conflicts.~~ Add retries and fallback to actual API calls to status updates. Dec 9, 2019

dprotaso reviewed Dec 9, 2019

View reviewed changes

Remove lister access from retry function.

2bfa75d

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 10, 2019

knative-prow-robot assigned mattmoor Dec 10, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 10, 2019

vagababov reviewed Dec 10, 2019

View reviewed changes

knative-prow-robot assigned vagababov Dec 10, 2019

vaikas mentioned this pull request Dec 10, 2019

Add retries for setting status. knative/eventing#2296

Closed

nachocano mentioned this pull request Dec 10, 2019

Add retries for Setting Status google/knative-gcp#442

Closed

dprotaso approved these changes Dec 10, 2019

View reviewed changes

vaikas removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 10, 2019

nak3 mentioned this pull request Dec 11, 2019

Translate to tcp probe in queue-proxy when exec probe was used for readinessprobe in user-container #5712

Merged

nak3 mentioned this pull request Dec 11, 2019

TestProbe in upgrade-test (pull-knative-serving-upgrade-tests) is broken #6197

Closed

knative-prow-robot merged commit 9d535d0 into knative:master Dec 11, 2019

This was referenced Jan 22, 2020

add retries/fallbacks for update status google/knative-gcp#508

Merged

add retry func for reconciler knative/pkg#1008

Merged

Use retries from knative/pkg and add UTs to reconcilers that use retries #6657

Merged

nlopezgi mentioned this pull request Jan 29, 2020

Add retries and fallback to actual API calls to status updates. knative/eventing#2465

Closed

linkvt mentioned this pull request Nov 12, 2025

Feature Request: Suppress the object has been modified output by default #15746

Closed

Conversation

markusthoemmes commented Dec 5, 2019

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

dprotaso commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markusthoemmes commented Dec 5, 2019

Uh oh!

mattmoor commented Dec 5, 2019

Uh oh!

vagababov commented Dec 5, 2019

Uh oh!

dprotaso commented Dec 6, 2019

Uh oh!

mattmoor commented Dec 6, 2019

Uh oh!

dprotaso commented Dec 6, 2019

Uh oh!

vagababov commented Dec 6, 2019

Uh oh!

markusthoemmes commented Dec 9, 2019

Uh oh!

dprotaso commented Dec 9, 2019

Uh oh!

dprotaso Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

dprotaso Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dprotaso Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

markusthoemmes Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

vagababov Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

markusthoemmes Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

mattmoor Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

dprotaso Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Dec 10, 2019

Uh oh!

knative-test-reporter-robot commented Dec 10, 2019

Uh oh!

markusthoemmes commented Dec 10, 2019

Uh oh!

markusthoemmes commented Dec 10, 2019

Uh oh!

mattmoor commented Dec 10, 2019

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

vaikas commented Dec 10, 2019

Uh oh!

knative-prow-robot commented Dec 10, 2019

Uh oh!

vaikas commented Dec 10, 2019

Uh oh!

markusthoemmes commented Dec 11, 2019

Uh oh!

markusthoemmes commented Dec 11, 2019

Uh oh!

nlopezgi commented Jan 28, 2020

Uh oh!

nlopezgi commented Jan 29, 2020

dprotaso commented Dec 5, 2019 •

edited

Loading

dprotaso Dec 9, 2019 •

edited

Loading