Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change to use the FilteredPodInformer to only watch Tekton relevant pods #3725

Merged
merged 1 commit into from
Feb 4, 2021
Merged

Change to use the FilteredPodInformer to only watch Tekton relevant pods #3725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cmd/controller/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,13 @@ import (
"os"

"github.com/tektoncd/pipeline/pkg/apis/pipeline"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/pkg/reconciler/pipelinerun"
"github.com/tektoncd/pipeline/pkg/reconciler/taskrun"
"github.com/tektoncd/pipeline/pkg/version"
corev1 "k8s.io/api/core/v1"
"k8s.io/client-go/rest"
filteredinformerfactory "knative.dev/pkg/client/injection/kube/informers/factory/filtered"
"knative.dev/pkg/controller"
"knative.dev/pkg/injection"
"knative.dev/pkg/injection/sharedmain"
Expand Down Expand Up @@ -109,6 +111,7 @@ func main() {
log.Fatal(http.ListenAndServe(":"+port, mux))
}()

ctx = filteredinformerfactory.WithSelectors(ctx, v1beta1.ManagedByLabelKey)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused about how this works, so I'd like to check that my understanding is correct.

This WithSelectors modifies the ctx to make it possible to filter on the managed-by label, which we take advantage of later in taskrun/controller.go#55 to create an informer with that ability. Is that correct?

That's a bit... subtle... but I don't know of a better way myself either, and the benefit seems clear.

Is there any kind of test you could add that would demonstrate that the controller doesn't get informed of a Pod it's not managing? That would help ensure that the logic is correct, and guard against future regressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review Jason!

This WithSelectors modifies the ctx to make it possible to filter on the managed-by label, which we take advantage of later in taskrun/controller.go#55 to create an informer with that ability. Is that correct?

Yes, correct.

  • First of all, we need to create a FilteredInformerFactory to setup the FilteredPodInformer, we can pass an label array to filter and cache resources by using multiple labels.
  • Then in the taskrun controller logic, when getting pod resource from informer (taskrun/controller.go#55), the Tekton controller logic can choose ONE of label cached resource from informer store, like the managed-by label we use

Is there any kind of test you could add that would demonstrate that the controller doesn't get informed of a Pod it's not managing? That would help ensure that the logic is correct, and guard against future regressions.

Actually, I tried to add an unit test to cover the fail case. but I found it is hard to me to do that. :(
First of all, it is a framework change, not a functional change
I tried to create the context but without selector by using the knative.dev/pkg/reconciler/testing original SetupFakeContext function. Yes, it will report an error:

=== RUN   TestFilteredPodInformer
    logger.go:130: 2021-01-29T11:02:28.254+0800	PANIC	fake/fake_filtered_factory.go:47	Unable to fetch labelkey from context.
--- FAIL: TestFilteredPodInformer (0.00s)
panic: Unable to fetch labelkey from context. [recovered]
	panic: Unable to fetch labelkey from context.

But it is a panic, the SetupFakeContext function cannot return an error so that I can check if it is the error of Unable to fetch labelkey from context. So we gave up and assume, if in future we miss the WithSelector in context when setting up the filetered pod informer, we will see the same error and all related unit test will fail too. Like the tests which I modified:

  • pkg/reconciler/pipelinerun/metrics_test.go
  • pkg/reconciler/taskrun/metrics_test.go
  • test/v1alpha1/wait_test.go

Or please let me know if you have any good idea! Thanks! :)

sharedmain.MainWithConfig(ctx, ControllerLogKey, cfg,
taskrun.NewController(*namespace, images),
pipelinerun.NewController(*namespace, images),
Expand Down
6 changes: 3 additions & 3 deletions pkg/apis/pipeline/v1alpha1/taskrun_defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import (

var _ apis.Defaultable = (*TaskRun)(nil)

const managedByLabelKey = "app.kubernetes.io/managed-by"
const ManagedByLabelKey = "app.kubernetes.io/managed-by"

func (tr *TaskRun) SetDefaults(ctx context.Context) {
ctx = apis.WithinParent(ctx, tr.ObjectMeta)
Expand All @@ -41,8 +41,8 @@ func (tr *TaskRun) SetDefaults(ctx context.Context) {
if tr.ObjectMeta.Labels == nil {
tr.ObjectMeta.Labels = map[string]string{}
}
if _, found := tr.ObjectMeta.Labels[managedByLabelKey]; !found {
tr.ObjectMeta.Labels[managedByLabelKey] = cfg.Defaults.DefaultManagedByLabelValue
if _, found := tr.ObjectMeta.Labels[ManagedByLabelKey]; !found {
tr.ObjectMeta.Labels[ManagedByLabelKey] = cfg.Defaults.DefaultManagedByLabelValue
}
}

Expand Down
6 changes: 3 additions & 3 deletions pkg/apis/pipeline/v1beta1/taskrun_defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import (

var _ apis.Defaultable = (*TaskRun)(nil)

const managedByLabelKey = "app.kubernetes.io/managed-by"
const ManagedByLabelKey = "app.kubernetes.io/managed-by"

func (tr *TaskRun) SetDefaults(ctx context.Context) {
ctx = apis.WithinParent(ctx, tr.ObjectMeta)
Expand All @@ -39,8 +39,8 @@ func (tr *TaskRun) SetDefaults(ctx context.Context) {
if tr.ObjectMeta.Labels == nil {
tr.ObjectMeta.Labels = map[string]string{}
}
if _, found := tr.ObjectMeta.Labels[managedByLabelKey]; !found {
tr.ObjectMeta.Labels[managedByLabelKey] = cfg.Defaults.DefaultManagedByLabelValue
if _, found := tr.ObjectMeta.Labels[ManagedByLabelKey]; !found {
tr.ObjectMeta.Labels[ManagedByLabelKey] = cfg.Defaults.DefaultManagedByLabelValue
}
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/reconciler/pipelinerun/metrics_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,13 @@ import (
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
fakepipelineruninformer "github.com/tektoncd/pipeline/pkg/client/injection/informers/pipeline/v1beta1/pipelinerun/fake"
"github.com/tektoncd/pipeline/pkg/names"
ttesting "github.com/tektoncd/pipeline/pkg/reconciler/testing"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/pkg/apis"
duckv1beta1 "knative.dev/pkg/apis/duck/v1beta1"
"knative.dev/pkg/metrics/metricstest" // Required to setup metrics env for testing
_ "knative.dev/pkg/metrics/testing"
rtesting "knative.dev/pkg/reconciler/testing"
)

var (
Expand Down Expand Up @@ -154,7 +154,7 @@ func TestRecordRunningPipelineRunsCount(t *testing.T) {
}
}

ctx, _ := rtesting.SetupFakeContext(t)
ctx, _ := ttesting.SetupFakeContext(t)
informer := fakepipelineruninformer.Get(ctx)
// Add N randomly-named PipelineRuns with differently-succeeded statuses.
for _, tr := range []*v1beta1.PipelineRun{
Expand Down
4 changes: 2 additions & 2 deletions pkg/reconciler/taskrun/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import (
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/tools/cache"
kubeclient "knative.dev/pkg/client/injection/kube/client"
podinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/pod"
filteredpodinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/pod/filtered"
"knative.dev/pkg/configmap"
"knative.dev/pkg/controller"
"knative.dev/pkg/kmeta"
Expand All @@ -52,7 +52,7 @@ func NewController(namespace string, images pipeline.Images) func(context.Contex
taskRunInformer := taskruninformer.Get(ctx)
taskInformer := taskinformer.Get(ctx)
clusterTaskInformer := clustertaskinformer.Get(ctx)
podInformer := podinformer.Get(ctx)
podInformer := filteredpodinformer.Get(ctx, v1beta1.ManagedByLabelKey)
resourceInformer := resourceinformer.Get(ctx)
metrics, err := NewRecorder()
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions pkg/reconciler/taskrun/metrics_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,13 @@ import (
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
faketaskruninformer "github.com/tektoncd/pipeline/pkg/client/injection/informers/pipeline/v1beta1/taskrun/fake"
"github.com/tektoncd/pipeline/pkg/names"
ttesting "github.com/tektoncd/pipeline/pkg/reconciler/testing"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/pkg/apis"
duckv1beta1 "knative.dev/pkg/apis/duck/v1beta1"
"knative.dev/pkg/metrics/metricstest"
_ "knative.dev/pkg/metrics/testing"
rtesting "knative.dev/pkg/reconciler/testing"
)

var (
Expand Down Expand Up @@ -243,7 +243,7 @@ func TestRecordRunningTaskRunsCount(t *testing.T) {
}
}

ctx, _ := rtesting.SetupFakeContext(t)
ctx, _ := ttesting.SetupFakeContext(t)
informer := faketaskruninformer.Get(ctx)
// Add N randomly-named TaskRuns with differently-succeeded statuses.
for _, tr := range []*v1beta1.TaskRun{
Expand Down
26 changes: 24 additions & 2 deletions pkg/reconciler/testing/logger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,28 +4,50 @@ import (
"context"
"testing"

"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/record"
filteredinformerfactory "knative.dev/pkg/client/injection/kube/informers/factory/filtered"
"knative.dev/pkg/injection"
logtesting "knative.dev/pkg/logging/testing"

"github.com/tektoncd/pipeline/pkg/reconciler/events/cloudevent"
"go.uber.org/zap"
"go.uber.org/zap/zaptest"
"knative.dev/pkg/controller"
"knative.dev/pkg/logging"
rtesting "knative.dev/pkg/reconciler/testing"

// Import for creating fake filtered factory in the test
_ "knative.dev/pkg/client/injection/kube/informers/factory/filtered/fake"
)

// SetupFakeContext sets up the the Context and the fake filtered informers for the tests.
func SetupFakeContext(t *testing.T) (context.Context, []controller.Informer) {
ctx, informer := rtesting.SetupFakeContext(t)
ctx, _, informer := SetupFakeContextWithLabelKey(t)
cloudEventClientBehaviour := cloudevent.FakeClientBehaviour{
SendSuccessfully: true,
}
ctx = cloudevent.WithClient(ctx, &cloudEventClientBehaviour)
return WithLogger(ctx, t), informer
}

// WithLogger returns the the Logger
func WithLogger(ctx context.Context, t *testing.T) context.Context {
return logging.WithLogger(ctx, TestLogger(t))
}

// TestLogger sets up the the Logger
func TestLogger(t *testing.T) *zap.SugaredLogger {
logger := zaptest.NewLogger(t, zaptest.WrapOptions(zap.AddCaller()))
return logger.Sugar().Named(t.Name())
}

// SetupFakeContextWithLabelKey sets up the the Context and the fake informers for the tests
// The provided context includes the FilteredInformerFactory LabelKey.
func SetupFakeContextWithLabelKey(t zaptest.TestingT) (context.Context, context.CancelFunc, []controller.Informer) {
ctx, c := context.WithCancel(logtesting.TestContextWithLogger(t))
ctx = controller.WithEventRecorder(ctx, record.NewFakeRecorder(1000))
ctx = filteredinformerfactory.WithSelectors(ctx, v1beta1.ManagedByLabelKey)
ctx, is := injection.Fake.SetupInformers(ctx, &rest.Config{})
return ctx, c, is
}
4 changes: 2 additions & 2 deletions test/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ import (
"k8s.io/client-go/tools/record"
fakekubeclient "knative.dev/pkg/client/injection/kube/client/fake"
fakeconfigmapinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/fake"
fakepodinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/pod/fake"
fakefilteredpodinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/pod/filtered/fake"
fakeserviceaccountinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/serviceaccount/fake"
"knative.dev/pkg/controller"
)
Expand Down Expand Up @@ -174,7 +174,7 @@ func SeedTestData(t *testing.T, ctx context.Context, d Data) (Clients, Informers
ClusterTask: fakeclustertaskinformer.Get(ctx),
PipelineResource: fakeresourceinformer.Get(ctx),
Condition: fakeconditioninformer.Get(ctx),
Pod: fakepodinformer.Get(ctx),
Pod: fakefilteredpodinformer.Get(ctx, v1beta1.ManagedByLabelKey),
ConfigMap: fakeconfigmapinformer.Get(ctx),
ServiceAccount: fakeserviceaccountinformer.Get(ctx),
}
Expand Down
4 changes: 2 additions & 2 deletions test/v1alpha1/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import (
coreinformers "k8s.io/client-go/informers/core/v1"
fakekubeclientset "k8s.io/client-go/kubernetes/fake"
fakekubeclient "knative.dev/pkg/client/injection/kube/client/fake"
fakepodinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/pod/fake"
fakefilteredpodinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/pod/filtered/fake"
"knative.dev/pkg/controller"
)

Expand Down Expand Up @@ -101,7 +101,7 @@ func SeedTestData(t *testing.T, ctx context.Context, d Data) (Clients, Informers
ClusterTask: fakeclustertaskinformer.Get(ctx),
PipelineResource: fakeresourceinformer.Get(ctx),
Condition: fakeconditioninformer.Get(ctx),
Pod: fakepodinformer.Get(ctx),
Pod: fakefilteredpodinformer.Get(ctx, v1alpha1.ManagedByLabelKey),
}

for _, pr := range d.PipelineRuns {
Expand Down
4 changes: 2 additions & 2 deletions test/v1alpha1/wait_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@ import (
"time"

"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
ttesting "github.com/tektoncd/pipeline/pkg/reconciler/testing"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/pkg/apis"
duckv1beta1 "knative.dev/pkg/apis/duck/v1beta1"
rtesting "knative.dev/pkg/reconciler/testing"
)

var (
Expand Down Expand Up @@ -101,7 +101,7 @@ func TestWaitForPipelineRunStateFailed(t *testing.T) {
}

func fakeClients(t *testing.T, d Data) (*clients, context.Context, func()) {
ctx, _ := rtesting.SetupFakeContext(t)
ctx, _ := ttesting.SetupFakeContext(t)
ctx, cancel := context.WithCancel(ctx)
fakeClients, _ := SeedTestData(t, ctx, d)
return &clients{
Expand Down
40 changes: 0 additions & 40 deletions vendor/knative.dev/pkg/client/injection/kube/informers/core/v1/pod/fake/fake.go
View file
Open in desktop

This file was deleted.

52 changes: 52 additions & 0 deletions vendor/knative.dev/pkg/client/injection/kube/informers/core/v1/pod/filtered/fake/fake.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

37 changes: 25 additions & 12 deletions ...jection/kube/informers/core/v1/pod/pod.go → ...ube/informers/core/v1/pod/filtered/pod.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading