Set ownerReference to build->buildrun

[qu1queee]

Fixes #324

This PR is the continuation of #371 . I addressed all concerns there into this PR, and preserve @routerhan commits.

This PR replaces the finalizers approach we have in the Build -> BuildRun in favour of OwnerReferences for the same resources, aligning to the same approach as we have between BuildRuns -> TaskRuns. The PR have one commit for fixing go linters I found by using staticcheck ./...(I couldn´t avoid fixing this now)

@zhangtbj @HeavyWombat for your information:

Note: When dealing with OwnerReferences between the Build(owner) and BuildRun(owned), both controllers Build and BuildRun plays a role on ensuring that the BuildRun gets the Ownerreference or that the OwnerReference is removed. Following scenarios to consider:

When the Build have the build.build.dev/build-run-deletion annotation set to true

For the above case, the Ownerreference is added during the CREATION event of a BuildRun, meaning on the BuildRun controller side.

When the Build have the build.build.dev/build-run-deletion annotation set to true, but the user modifies the annotation and set this back to false.

For the above case, the Ownerreference needs to be removed during the UPDATE event of a Build, meaning we need logic on the Build controller reconciler side.

When the Build have the build.build.dev/build-run-deletion annotation set to true, but the user modifies the annotation and set this back to false, but then the user changes his mind and modify it one more time, and set it back to the original value, true.

For the above case, the Ownerreference needs to be removed or added during the UPDATE event of a Build, meaning we need logic on the Build controller reconciler side.

Also please consider that this PR have quite a lot of test cases, both unit-tests and integration-tests. I recommend you to read the integration tests ones.

[HeavyWombat]

Provided a couple of comments where I would like to see the feedback first.

[qu1queee]

/retest

[zhangtbj]

if it is else, the end user may set the AnnotationBuildRunDeletion to yes or aha, then it will also enter this logic which is not good.

[HeavyWombat]

Good point, I would suggest to rewrite the if/else to a switch:

switch b.GetAnnotations()[build.AnnotationBuildRunDeletion] {
	case "true":
		// ...

	case "false":
		// ...

	default:
		// error case ...
}

Could also improve readability.

[qu1queee]

this was on purpose, the important value was "false", but I agree it should be more strict. Fixed.

[qu1queee]

hold on, I didnt see @HeavyWombat comment, enhancing

[qu1queee]

@HeavyWombat so I added this in

build/pkg/controller/build/build_controller.go

Lines 326 to 358 in 98b81eb

	switch b.GetAnnotations()[build.AnnotationBuildRunDeletion] {
	case "true":
	// if the buildRun does not have an ownerreference to the Build, lets add it.
	for _, buildRun := range buildRunList.Items {
	if index := r.validateBuildOwnerReference(buildRun.OwnerReferences, b); index == -1 {
	if err := r.setOwnerReferenceFunc(b, &buildRun, r.scheme); err != nil {
	b.Status.Reason = fmt.Sprintf("unexpected error when trying to set the ownerreference: %v", err)
	if err := r.client.Status().Update(ctx, b); err != nil {
	return err
	}
	}
	if err = r.client.Update(ctx, &buildRun); err != nil {
	return err
	}
	ctxlog.Info(ctx, fmt.Sprintf("succesfully updated BuildRun %s", buildRun.Name), namespace, buildRun.Namespace, name, buildRun.Name)
	}
	}
	case "false":
	// if the buildRun have an ownerreference to the Build, lets remove it
	for _, buildRun := range buildRunList.Items {
	if index := r.validateBuildOwnerReference(buildRun.OwnerReferences, b); index != -1 {
	buildRun.OwnerReferences = removeOwnerReferenceByIndex(buildRun.OwnerReferences, index)
	if err := r.client.Update(ctx, &buildRun); err != nil {
	return err
	}
	ctxlog.Info(ctx, fmt.Sprintf("succesfully updated BuildRun %s", buildRun.Name), namespace, buildRun.Namespace, name, buildRun.Name)
	}
	}
	default:
	ctxlog.Info(ctx, fmt.Sprintf("the annotation %s was not properly defined, supported values are true or false", build.AnnotationBuildRunDeletion), namespace, b.Namespace, name, b.Name)
	return fmt.Errorf("the annotation %s was not properly defined, supported values are true or false", build.AnnotationBuildRunDeletion)
	}

There is one caveat, if the user defines a wrong annotation value (not true or false), then the logic will return an error (ignored in the reconcile parent func) and we will get an INFO log (that illustrates the problem). Ideally I think we want the Build to reflect this issue in its Status.Reason and fail the Build Registration.

But I will prefer to do the above in a second pr, mainly because this use-case was not supported before. Opinions?

[SaschaSchwarze0]

Agree, an annotation value that is neither true nor false is good to be reflected in the build status.I am okay doing this separately.

[zhangtbj]

I am thinking if dynamically update the buildrun in build is a good way for the ownerReference way.

In the new solution, we need to get all buildrun every time no matter if we need to update the buildrun or not in each reconciliation. if there are lots of buildrun or once the build controller is restart, it may be slow.

So our goal is we can provide an option to allow end user:

• delete the buildrun with build during the build deletion
• keep all buildrun during the build deletion

So not sure if we can:

• Set ALL new create buildrun with ownerReference DIRECTLY and BY DEFAULT
• Do not update the buildrun in build to improve performance
• Remove the AnnotationBuildRunDeletion and control it from UX level
• When deleting the build, by default, all related buildrun will be deleted together. But if the user want to keep the buildruns by using UI or CLI/kubectl, we use the orphan delete policy:

curl -X DELETE 
<KUBE_API_SERVER>/.../<BUILD>...
-d '{
  "kind":"DeleteOptions",
  "apiVersion":"v1",
  "propagationPolicy":"Orphan"
}' 
-H "Content-Type: application/json"

or use kubectl delete --cascade=false to delete the build but keep all buildruns

[HeavyWombat]

Added comments where I see where we could further improve.

[qu1queee]

@zhangtbj thanks for the feedback.

In the new solution, we need to get all buildrun every time no matter if we need to update the buildrun or not in each reconciliation. if there are lots of buildrun or once the build controller is restart, it may be slow.

The above is a valid concern but is not fully accurate. It is correct that the Build controller will be looking for BuildRuns on every reconcile, however some key things to know are:

• Build controller only reconciles on CREATE/UPDATE events.
• For a CREATE event, a Build have 0 BuildRuns, therefore is not a concern in terms of performance. Actually listing is not that expensive for the amount of BuildRuns a Build can have.
• For an UPDATE event where none BuildRuns are associated to a Build , listing BuildRuns is almost a no-op.
• For an UPDATE event where BuildRuns are associated to a Build. During reconciliations, all associated BuildRuns will be listed(which is the only use case relate to what you mentioned). I dont think this is a concern in terms of performance. We have a selective filter for listing,only associated BuildRuns which are usually few.

Set ALL new create buildrun with ownerReference DIRECTLY and BY DEFAULT

This is already done in the buildrun controller, during the BuildRun creation only if the annotation is set to true.

Do not update the buildrun in build to improve performance

This is not possible at this point in time. The annotation belongs to the Build, and we watch for events of Build objects only in the Build controller.

Remove the AnnotationBuildRunDeletion and control it from UX level

I´m not sure what you mean.

When deleting the build, by default, all related buildrun will be deleted together. But if the user want to keep the buildruns by using UI or CLI/kubectl, we use the orphan delete policy

Not sure if I understand your point. If you want to keep the associated BuildRuns then do not use the "annotation". If you use the annotation all dependent objects will be deleted.

[zhangtbj]

What I mean is:

• We drop the annotation AnnotationBuildRunDeletion and do not use it any more, and for each new created buildrun, we set the OwnerReferences by default
• The end user decide to delete all or keep all buildruns on UX side during the Build deletion:
  1. If end user wants to delete all buildruns, the OwnerReferences is set by default, so once delete the build, all buildruns will be deleted together
  2. If end user wants to keep all buildruns, they can operate on UI, like [x] Keep all buildruns, then during the build deletion, the UI can have a magic to call the kube api with orphan delete policy then all buildruns will be kept. Or from cli/kubectl level, just use a similar command kubectl delete --cascade=false can keep the all buildruns

It can keep our logic more easy and clear, I think.

[qu1queee]

@zhangtbj can u take this requirement as a separate issue then? this pr is about replacing finalizers in favor of ownerreferenes, by keeping the same behaviour and more importantly adding test cases for the initial feature.

I honestly do not understand the UX rational you are saying. But as I said, please open another issue, this is not the right PR for it.

[zhangtbj]

Sure thing.

Let me open another new issue to track the improvement for that.

[HeavyWombat]

/lgtm

[SaschaSchwarze0]

/approve

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HeavyWombat, 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