use Source informer instead of conditions #4296

vaikas · 2020-10-13T09:15:13Z

Proposed Changes

Use source informer instead of conditions. Mismatch in informer / lister being used in the trigger reconciler..

Release Note

- 🐛 Fix bug
Dependency readiness could sometimes be missed because mismatched informer/lister was being used in the trigger reconciler.

Docs

codecov · 2020-10-13T09:21:52Z

Codecov Report

Merging #4296 into master will not change coverage.
The diff coverage is 83.33%.

@@           Coverage Diff           @@
##           master    #4296   +/-   ##
=======================================
  Coverage   80.26%   80.26%           
=======================================
  Files         287      287           
  Lines        7889     7889           
=======================================
  Hits         6332     6332           
  Misses       1174     1174           
  Partials      383      383

Impacted Files	Coverage Δ
pkg/apis/eventing/v1/trigger_lifecycle.go	`45.00% <ø> (ø)`
pkg/reconciler/mtbroker/trigger/trigger.go	`73.07% <80.00%> (ø)`
pkg/reconciler/mtbroker/trigger/controller.go	`77.14% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 109f4c2...9d0d741. Read the comment docs.

pierDipi

/lgtm

pierDipi · 2020-10-13T09:48:23Z

pkg/reconciler/mtbroker/trigger/trigger.go

 	if dependencyAnnotation, ok := t.GetAnnotations()[eventingv1.DependencyAnnotation]; ok {
 		dependencyObjRef, err := eventingv1.GetObjRefFromDependencyAnnotation(dependencyAnnotation)
 		if err != nil {
 			t.Status.MarkDependencyFailed("ReferenceError", "Unable to unmarshal objectReference from dependency annotation of trigger: %v", err)
 			return fmt.Errorf("getting object ref from dependency annotation %q: %v", dependencyAnnotation, err)
 		}
-		trackKResource := r.kresourceTracker.TrackInNamespace(ctx, b)
+		trackSource := r.sourceTracker.TrackInNamespace(ctx, t)


Good catch!

knative-prow-robot · 2020-10-13T09:57:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pierDipi, vaikas

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~~ [pierDipi,vaikas]

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

vaikas · 2020-10-13T10:05:24Z

Sinkbinding test failed with the deployment not getting to be ready, so not really surprising that the events were not observed:

          "status": {
            "conditions": [
              {
                "lastTransitionTime": "2020-10-13T09:31:30Z",
                "lastUpdateTime": "2020-10-13T09:31:30Z",
                "message": "Deployment does not have minimum availability.",
                "reason": "MinimumReplicasUnavailable",
                "status": "False",
                "type": "Available"
              },
              {
                "lastTransitionTime": "2020-10-13T09:31:30Z",
                "lastUpdateTime": "2020-10-13T09:31:30Z",
                "message": "ReplicaSet \"e2e-sink-binding-deployment-84d755c8\" is progressing.",
                "reason": "ReplicaSetUpdated",
                "status": "True",
                "type": "Progressing"
              }
            ],
            "observedGeneration": 1,
            "replicas": 1,
            "unavailableReplicas": 1,
            "updatedReplicas": 1

* use Source informer instead of conditions * remove now unused broker variable

* use Source informer instead of conditions (#4296) * use Source informer instead of conditions * remove now unused broker variable * signature changed so can not just straight cp

grantr · 2020-10-13T23:03:45Z

@vaikas can you write a more detailed explanation of what the mismatch was? Why did changing from the kresource duck to the source duck fix it? Technically this feature should only need the Ready status of the kresource duck.

vaikas · 2020-10-14T08:48:36Z

The other bit (in addition to the duck change, was the change to which Informer we use) here:
https://github.com/knative/eventing/pull/4296/files#diff-799e9e97404c7e93e75ee42a60f1ab4c15bd51bf04cd7a923d7f71ea7d478c7dR69

We were using the Conditions informer, but using the KResource lister and hence hilarity ensued. We do not have a KResource informer that I'm aware of which would've also been a way to fix this (namely, use the same informer / lister). But since the feature is explicitly tracking Sources, seemed like a fine way to fix it by changing it to use informer for Source and hence the matching lister.

grantr · 2020-10-14T15:53:38Z

Thanks for the explanation, I get it now :)

use Source informer instead of conditions

5301f9e

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 13, 2020

knative-prow-robot requested review from nachocano and pierDipi October 13, 2020 09:15

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 13, 2020

remove now unused broker variable

9d0d741

pierDipi approved these changes Oct 13, 2020

View reviewed changes

knative-prow-robot assigned pierDipi Oct 13, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2020

knative-prow-robot merged commit 2078491 into knative:master Oct 13, 2020

This was referenced Oct 13, 2020

use the correct informer/lister pair knative-extensions/eventing-rabbitmq#123

Merged

grab the latest eventing to reduce flakiness mattmoor/mink#138

Merged

vaikas deleted the repro-4294 branch October 13, 2020 11:45

vaikas added a commit to vaikas/eventing that referenced this pull request Oct 13, 2020

use Source informer instead of conditions (knative#4296)

5ea7f94

* use Source informer instead of conditions * remove now unused broker variable

vaikas added a commit to vaikas/eventing that referenced this pull request Oct 13, 2020

use Source informer instead of conditions (knative#4296)

fda8b39

* use Source informer instead of conditions * remove now unused broker variable

vaikas added a commit to vaikas/eventing that referenced this pull request Oct 13, 2020

use Source informer instead of conditions (knative#4296)

44efddc

* use Source informer instead of conditions * remove now unused broker variable

knative-prow-robot pushed a commit that referenced this pull request Oct 13, 2020

use Source informer instead of conditions (#4296) (#4300)

8be947b

* use Source informer instead of conditions * remove now unused broker variable

grantr mentioned this pull request Oct 13, 2020

Figure out if we need knative/eventing#4296 google/knative-gcp#1835

Closed

Ectelion mentioned this pull request Oct 31, 2020

Use consistent informer and lister for dependency tracking google/knative-gcp#1883

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use Source informer instead of conditions #4296

use Source informer instead of conditions #4296

vaikas commented Oct 13, 2020

codecov bot commented Oct 13, 2020 •

edited

Loading

pierDipi left a comment

pierDipi Oct 13, 2020

knative-prow-robot commented Oct 13, 2020

vaikas commented Oct 13, 2020

grantr commented Oct 13, 2020

vaikas commented Oct 14, 2020

grantr commented Oct 14, 2020

use Source informer instead of conditions #4296

use Source informer instead of conditions #4296

Conversation

vaikas commented Oct 13, 2020

Proposed Changes

codecov bot commented Oct 13, 2020 • edited Loading

Codecov Report

pierDipi left a comment

Choose a reason for hiding this comment

pierDipi Oct 13, 2020

Choose a reason for hiding this comment

knative-prow-robot commented Oct 13, 2020

vaikas commented Oct 13, 2020

grantr commented Oct 13, 2020

vaikas commented Oct 14, 2020

grantr commented Oct 14, 2020

codecov bot commented Oct 13, 2020 •

edited

Loading