Standardize True/False/Unknown status throughout eventing by capri-xiyue · Pull Request #2341 · knative/eventing

capri-xiyue · 2019-12-26T21:59:02Z

Fixes #2187

Proposed Changes

Standardize True/False/Unknown status throughout eventing by following these rules

when the dependency exists, then the dependant status should be the dependency Ready status.
when the dependency readiness cannot be established (the dependant couldn't get the dependency for various reason), the status should be Unknown
when the dependency does not exist, the status should be False.

Release Note

n3wscott · 2019-12-27T19:21:33Z

This PR is much too large. I would recommend splitting it into one PR per resource.

capri-xiyue · 2019-12-27T20:49:02Z

/hold

grantr

First batch of comments. Thanks @capri-xiyue for doing this consistency work!

grantr · 2019-12-27T22:02:52Z

+		if bc := brokerCondSet.Manage(bs).GetCondition(BrokerConditionReady); bc != nil {
+			msg = bc.Message
+		}
+		if bs.IsUnknown() {


If both IsReady() and IsUnknown() return false, is it guaranteed that IsFalse will return true? Or do the status conventions define False as !Ready && !Unknown?

Put another way, I'm wondering what we should do if IsReady, IsUnknown, and IsFalse all return false. Maybe that never happens?

I added the logic that mark the Broker unknown when the status is invalid (the status is not one of Unknown,False, True)

grantr · 2019-12-27T22:25:10Z

+				} else if test.dependencyStatus == corev1.ConditionUnknown {
+					ts.MarkDependencyUnknown("The status of dependency is unknown", "The status of dependency is unknown: nil")
+				} else {
+					ts.MarkDependencyFailed("The status of dependency is false", "The status of dependency is unknown: nil")


Is this last parameter supposed to say unknown or false?

Yep. The convention of the codebase is to use "Failed" instead of "False"

That is only for batch type resources.

What is the definition of "batch type resource"? I checked the codebase, for example https://github.com/knative/eventing/blob/master/pkg/apis/eventing/v1alpha1/broker_lifecycle.go, it use MarkXXXXFailed for TriggerChannel, Ingress and Filter.

Think of it as 'run to completion', once you create a managed condition set, you either create a "living" or "batch". Living means it has a Top Level Condition that is Ready (True, False, or Unknown), creating a Batch means it has a Top Level Condition that is Succeeded (True, False, or Unknown). I think Batch is a poor name but I think it comes from there since k8s Batch resources have this condition for them.

I checked k8s docs, https://kubernetes.io/docs/concepts/workloads/controllers/jobs-run-to-completion/ is an example of batch type resources, right? In knative eventing, is there any resources that belongs to batch type resources. It looks like we should create another issue to fix this. I checked the code, knative eventing uses MarkXXXFailed for all resources even they belongs to "living".

n3wscott · 2020-01-06T20:23:27Z

 }

+// GetTopLevelCondition returns the top level Condition.
+func (bs *BrokerStatus) GetTopLevelCondition() *apis.Condition {


This change needs to happen in Serving, Sample-Source and Sample-Controller now too right?

Yep. I think this change needs to happen in Serving and Sample-Source. But it won't break anything if people don't use it now. People can also use GetCondition(t apis.ConditionType) to get the top level condition. GetTopLevelCondition just provides a more convenient way to achieve it. What do you mean by "Sample-Controller"(The controller for reconcile)?

n3wscott · 2020-01-06T20:33:57Z

+	} else {
+		msg := bc.Message
+		if bc.IsUnknown() {
+			et.MarkBrokerUnknown("BrokerUnknown", "The status of Broker is Unknown: %s", msg)


I think you can just use the condition directly to propagate: https://github.com/knative/serving/blob/ec457adcb1466abfcbc1c0a069e60285e769d135/pkg/apis/serving/v1alpha1/route_lifecycle.go#L157-L171

The current way is dropping the real reason and messages so it makes it hard to use.

n3wscott · 2020-01-06T20:34:24Z

-		msg := "nil"
-		if bc := brokerCondSet.Manage(bs).GetCondition(BrokerConditionReady); bc != nil {
-			msg = bc.Message
+		msg := bc.Message


see https://github.com/knative/serving/blob/ec457adcb1466abfcbc1c0a069e60285e769d135/pkg/apis/serving/v1alpha1/route_lifecycle.go#L157-L171

n3wscott · 2020-01-06T20:36:45Z

+				} else if test.dependencyStatus == corev1.ConditionUnknown {
+					ts.MarkDependencyUnknown("The status of dependency is unknown", "The status of dependency is unknown: nil")
+				} else {
+					ts.MarkDependencyFailed("The status of dependency is false", "The status of dependency is unknown: nil")


That is only for batch type resources.

n3wscott · 2020-01-06T20:41:54Z

 // subCondSet is a condition set with Ready as the happy condition and
 // ReferencesResolved and ChannelReady as the dependent conditions.
-var subCondSet = apis.NewLivingConditionSet(SubscriptionConditionReferencesResolved, SubscriptionConditionAddedToChannel, SubscriptionConditionChannelReady)
+var SubCondSet = apis.NewLivingConditionSet(SubscriptionConditionReferencesResolved, SubscriptionConditionAddedToChannel, SubscriptionConditionChannelReady)


do you want to export this?

For this

eventing/pkg/apis/eventing/v1alpha1/trigger_lifecycle.go

Line 92 in ab85ce8

sc := messagingv1alpha1.SubCondSet.Manage(ss).GetTopLevelCondition()

, I will need SubCondSet to get top level condition. Is there any disadvantage to export this?

something could edit it and do unexpected things

capri-xiyue · 2020-01-06T22:37:07Z

/unhold

knative-metrics-robot · 2020-01-06T22:39:08Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/eventing/v1alpha1/broker_lifecycle.go	100.0%	95.8%	-4.2
pkg/apis/eventing/v1alpha1/eventtype_lifecycle.go	100.0%	65.0%	-35.0
pkg/apis/eventing/v1alpha1/test_helper.go	82.4%	79.5%	-2.8
pkg/apis/eventing/v1alpha1/trigger_lifecycle.go	67.7%	51.1%	-16.6
pkg/apis/legacysources/v1alpha1/apiserver_lifecycle.go	55.0%	60.0%	5.0
pkg/apis/messaging/v1alpha1/channel_lifecycle.go	100.0%	89.3%	-10.7
pkg/apis/messaging/v1alpha1/in_memory_channel_lifecycle.go	100.0%	80.6%	-19.4
pkg/apis/messaging/v1alpha1/subscription_lifecycle.go	100.0%	84.6%	-15.4
pkg/reconciler/eventtype/eventtype.go	87.0%	83.0%	-4.0
pkg/reconciler/inmemorychannel/controller/inmemorychannel.go	81.1%	79.8%	-1.3
pkg/reconciler/subscription/subscription.go	78.8%	77.9%	-0.8

n3wscott · 2020-01-06T22:54:47Z

Thanks for the cleanup and using the case statements. Looks real nice!

/lgtm
/approve

knative-prow-robot · 2020-01-06T22:55:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: capri-xiyue, n3wscott

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:

~~OWNERS~~ [n3wscott]

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

Standardize True/False/Unknown status throughout eventing

3f0c03c

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

knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 26, 2019

capri-xiyue changed the title ~~[WIPStandardize True/False/Unknown status throughout eventing~~ [WIP]Standardize True/False/Unknown status throughout eventing Dec 26, 2019

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 26, 2019

knative-prow-robot requested review from nachocano and vaikas December 26, 2019 21:59

capri-xiyue commented Dec 26, 2019

View reviewed changes

Comment thread Gopkg.toml Outdated

capri-xiyue assigned grantr and unassigned grantr Dec 26, 2019

n3wscott reviewed Dec 27, 2019

View reviewed changes

Comment thread pkg/apis/eventing/v1alpha1/eventtype_lifecycle.go Outdated

n3wscott reviewed Dec 27, 2019

View reviewed changes

Comment thread pkg/apis/eventing/v1alpha1/trigger_lifecycle.go Outdated

n3wscott reviewed Dec 27, 2019

View reviewed changes

Comment thread pkg/apis/eventing/v1alpha1/trigger_lifecycle.go Outdated

n3wscott reviewed Dec 27, 2019

View reviewed changes

Comment thread pkg/apis/eventing/v1alpha1/trigger_lifecycle.go Outdated

capri-xiyue changed the title ~~[WIP]Standardize True/False/Unknown status throughout eventing~~ Standardize True/False/Unknown status throughout eventing Dec 27, 2019

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 27, 2019

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

grantr reviewed Dec 27, 2019

View reviewed changes

capri-xiyue added 2 commits January 2, 2020 13:33

modified the code based on CR

d97efb1

update knative/pkg dependencies

2545826

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 6, 2020

resolve conflicts with branch master

4ef20a4

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 6, 2020

added unit tests for eventtype_lifeccyle

ab85ce8

n3wscott reviewed Jan 6, 2020

View reviewed changes

modified the code based on comments

2ec7afe

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 6, 2020

knative-prow-robot assigned n3wscott Jan 6, 2020

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

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 6, 2020

knative-prow-robot merged commit b1ced4e into knative:master Jan 6, 2020

capri-xiyue mentioned this pull request Jan 14, 2020

Standardize True/False/Unknown status throughout knative-gcp google/knative-gcp#481

Merged

grantr mentioned this pull request Feb 7, 2020

Add grantr as Eventing WG lead knative/community#83

Merged

6 tasks

Conversation

capri-xiyue commented Dec 26, 2019

Proposed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

n3wscott commented Dec 27, 2019

Uh oh!

capri-xiyue commented Dec 27, 2019

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

capri-xiyue Jan 2, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

capri-xiyue Jan 6, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

capri-xiyue commented Jan 6, 2020

Uh oh!

knative-metrics-robot commented Jan 6, 2020

Uh oh!

n3wscott commented Jan 6, 2020

Uh oh!

knative-prow-robot commented Jan 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

capri-xiyue Jan 2, 2020 •

edited

Loading

capri-xiyue Jan 6, 2020 •

edited

Loading