diff --git a/pkg/reconciler/v1alpha1/configuration/configuration.go b/pkg/reconciler/v1alpha1/configuration/configuration.go index dbd747b7b877..e1df0c5c1e36 100644 --- a/pkg/reconciler/v1alpha1/configuration/configuration.go +++ b/pkg/reconciler/v1alpha1/configuration/configuration.go @@ -31,6 +31,7 @@ import ( "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/labels" "k8s.io/client-go/tools/cache" @@ -224,16 +225,32 @@ func (c *Reconciler) createRevision(ctx context.Context, config *v1alpha1.Config build := resources.MakeBuild(config) gvr, _ := meta.UnsafeGuessKindToResource(build.GroupVersionKind()) - created, err := c.DynamicClientSet.Resource(gvr).Namespace(build.GetNamespace()).Create(build) + // First, see if a build with this spec already exists. + buildHash := build.GetLabels()[serving.BuildHashLabelKey] + ul, err := c.DynamicClientSet.Resource(gvr).Namespace(build.GetNamespace()).List(metav1.ListOptions{ + LabelSelector: fmt.Sprintf("%s=%s", serving.BuildHashLabelKey, buildHash), + }) if err != nil { return nil, err } - logger.Infof("Created Build:\n%+v", created.GetName()) - c.Recorder.Eventf(config, corev1.EventTypeNormal, "Created", "Created Build %q", created.GetName()) + + var result *unstructured.Unstructured + if len(ul.Items) != 0 { + // If one exists, then have the Revision reference it. + result = &ul.Items[0] + } else { + // Otherwise, create a build and reference that. + result, err = c.DynamicClientSet.Resource(gvr).Namespace(build.GetNamespace()).Create(build) + if err != nil { + return nil, err + } + logger.Infof("Created Build:\n%+v", result.GetName()) + c.Recorder.Eventf(config, corev1.EventTypeNormal, "Created", "Created Build %q", result.GetName()) + } buildRef = &corev1.ObjectReference{ - APIVersion: created.GetAPIVersion(), - Kind: created.GetKind(), - Name: created.GetName(), + APIVersion: result.GetAPIVersion(), + Kind: result.GetKind(), + Name: result.GetName(), } } diff --git a/pkg/reconciler/v1alpha1/configuration/configuration_test.go b/pkg/reconciler/v1alpha1/configuration/configuration_test.go index 06bca15e918d..8a49320d3684 100644 --- a/pkg/reconciler/v1alpha1/configuration/configuration_test.go +++ b/pkg/reconciler/v1alpha1/configuration/configuration_test.go @@ -107,6 +107,33 @@ func TestReconcile(t *testing.T) { }), "Bogus"), }}, Key: "foo/validation-failure", + }, { + Name: "elide build when a matching one already exists", + Objects: []runtime.Object{ + cfgWithBuild("need-rev-and-build", "foo", 99998, &buildSpec), + // An existing build is reused! + resources.MakeBuild(cfgWithBuild("something-else", "foo", 12345, &buildSpec)), + }, + WantCreates: []metav1.Object{ + resources.MakeRevision(cfgWithBuild("need-rev-and-build", "foo", 99998, &buildSpec), &corev1.ObjectReference{ + APIVersion: "build.knative.dev/v1alpha1", + Kind: "Build", + Name: "something-else-12345", + }), + }, + WantUpdates: []clientgotesting.UpdateActionImpl{{ + Object: cfgWithBuildAndStatus("need-rev-and-build", "foo", 99998, &buildSpec, + v1alpha1.ConfigurationStatus{ + LatestCreatedRevisionName: "need-rev-and-build-99998", + ObservedGeneration: 99998, + Conditions: duckv1alpha1.Conditions{{ + Type: v1alpha1.ConfigurationConditionReady, + Status: corev1.ConditionUnknown, + }}, + }, + ), + }}, + Key: "foo/need-rev-and-build", }, { Name: "create revision matching generation with build", Objects: []runtime.Object{ diff --git a/test/conformance/route_test.go b/test/conformance/route_test.go index ddc8ee7ffe77..46e619d3d2dc 100644 --- a/test/conformance/route_test.go +++ b/test/conformance/route_test.go @@ -42,13 +42,11 @@ func createRouteAndConfig(logger *logging.BaseLogger, clients *test.Clients, nam } func updateConfigWithImage(clients *test.Clients, names test.ResourceNames, imagePaths []string) error { - patches := []jsonpatch.JsonPatchOperation{ - { - Operation: "replace", - Path: "/spec/revisionTemplate/spec/container/image", - Value: imagePaths[1], - }, - } + patches := []jsonpatch.JsonPatchOperation{{ + Operation: "replace", + Path: "/spec/revisionTemplate/spec/container/image", + Value: imagePaths[1], + }} patchBytes, err := json.Marshal(patches) if err != nil { return err diff --git a/test/e2e/build_test.go b/test/e2e/build_test.go index d18984578ff7..47f68df56131 100644 --- a/test/e2e/build_test.go +++ b/test/e2e/build_test.go @@ -18,15 +18,19 @@ limitations under the License. package e2e import ( + "encoding/json" "net/http" "testing" + "github.com/google/go-cmp/cmp" buildv1alpha1 "github.com/knative/build/pkg/apis/build/v1alpha1" "github.com/knative/pkg/apis/duck" testbuildv1alpha1 "github.com/knative/serving/test/apis/testing/v1alpha1" + "github.com/mattbaird/jsonpatch" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1" pkgTest "github.com/knative/pkg/test" @@ -41,7 +45,7 @@ func TestBuildSpecAndServe(t *testing.T) { clients := Setup(t) // Add test case specific name to its own logger. - logger := logging.GetContextLogger("TestBuildAndServe") + logger := logging.GetContextLogger("TestBuildSpecAndServe") imagePath := test.ImagePath("helloworld") @@ -116,6 +120,30 @@ func TestBuildSpecAndServe(t *testing.T) { } else if cond.Status != corev1.ConditionTrue { t.Fatalf("Build %q was not successful", buildName) } + + // Update the Configuration with an environment variable, which should trigger a new revision + // to be created, but without creating a new build. + if err := updateConfigWithEnvVars(clients, names, []corev1.EnvVar{{ + Name: "FOO", + Value: "bar", + }}); err != nil { + t.Fatalf("Failed to update config with environment variables: %v", err) + } + + nextRevName, err := getNextRevisionName(clients, names) + if err != nil { + t.Fatalf("Error waiting for next revision to be created: %v", err) + } + names.Revision = nextRevName + + nextRev, err := clients.ServingClient.Revisions.Get(nextRevName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Failed to get latest Revision: %v", err) + } + + if diff := cmp.Diff(rev.Spec.BuildRef, nextRev.Spec.BuildRef); diff != "" { + t.Fatalf("Unexpected differences in BuildRef: %v", diff) + } } func TestBuildAndServe(t *testing.T) { @@ -189,6 +217,53 @@ func TestBuildAndServe(t *testing.T) { } else if cond.Status != corev1.ConditionTrue { t.Fatalf("Build %q was not successful", buildName) } + + // Update the Configuration with an environment variable, which should trigger a new revision + // to be created, but without creating a new build. + if err := updateConfigWithEnvVars(clients, names, []corev1.EnvVar{{ + Name: "FOO", + Value: "bar", + }}); err != nil { + t.Fatalf("Failed to update config with environment variables: %v", err) + } + + nextRevName, err := getNextRevisionName(clients, names) + if err != nil { + t.Fatalf("Error waiting for next revision to be created: %v", err) + } + names.Revision = nextRevName + + nextRev, err := clients.ServingClient.Revisions.Get(nextRevName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Failed to get latest Revision: %v", err) + } + + if diff := cmp.Diff(rev.Spec.BuildRef, nextRev.Spec.BuildRef); diff != "" { + t.Fatalf("Unexpected differences in BuildRef: %v", diff) + } + + // Update the Configuration's Build with an annotation, which should trigger the creation + // of BOTH a Build and a Revision. + if err := updateConfigWithBuildAnnotation(clients, names, map[string]string{ + "testing.knative.dev/foo": "bar", + }); err != nil { + t.Fatalf("Failed to update config with environment variables: %v", err) + } + + nextRevName, err = getNextRevisionName(clients, names) + if err != nil { + t.Fatalf("Error waiting for next revision to be created: %v", err) + } + names.Revision = nextRevName + + nextRev, err = clients.ServingClient.Revisions.Get(nextRevName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Failed to get latest Revision: %v", err) + } + + if diff := cmp.Diff(rev.Spec.BuildRef, nextRev.Spec.BuildRef); diff == "" { + t.Fatalf("Got matching BuildRef, wanted different: %#v", rev.Spec.BuildRef) + } } func TestBuildFailure(t *testing.T) { @@ -258,3 +333,49 @@ func TestBuildFailure(t *testing.T) { t.Fatalf("Build %q was not unsuccessful", buildName) } } + +func getNextRevisionName(clients *test.Clients, names test.ResourceNames) (string, error) { + var newRevisionName string + err := test.WaitForConfigurationState(clients.ServingClient, names.Config, func(c *v1alpha1.Configuration) (bool, error) { + if c.Status.LatestCreatedRevisionName != names.Revision { + newRevisionName = c.Status.LatestCreatedRevisionName + return true, nil + } + return false, nil + }, "ConfigurationUpdatedWithRevision") + return newRevisionName, err +} + +func updateConfigWithEnvVars(clients *test.Clients, names test.ResourceNames, ev []corev1.EnvVar) error { + patches := []jsonpatch.JsonPatchOperation{{ + Operation: "add", + Path: "/spec/revisionTemplate/spec/container/env", + Value: ev, + }} + patchBytes, err := json.Marshal(patches) + if err != nil { + return err + } + _, err = clients.ServingClient.Configs.Patch(names.Config, types.JSONPatchType, patchBytes, "") + if err != nil { + return err + } + return nil +} + +func updateConfigWithBuildAnnotation(clients *test.Clients, names test.ResourceNames, ann map[string]string) error { + patches := []jsonpatch.JsonPatchOperation{{ + Operation: "add", + Path: "/spec/build/metadata/annotations", + Value: ann, + }} + patchBytes, err := json.Marshal(patches) + if err != nil { + return err + } + _, err = clients.ServingClient.Configs.Patch(names.Config, types.JSONPatchType, patchBytes, "") + if err != nil { + return err + } + return nil +}