From 5d8630b227c4bf6c3e248764e3ff3137a5f36b58 Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Tue, 8 Jan 2019 20:02:07 +0100 Subject: [PATCH] =?UTF-8?q?Add=20possibility=20for=20a=20client=20to=20can?= =?UTF-8?q?cel=20a=20build=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit … by updating the status of a build. It will mark the build as failed and clean any pods related to it. Signed-off-by: Vincent Demeester --- Gopkg.lock | 1 + pkg/apis/build/v1alpha1/build_types.go | 15 ++++ pkg/reconciler/build/build.go | 29 +++++- pkg/reconciler/build/build_test.go | 59 ++++++++++++ test/e2e/cancelled_test.go | 120 +++++++++++++++++++++++++ test/e2e/simple_test.go | 1 - 6 files changed, 223 insertions(+), 2 deletions(-) create mode 100644 test/e2e/cancelled_test.go diff --git a/Gopkg.lock b/Gopkg.lock index 09a952df..20084395 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -856,6 +856,7 @@ "k8s.io/apimachinery/pkg/types", "k8s.io/apimachinery/pkg/util/runtime", "k8s.io/apimachinery/pkg/util/sets/types", + "k8s.io/apimachinery/pkg/util/wait", "k8s.io/apimachinery/pkg/watch", "k8s.io/client-go/discovery", "k8s.io/client-go/discovery/fake", diff --git a/pkg/apis/build/v1alpha1/build_types.go b/pkg/apis/build/v1alpha1/build_types.go index c691e437..acfbca4f 100644 --- a/pkg/apis/build/v1alpha1/build_types.go +++ b/pkg/apis/build/v1alpha1/build_types.go @@ -98,8 +98,21 @@ type BuildSpec struct { // If specified, the pod's scheduling constraints // +optional Affinity *corev1.Affinity `json:"affinity,omitempty"` + + // Used for cancelling a job (and maybe more later on) + // +optional + Status BuildSpecStatus } +// BuildSpecStatus defines the build spec status the user can provide +type BuildSpecStatus string + +const ( + // BuildSpecStatusCancelled indicates that the user wants to cancel the build, + // if not already cancelled or terminated + BuildSpecStatusCancelled = "BuildCancelled" +) + // TemplateKind defines the type of BuildTemplate used by the build. type TemplateKind string @@ -281,6 +294,8 @@ type GoogleSpec struct { // will be False. const BuildSucceeded = duckv1alpha1.ConditionSucceeded +const BuildCancelled duckv1alpha1.ConditionType = "Cancelled" + var buildCondSet = duckv1alpha1.NewBatchConditionSet() // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/pkg/reconciler/build/build.go b/pkg/reconciler/build/build.go index 9886bec9..c9294842 100644 --- a/pkg/reconciler/build/build.go +++ b/pkg/reconciler/build/build.go @@ -153,6 +153,28 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { return nil } + // If the build's status is cancelled, kill resources and update status + if isCancelled(build.Spec) { + logger.Warnf("Build has been cancelled: %v", build.Name) + build.Status.SetCondition(&duckv1alpha1.Condition{ + Type: v1alpha1.BuildSucceeded, + Status: corev1.ConditionFalse, + Reason: "BuildCancelled", + Message: fmt.Sprintf("Build %q was cancelled", build.Name), + }) + if err := c.updateStatus(build); err != nil { + return err + } + if build.Status.Cluster == nil { + logger.Warnf("build %q has no pod running yet", build.Name) + return nil + } + if err := c.kubeclientset.CoreV1().Pods(build.Namespace).Delete(build.Status.Cluster.PodName, &metav1.DeleteOptions{}); err != nil { + return err + } + return nil + } + // If the build hasn't started yet, validate it and create a Pod for it // and record that pod's name in the build status. var p *corev1.Pod @@ -257,12 +279,17 @@ func (c *Reconciler) startPodForBuild(build *v1alpha1.Build) (*corev1.Pod, error return c.kubeclientset.CoreV1().Pods(p.Namespace).Create(p) } -// IsDone returns true if the build's status indicates the build is done. +// isDone returns true if the build's status indicates the build is done. func isDone(status *v1alpha1.BuildStatus) bool { cond := status.GetCondition(v1alpha1.BuildSucceeded) return cond != nil && cond.Status != corev1.ConditionUnknown } +// isCancelled returns true if the build's spec indicates the build is cancelled. +func isCancelled(buildSpec v1alpha1.BuildSpec) bool { + return buildSpec.Status == v1alpha1.BuildSpecStatusCancelled +} + func (c *Reconciler) checkTimeout(build *v1alpha1.Build) error { // If build has not started timeout, startTime should be zero. if build.Status.StartTime.IsZero() { diff --git a/pkg/reconciler/build/build_test.go b/pkg/reconciler/build/build_test.go index 10ddfcaa..474fa5f0 100644 --- a/pkg/reconciler/build/build_test.go +++ b/pkg/reconciler/build/build_test.go @@ -561,3 +561,62 @@ func TestTimeoutFlow(t *testing.T) { t.Errorf("Unexpected build status %s", d) } } + +func TestCancelledFlow(t *testing.T) { + b := newBuild("cancelled") + + f := &fixture{ + t: t, + objects: []runtime.Object{b}, + client: fake.NewSimpleClientset(b), + kubeclient: k8sfake.NewSimpleClientset(), + } + f.createServiceAccount() + + r, i, k8sI := f.newReconciler() + f.updateIndex(i, b) + stopCh := make(chan struct{}) + defer close(stopCh) + i.Start(stopCh) + k8sI.Start(stopCh) + + ctx := context.Background() + if err := r.Reconcile(ctx, getKey(b, t)); err != nil { + t.Errorf("Not Expect error when syncing build") + } + + buildClient := f.client.BuildV1alpha1().Builds(b.Namespace) + b, err := buildClient.Get(b.Name, metav1.GetOptions{}) + if err != nil { + t.Errorf("error fetching build: %v", err) + } + + // Get pod info + b, err = buildClient.Get(b.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("error getting build: %v", err) + } + if b.Status.Cluster == nil || b.Status.Cluster.PodName == "" { + t.Fatalf("build status did not specify podName: %v", b.Status.Cluster) + } + b.Spec.Status = v1alpha1.BuildSpecStatusCancelled + + f.updateIndex(i, b) + if err := r.Reconcile(ctx, getKey(b, t)); err != nil { + t.Errorf("error syncing build: %v", err) + } + + // Check that the build has the expected cancelled status. + b, err = buildClient.Get(b.Name, metav1.GetOptions{}) + if err != nil { + t.Errorf("error fetching build: %v", err) + } + if d := cmp.Diff(b.Status.GetCondition(duckv1alpha1.ConditionSucceeded), &duckv1alpha1.Condition{ + Type: duckv1alpha1.ConditionSucceeded, + Status: corev1.ConditionFalse, + Reason: "BuildCancelled", + Message: fmt.Sprintf("Build %q was cancelled", b.Name), + }, ignoreVolatileTime); d != "" { + t.Errorf("Unexpected build status %s", d) + } +} diff --git a/test/e2e/cancelled_test.go b/test/e2e/cancelled_test.go new file mode 100644 index 00000000..b49fdbe9 --- /dev/null +++ b/test/e2e/cancelled_test.go @@ -0,0 +1,120 @@ +// +build e2e + +/* +Copyright 2018 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 e2e + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/google/go-cmp/cmp" + "github.com/knative/build/pkg/apis/build/v1alpha1" + duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1" + "github.com/knative/pkg/test" + "github.com/knative/pkg/test/logging" + "go.opencensus.io/trace" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/wait" +) + +const ( + interval = 1 * time.Second + timeout = 5 * time.Minute +) + +// TestCancelledBuild tests that a build with a long running step +// so that we have time to cancel it. +func TestCancelledBuild(t *testing.T) { + logger := logging.GetContextLogger("TestCancelledBuild") + + clients := buildClients(logger) + buildName := "cancelled-build" + + test.CleanupOnInterrupt(func() { teardownBuild(clients, logger, buildName) }, logger) + defer teardownBuild(clients, logger, buildName) + + if _, err := clients.buildClient.builds.Create(&v1alpha1.Build{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: buildTestNamespace, + Name: buildName, + }, + Spec: v1alpha1.BuildSpec{ + Steps: []corev1.Container{{ + Image: "ubuntu", + Command: []string{"bash"}, + Args: []string{"-c", "sleep 5000"}, // fails. + }}, + }, + }); err != nil { + t.Fatalf("Error creating build: %v", err) + } + + // Wait for the build to be running + if err := waitForBuildState(clients, buildName, func(b *v1alpha1.Build) (bool, error) { + c := b.Status.GetCondition(duckv1alpha1.ConditionSucceeded) + if c != nil { + if c.Status == corev1.ConditionTrue || c.Status == corev1.ConditionFalse { + return true, fmt.Errorf("build %s already finished!", buildName) + } else if c.Status == corev1.ConditionUnknown && (c.Reason == "Building" || c.Reason == "Pending") { + return true, nil + } + } + return false, nil + }, "ToBeCancelledBuildRunning"); err != nil { + t.Fatalf("Error waiting for build %q to be running: %v", buildName, err) + } + + b, err := clients.buildClient.builds.Get(buildName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Error getting build %s: %v", buildName, err) + } + b.Spec.Status = v1alpha1.BuildSpecStatusCancelled + if _, err := clients.buildClient.builds.Update(b); err != nil { + t.Fatalf("Error update build status for %s: %v", buildName, err) + } + + b, err = clients.buildClient.watchBuild(buildName) + if err == nil { + t.Fatalf("watchBuild did not return expected `cancelled` error") + } + if d := cmp.Diff(b.Status.GetCondition(duckv1alpha1.ConditionSucceeded), &duckv1alpha1.Condition{ + Type: duckv1alpha1.ConditionSucceeded, + Status: corev1.ConditionFalse, + Reason: "BuildCancelled", + Message: fmt.Sprintf("Build %q was cancelled", b.Name), + }, ignoreVolatileTime); d != "" { + t.Errorf("Unexpected build status %s", d) + } +} + +func waitForBuildState(c *clients, name string, inState func(b *v1alpha1.Build) (bool, error), desc string) error { + metricName := fmt.Sprintf("WaitForBuildState/%s/%s", name, desc) + _, span := trace.StartSpan(context.Background(), metricName) + defer span.End() + + return wait.PollImmediate(interval, timeout, func() (bool, error) { + r, err := c.buildClient.builds.Get(name, metav1.GetOptions{}) + if err != nil { + return true, err + } + return inState(r) + }) +} diff --git a/test/e2e/simple_test.go b/test/e2e/simple_test.go index 8465f376..0148581d 100644 --- a/test/e2e/simple_test.go +++ b/test/e2e/simple_test.go @@ -536,7 +536,6 @@ func TestBuildWithSources(t *testing.T) { } // TestSimpleBuildWithHybridSources tests hybrid input sources can be accessed in all steps - func TestSimpleBuildWithHybridSources(t *testing.T) { logger := logging.GetContextLogger("TestSimpleBuildWithHybridSources") clients := buildClients(logger)