Repository: knative/pkg
Pull request #1442 (2020-06-23 20:35:45)

remove krshapedlogic tag, this is default now
- remove krshapedlogic tag, this is default now

knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1442#" title="Author self-approved">whaught</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **n3wscott**
You can assign the PR to them by writing `/assign @n3wscott` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[apis/OWNERS](https://github.com/knative/pkg/blob/master/apis/OWNERS)**
- **[injection/OWNERS](https://github.com/knative/pkg/blob/master/injection/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["n3wscott"]} -->

Patch:
diff --git a/apiextensions/storageversion/migrator.go b/apiextensions/storageversion/migrator.go
index 45a06ab92..657c30e3f 100644
--- a/apiextensions/storageversion/migrator.go
+++ b/apiextensions/storageversion/migrator.go
@@ -20,7 +20,7 @@ import (
 	"context"
 	"fmt"
 
-	apix "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
+	apix "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apixclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -54,7 +54,7 @@ func NewMigrator(d dynamic.Interface, a apixclient.Interface) *Migrator {
 // Finally the migrator will update the CRD's status and drop older storage
 // versions
 func (m *Migrator) Migrate(ctx context.Context, gr schema.GroupResource) error {
-	crdClient := m.apixClient.ApiextensionsV1beta1().CustomResourceDefinitions()
+	crdClient := m.apixClient.ApiextensionsV1().CustomResourceDefinitions()
 
 	crd, err := crdClient.Get(gr.String(), metav1.GetOptions{})
 	if err != nil {
@@ -63,6 +63,10 @@ func (m *Migrator) Migrate(ctx context.Context, gr schema.GroupResource) error {
 
 	version := storageVersion(crd)
 
+	if version == "" {
+		return fmt.Errorf("unable to determine storage version for %s", gr)
+	}
+
 	if err := m.migrateResources(ctx, gr.WithVersion(version)); err != nil {
 		return err
 	}
@@ -112,9 +116,5 @@ func storageVersion(crd *apix.CustomResourceDefinition) string {
 		}
 	}
 
-	if version == "" {
-		version = crd.Spec.Version
-	}
-
 	return version
 }
diff --git a/apis/condition_set.go b/apis/condition_set.go
index 8d02fec48..e354167b9 100644
--- a/apis/condition_set.go
+++ b/apis/condition_set.go
@@ -184,25 +184,25 @@ func (r conditionsImpl) GetCondition(t ConditionType) *Condition {
 
 // SetCondition sets or updates the Condition on Conditions for Condition.Type.
 // If there is an update, Conditions are stored back sorted.
-func (r conditionsImpl) SetCondition(new Condition) {
+func (r conditionsImpl) SetCondition(cond Condition) {
 	if r.accessor == nil {
 		return
 	}
-	t := new.Type
+	t := cond.Type
 	var conditions Conditions
 	for _, c := range r.accessor.GetConditions() {
 		if c.Type != t {
 			conditions = append(conditions, c)
 		} else {
 			// If we'd only update the LastTransitionTime, then return.
-			new.LastTransitionTime = c.LastTransitionTime
-			if reflect.DeepEqual(&new, &c) {
+			cond.LastTransitionTime = c.LastTransitionTime
+			if reflect.DeepEqual(cond, c) {
 				return
 			}
 		}
 	}
-	new.LastTransitionTime = VolatileTime{Inner: metav1.NewTime(time.Now())}
-	conditions = append(conditions, new)
+	cond.LastTransitionTime = VolatileTime{Inner: metav1.NewTime(time.Now())}
+	conditions = append(conditions, cond)
 	// Sorted for convenience of the consumer, i.e. kubectl.
 	sort.Slice(conditions, func(i, j int) bool { return conditions[i].Type < conditions[j].Type })
 	r.accessor.SetConditions(conditions)
diff --git a/apis/deprecated.go b/apis/deprecated.go
index b930fbb91..fc9bca5f1 100644
--- a/apis/deprecated.go
+++ b/apis/deprecated.go
@@ -36,7 +36,7 @@ func CheckDeprecated(ctx context.Context, obj interface{}) *FieldError {
 // CheckDeprecated checks whether the provided named deprecated fields
 // are set in a context where deprecation is disallowed.
 // This is a json shallow check. We will recursively check inlined structs.
-func CheckDeprecatedUpdate(ctx context.Context, obj interface{}, original interface{}) *FieldError {
+func CheckDeprecatedUpdate(ctx context.Context, obj, original interface{}) *FieldError {
 	if IsDeprecatedAllowed(ctx) {
 		return nil
 	}
diff --git a/apis/field_error.go b/apis/field_error.go
index 3cedeecbb..cc353b58b 100644
--- a/apis/field_error.go
+++ b/apis/field_error.go
@@ -204,11 +204,12 @@ func flatten(path []string) string {
 	var newPath []string
 	for _, part := range path {
 		for _, p := range strings.Split(part, ".") {
-			if p == CurrentField {
+			switch {
+			case p == CurrentField:
 				continue
-			} else if len(newPath) > 0 && isIndex(p) {
+			case len(newPath) > 0 && isIndex(p):
 				newPath[len(newPath)-1] += p
-			} else {
+			default:
 				newPath = append(newPath, p)
 			}
 		}
@@ -383,7 +384,7 @@ func ErrOutOfBoundsValue(value, lower, upper interface{}, fieldPath string) *Fie
 func CheckDisallowedFields(request, maskedRequest interface{}) *FieldError {
 	if disallowed, err := kmp.CompareSetFields(request, maskedRequest); err != nil {
 		return &FieldError{
-			Message: fmt.Sprintf("Internal Error"),
+			Message: "Internal Error",
 			Paths:   []string{CurrentField},
 		}
 	} else if len(disallowed) > 0 {
diff --git a/injection/README.md b/injection/README.md
index e23ff32f5..30d75866e 100644
--- a/injection/README.md
+++ b/injection/README.md
@@ -454,14 +454,14 @@ for that resource.
 
 #### Annotation based common logic
 
-**krshaped=true may become the default if omitted in the future**
+**krshapedlogic=false may be used to omit common reconciler logic**
 
 Reconcilers can handle common logic for resources that conform to the KRShaped
 interface. This allows the generated code to automatically increment
 ObservedGeneration.
 
 ```go
-// +genreconciler:krshapedlogic=true
+// +genreconciler
 ```
 
 Setting this annotation will emit the following in the generated reconciler.
diff --git a/leaderelection/config.go b/leaderelection/config.go
index 44378d30c..8b7c02058 100644
--- a/leaderelection/config.go
+++ b/leaderelection/config.go
@@ -126,6 +126,14 @@ type ComponentConfig struct {
 	RetryPeriod   time.Duration
 }
 
+// StatefulSetConfig represents the required information for a StatefulSet service.
+type StatefulSetConfig struct {
+	StatefulSetName string
+	ServiceName     string
+	Port            string
+	Protocol        string
+}
+
 func defaultComponentConfig(name string) ComponentConfig {
 	return ComponentConfig{
 		Component:   name,
diff --git a/leaderelection/context.go b/leaderelection/context.go
index a0d8555f9..d273a572e 100644
--- a/leaderelection/context.go
+++ b/leaderelection/context.go
@@ -28,6 +28,7 @@ import (
 	"k8s.io/client-go/tools/leaderelection"
 	"k8s.io/client-go/tools/leaderelection/resourcelock"
 	"knative.dev/pkg/logging"
+	"knative.dev/pkg/network"
 	"knative.dev/pkg/reconciler"
 	"knative.dev/pkg/system"
 )
@@ -42,6 +43,16 @@ func WithStandardLeaderElectorBuilder(ctx context.Context, kc kubernetes.Interfa
 	})
 }
 
+// WithStatefulSetLeaderElectorBuilder infuses a context with the ability to build
+// Electors which are assigned leadership based on the StatefulSet ordinal from
+// the provided component configuration.
+func WithStatefulSetLeaderElectorBuilder(ctx context.Context, cc ComponentConfig, ssc StatefulSetConfig) context.Context {
+	return context.WithValue(ctx, builderKey{}, &statefulSetBuilder{
+		lec: cc,
+		ssc: ssc,
+	})
+}
+
 // HasLeaderElection returns whether there is leader election configuration
 // associated with the context
 func HasLeaderElection(ctx context.Context) bool {
@@ -61,8 +72,9 @@ func BuildElector(ctx context.Context, la reconciler.LeaderAware, name string, e
 		switch builder := val.(type) {
 		case *standardBuilder:
 			return builder.BuildElector(ctx, la, name, enq)
+		case *statefulSetBuilder:
+			return builder.BuildElector(ctx, la, enq)
 		}
-		// TODO(mattmoor): Add a flavor of builder that relies on StatefulSet to partition the key space.
 	}
 
 	return &unopposedElector{
@@ -90,10 +102,11 @@ func (b *standardBuilder) BuildElector(ctx context.Context, la reconciler.Leader
 	buckets := make([]Elector, 0, b.lec.Buckets)
 	for i := uint32(0); i < b.lec.Buckets; i++ {
 		bkt := &bucket{
-			component: b.lec.Component,
-			name:      name,
-			index:     i,
-			total:     b.lec.Buckets,
+			// The resource name is the lowercase:
+			//   {component}.{workqueue}.{index}-of-{total}
+			name:  strings.ToLower(fmt.Sprintf("%s.%s.%02d-of-%02d", b.lec.Component, name, i, b.lec.Buckets)),
+			index: i,
+			total: b.lec.Buckets,
 		}
 
 		rl, err := resourcelock.New(b.lec.ResourceLock,
@@ -145,6 +158,35 @@ func (b *standardBuilder) BuildElector(ctx context.Context, la reconciler.Leader
 	return &runAll{les: buckets}, nil
 }
 
+type statefulSetBuilder struct {
+	lec ComponentConfig
+	ssc StatefulSetConfig
+}
+
+func (b *statefulSetBuilder) BuildElector(ctx context.Context, la reconciler.LeaderAware, enq func(reconciler.Bucket, types.NamespacedName)) (Elector, error) {
+	logger := logging.FromContext(ctx)
+
+	ordinal, err := ControllerOrdinal()
+	if err != nil {
+		return nil, err
+	}
+
+	logger.Infof("%s will run in StatefulSet ordinal assignement mode with ordinal %d", b.lec.Component, ordinal)
+
+	return &unopposedElector{
+		bkt: &bucket{
+			// The name is the full pod DNS of the owner pod of this bucket.
+			name: fmt.Sprintf("%s://%s-%d.%s.%s.svc.%s:%s", b.ssc.Protocol,
+				b.ssc.StatefulSetName, ordinal, b.ssc.ServiceName,
+				system.Namespace(), network.GetClusterDomainName(), b.ssc.Port),
+			index: uint32(ordinal),
+			total: b.lec.Buckets,
+		},
+		la:  la,
+		enq: enq,
+	}, nil
+}
+
 // unopposedElector promotes when run without needing to be elected.
 type unopposedElector struct {
 	bkt reconciler.Bucket
@@ -197,8 +239,7 @@ func (ruc *runUntilCancelled) Run(ctx context.Context) {
 }
 
 type bucket struct {
-	component string
-	name      string
+	name string
 
 	// We are bucket {index} of {total}
 	index uint32
@@ -209,9 +250,7 @@ var _ reconciler.Bucket = (*bucket)(nil)
 
 // Name implements reconciler.Bucket
 func (b *bucket) Name() string {
-	// The resource name is the lowercase:
-	//   {component}.{workqueue}.{index}-of-{total}
-	return strings.ToLower(fmt.Sprintf("%s.%s.%02d-of-%02d", b.component, b.name, b.index, b.total))
+	return b.name
 }
 
 // Has implements reconciler.Bucket
diff --git a/leaderelection/ordinal.go b/leaderelection/ordinal.go
new file mode 100644
index 000000000..94a3775a0
--- /dev/null
+++ b/leaderelection/ordinal.go
@@ -0,0 +1,39 @@
+/*
+Copyright 2020 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package leaderelection
+
+import (
+	"fmt"
+	"os"
+	"strconv"
+	"strings"
+)
+
+// If run a process on Kubernetes, the value of this environment variable
+// should be set to the pod name via the downward API.
+const controllerOrdinalEnv = "CONTROLLER_ORDINAL"
+
+// ControllerOrdinal tries to get ordinal from the pod name of a StatefulSet,
+// which is provided from the environment variable CONTROLLER_ORDINAL.
+func ControllerOrdinal() (uint64, error) {
+	v := os.Getenv(controllerOrdinalEnv)
+	if i := strings.LastIndex(v, "-"); i != -1 {
+		return strconv.ParseUint(v[i+1:], 10, 64)
+	}
+
+	return 0, fmt.Errorf("ordinal not found in %s=%s", controllerOrdinalEnv, v)
+}
diff --git a/metrics/record.go b/metrics/record.go
index d6e433a17..c46279179 100644
--- a/metrics/record.go
+++ b/metrics/record.go
@@ -40,7 +40,7 @@ func RecordBatch(ctx context.Context, mss ...stats.Measurement) {
 // be used to create a view.Distribution.
 func Buckets125(low, high float64) []float64 {
 	buckets := []float64{low}
-	for last := low; last < high; last = last * 10 {
+	for last := low; last < high; last *= 10 {
 		buckets = append(buckets, 2*last, 5*last, 10*last)
 	}
 	return buckets
diff --git a/reconciler/leader.go b/reconciler/leader.go
index 3d5f68fdc..6fece8526 100644
--- a/reconciler/leader.go
+++ b/reconciler/leader.go
@@ -95,7 +95,6 @@ func (laf *LeaderAwareFuncs) Promote(b Bucket, enq func(Bucket, types.Namespaced
 			laf.buckets = make(map[string]Bucket, 1)
 		}
 		laf.buckets[b.Name()] = b
-		return
 	}()
 
 	if promote := laf.PromoteFunc; promote != nil {
diff --git a/tracing/config/tracing.go b/tracing/config/tracing.go
index b3aceee16..ecc2cc7cd 100644
--- a/tracing/config/tracing.go
+++ b/tracing/config/tracing.go
@@ -86,16 +86,14 @@ func NewTracingConfigFromMap(cfgMap map[string]string) (*Config, error) {
 		default:
 			return nil, fmt.Errorf("unsupported tracing backend value %q", backend)
 		}
-	} else {
+	} else if enable, ok := cfgMap[enableKey]; ok {
 		// For backwards compatibility, parse the enabled flag as Zipkin.
-		if enable, ok := cfgMap[enableKey]; ok {
-			enableBool, err := strconv.ParseBool(enable)
-			if err != nil {
-				return nil, fmt.Errorf("failed parsing tracing config %q: %w", enableKey, err)
-			}
-			if enableBool {
-				tc.Backend = Zipkin
-			}
+		enableBool, err := strconv.ParseBool(enable)
+		if err != nil {
+			return nil, fmt.Errorf("failed parsing tracing config %q: %w", enableKey, err)
+		}
+		if enableBool {
+			tc.Backend = Zipkin
 		}
 	}
 
diff --git a/webhook/admission.go b/webhook/admission.go
index 086f39dab..46d4c08c4 100644
--- a/webhook/admission.go
+++ b/webhook/admission.go
@@ -88,7 +88,13 @@ func admissionHandler(rootLogger *zap.SugaredLogger, stats StatsReporter, c Admi
 
 		ctx := logging.WithLogger(r.Context(), logger)
 
-		var response admissionv1.AdmissionReview
+		response := admissionv1.AdmissionReview{
+			// Use the same type meta as the request - this is required by the K8s API
+			// note: v1beta1 & v1 AdmissionReview shapes are identical so even though
+			// we're using v1 types we still support v1beta1 admission requests
+			TypeMeta: review.TypeMeta,
+		}
+
 		reviewResponse := c.Admit(ctx, review.Request)
 		var patchType string
 		if reviewResponse.PatchType != nil {


Test patch:
diff --git a/apiextensions/storageversion/migrator_test.go b/apiextensions/storageversion/migrator_test.go
index ff73b3b16..90c37bfff 100644
--- a/apiextensions/storageversion/migrator_test.go
+++ b/apiextensions/storageversion/migrator_test.go
@@ -22,7 +22,7 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
-	apix "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
+	apix "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apixFake "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/fake"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
diff --git a/apis/duck/typed_test.go b/apis/duck/typed_test.go
index 08b693c78..4ef97e170 100644
--- a/apis/duck/typed_test.go
+++ b/apis/duck/typed_test.go
@@ -109,8 +109,8 @@ func TestInvalidResource(t *testing.T) {
 
 	_, _, got := tif.Get(SchemeGroupVersion.WithResource("resources"))
 
-	if got != testErr {
-		t.Errorf("Error = %v, want: %v", got, testErr)
+	if got != errTest {
+		t.Errorf("Error = %v, want: %v", got, errTest)
 	}
 }
 
@@ -296,7 +296,7 @@ func (bo *badObject) DeepCopyObject() runtime.Object {
 	return &badObject{}
 }
 
-var testErr = errors.New("failed to get list")
+var errTest = errors.New("failed to get list")
 
 type invalidResourceClient struct {
 	*fake.FakeDynamicClient
@@ -311,5 +311,5 @@ type invalidResource struct {
 }
 
 func (*invalidResource) List(options metav1.ListOptions) (*unstructured.UnstructuredList, error) {
-	return nil, testErr
+	return nil, errTest
 }
diff --git a/apis/test/example/v1alpha1/foo_types.go b/apis/test/example/v1alpha1/foo_types.go
index c681ea796..e97163755 100644
--- a/apis/test/example/v1alpha1/foo_types.go
+++ b/apis/test/example/v1alpha1/foo_types.go
@@ -27,7 +27,7 @@ import (
 )
 
 // +genclient
-// +genreconciler:krshapedlogic=true
+// +genreconciler
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 
 // Foo is for testing.
diff --git a/apis/test/pub/v1alpha1/bar_types.go b/apis/test/pub/v1alpha1/bar_types.go
index 48e4f081b..9c3cb5fd4 100644
--- a/apis/test/pub/v1alpha1/bar_types.go
+++ b/apis/test/pub/v1alpha1/bar_types.go
@@ -27,7 +27,7 @@ import (
 )
 
 // +genclient
-// +genreconciler:class=example.com/filter.class,krshapedlogic=true
+// +genreconciler:class=example.com/filter.class
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 
 // Bar is for testing.
diff --git a/controller/controller_test.go b/controller/controller_test.go
index 8d8e14e72..3556af61a 100644
--- a/controller/controller_test.go
+++ b/controller/controller_test.go
@@ -933,8 +933,8 @@ func TestStartAndShutdownWithLeaderAwareWithLostElection(t *testing.T) {
 			Spec: coordinationv1.LeaseSpec{
 				HolderIdentity:       ptr.String("not-us"),
 				LeaseDurationSeconds: ptr.Int32(3000),
-				AcquireTime:          &metav1.MicroTime{time.Now()},
-				RenewTime:            &metav1.MicroTime{time.Now().Add(3000 * time.Second)},
+				AcquireTime:          &metav1.MicroTime{Time: time.Now()},
+				RenewTime:            &metav1.MicroTime{Time: time.Now().Add(3000 * time.Second)},
 			},
 		},
 	)
diff --git a/leaderelection/context_test.go b/leaderelection/context_test.go
index 5e9bc6ce0..76e75caae 100644
--- a/leaderelection/context_test.go
+++ b/leaderelection/context_test.go
@@ -21,6 +21,7 @@ package leaderelection
 
 import (
 	"context"
+	"os"
 	"testing"
 	"time"
 
@@ -95,7 +96,7 @@ func TestWithBuilder(t *testing.T) {
 
 	le, err := BuildElector(ctx, laf, "name", enq)
 	if err != nil {
-		t.Errorf("BuildElector() = %v", err)
+		t.Fatalf("BuildElector() = %v", err)
 	}
 
 	// We shouldn't see leases until we Run the elector.
@@ -144,3 +145,71 @@ func TestWithBuilder(t *testing.T) {
 		t.Fatal("Timed out waiting for demotion.")
 	}
 }
+
+func TestWithStatefulSetBuilder(t *testing.T) {
+	cc := ComponentConfig{
+		Component:   "component",
+		LeaderElect: true,
+		Buckets:     1,
+	}
+	podDNS := "ws://as-0.autoscaler.knative-testing.svc.cluster.local:8080"
+	ctx := context.Background()
+
+	promoted := make(chan struct{})
+	laf := &reconciler.LeaderAwareFuncs{
+		PromoteFunc: func(bkt reconciler.Bucket, enq func(reconciler.Bucket, types.NamespacedName)) error {
+			close(promoted)
+			return nil
+		},
+	}
+	enq := func(reconciler.Bucket, types.NamespacedName) {}
+
+	ctx = WithStatefulSetLeaderElectorBuilder(ctx, cc, StatefulSetConfig{
+		ServiceName:     "autoscaler",
+		StatefulSetName: "as",
+		Protocol:        "ws",
+		Port:            "8080",
+	})
+	if !HasLeaderElection(ctx) {
+		t.Error("HasLeaderElection() = false, wanted true")
+	}
+
+	le, err := BuildElector(ctx, laf, "name", enq)
+	if err == nil {
+		// controller ordinal env not set
+		t.Error("expected BuildElector() returns error but got none")
+	}
+
+	os.Setenv(controllerOrdinalEnv, "as-0")
+	defer os.Unsetenv(controllerOrdinalEnv)
+	le, err = BuildElector(ctx, laf, "name", enq)
+	if err != nil {
+		t.Fatalf("BuildElector() = %v", err)
+	}
+
+	ule, ok := le.(*unopposedElector)
+	if !ok {
+		t.Fatalf("BuildElector() = %T, wanted an unopposedElector", le)
+	}
+	if got, want := ule.bkt.Name(), podDNS; got != want {
+		t.Errorf("bkt.Name() = %s, wanted %s", got, want)
+	}
+
+	// Shouldn't be promoted until we Run the elector.
+	select {
+	case <-promoted:
+		t.Error("Got promoted, want no actions.")
+	default:
+	}
+
+	ctx, cancel := context.WithCancel(context.Background())
+	t.Cleanup(cancel)
+	go le.Run(ctx)
+
+	select {
+	case <-promoted:
+		// We expect to have been promoted.
+	case <-time.After(1 * time.Second):
+		t.Fatal("Timed out waiting for promotion.")
+	}
+}
diff --git a/leaderelection/ordinal_test.go b/leaderelection/ordinal_test.go
new file mode 100644
index 000000000..ce300a33b
--- /dev/null
+++ b/leaderelection/ordinal_test.go
@@ -0,0 +1,72 @@
+/*
+Copyright 2020 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package leaderelection
+
+import (
+	"fmt"
+	"os"
+	"testing"
+)
+
+func TestControllerOrdinal(t *testing.T) {
+	testCases := []struct {
+		testname string
+		podName  string
+		want     uint64
+		err      error
+	}{{
+		testname: "NotSet",
+		err:      fmt.Errorf("ordinal not found in %s=", controllerOrdinalEnv),
+	}, {
+		testname: "NoHyphen",
+		podName:  "as",
+		err:      fmt.Errorf("ordinal not found in %s=as", controllerOrdinalEnv),
+	}, {
+		testname: "InvalidOrdinal",
+		podName:  "as-invalid",
+		err:      fmt.Errorf(`strconv.ParseUint: parsing "invalid": invalid syntax`),
+	}, {
+		testname: "ValidName",
+		podName:  "as-0",
+	}, {
+		testname: "ValidName",
+		podName:  "as-1",
+		want:     1,
+	}}
+
+	defer os.Unsetenv(controllerOrdinalEnv)
+	for _, tt := range testCases {
+		t.Run(tt.testname, func(t *testing.T) {
+			if tt.podName != "" {
+				if os.Setenv(controllerOrdinalEnv, tt.podName) != nil {
+					t.Fatalf("fail to set env var %s=%s", controllerOrdinalEnv, tt.podName)
+				}
+			}
+
+			got, gotErr := ControllerOrdinal()
+			if tt.err != nil {
+				if gotErr == nil || gotErr.Error() != tt.err.Error() {
+					t.Errorf("got %v, want = %v, ", gotErr, tt.err)
+				}
+			} else if gotErr != nil {
+				t.Error("ControllerOrdinal() =", gotErr)
+			} else if got != tt.want {
+				t.Errorf("ControllerOrdinal() = %d, want = %d", got, tt.want)
+			}
+		})
+	}
+}
diff --git a/reconciler/testing/hooks_test.go b/reconciler/testing/hooks_test.go
index 0fc718f48..a6d297927 100644
--- a/reconciler/testing/hooks_test.go
+++ b/reconciler/testing/hooks_test.go
@@ -134,7 +134,7 @@ func TestMultiUpdate(t *testing.T) {
 
 	updates := 0
 	h.OnUpdate(&f.Fake, "pods", func(obj runtime.Object) HookResult {
-		updates = updates + 1
+		updates++
 		switch updates {
 		case 1:
 		case 2:
diff --git a/test/gcs/mock/mock.go b/test/gcs/mock/mock.go
index 44b25e3ec..801b0f1e3 100644
--- a/test/gcs/mock/mock.go
+++ b/test/gcs/mock/mock.go
@@ -95,7 +95,7 @@ func (c *clientMocker) getError(funcName Method) (bool, error) {
 			delete(c.err, funcName)
 			return true, val.Err
 		}
-		val.NumCall = val.NumCall - 1
+		val.NumCall--
 	}
 	return false, nil
 }
diff --git a/test/ghutil/fakeghutil/fakeghutil.go b/test/ghutil/fakeghutil/fakeghutil.go
index 7ac563580..51d8e2bd9 100644
--- a/test/ghutil/fakeghutil/fakeghutil.go
+++ b/test/ghutil/fakeghutil/fakeghutil.go
@@ -333,7 +333,7 @@ func (fgc *FakeGithubClient) CreatePullRequest(org, repo, head, base, title, bod
 
 // ListBranches lists branchs for given repo
 func (fgc *FakeGithubClient) ListBranches(org, repo string) ([]*github.Branch, error) {
-	var branches []*github.Branch
+	branches := make([]*github.Branch, 0, len(fgc.Branches))
 	for _, b := range fgc.Branches {
 		branches = append(branches, b...)
 	}
diff --git a/test/interactive/docker_test.go b/test/interactive/docker_test.go
index 29dc297c8..b43f4413f 100644
--- a/test/interactive/docker_test.go
+++ b/test/interactive/docker_test.go
@@ -40,7 +40,7 @@ func TestEnv(t *testing.T) {
 		if !exists {
 			break
 		}
-		badName = badName + "z"
+		badName += "z"
 	}
 	err = e.PromoteFromEnv(badName)
 	if err == nil {
diff --git a/test/webhook-apicoverage/view/html_template.go b/test/webhook-apicoverage/view/html_template.go
index 89084c8fe..15539598d 100644
--- a/test/webhook-apicoverage/view/html_template.go
+++ b/test/webhook-apicoverage/view/html_template.go
@@ -16,11 +16,7 @@ limitations under the License.
 
 package view
 
-import (
-	"fmt"
-)
-
-var TypeCoverageTempl = fmt.Sprint(`<!DOCTYPE html>
+var TypeCoverageTempl = `<!DOCTYPE html>
 <html>
 <style type="text/css">
   <!--
@@ -81,9 +77,9 @@ var TypeCoverageTempl = fmt.Sprint(`<!DOCTYPE html>
 </table>
 </body>
 </html>
-`)
+`
 
-var AggregateCoverageTmpl = fmt.Sprint(`<!DOCTYPE html>
+var AggregateCoverageTmpl = `<!DOCTYPE html>
 <html>
 <style type="text/css">
   <!--
@@ -108,4 +104,4 @@ var AggregateCoverageTmpl = fmt.Sprint(`<!DOCTYPE html>
 </table>
 </body>
 </html>
-`)
+`
diff --git a/test/webhook-apicoverage/view/xml_template.go b/test/webhook-apicoverage/view/xml_template.go
index 019945e71..487a38078 100644
--- a/test/webhook-apicoverage/view/xml_template.go
+++ b/test/webhook-apicoverage/view/xml_template.go
@@ -16,11 +16,7 @@ limitations under the License.
 
 package view
 
-import (
-	"fmt"
-)
-
-var JunitResultTmpl = fmt.Sprint(`<testsuites>
+var JunitResultTmpl = `<testsuites>
   <testsuite name="" time="0" {{ if .IsFailedBuild }} failures="1" {{ else }} failures = "0" {{ end }} tests="0">
       <testcase name="Overall" time="0" classname="go_coverage">
 				{{ if .IsFailedBuild }}
@@ -38,4 +34,4 @@ var JunitResultTmpl = fmt.Sprint(`<testsuites>
       </testcase>
     {{end}}
   </testsuite>
-</testsuites>`)
+</testsuites>`
diff --git a/testutils/clustermanager/perf-tests/pkg/benchmark.go b/testutils/clustermanager/perf-tests/pkg/benchmark.go
index 695678a17..221e18584 100644
--- a/testutils/clustermanager/perf-tests/pkg/benchmark.go
+++ b/testutils/clustermanager/perf-tests/pkg/benchmark.go
@@ -62,13 +62,15 @@ type ClusterConfig struct {
 // here we returns all subfolder names of the root folder.
 func benchmarkNames(benchmarkRoot string) ([]string, error) {
 	names := make([]string, 0)
-	dirs, err := ioutil.ReadDir(benchmarkRoot)
+	files, err := ioutil.ReadDir(benchmarkRoot)
 	if err != nil {
 		return names, fmt.Errorf("failed to list all benchmarks under %q: %w", benchmarkRoot, err)
 	}
 
-	for _, dir := range dirs {
-		names = append(names, dir.Name())
+	for _, f := range files {
+		if f.Mode().IsDir() {
+			names = append(names, f.Name())
+		}
 	}
 	return names, nil
 }
diff --git a/testutils/clustermanager/perf-tests/pkg/testdir/test-benchmark b/testutils/clustermanager/perf-tests/pkg/testdir/test-benchmark
new file mode 100644
index 000000000..82308ea97
--- /dev/null
+++ b/testutils/clustermanager/perf-tests/pkg/testdir/test-benchmark
@@ -0,0 +1,1 @@
+A file will not be considered as a benchmark.
diff --git a/tracing/http_test.go b/tracing/http_test.go
index c5ede3733..d0e6e7b66 100644
--- a/tracing/http_test.go
+++ b/tracing/http_test.go
@@ -21,30 +21,27 @@ import (
 	"net/url"
 	"testing"
 
-	"github.com/google/go-cmp/cmp"
 	. "knative.dev/pkg/tracing"
 	"knative.dev/pkg/tracing/config"
 	. "knative.dev/pkg/tracing/testing"
 )
 
 type fakeWriter struct {
-	lastWrite *[]byte
+	lastWrite string
 }
 
-func (fw fakeWriter) Header() http.Header {
+func (fw *fakeWriter) Header() http.Header {
 	return http.Header{}
 }
 
-func (fw fakeWriter) Write(data []byte) (int, error) {
-	*fw.lastWrite = data
+func (fw *fakeWriter) Write(data []byte) (int, error) {
+	fw.lastWrite = string(data)
 	return len(data), nil
 }
 
-func (fw fakeWriter) WriteHeader(statusCode int) {
-}
+func (fw *fakeWriter) WriteHeader(statusCode int) {}
 
-type testHandler struct {
-}
+type testHandler struct{}
 
 func (th *testHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	w.Write([]byte("fake"))
@@ -58,9 +55,11 @@ func TestHTTPSpanMiddleware(t *testing.T) {
 
 	// Create tracer with reporter recorder
 	reporter, co := FakeZipkinExporter()
-	defer reporter.Close()
 	oct := NewOpenCensusTracer(co)
-	defer oct.Finish()
+	t.Cleanup(func() {
+		reporter.Close()
+		oct.Finish()
+	})
 
 	if err := oct.ApplyConfig(&cfg); err != nil {
 		t.Errorf("Failed to apply tracer config: %v", err)
@@ -69,22 +68,21 @@ func TestHTTPSpanMiddleware(t *testing.T) {
 	next := testHandler{}
 	middleware := HTTPSpanMiddleware(&next)
 
-	var lastWrite []byte
-	fw := fakeWriter{lastWrite: &lastWrite}
+	fw := &fakeWriter{}
 
 	req, err := http.NewRequest("GET", "http://test.example.com", nil)
 	if err != nil {
-		t.Errorf("Failed to make fake request: %v", err)
+		t.Fatal("Failed to make fake request:", err)
 	}
-	traceID := "821e0d50d931235a5ba3fa42eddddd8f"
+	const traceID = "821e0d50d931235a5ba3fa42eddddd8f"
 	req.Header["X-B3-Traceid"] = []string{traceID}
 	req.Header["X-B3-Spanid"] = []string{"b3bd5e1c4318c78a"}
 
 	middleware.ServeHTTP(fw, req)
 
 	// Assert our next handler was called
-	if diff := cmp.Diff([]byte("fake"), lastWrite); diff != "" {
-		t.Errorf("Got http response (-want, +got) = %v", diff)
+	if got, want := fw.lastWrite, "fake"; got != want {
+		t.Errorf("HTTP Response: %q, want: %q", got, want)
 	}
 
 	spans := reporter.Flush()
@@ -104,53 +102,54 @@ func TestHTTPSpanIgnoringPaths(t *testing.T) {
 
 	// Create tracer with reporter recorder
 	reporter, co := FakeZipkinExporter()
-	defer reporter.Close()
 	oct := NewOpenCensusTracer(co)
-	defer oct.Finish()
+	t.Cleanup(func() {
+		reporter.Close()
+		oct.Finish()
+	})
 
 	if err := oct.ApplyConfig(&cfg); err != nil {
-		t.Errorf("Failed to apply tracer config: %v", err)
+		t.Fatal("Failed to apply tracer config:", err)
 	}
 
 	paths := []string{"/readyz"}
 	middleware := HTTPSpanIgnoringPaths(paths...)(&testHandler{})
 
-	testCases := map[string]struct {
+	testCases := []struct {
+		name   string
 		path   string
 		traced bool
-	}{
-		"traced": {
-			path:   "/",
-			traced: true,
-		},
-		"ignored": {
-			path:   paths[0],
-			traced: false,
-		},
-	}
-	for n, tc := range testCases {
-		t.Run(n, func(t *testing.T) {
-			var lastWrite []byte
-			fw := fakeWriter{lastWrite: &lastWrite}
+	}{{
+		name:   "traced",
+		path:   "/",
+		traced: true,
+	}, {
+		name:   "ignored",
+		path:   paths[0],
+		traced: false,
+	}}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			fw := &fakeWriter{}
 
 			u := &url.URL{
 				Scheme: "http",
 				Host:   "test.example.com",
 				Path:   tc.path,
 			}
-			req, err := http.NewRequest("GET", u.String(), nil)
+			req, err := http.NewRequest(http.MethodGet, u.String(), nil)
 			if err != nil {
-				t.Errorf("Failed to make fake request: %v", err)
+				t.Fatal("Failed to make fake request:", err)
 			}
-			traceID := "821e0d50d931235a5ba3fa42eddddd8f"
+			const traceID = "821e0d50d931235a5ba3fa42eddddd8f"
 			req.Header.Set("X-B3-Traceid", traceID)
 			req.Header.Set("X-B3-Spanid", "b3bd5e1c4318c78a")
 
 			middleware.ServeHTTP(fw, req)
 
 			// Assert our next handler was called
-			if diff := cmp.Diff([]byte("fake"), lastWrite); diff != "" {
-				t.Errorf("Got http response (-want, +got) = %v", diff)
+			if got, want := string(fw.lastWrite), "fake"; got != want {
+				t.Errorf("HTTP response: %q, want: %q", got, want)
 			}
 
 			spans := reporter.Flush()
@@ -161,10 +160,8 @@ func TestHTTPSpanIgnoringPaths(t *testing.T) {
 				if got := spans[0].TraceID.String(); got != traceID {
 					t.Errorf("spans[0].TraceID = %s, want %s", got, traceID)
 				}
-			} else {
-				if len(spans) != 0 {
-					t.Errorf("Got %d spans, expected 0: spans = %v", len(spans), spans)
-				}
+			} else if len(spans) != 0 {
+				t.Errorf("Got %d spans, expected 0: spans = %v", len(spans), spans)
 			}
 		})
 	}
diff --git a/webhook/admission_integration_test.go b/webhook/admission_integration_test.go
index 82be9bb1f..244124578 100644
--- a/webhook/admission_integration_test.go
+++ b/webhook/admission_integration_test.go
@@ -28,6 +28,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	"golang.org/x/sync/errgroup"
 	jsonpatch "gomodules.xyz/jsonpatch/v2"
 	admissionv1 "k8s.io/api/admission/v1"
@@ -156,6 +157,11 @@ func TestAdmissionValidResponseForResource(t *testing.T) {
 			t.Errorf("Failed to decode response: %v", err)
 			return
 		}
+
+		if diff := cmp.Diff(rev.TypeMeta, reviewResponse.TypeMeta); diff != "" {
+			t.Errorf("expected the response typeMeta to be the same as the request (-want, +got)\n%s", diff)
+			return
+		}
 	}()
 
 	// Check that Admit calls block when they are initiated before informers sync.
diff --git a/webhook/resourcesemantics/defaulting/user_info_test.go b/webhook/resourcesemantics/defaulting/user_info_test.go
index b7923fc2c..89ef5ce42 100644
--- a/webhook/resourcesemantics/defaulting/user_info_test.go
+++ b/webhook/resourcesemantics/defaulting/user_info_test.go
@@ -64,12 +64,9 @@ func TestSetUserInfoAnnotationsWhenWithinCreate(t *testing.T) {
 			"pkg.knative.dev/lastModifier": user1,
 		},
 	}, {
-		name: "test create (should not touch annotations when no user info available)",
-		configureContext: func(ctx context.Context) context.Context {
-			return apis.WithinCreate(ctx)
-		},
-		setup: func(ctx context.Context, r *Resource) {
-		},
+		name:                "test create (should not touch annotations when no user info available)",
+		configureContext:    apis.WithinCreate,
+		setup:               func(ctx context.Context, r *Resource) {},
 		expectedAnnotations: nil,
 	}}