Add support for Conditions in the BuildRun resource #515

qu1queee · 2020-12-09T20:26:59Z

Fixes #307

In a nutshell this PR adds support for Conditions, but keep the old fields Status.Reason and Status.Succeeded for backwards compatibility.(we should remove them at some point in the future).

Based on each commit message(6 commits), this PR is doing the following changes:

Add condition test cases to integration test. Adds test cases for the three main status condition:
- unknown (ramp-up and early phases of buildrun)
- true (buildrun runs through fine)
- false (error scenarios like timeout or misconfiguration)
Introduce conditions for buildruns
- Add function to translate the taskrun condition into a buildrun one.
Refactor build and buildrun timeout logic
- Externalize logic to get the effective timeout from build and buildrun tuple into a separate function.
Change CRD printer columns to use conditions
- Change Succeeded column to use the Succeeded condition Status field.
- Change Reason column to use the Succeeded condition Ready field.
- Add Message column based on the Succeeded condition Message field.
Update docs with new printer output
- Update buildrun.md documentation with a new example that shows the new columns.
Update e2e tests with BuildRun Conditions
- Small refactoring to ensure we are asserting the fields of the Conditions object, rather than the old Status fields

If you are wondering how this looks:

For Succeeded BuildRuns:

For Failed BuildRuns:

On this scenario, we introduced a new field, that will be only populated on failures. This is useful for third-parties that wanna get the exact container that failed on the pod, by consuming the values of this new field(status.failedAt)

For BuildRuns that timedOut:

Add test cases for the three main status condition: - unknown (ramp-up and early phases of buildrun) - true (buildrun runs through fine) - false (error scenarios like timeout or misconfiguration) Signed-off-by: Enrique Eduardo Encalada Olivas <[email protected]>

Add function to translate the taskrun condition into a buildrun one. Signed-off-by: Enrique Eduardo Encalada Olivas <[email protected]>

Externalize logic to get the effective timeout from build and builrun tuple into a separate function. Signed-off-by: Enrique Eduardo Encalada Olivas <[email protected]>

adambkaplan · 2020-12-10T17:03:32Z

Created #517 to track the removal of these redundant fields. I've also created a master tracking issue for breaking API changes we want to consider: #516.

adambkaplan · 2020-12-10T17:08:24Z

pkg/controller/buildrun/buildrun_controller.go

+	case v1beta1.TaskRunReasonTimedOut:
+		reason = "BuildRunTimeout"
+		message = fmt.Sprintf("BuildRun %s failed to finish within %s",
+			taskRun.GetLabels()[buildv1alpha1.LabelBuildRun],


nit - we probably don't need the name since in the message will be inside the BuildRun object.

From a user that is looking at the Conditions directly from the BuildRun I agree, but for the scenarios where someone is building a workflow on top(e.g. Shipwright/CLI), this will help to make the message very explicit. Therefore I would opt to not change this.

adambkaplan · 2020-12-10T17:09:34Z

pkg/controller/buildrun/buildrun_controller.go

+		)
+
+	case v1beta1.TaskRunReasonFailed:
+		if taskRun.Status.CompletionTime != nil {


What happens if the TaskRun is not complete? Would we expect this to be a time out error?

In theory no. If the TaskRun Condition.Reason is Failed, this means that something failed in the Pod and Completion will be set always after the Failed is set in the TaskRun Condition.Reason. You can dive in the code to verify that part.

Also, Tekton have this nice table that illustrates the above, see table from tekton. You will find there that whevener the Reason is Failed, the CompletionTime is set.

Therefore, to answer your question, a TaskRun cannot be Failed and "not complete" .

adambkaplan · 2020-12-10T17:13:17Z

pkg/controller/buildrun/buildrun_controller.go

+			}
+
+			if failedContainer != nil {
+				message = fmt.Sprintf("buildrun step failed in pod %s, for detailed information: kubectl --namespace %s logs %s --container=%s",


Perhaps we bubble up terminated state message from the pod here, rather than a "look at the logs" message? https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.20/#containerstateterminated-v1-core

@adambkaplan interesting, let me play with that tomorrow, and then I can give you a proper answer. Note: This constructed message is very similar to what Tekton does on a failed Taskrun, it provides you some metadata + the kubectl cmd to execute.

@adambkaplan I took a look on this, this will not work, while Tekton does not exposes internal results. They override the message with InternalTektonResult, this will look like:

state: terminated: containerID: containerd://fd0627262107d2339abc75261afd4ca66a91f1b6a3f19e2a3ae68a9ff817138e exitCode: 1 finishedAt: "2020-12-14T17:06:19Z" message: '[{"key":"StartedAt","value":"2020-12-14T17:06:19.706Z","type":"InternalTektonResult"}]' reason: Error startedAt: "2020-12-14T17:06:13Z"

I think the current approach is good for us, also it will help users to move into the right direction, which is "getting the logs of the failed step".

deploy/crds/build.dev_buildruns_crd.yaml

pkg/apis/build/v1alpha1/buildrun_types.go

SaschaSchwarze0

Sry for the late feedback. Looks good. Just a few suggestions.

pkg/controller/buildrun/buildrun_controller.go

test/e2e/validators.go

Change `Succeeded` column to use the Succeeded condition Status field. Change `Reason` column to use the Succeeded condition Ready field. Add `Message` column based on the Succeeded condition Message field.

Update `buildrun.md` documentation with a new example that shows the new columns.

Small refactoring to ensure we are asserting the fields of the Conditions object, rather than the old Status fields Signed-off-by: Matthias Diester <[email protected]>

SaschaSchwarze0

/lgtm

openshift-ci-robot · 2020-12-17T12:50:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SaschaSchwarze0

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~~ [SaschaSchwarze0]

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

SaschaSchwarze0 · 2020-12-17T13:12:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SaschaSchwarze0

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:

Sry @adambkaplan, this was not desired. Wanted to wait for your feedback. I still do not understand why the approved tag is added for me when I did the GitHub approve vs this did not happen for Jordan. If my /lgtm at the same time is the cause, then this is not intuitive.

gabemontero · 2021-01-08T17:15:56Z

[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: SaschaSchwarze0
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:

Sry @adambkaplan, this was not desired. Wanted to wait for your feedback. I still do not understand why the approved tag is added for me when I did the GitHub approve vs this did not happen for Jordan. If my /lgtm at the same time is the cause, then this is not intuitive.

Don't know if you got the answer to your question @SaschaSchwarze0 but a lgtm from an approver in the OWNERS file adds both lgtm and approved labels.... your ID is listed as an approver in https://github.com/shipwright-io/build/blob/master/OWNERS for example, at least with the latest version

I don't know the precise github wiring that makes this happen but it is the same in the openshift repos.

By comparison, @zhangtbj 's review approval via #515 (review) is not the same as an PR lgtm or approve wrt the github wiring I am referring to.

gabemontero · 2021-01-08T17:20:22Z

I've added discussing any post merge actions to the Jan 11 community agenda ^^

gabemontero · 2021-01-08T17:28:46Z

[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: SaschaSchwarze0
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:

Sry @adambkaplan, this was not desired. Wanted to wait for your feedback. I still do not understand why the approved tag is added for me when I did the GitHub approve vs this did not happen for Jordan. If my /lgtm at the same time is the cause, then this is not intuitive.

Don't know if you got the answer to your question @SaschaSchwarze0 but a lgtm from an approver in the OWNERS file adds both lgtm and approved labels.... your ID is listed as an approver in https://github.com/shipwright-io/build/blob/master/OWNERS for example, at least with the latest version

I don't know the precise github wiring that makes this happen but it is the same in the openshift repos.

Duh, forgot the automatic approve process provides a link to the process and how things work. The lgtm from owner adding approve quirk is discussed at https://github.com/kubernetes/community/blob/master/contributors/guide/owners.md#quirks-of-the-process

This is all part of the k8s prow infrastructure used by k8s and openshift, which has been adopted for our repo.

By comparison, @zhangtbj 's review approval via #515 (review) is not the same as an PR lgtm or approve wrt the github wiring I am referring to.

zhangtbj · 2021-01-09T05:44:15Z

Interesting. I am not sure if I operated something wrong before.

As I know, there are many reviewers for a PR. and if I review this PR or be asked to review this PR and after it looks good to me. I will or what I thought:

Approve my review by using below operation then wait for other's review (At that time, no approved label is added)

If other reviewers review this PR and can also approve by using above way, like Sascha did
Then, someone can mark /lgtm on the issue
If all agree or no other comment, then the approver can mark /approve on the issue to accept the PR

But seems the last action is done automatically if approver approve the PR before?

But for sure, let us discuss and confirm the correct process in next meeting.

Thanks!

SaschaSchwarze0 · 2021-01-09T16:56:27Z

[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: SaschaSchwarze0
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:

Sry @adambkaplan, this was not desired. Wanted to wait for your feedback. I still do not understand why the approved tag is added for me when I did the GitHub approve vs this did not happen for Jordan. If my /lgtm at the same time is the cause, then this is not intuitive.

Don't know if you got the answer to your question @SaschaSchwarze0 but a lgtm from an approver in the OWNERS file adds both lgtm and approved labels.... your ID is listed as an approver in https://github.com/shipwright-io/build/blob/master/OWNERS for example, at least with the latest version

I don't know the precise github wiring that makes this happen but it is the same in the openshift repos.

By comparison, @zhangtbj 's review approval via #515 (review) is not the same as an PR lgtm or approve wrt the github wiring I am referring to.

Yeah, probably, for somebody who worked on the OSS space for some time, this is normal. For somebody new it is imo not intuitive: there are the /lgtm and /approve commands and there are the lgtm and approved labels. Intuitively, I guess that 99 % assume a 1-1 relationship between the command and the label that have the same name. This is not the case here.

Anyway, joking now, software engineers often tend to try to type as less as possible while still doing as much as possible. I personal am capable and have the time to write both commands. But anyway, software engineers also need to be capable of learning non-logical things as they exist everywhere. Cheers.

HeavyWombat added 3 commits December 8, 2020 11:49

Introduce conditions for buildruns

6ce5153

Add function to translate the taskrun condition into a buildrun one. Signed-off-by: Enrique Eduardo Encalada Olivas <[email protected]>

Refactor build and buildrun timeout logic

02b142c

Externalize logic to get the effective timeout from build and builrun tuple into a separate function. Signed-off-by: Enrique Eduardo Encalada Olivas <[email protected]>

openshift-ci-robot requested review from adambkaplan and otaviof December 9, 2020 20:27

qu1queee requested review from zhangtbj and SaschaSchwarze0 December 9, 2020 20:27

qu1queee force-pushed the br_conditions_imp branch 2 times, most recently from 3bac41b to c0516bd Compare December 10, 2020 14:30

adambkaplan mentioned this pull request Dec 10, 2020

Remove BuildRun status.Succeeded and status.Reason #517

Open

adambkaplan requested changes Dec 10, 2020

View reviewed changes

zhangtbj suggested changes Dec 13, 2020

View reviewed changes

deploy/crds/build.dev_buildruns_crd.yaml Outdated Show resolved Hide resolved

pkg/apis/build/v1alpha1/buildrun_types.go Outdated Show resolved Hide resolved

qu1queee force-pushed the br_conditions_imp branch 4 times, most recently from 531be8a to 7d9b581 Compare December 15, 2020 14:22

qu1queee requested review from zhangtbj and adambkaplan December 15, 2020 14:23

zhangtbj approved these changes Dec 16, 2020

View reviewed changes

SaschaSchwarze0 reviewed Dec 16, 2020

View reviewed changes

pkg/controller/buildrun/buildrun_controller.go Outdated Show resolved Hide resolved

pkg/controller/buildrun/buildrun_controller.go Outdated Show resolved Hide resolved

test/e2e/validators.go Outdated Show resolved Hide resolved

qu1queee force-pushed the br_conditions_imp branch from 7d9b581 to f735d89 Compare December 16, 2020 13:02

qu1queee requested a review from SaschaSchwarze0 December 16, 2020 13:02

HeavyWombat and others added 3 commits December 16, 2020 21:42

Change CRD printer columns to use conditions

813d784

Change `Succeeded` column to use the Succeeded condition Status field. Change `Reason` column to use the Succeeded condition Ready field. Add `Message` column based on the Succeeded condition Message field.

Update docs with new printer output

749e753

Update `buildrun.md` documentation with a new example that shows the new columns.

Update e2e tests with BuildRun Conditions

c0ffc16

Small refactoring to ensure we are asserting the fields of the Conditions object, rather than the old Status fields Signed-off-by: Matthias Diester <[email protected]>

qu1queee force-pushed the br_conditions_imp branch from f735d89 to c0ffc16 Compare December 16, 2020 20:42

SaschaSchwarze0 approved these changes Dec 17, 2020

View reviewed changes

openshift-ci-robot assigned SaschaSchwarze0 Dec 17, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 17, 2020

openshift-merge-robot merged commit 72c80ce into shipwright-io:master Dec 17, 2020

gabemontero mentioned this pull request Jan 8, 2021

January 11, 2021 Community Meeting #528

Closed

zhangtbj mentioned this pull request Jan 21, 2021

Propagate annotations from BuildStrategy to TaskRun #539

Merged

qu1queee deleted the br_conditions_imp branch February 15, 2021 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Conditions in the BuildRun resource #515

Add support for Conditions in the BuildRun resource #515

qu1queee commented Dec 9, 2020

adambkaplan commented Dec 10, 2020

adambkaplan Dec 10, 2020

qu1queee Dec 15, 2020

adambkaplan Dec 10, 2020

qu1queee Dec 14, 2020

adambkaplan Dec 10, 2020

qu1queee Dec 14, 2020

qu1queee Dec 15, 2020

SaschaSchwarze0 left a comment

SaschaSchwarze0 left a comment

openshift-ci-robot commented Dec 17, 2020

SaschaSchwarze0 commented Dec 17, 2020

gabemontero commented Jan 8, 2021

gabemontero commented Jan 8, 2021

gabemontero commented Jan 8, 2021

zhangtbj commented Jan 9, 2021

SaschaSchwarze0 commented Jan 9, 2021

Add support for Conditions in the BuildRun resource #515

Add support for Conditions in the BuildRun resource #515

Conversation

qu1queee commented Dec 9, 2020

adambkaplan commented Dec 10, 2020

adambkaplan Dec 10, 2020

Choose a reason for hiding this comment

qu1queee Dec 15, 2020

Choose a reason for hiding this comment

adambkaplan Dec 10, 2020

Choose a reason for hiding this comment

qu1queee Dec 14, 2020

Choose a reason for hiding this comment

adambkaplan Dec 10, 2020

Choose a reason for hiding this comment

qu1queee Dec 14, 2020

Choose a reason for hiding this comment

qu1queee Dec 15, 2020

Choose a reason for hiding this comment

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Dec 17, 2020

SaschaSchwarze0 commented Dec 17, 2020

gabemontero commented Jan 8, 2021

gabemontero commented Jan 8, 2021

gabemontero commented Jan 8, 2021

zhangtbj commented Jan 9, 2021

SaschaSchwarze0 commented Jan 9, 2021