Remove builder interface, linearize pod creation and watching by imjasonh · Pull Request #464 · knative/build

imjasonh · 2018-11-09T20:58:20Z

Proposed Changes

Remove the builder.Interface which effectively only had a single implementation for on-cluster builds
Linearize pod creation, so it can be read and understood more clearly from top-to-bottom
Remove Operation, callbacks, mutexes, etc.

With this change, we'll listen for all pod events on the cluster, and when we see one for a pod owned by a Build, we'll update that build with the Pod's status.

Release Note

NONE

knative-prow-robot

@imjasonh: 0 warnings.

Details

In response to this:

Proposed Changes

Remove the builder.Interface which effectively only had a single implementation for on-cluster builds

Linearize pod creation, so it can be read and understood more clearly from top-to-bottom

Remove Operation, callbacks, mutexes, etc.

With this change, we'll listen for all pod events on the cluster, and when we see one for a pod owned by a Build, we'll update that build with the Pod's status.

Release Note
NONE

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.

imjasonh · 2018-11-09T21:40:05Z

/test pull-knative-build-integration-tests

shashwathi · 2018-11-09T22:22:29Z

 			Namespace: build.Namespace,
-			// Ensure our Pod gets a unique name.
-			GenerateName: fmt.Sprintf("%s-", build.Name),
+			Name:      fmt.Sprintf("pod-for-%s", build.Name),


How about $(build_name)-pod?
For example if k8s deployment is used, the pod names have deployment name as prefix. It is easier to associate the parent resource with prefix rather than suffix.

I do not have strong opinion on what follows after build_name. Probably a better question to ask is if build can only have a pod why do we need different name.

Good call, done.

FWIW, the reason I dropped GenerateName is because the fake k8s implementation doesn't generate names, and so pods were always created as "". 😠

imjasonh · 2018-11-10T02:08:23Z

The failing test is the YAML test for test-with-low-timeout:

I1109 22:02:23.123] test-with-low-timeout     Succeeded   Unknown   2018-11-09T21:56:46Z   <none>     <none>

It looks like the timeout isn't kicking in and the build continues executing ~forever. Oddly this doesn't seem to happen when I run e2e-tests.sh locally. I'll investigate more and unhold this.

/hold

googlebot · 2018-11-10T03:12:28Z

CLAs look good, thanks!

imjasonh · 2018-11-11T01:40:44Z

/test pull-knative-build-integration-tests

mattmoor · 2018-11-11T16:11:18Z

-	controllers := []controller.Interface{
-		build.NewController(logger, kubeClient, buildClient, buildInformer, buildTemplateInformer, clusterBuildTemplateInformer, bldr),
+	controllers := []*controller.Impl{
+		build.NewController(logger, kubeClient, kubeInformerFactory, buildClient, buildInformer, buildTemplateInformer, clusterBuildTemplateInformer),


The pattern we're using constructs the Pod informer here and passes it in, so that it can be included in the synchronization below.

mattmoor · 2018-11-11T16:15:52Z

+	kubeinformers.Core().V1().Pods().Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
+		AddFunc:    r.addPodEvent,
+		UpdateFunc: r.updatePodEvent,
+		DeleteFunc: r.deletePodEvent,


For this to be properly level-based, these should use impl.EnqueueControllerOf (possibly wrapped with controller.PassNew). For a Pod informer, you also certainly want to use a Filter on the OwnerReference Kind being Build.

In level-based reconciliation, all of the logic is oriented in terms of reconciling Build keys (Reconcile()), and these triggers are very thin, they just figure out what Build to queue and enqueue it.

Thanks for that tip. I tried switching to this and had some trouble in tests getting a signal that the enqueued update happened. Do you happen to have a link to how you handle this in Serving?

Also, good idea filtering for build-owned pods. How do I express that filter to podsLister? NewSharedInformerFactoryWithOptions with a TweakListOptions to specify a FieldSelector?

vdemeester · 2018-11-19T09:47:55Z

-	"k8s.io/client-go/tools/clientcmd"
-
-	// Uncomment the following line to load the gcp plugin (only required to authenticate against GKE clusters).
+	"k8s.io/client-go/tools/clientcmd" // Uncomment the following line to load the gcp plugin (only required to authenticate against GKE clusters).


This comment is not at the right place 😅

imjasonh · 2018-11-19T14:39:55Z

/hold

I have quite a few unpushed changes, I'm thinking this might be better as more smaller PRs. Unfortunately life stuff is taking up a lot of time lately, I probably can't promise much progress until after the Thanksgiving holiday.

imjasonh · 2018-11-20T17:03:37Z

/hold cancel

This change is ready now, passing unit and integration tests. Unfortunately it's pretty big, sorry about that, my intention had been to split it up but it looked like it was all pretty tightly intertwined. The good news is I think this is a lot easier to read and understand (and test!) after this change. It should now be pretty straightforward to forklift this into the TaskRun controller and make it responsible for creating Pods directly.

vdemeester

SGTM 🐯
/approve

shashwathi

/lgtm

shashwathi · 2018-11-26T14:24:49Z

+	if err != nil {
+		return err
 	}
+	newb.Status = u.Status


We could consider checking if there is any change in status object before updating it.
This could be changed in further PR too.

shashwathi · 2018-11-26T14:32:12Z

 		}
 	}
-	return nil, errors.New("watch ended before build completion")
+	return latest, errWatchTimeout


I like this classification of errors.

vdemeester · 2018-11-26T14:58:27Z

oh @imjasonh needs a rebase 🙏

… pod_test.go

vdemeester

/lgtm

knative-prow-robot · 2018-11-26T15:55:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ImJasonH, shashwathi, vdemeester

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~~ [ImJasonH,shashwathi]

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

knative-metrics-robot · 2018-11-26T15:56:26Z

The following is the coverage report on pkg/.
Say /test pull-knative-build-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/build/build.go	76.4%	76.7%	0.3
pkg/reconciler/build/resources/pod.go	84.5%	88.0%	3.6
pkg/reconciler/build/validate_build.go	93.0%	91.4%	-1.6

…e#464) * Remove builder interface, linearize pod creation and watching * Remove pkg/buildtest/wait.go * Create pods like <build-name>-pod * Remove pkg/controller * Improve TestLowTimeout, still needs work * Move BuildStatusFromPod to resources pkg * Skip statuses for multiple sources; add tests for BuildStatusFromPod * Lower timeout, log more * pass build to isTimeout * Generate unique names for each pod, otherwise conflict; move tests to pod_test.go * Sync pod lister before starting controller * update deps * Reconcile in response to pod events * Remove commented import

knative-prow-robot assigned shashwathi Nov 9, 2018

knative-prow-robot requested review from mattmoor and shashwathi November 9, 2018 20:58

knative-prow-robot added the size/XXL label Nov 9, 2018

knative-prow-robot reviewed Nov 9, 2018

View reviewed changes

knative-prow-robot added the approved label Nov 9, 2018

imjasonh force-pushed the builder branch from 1ed73e3 to 5c6f294 Compare November 9, 2018 21:00

shashwathi reviewed Nov 9, 2018

View reviewed changes

knative-prow-robot added the do-not-merge/hold label Nov 10, 2018

imjasonh mentioned this pull request Nov 10, 2018

Delete unused testdata directory, useless test, unused test methods #467

Merged

imjasonh force-pushed the builder branch from 8cf18c4 to 9f91eca Compare November 10, 2018 03:12

knative deleted a comment from googlebot Nov 10, 2018

mattmoor reviewed Nov 11, 2018

View reviewed changes

imjasonh force-pushed the builder branch 2 times, most recently from 7b5dbd7 to 28759d3 Compare November 13, 2018 21:18

imjasonh mentioned this pull request Nov 15, 2018

Add Build status fields to TaskRun status tektoncd/pipeline#236

Closed

vdemeester reviewed Nov 19, 2018

View reviewed changes

knative-prow-robot removed the do-not-merge/hold label Nov 20, 2018

vdemeester approved these changes Nov 20, 2018

View reviewed changes

This was referenced Nov 21, 2018

Break Pipelines' dependency on Build tektoncd/pipeline#252

Closed

Fix swapped reason/message when pod is initializing #476

Merged

shashwathi approved these changes Nov 26, 2018

View reviewed changes

knative-prow-robot added the lgtm label Nov 26, 2018

imjasonh force-pushed the builder branch from 5ec9170 to 5c6f294 Compare November 26, 2018 15:48

knative-prow-robot removed the lgtm label Nov 26, 2018

imjasonh added 14 commits November 26, 2018 10:51

Remove builder interface, linearize pod creation and watching

3d3d177

Remove pkg/buildtest/wait.go

f223a1c

Create pods like <build-name>-pod

07e746c

Remove pkg/controller

9cd5948

Improve TestLowTimeout, still needs work

4b7ed5f

Move BuildStatusFromPod to resources pkg

e57ea66

Skip statuses for multiple sources; add tests for BuildStatusFromPod

eb02e12

Lower timeout, log more

afd2d71

pass build to isTimeout

d70091c

Generate unique names for each pod, otherwise conflict; move tests to…

3ef7778

… pod_test.go

Sync pod lister before starting controller

cd5d4db

update deps

0170839

Reconcile in response to pod events

9061535

Remove commented import

1093277

imjasonh force-pushed the builder branch from 5c6f294 to 1093277 Compare November 26, 2018 15:54

vdemeester approved these changes Nov 26, 2018

View reviewed changes

knative-prow-robot assigned vdemeester Nov 26, 2018

knative-prow-robot added the lgtm label Nov 26, 2018

knative-prow-robot merged commit 6343297 into knative:master Nov 26, 2018

imjasonh mentioned this pull request Nov 26, 2018

No init containers #470

Closed

Conversation

imjasonh commented Nov 9, 2018

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

imjasonh commented Nov 9, 2018

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!

imjasonh commented Nov 10, 2018

Uh oh!

googlebot commented Nov 10, 2018

Uh oh!

imjasonh commented Nov 11, 2018

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!

imjasonh commented Nov 19, 2018

Uh oh!

imjasonh commented Nov 20, 2018

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

shashwathi left a comment

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!

vdemeester commented Nov 26, 2018

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Nov 26, 2018

Uh oh!

knative-metrics-robot commented Nov 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants