🧹 Logging overhaul (#294)

* An overhaul of logging

Signed-off-by: Chris Suszyński <[email protected]>

* Dump events only on failed tests by default

* Move initialization to factory

Signed-off-by: Chris Suszyński <[email protected]>

* Proper error handing at early initialization stage

Signed-off-by: Chris Suszyński <[email protected]>

* Update deps

* Better docs for the environment.NewStandardGlobalEnvironment func

* Rename starters to initializers

* Rename the flags interface

* Rename interimLogger to defaultLogger

* Export Cleanup opt

* Use Result interface instead of simple bool in Emitter.Finished method.

* Move image registration to within the installation.

* Moving initialization further up in the stack

* Channel non-blocking solution for RegisterPackages and ProduceImages.

Use producer-consumer channel-based non-blocking solution for
RegisterPackages and ProduceImages to satisfy the race detection
checker.

* Less verbose output of Feature state

* WithImages will not influence the adjacent tests anymore

* Deprecate manifest.InstallLocalYaml in favor of manifest.InstallYamlFS.

* Include kn-plugin-event in downstream tests

.github/workflows/kind-e2e.yaml

@@ -13,17 +13,13 @@ jobs:
       matrix:
         go-version: [ 1.17.x ]
         platform: [ ubuntu-latest ]
-        ko-version: [ 0.11.2 ]
         kind-version: [ 0.14.0 ]
 
     name: e2e tests
     runs-on: ${{ matrix.platform }}
 
     env:
-      GOPATH: ${{ github.workspace }}
-      GO111MODULE: off
       KO_DOCKER_REPO: kind.local
-      KO_VERSION: ${{ matrix.ko-version }}
       KIND_VERSION: ${{ matrix.kind-version }}
 
     steps:
@@ -33,14 +29,6 @@ jobs:
         go-version: ${{ matrix.go-version }}
       id: go
 
-    - name: Install Dependencies
-      run: |
-        echo '::group:: install ko ${KO_VERSION}'
-        curl -L https://github.com/google/ko/releases/download/v${KO_VERSION}/ko_${KO_VERSION}_Linux_x86_64.tar.gz | tar xzf - ko
-        chmod +x ./ko
-        sudo mv ko /usr/local/bin
-        echo '::endgroup::'
-
     - name: Check out code onto GOPATH
       uses: actions/checkout@v2
       with:
@@ -96,7 +84,8 @@ jobs:
         kubectl apply -f ./test/config
 
         # Run the tests tagged as e2e on the KinD cluster.
-        go test -v -race -count=1 -timeout=15m -tags=e2e ./test/...
+        go run gotest.tools/[email protected] --format testname -- \
+          -race -count=1 -timeout=15m -tags=e2e ./test/...
 
     - name: Gather Failure Data
       if: ${{ failure() }}

.github/workflows/knative-downstream.yaml

@@ -38,6 +38,8 @@ jobs:
             org: knative-sandbox
           - repo: eventing-kafka-broker
             org: knative-sandbox
+          - repo: kn-plugin-event
+            org: knative-sandbox
 
     runs-on: ubuntu-latest
     env:

README.md

@@ -42,8 +42,6 @@ used for the rest of the test run, it is a singleton.
 // +build e2e
 
 import (
-	"knative.dev/pkg/injection"
-	"knative.dev/reconciler-test/pkg/logging"
 	"knative.dev/reconciler-test/pkg/environment"
 )
 
@@ -54,24 +52,7 @@ var global environment.GlobalEnvironment
 
 // TestMain is the first entry point for `go test`.
 func TestMain(m *testing.M) {
-	// environment.InitFlags registers state, level and feature filter flags.
-	environment.InitFlags(flag.CommandLine)
-
-	// We get a chance to parse flags to include the framework flags for the
-	// framework as well as any additional flags included in the integration.
-	flag.Parse()
-
-	// EnableInjectionOrDie will enable client injection, this is used by the
-	// testing framework for namespace management, and could be leveraged by
-	// features to pull Kubernetes clients or the test environment out of the
-	// context passed in the features.
-	ctx, startInformers := injection.EnableInjectionOrDie(
-		logging.WithTestLogger(nil), nil) // nolint
-	startInformers()
-
-	// global is used to make instances of Environments, NewGlobalEnvironment
-	// is passing and saving the client injection enabled context for use later.
-	global = environment.NewGlobalEnvironment(ctx)
+	global = environment.NewStandardGlobalEnvironment()
 
 	// Run the tests.
 	os.Exit(m.Run())

go.mod

@@ -18,7 +18,6 @@ require (
 	k8s.io/api v0.23.8
 	k8s.io/apimachinery v0.23.8
 	k8s.io/client-go v0.23.8
-	k8s.io/klog v1.0.0
 	knative.dev/hack v0.0.0-20220701014203-65c463ac8c98
 	knative.dev/pkg v0.0.0-20220705130606-e60d250dc637
 	sigs.k8s.io/yaml v1.3.0

go.sum

@@ -1156,8 +1156,6 @@ k8s.io/component-base v0.23.8/go.mod h1:rCj6EeaYLsNneVoFuSPL/AlEWmomc39j9M9i4NpR
 k8s.io/gengo v0.0.0-20210813121822-485abfe95c7c/go.mod h1:FiNAH4ZV3gBg2Kwh89tzAEV2be7d5xI0vBa/VySYy3E=
 k8s.io/gengo v0.0.0-20220307231824-4627b89bbf1b h1:vEhKDJESYfeRiaBNmRvO+/12RAo1cFeu6vGm1fBFY34=
 k8s.io/gengo v0.0.0-20220307231824-4627b89bbf1b/go.mod h1:FiNAH4ZV3gBg2Kwh89tzAEV2be7d5xI0vBa/VySYy3E=
-k8s.io/klog v1.0.0 h1:Pt+yjF5aB1xDSVbau4VsWe+dQNzA0qv1LlXdC2dF6Q8=
-k8s.io/klog v1.0.0/go.mod h1:4Bi6QPql/J/LkTDqv7R/cd3hPo4k2DG6Ptcz060Ez5I=
 k8s.io/klog/v2 v2.0.0/go.mod h1:PBfzABfn139FHAV07az/IF9Wp1bkk3vpT2XSJ76fSDE=
 k8s.io/klog/v2 v2.2.0/go.mod h1:Od+F08eJP+W3HUb4pSrPpgp9DGU4GzlpG/TmITuYh/Y=
 k8s.io/klog/v2 v2.30.0/go.mod h1:y1WjHnz7Dj687irZUWR/WLkLc5N1YHtjLdmgWjndZn0=

pkg/environment/images.go

@@ -17,34 +17,43 @@ limitations under the License.
 package environment
 
 import (
+	"context"
 	"fmt"
 	"strings"
 	"sync"
 
-	"knative.dev/reconciler-test/pkg/images"
+	"knative.dev/pkg/logging"
+	"knative.dev/reconciler-test/pkg/images/ko"
 )
 
-var packages = []string(nil)
-var packageToImageConfig = map[string]string{}
-var packaged sync.Once
+var (
+	// CurrentImageProducer is the function that will be used to produce the
+	// container images. By default, it is ko.Publish, but can be overridden.
+	CurrentImageProducer = ImageProducer(ko.Publish)
+
+	// produceImagesLock is used to ensure that ProduceImages is only called
+	// once at the time.
+	produceImagesLock = sync.Mutex{}
+)
+
+// parallelQueueSize is the max number of packages at one time in queue to be
+// consumed by image producer.
+const parallelQueueSize = 1_000
+
+// ImageProducer is a function that will be used to produce the container images.
+type ImageProducer func(ctx context.Context, pack string) (string, error)
 
 // RegisterPackage registers an interest in producing an image based on the
-// provide package.
+// provided package.
 // Can be called multiple times with the same package.
 // A package will be used to produce the image and used
 // like `image: ko://<package>` inside test yaml.
-func RegisterPackage(pack ...string) {
-	for _, p := range pack {
-		exists := false
-		for _, k := range packages {
-			if p == k {
-				exists = true
-				break
-			}
-		}
-		if !exists {
-			packages = append(packages, p)
-		}
+func RegisterPackage(pack ...string) EnvOpts {
+	return func(ctx context.Context, _ Environment) (context.Context, error) {
+		rk := registeredPackagesKey{}
+		rk.register(ctx, pack)
+		store := rk.get(ctx)
+		return context.WithValue(ctx, rk, store), nil
 	}
 }
 
@@ -53,26 +62,102 @@ func RegisterPackage(pack ...string) {
 // init() method. An images value should be a container registry image. The
 // images map is presented to the templates on the field `images`, and used
 // like `image: <key>` inside test yaml.
-func WithImages(images map[string]string) {
-	packaged.Do(func() {
-		packageToImageConfig = images
-	})
+func WithImages(given map[string]string) EnvOpts {
+	return func(ctx context.Context, _ Environment) (context.Context, error) {
+		ik := imageStoreKey{}
+		store := ik.new()
+		store.refs = given
+		return context.WithValue(ctx, ik, store), nil
+	}
 }
 
 // ProduceImages returns back the packages that have been added.
 // Will produce images once, can be called many times.
-func ProduceImages() (map[string]string, error) {
-	var propErr error
-	packaged.Do(func() {
-		for _, pack := range packages {
-			image, err := images.KoPublish(pack)
-			if err != nil {
-				fmt.Printf("error attempting to ko publish: %s\n", err)
-				propErr = err
-				return
-			}
-			packageToImageConfig["ko://"+pack] = strings.TrimSpace(image)
+func ProduceImages(ctx context.Context) (map[string]string, error) {
+	produceImagesLock.Lock()
+	defer produceImagesLock.Unlock()
+	rk := registeredPackagesKey{}
+	ik := imageStoreKey{}
+	store := ik.get(ctx)
+	for _, pack := range rk.packages(ctx) {
+		koPack := fmt.Sprintf("ko://%s", pack)
+		if store.refs[koPack] != "" {
+			continue
 		}
-	})
-	return packageToImageConfig, propErr
+		image, err := CurrentImageProducer(ctx, pack)
+		if err != nil {
+			return nil, err
+		}
+		store.refs[koPack] = strings.TrimSpace(image)
+	}
+	return store.refs, nil
+}
+
+func initializeImageStores(ctx context.Context) context.Context {
+	var emptyPkgs []string
+	emptyImgs := make(map[string]string)
+	mctx, err := UnionOpts(
+		RegisterPackage(emptyPkgs...),
+		WithImages(emptyImgs),
+	)(ctx, nil)
+	if err != nil {
+		logging.FromContext(ctx).
+			Fatal("Failed to initialize image stores: ", err)
+	}
+	return mctx
+}
+
+type registeredPackagesKey struct{}
+
+type packagesStore struct {
+	refs chan string
+}
+
+func (k registeredPackagesKey) get(ctx context.Context) *packagesStore {
+	if registered, ok := ctx.Value(k).(*packagesStore); ok {
+		return registered
+	}
+	return &packagesStore{
+		refs: make(chan string, parallelQueueSize),
+	}
+}
+
+func (k registeredPackagesKey) packages(ctx context.Context) []string {
+	store := k.get(ctx)
+	refs := make([]string, 0)
+	for {
+		select {
+		case ref := <-store.refs:
+			refs = append(refs, ref)
+		default:
+			return refs
+		}
+	}
+}
+
+func (k registeredPackagesKey) register(ctx context.Context, packs []string) {
+	store := k.get(ctx)
+	for _, pack := range packs {
+		pack = strings.TrimPrefix(pack, "ko://")
+		store.refs <- pack
+	}
+}
+
+type imageStoreKey struct{}
+
+func (k imageStoreKey) new() *imageStore {
+	return &imageStore{
+		refs: make(map[string]string),
+	}
+}
+
+func (k imageStoreKey) get(ctx context.Context) *imageStore {
+	if i, ok := ctx.Value(k).(*imageStore); ok {
+		return i
+	}
+	return k.new()
+}
+
+type imageStore struct {
+	refs map[string]string
 }

pkg/environment/interfaces.go

@@ -57,12 +57,6 @@ type Environment interface {
 	// FeatureState returns the requirement level for this environment.
 	FeatureState() feature.States
 
-	// Images returns back the name to container image mapping to be used with
-	// yaml template parsing.
-	// The map will be in the form `key`: `image` and `key` and the intention
-	// usage is to use this key to string substitute for image in test yaml.
-	Images() map[string]string
-
 	// TemplateConfig returns the base template config to use when processing
 	// yaml templates.
 	TemplateConfig(base map[string]interface{}) map[string]interface{}
@@ -82,3 +76,17 @@ type Environment interface {
 	// References will be cleaned up if registered.
 	Finish()
 }
+
+// UnionOpts joins the given opts into a single opts function.
+func UnionOpts(opts ...EnvOpts) EnvOpts {
+	return func(ctx context.Context, env Environment) (context.Context, error) {
+		for _, opt := range opts {
+			var err error
+			ctx, err = opt(ctx, env)
+			if err != nil {
+				return ctx, err
+			}
+		}
+		return ctx, nil
+	}
+}

pkg/environment/logging.go

@@ -17,7 +17,11 @@ limitations under the License.
 package environment
 
 import (
+	"context"
+
+	"go.uber.org/zap/zaptest"
 	"knative.dev/reconciler-test/pkg/feature"
+	testlog "knative.dev/reconciler-test/pkg/logging"
 )
 
 // loggingSteps returns a number of steps that logs environment-managed resources.
@@ -30,3 +34,10 @@ func (mr *MagicEnvironment) loggingSteps() []feature.Step {
 		Fn:   feature.LogReferences(mr.refs...),
 	}}
 }
+
+// WithTestLogger returns a context with test logger configured.
+func WithTestLogger(t zaptest.TestingT, opts ...zaptest.LoggerOption) EnvOpts {
+	return func(ctx context.Context, env Environment) (context.Context, error) {
+		return testlog.WithTestLogger(ctx, t, opts...), nil
+	}
+}