From 897aa443bb669192898369ae9f997548f5ba0dde Mon Sep 17 00:00:00 2001 From: Matt Moore Date: Fri, 9 Nov 2018 04:20:51 +0000 Subject: [PATCH] Simplify some of the object construction in Revision controller. An initial cleanup while looking into: https://github.com/knative/serving/issues/2442 --- pkg/reconciler/v1alpha1/revision/cruds.go | 4 +- .../v1alpha1/revision/revision_test.go | 4 +- .../v1alpha1/revision/table_test.go | 158 ++++++++---------- 3 files changed, 70 insertions(+), 96 deletions(-) diff --git a/pkg/reconciler/v1alpha1/revision/cruds.go b/pkg/reconciler/v1alpha1/revision/cruds.go index e6715d777775..33565915c21d 100644 --- a/pkg/reconciler/v1alpha1/revision/cruds.go +++ b/pkg/reconciler/v1alpha1/revision/cruds.go @@ -23,7 +23,7 @@ import ( caching "github.com/knative/caching/pkg/apis/caching/v1alpha1" "github.com/knative/pkg/logging" - kpa "github.com/knative/serving/pkg/apis/autoscaling/v1alpha1" + kpav1alpha1 "github.com/knative/serving/pkg/apis/autoscaling/v1alpha1" "github.com/knative/serving/pkg/apis/serving/v1alpha1" "github.com/knative/serving/pkg/reconciler/v1alpha1/revision/config" "github.com/knative/serving/pkg/reconciler/v1alpha1/revision/resources" @@ -56,7 +56,7 @@ func (c *Reconciler) createImageCache(ctx context.Context, rev *v1alpha1.Revisio return c.CachingClientSet.CachingV1alpha1().Images(image.Namespace).Create(image) } -func (c *Reconciler) createKPA(ctx context.Context, rev *v1alpha1.Revision) (*kpa.PodAutoscaler, error) { +func (c *Reconciler) createKPA(ctx context.Context, rev *v1alpha1.Revision) (*kpav1alpha1.PodAutoscaler, error) { kpa := resources.MakeKPA(rev) return c.ServingClientSet.AutoscalingV1alpha1().PodAutoscalers(kpa.Namespace).Create(kpa) diff --git a/pkg/reconciler/v1alpha1/revision/revision_test.go b/pkg/reconciler/v1alpha1/revision/revision_test.go index 4cff4117137a..d5f6e03fe580 100644 --- a/pkg/reconciler/v1alpha1/revision/revision_test.go +++ b/pkg/reconciler/v1alpha1/revision/revision_test.go @@ -37,7 +37,7 @@ import ( "github.com/knative/pkg/configmap" ctrl "github.com/knative/pkg/controller" "github.com/knative/pkg/kmeta" - kpa "github.com/knative/serving/pkg/apis/autoscaling/v1alpha1" + kpav1alpha1 "github.com/knative/serving/pkg/apis/autoscaling/v1alpha1" "github.com/knative/serving/pkg/apis/serving" "github.com/knative/serving/pkg/apis/serving/v1alpha1" "github.com/knative/serving/pkg/autoscaler" @@ -88,7 +88,7 @@ func getTestReadyEndpoints(revName string) *corev1.Endpoints { } } -func getTestReadyKPA(rev *v1alpha1.Revision) *kpa.PodAutoscaler { +func getTestReadyKPA(rev *v1alpha1.Revision) *kpav1alpha1.PodAutoscaler { kpa := resources.MakeKPA(rev) kpa.Status.InitializeConditions() kpa.Status.MarkActive() diff --git a/pkg/reconciler/v1alpha1/revision/table_test.go b/pkg/reconciler/v1alpha1/revision/table_test.go index 6484f0ad8300..08ce74dbc069 100644 --- a/pkg/reconciler/v1alpha1/revision/table_test.go +++ b/pkg/reconciler/v1alpha1/revision/table_test.go @@ -101,30 +101,6 @@ var ( // This is heavily based on the way the OpenShift Ingress controller tests its reconciliation method. func TestReconcile(t *testing.T) { - // Create short-hand aliases that pass through the above config and Active to getRev and friends. - rev := func(namespace, name, image string) *v1alpha1.Revision { - return getRev(namespace, name, image) - } - deploy := func(namespace, name, image string) *appsv1.Deployment { - return getDeploy(namespace, name, image, ReconcilerTestConfig()) - } - image := func(namespace, name, image string) *caching.Image { - i, err := getImage(namespace, name, image, ReconcilerTestConfig()) - if err != nil { - t.Fatalf("Error building image: %v", err) - } - return i - } - kpa := func(namespace, name, image string) *kpav1alpha1.PodAutoscaler { - return getKPA(namespace, name, image) - } - svc := func(namespace, name, image string) *corev1.Service { - return getService(namespace, name, image) - } - endpoints := func(namespace, name, image string) *corev1.Endpoints { - return getEndpoints(namespace, name, image) - } - table := TableTest{{ Name: "bad workqueue key", // Make sure Reconcile handles bad keys. @@ -1295,30 +1271,6 @@ func TestReconcile(t *testing.T) { } func TestReconcileWithVarLogEnabled(t *testing.T) { - config := ReconcilerTestConfig() - config.Observability.EnableVarLogCollection = true - - // Create short-hand aliases that pass through the above config and Active to getRev and friends. - rev := func(namespace, name, image string) *v1alpha1.Revision { - return getRev(namespace, name, image) - } - deploy := func(namespace, name, image string) *appsv1.Deployment { - return getDeploy(namespace, name, image, config) - } - image := func(namespace, name, image string) *caching.Image { - i, err := getImage(namespace, name, image, config) - if err != nil { - t.Fatalf("Error building image: %v", err) - } - return i - } - kpa := func(namespace, name, image string) *kpav1alpha1.PodAutoscaler { - return getKPA(namespace, name, image) - } - svc := func(namespace, name, image string) *corev1.Service { - return getService(namespace, name, image) - } - table := TableTest{{ Name: "first revision reconciliation (with /var/log enabled)", // Test a successful reconciliation flow with /var/log enabled. @@ -1331,10 +1283,10 @@ func TestReconcileWithVarLogEnabled(t *testing.T) { WantCreates: []metav1.Object{ // The first reconciliation of a Revision creates the following resources. kpa("foo", "first-reconcile-var-log", "busybox"), - deploy("foo", "first-reconcile-var-log", "busybox"), + deploy("foo", "first-reconcile-var-log", "busybox", EnableVarLog), svc("foo", "first-reconcile-var-log", "busybox"), - resources.MakeFluentdConfigMap(rev("foo", "first-reconcile-var-log", "busybox"), config.Observability), - image("foo", "first-reconcile-var-log", "busybox"), + fluentdConfigMap("foo", "first-reconcile-var-log", "busybox", EnableVarLog), + image("foo", "first-reconcile-var-log", "busybox", EnableVarLog), }, WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: makeStatus( @@ -1359,10 +1311,10 @@ func TestReconcileWithVarLogEnabled(t *testing.T) { }, WantCreates: []metav1.Object{ // The first reconciliation of a Revision creates the following resources. - deploy("foo", "create-configmap-failure", "busybox"), + deploy("foo", "create-configmap-failure", "busybox", EnableVarLog), svc("foo", "create-configmap-failure", "busybox"), - resources.MakeFluentdConfigMap(rev("foo", "create-configmap-failure", "busybox"), config.Observability), - image("foo", "create-configmap-failure", "busybox"), + fluentdConfigMap("foo", "create-configmap-failure", "busybox", EnableVarLog), + image("foo", "create-configmap-failure", "busybox", EnableVarLog), // We don't create the autoscaler resources if we fail to create the fluentd configmap }, WantUpdates: []clientgotesting.UpdateActionImpl{{ @@ -1411,10 +1363,10 @@ func TestReconcileWithVarLogEnabled(t *testing.T) { Conditions: allUnknownConditions, }), kpa("foo", "steady-state", "busybox"), - deploy("foo", "steady-state", "busybox"), + deploy("foo", "steady-state", "busybox", EnableVarLog), svc("foo", "steady-state", "busybox"), - resources.MakeFluentdConfigMap(rev("foo", "steady-state", "busybox"), config.Observability), - image("foo", "steady-state", "busybox"), + fluentdConfigMap("foo", "steady-state", "busybox", EnableVarLog), + image("foo", "steady-state", "busybox", EnableVarLog), }, Key: "foo/steady-state", }, { @@ -1429,26 +1381,20 @@ func TestReconcileWithVarLogEnabled(t *testing.T) { Conditions: allUnknownConditions, }), kpa("foo", "update-fluentd-config", "busybox"), - deploy("foo", "update-fluentd-config", "busybox"), + deploy("foo", "update-fluentd-config", "busybox", EnableVarLog), svc("foo", "update-fluentd-config", "busybox"), &corev1.ConfigMap{ // Use the ObjectMeta, but discard the rest. - ObjectMeta: resources.MakeFluentdConfigMap( - rev("foo", "update-fluentd-config", "busybox"), - config.Observability, - ).ObjectMeta, + ObjectMeta: fluentdConfigMap("foo", "update-fluentd-config", "busybox", EnableVarLog).ObjectMeta, Data: map[string]string{ "bad key": "bad value", }, }, - image("foo", "update-fluentd-config", "busybox"), + image("foo", "update-fluentd-config", "busybox", EnableVarLog), }, WantUpdates: []clientgotesting.UpdateActionImpl{{ // We should see a single update to the configmap we expect. - Object: resources.MakeFluentdConfigMap( - rev("foo", "update-fluentd-config", "busybox"), - config.Observability, - ), + Object: fluentdConfigMap("foo", "update-fluentd-config", "busybox", EnableVarLog), }}, Key: "foo/update-fluentd-config", }, { @@ -1466,24 +1412,27 @@ func TestReconcileWithVarLogEnabled(t *testing.T) { LogURL: "http://logger.io/test-uid", Conditions: allUnknownConditions, }), - deploy("foo", "update-configmap-failure", "busybox"), + deploy("foo", "update-configmap-failure", "busybox", EnableVarLog), svc("foo", "update-configmap-failure", "busybox"), &corev1.ConfigMap{ // Use the ObjectMeta, but discard the rest. - ObjectMeta: resources.MakeFluentdConfigMap(rev("foo", "update-configmap-failure", "busybox"), config.Observability).ObjectMeta, + ObjectMeta: fluentdConfigMap("foo", "update-configmap-failure", "busybox", EnableVarLog).ObjectMeta, Data: map[string]string{ "bad key": "bad value", }, }, - image("foo", "update-configmap-failure", "busybox"), + image("foo", "update-configmap-failure", "busybox", EnableVarLog), }, WantUpdates: []clientgotesting.UpdateActionImpl{{ // We should see a single update to the configmap we expect. - Object: resources.MakeFluentdConfigMap(rev("foo", "update-configmap-failure", "busybox"), config.Observability), + Object: fluentdConfigMap("foo", "update-configmap-failure", "busybox", EnableVarLog), }}, Key: "foo/update-configmap-failure", }} + config := ReconcilerTestConfig() + EnableVarLog(config) + table.Test(t, MakeFactory(func(listers *Listers, opt reconciler.Options) controller.Reconciler { return &Reconciler{ Base: reconciler.NewBase(opt, controllerAgentName), @@ -1505,14 +1454,6 @@ var ( boolTrue = true ) -func om(namespace, name string) metav1.ObjectMeta { - return metav1.ObjectMeta{ - Name: name, - Namespace: namespace, - UID: "test-uid", - } -} - func makeStatus(rev *v1alpha1.Revision, status v1alpha1.RevisionStatus) *v1alpha1.Revision { rev.Status = status return rev @@ -1572,41 +1513,68 @@ func build(namespace, name string, conds ...duckv1alpha1.Condition) *unstructure return u } -func getRev(namespace, name string, image string) *v1alpha1.Revision { +func rev(namespace, name string, image string) *v1alpha1.Revision { return &v1alpha1.Revision{ - ObjectMeta: om(namespace, name), + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + UID: "test-uid", + }, Spec: v1alpha1.RevisionSpec{ Container: corev1.Container{Image: image}, }, } } -func getDeploy(namespace, name string, image string, config *config.Config) *appsv1.Deployment { +func deploy(namespace, name string, image string, co ...ConfigOption) *appsv1.Deployment { + config := ReconcilerTestConfig() + for _, opt := range co { + opt(config) + } - rev := getRev(namespace, name, image) + rev := rev(namespace, name, image) return resources.MakeDeployment(rev, config.Logging, config.Network, config.Observability, config.Autoscaler, config.Controller) } -func getImage(namespace, name string, image string, config *config.Config) (*caching.Image, error) { - rev := getRev(namespace, name, image) +func image(namespace, name string, image string, co ...ConfigOption) *caching.Image { + config := ReconcilerTestConfig() + for _, opt := range co { + opt(config) + } + + rev := rev(namespace, name, image) deploy := resources.MakeDeployment(rev, config.Logging, config.Network, config.Observability, config.Autoscaler, config.Controller) - return resources.MakeImageCache(rev, deploy) + img, err := resources.MakeImageCache(rev, deploy) + if err != nil { + panic(err.Error()) + } + return img } -func getKPA(namespace, name string, image string) *kpav1alpha1.PodAutoscaler { - rev := getRev(namespace, name, image) +func fluentdConfigMap(namespace, name string, image string, co ...ConfigOption) *corev1.ConfigMap { + config := ReconcilerTestConfig() + for _, opt := range co { + opt(config) + } + + rev := rev(namespace, name, image) + return resources.MakeFluentdConfigMap(rev, config.Observability) +} + +func kpa(namespace, name string, image string) *kpav1alpha1.PodAutoscaler { + rev := rev(namespace, name, image) return resources.MakeKPA(rev) } -func getService(namespace, name string, image string) *corev1.Service { - rev := getRev(namespace, name, image) +func svc(namespace, name string, image string) *corev1.Service { + rev := rev(namespace, name, image) return resources.MakeK8sService(rev) } -func getEndpoints(namespace, name string, image string) *corev1.Endpoints { - service := getService(namespace, name, image) +func endpoints(namespace, name string, image string) *corev1.Endpoints { + service := svc(namespace, name, image) return &corev1.Endpoints{ ObjectMeta: metav1.ObjectMeta{ Namespace: service.Namespace, @@ -1631,6 +1599,8 @@ func (t *testConfigStore) WatchConfigs(w configmap.Watcher) {} var _ configStore = (*testConfigStore)(nil) +type ConfigOption func(*config.Config) + func ReconcilerTestConfig() *config.Config { return &config.Config{ Controller: getTestControllerConfig(), @@ -1642,3 +1612,7 @@ func ReconcilerTestConfig() *config.Config { Autoscaler: &autoscaler.Config{}, } } + +func EnableVarLog(cfg *config.Config) { + cfg.Observability.EnableVarLogCollection = true +}