Move Build Cancelled flag from Status to Spec

pkg/build/api/deep_copy_generated.go

@@ -415,6 +415,7 @@ func DeepCopy_api_BuildSpec(in BuildSpec, out *BuildSpec, c *conversion.Cloner)
 	if err := DeepCopy_api_CommonSpec(in.CommonSpec, &out.CommonSpec, c); err != nil {
 		return err
 	}
+	out.Cancelled = in.Cancelled
 	if in.TriggeredBy != nil {
 		in, out := in.TriggeredBy, &out.TriggeredBy
 		*out = make([]BuildTriggerCause, len(in))
@@ -431,7 +432,6 @@ func DeepCopy_api_BuildSpec(in BuildSpec, out *BuildSpec, c *conversion.Cloner)
 
 func DeepCopy_api_BuildStatus(in BuildStatus, out *BuildStatus, c *conversion.Cloner) error {
 	out.Phase = in.Phase
-	out.Cancelled = in.Cancelled
 	out.Reason = in.Reason
 	out.Message = in.Message
 	if in.StartTimestamp != nil {

pkg/build/api/types.go

@@ -67,6 +67,10 @@ type Build struct {
 type BuildSpec struct {
 	CommonSpec
 
+	// Cancelled describes if a cancel event was triggered for the build.
+	// +genconversion=false
+	Cancelled bool
+
 	// TriggeredBy describes which triggers started the most recent update to the
 	// build configuration and contains information about those triggers.
 	TriggeredBy []BuildTriggerCause
@@ -167,9 +171,6 @@ type BuildStatus struct {
 	// Phase is the point in the build lifecycle.
 	Phase BuildPhase
 
-	// Cancelled describes if a cancel event was triggered for the build.
-	Cancelled bool
-
 	// Reason is a brief CamelCase string that describes any failure and is meant for machine parsing and tidy display in the CLI.
 	Reason StatusReason
 

pkg/build/api/v1/conversion.go

@@ -10,6 +10,22 @@ import (
 	imageapi "github.com/openshift/origin/pkg/image/api"
 )
 
+func Convert_api_Build_To_v1_Build(in *newer.Build, out *Build, s conversion.Scope) error {
+	if err := autoConvert_api_Build_To_v1_Build(in, out, s); err != nil {
+		return err
+	}
+	out.Status.Cancelled = in.Spec.Cancelled
+	return nil
+}
+
+func Convert_v1_Build_To_api_Build(in *Build, out *newer.Build, s conversion.Scope) error {
+	if err := autoConvert_v1_Build_To_api_Build(in, out, s); err != nil {
+		return err
+	}
+	out.Spec.Cancelled = in.Status.Cancelled
+	return nil
+}
+
 func Convert_v1_BuildConfig_To_api_BuildConfig(in *BuildConfig, out *newer.BuildConfig, s conversion.Scope) error {
 	if err := autoConvert_v1_BuildConfig_To_api_BuildConfig(in, out, s); err != nil {
 		return err
@@ -170,6 +186,8 @@ func addConversionFuncs(scheme *runtime.Scheme) {
 		Convert_api_BuildSource_To_v1_BuildSource,
 		Convert_v1_BuildStrategy_To_api_BuildStrategy,
 		Convert_api_BuildStrategy_To_v1_BuildStrategy,
+		Convert_api_Build_To_v1_Build,
+		Convert_v1_Build_To_api_Build,
 	)
 
 	if err := scheme.AddFieldLabelConversionFunc("v1", "Build",

pkg/build/api/v1/conversion_generated.go

@@ -172,10 +172,6 @@ func autoConvert_v1_Build_To_api_Build(in *Build, out *build_api.Build, s conver
 	return nil
 }
 
-func Convert_v1_Build_To_api_Build(in *Build, out *build_api.Build, s conversion.Scope) error {
-	return autoConvert_v1_Build_To_api_Build(in, out, s)
-}
-
 func autoConvert_api_Build_To_v1_Build(in *build_api.Build, out *Build, s conversion.Scope) error {
 	if err := api.Convert_unversioned_TypeMeta_To_unversioned_TypeMeta(&in.TypeMeta, &out.TypeMeta, s); err != nil {
 		return err
@@ -192,10 +188,6 @@ func autoConvert_api_Build_To_v1_Build(in *build_api.Build, out *Build, s conver
 	return nil
 }
 
-func Convert_api_Build_To_v1_Build(in *build_api.Build, out *Build, s conversion.Scope) error {
-	return autoConvert_api_Build_To_v1_Build(in, out, s)
-}
-
 func autoConvert_v1_BuildConfig_To_api_BuildConfig(in *BuildConfig, out *build_api.BuildConfig, s conversion.Scope) error {
 	if err := api.Convert_unversioned_TypeMeta_To_unversioned_TypeMeta(&in.TypeMeta, &out.TypeMeta, s); err != nil {
 		return err
@@ -838,7 +830,6 @@ func Convert_api_BuildSpec_To_v1_BuildSpec(in *build_api.BuildSpec, out *BuildSp
 
 func autoConvert_v1_BuildStatus_To_api_BuildStatus(in *BuildStatus, out *build_api.BuildStatus, s conversion.Scope) error {
 	out.Phase = build_api.BuildPhase(in.Phase)
-	out.Cancelled = in.Cancelled
 	out.Reason = build_api.StatusReason(in.Reason)
 	out.Message = in.Message
 	out.StartTimestamp = in.StartTimestamp
@@ -863,7 +854,6 @@ func Convert_v1_BuildStatus_To_api_BuildStatus(in *BuildStatus, out *build_api.B
 
 func autoConvert_api_BuildStatus_To_v1_BuildStatus(in *build_api.BuildStatus, out *BuildStatus, s conversion.Scope) error {
 	out.Phase = BuildPhase(in.Phase)
-	out.Cancelled = in.Cancelled
 	out.Reason = StatusReason(in.Reason)
 	out.Message = in.Message
 	out.StartTimestamp = in.StartTimestamp

pkg/build/api/v1/types.go

@@ -127,6 +127,7 @@ type BuildStatus struct {
 	Phase BuildPhase `json:"phase" protobuf:"bytes,1,opt,name=phase,casttype=BuildPhase"`
 
 	// cancelled describes if a cancel event was triggered for the build.
+	// +genconversion=false
 	Cancelled bool `json:"cancelled,omitempty" protobuf:"varint,2,opt,name=cancelled"`
 
 	// reason is a brief CamelCase string that describes any failure and is meant for machine parsing and tidy display in the CLI.

pkg/build/api/validation/validation.go

@@ -33,6 +33,25 @@ func ValidateBuild(build *buildapi.Build) field.ErrorList {
 	return allErrs
 }
 
+func buildSpecSemanticEqualities() conversion.Equalities {
+	// Copy base semantic funcs
+	funcs := []interface{}{}
+	for _, fv := range kapi.Semantic.Equalities {
+		funcs = append(funcs, fv.Interface())
+	}
+	// Add func to whitelist Cancelled flag
+	funcs = append(funcs, func(a, b buildapi.BuildSpec) bool {
+		// Set the Cancelled flag to the same value to white-list
+		a.Cancelled = b.Cancelled
+
+		// Compare with original semantic set of equalities which do not include this function
+		return kapi.Semantic.DeepEqual(a, b)
+	})
+	return conversion.EqualitiesOrDie(funcs...)
+}
+
+var buildSpecSemantic = buildSpecSemanticEqualities()
+
 func ValidateBuildUpdate(build *buildapi.Build, older *buildapi.Build) field.ErrorList {
 	allErrs := field.ErrorList{}
 	allErrs = append(allErrs, validation.ValidateObjectMetaUpdate(&build.ObjectMeta, &older.ObjectMeta, field.NewPath("metadata"))...)
@@ -42,7 +61,11 @@ func ValidateBuildUpdate(build *buildapi.Build, older *buildapi.Build) field.Err
 	if buildutil.IsBuildComplete(older) && older.Status.Phase != build.Status.Phase {
 		allErrs = append(allErrs, field.Invalid(field.NewPath("status", "phase"), build.Status.Phase, "phase cannot be updated from a terminal state"))
 	}
-	if !kapi.Semantic.DeepEqual(build.Spec, older.Spec) {
+	// Cancelled is a one-way flag, cannot be unset
+	if older.Spec.Cancelled && !build.Spec.Cancelled {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "cancelled"), build.Spec.Cancelled, "cancelled cannot be changed once set"))
+	}
+	if !buildSpecSemantic.DeepEqual(build.Spec, older.Spec) {
 		diff, err := diffBuildSpec(build.Spec, older.Spec)
 		if err != nil {
 			glog.V(2).Infof("Error calculating build spec patch: %v", err)
@@ -71,10 +94,6 @@ func ValidateBuildDetailsUpdate(build *buildapi.Build, older *buildapi.Build) fi
 // ValidateBuildStatusUpdate validates a build status update
 func ValidateBuildStatusUpdate(build *buildapi.Build, older *buildapi.Build) field.ErrorList {
 	allErrs := field.ErrorList{}
-	// Ensure that the cancelled flag is not changed in a status update
-	if older.Status.Cancelled != build.Status.Cancelled {
-		allErrs = append(allErrs, field.Invalid(field.NewPath("status", "cancelled"), nil, "cannot update cancelled flag in a status update"))
-	}
 	return allErrs
 }
 

pkg/build/api/validation/validation_test.go

@@ -77,28 +77,6 @@ func TestValidateBuildDetailsUpdate(t *testing.T) {
 	}
 }
 
-func TestValidateBuildStatusUpdate(t *testing.T) {
-	older := testBuild()
-	newer := testBuild()
-
-	// Ensure that a change to the cancelled flag is not allowed through a status update
-	older.Status.Cancelled = false
-	newer.Status.Cancelled = true
-	errs := ValidateBuildStatusUpdate(newer, older)
-	if len(errs) == 0 {
-		t.Errorf("no error returned when changing cancelled flag on build status update")
-	}
-
-	// Ensure that a valid status update is allowed
-	older.Status.Cancelled = false
-	newer.Status.Cancelled = false
-	newer.Status.Phase = buildapi.BuildPhaseComplete
-	errs = ValidateBuildStatusUpdate(newer, older)
-	if len(errs) > 0 {
-		t.Errorf("expected no error, got: %v", errs)
-	}
-}
-
 func checkDockerStrategyEmptySourceError(result field.ErrorList) bool {
 	for _, err := range result {
 		if err.Type == field.ErrorTypeInvalid && strings.Contains(err.Field, "spec.source") && strings.Contains(err.Detail, "must provide a value for at least one source input(git, binary, dockerfile, images).") {
@@ -333,6 +311,12 @@ func newDefaultParameters() buildapi.BuildSpec {
 	}
 }
 
+func cancelledBuildSpec() buildapi.BuildSpec {
+	spec := newDefaultParameters()
+	spec.Cancelled = true
+	return spec
+}
+
 func newNonDefaultParameters() buildapi.BuildSpec {
 	o := newDefaultParameters()
 	o.Source.Git.URI = "changed"
@@ -364,6 +348,28 @@ func TestValidateBuildUpdate(t *testing.T) {
 		T      field.ErrorType
 		F      string
 	}{
+		"cancelled": {
+			Old: &buildapi.Build{
+				ObjectMeta: kapi.ObjectMeta{Namespace: kapi.NamespaceDefault, Name: "my-build", ResourceVersion: "1"},
+				Spec:       newDefaultParameters(),
+			},
+			Update: &buildapi.Build{
+				ObjectMeta: kapi.ObjectMeta{Namespace: kapi.NamespaceDefault, Name: "my-build", ResourceVersion: "1"},
+				Spec:       cancelledBuildSpec(),
+			},
+		},
+		"uncancel": {
+			Old: &buildapi.Build{
+				ObjectMeta: kapi.ObjectMeta{Namespace: kapi.NamespaceDefault, Name: "my-build", ResourceVersion: "1"},
+				Spec:       cancelledBuildSpec(),
+			},
+			Update: &buildapi.Build{
+				ObjectMeta: kapi.ObjectMeta{Namespace: kapi.NamespaceDefault, Name: "my-build", ResourceVersion: "1"},
+				Spec:       newDefaultParameters(),
+			},
+			T: field.ErrorTypeInvalid,
+			F: "spec.cancelled",
+		},
 		"changed spec": {
 			Old: &buildapi.Build{
 				ObjectMeta: kapi.ObjectMeta{Namespace: kapi.NamespaceDefault, Name: "my-build", ResourceVersion: "1"},
@@ -436,6 +442,12 @@ func TestValidateBuildUpdate(t *testing.T) {
 
 	for k, v := range errorCases {
 		errs := ValidateBuildUpdate(v.Update, v.Old)
+		if len(v.T) == 0 {
+			if len(errs) > 0 {
+				t.Errorf("%s: unexpected failure %v", k, errs)
+			}
+			continue
+		}
 		if len(errs) == 0 {
 			t.Errorf("expected failure %s for %v", k, v.Update)
 			continue

pkg/build/client/clients.go

@@ -11,6 +11,10 @@ type BuildStatusUpdater interface {
 	UpdateStatus(namespace string, build *buildapi.Build) error
 }
 
+type BuildUpdater interface {
+	Update(namespace string, build *buildapi.Build) error
+}
+
 // BuildLister provides methods for listing the Builds.
 type BuildLister interface {
 	List(namespace string, opts kapi.ListOptions) (*buildapi.BuildList, error)
@@ -27,6 +31,12 @@ func NewOSClientBuildClient(client osclient.Interface) *OSClientBuildClient {
 }
 
 // Update updates builds using the OpenShift client.
+func (c OSClientBuildClient) Update(namespace string, build *buildapi.Build) error {
+	_, e := c.Client.Builds(namespace).Update(build)
+	return e
+}
+
+// UpdateStatus updates build status using the OpenShift client.
 func (c OSClientBuildClient) UpdateStatus(namespace string, build *buildapi.Build) error {
 	_, e := c.Client.Builds(namespace).UpdateStatus(build)
 	return e

pkg/build/controller/controller.go

@@ -103,7 +103,7 @@ func (bc *BuildController) HandleBuild(build *buildapi.Build) error {
 	}
 
 	// A cancelling event was triggered for the build, delete its pod and update build status.
-	if build.Status.Cancelled && build.Status.Phase != buildapi.BuildPhaseCancelled {
+	if build.Spec.Cancelled && build.Status.Phase != buildapi.BuildPhaseCancelled {
 		glog.V(5).Infof("Marking build %s/%s as cancelled", build.Namespace, build.Name)
 		if err := bc.CancelBuild(build); err != nil {
 			build.Status.Reason = buildapi.StatusReasonCancelBuildFailed
@@ -140,7 +140,7 @@ func (bc *BuildController) HandleBuild(build *buildapi.Build) error {
 // build.Message.
 func (bc *BuildController) nextBuildPhase(build *buildapi.Build) error {
 	// If a cancelling event was triggered for the build, update build status.
-	if build.Status.Cancelled {
+	if build.Spec.Cancelled {
 		glog.V(4).Infof("Cancelling build %s/%s.", build.Namespace, build.Name)
 		build.Status.Phase = buildapi.BuildPhaseCancelled
 		build.Status.Reason = ""
@@ -374,7 +374,7 @@ func (bc *BuildPodDeleteController) HandleBuildPodDeletion(pod *kapi.Pod) error
 	build := obj.(*buildapi.Build)
 
 	// If build was cancelled, we'll leave HandleBuild to update the build
-	if build.Status.Cancelled {
+	if build.Spec.Cancelled {
 		glog.V(4).Infof("Cancelation for build was already triggered, ignoring")
 		return nil
 	}

pkg/build/controller/controller_test.go

@@ -23,6 +23,12 @@ func (okc *okBuildStatusUpdater) UpdateStatus(namespace string, build *buildapi.
 	return nil
 }
 
+type okBuildUpdater struct{}
+
+func (okc *okBuildUpdater) Update(namespace string, build *buildapi.Build) error {
+	return nil
+}
+
 type okBuildLister struct{}
 
 func (okc *okBuildLister) List(namespace string, opts kapi.ListOptions) (*buildapi.BuildList, error) {
@@ -158,7 +164,7 @@ func mockBuildController() *BuildController {
 		BuildStrategy:      &okStrategy{},
 		ImageStreamClient:  &okImageStreamClient{},
 		Recorder:           &record.FakeRecorder{},
-		RunPolicies:        policy.GetAllRunPolicies(&okBuildLister{}, &okBuildStatusUpdater{}),
+		RunPolicies:        policy.GetAllRunPolicies(&okBuildLister{}, &okBuildUpdater{}, &okBuildStatusUpdater{}),
 	}
 }
 

pkg/build/controller/factory/factory.go

@@ -62,6 +62,7 @@ func limitedLogAndRetry(buildupdater buildclient.BuildStatusUpdater, maxTimeout
 type BuildControllerFactory struct {
 	OSClient            osclient.Interface
 	KubeClient          kclient.Interface
+	BuildUpdater        buildclient.BuildUpdater
 	BuildStatusUpdater  buildclient.BuildStatusUpdater
 	BuildLister         buildclient.BuildLister
 	DockerBuildStrategy *strategy.DockerBuildStrategy
@@ -85,7 +86,7 @@ func (factory *BuildControllerFactory) Create() controller.RunnableController {
 		BuildLister:        factory.BuildLister,
 		ImageStreamClient:  client,
 		PodManager:         client,
-		RunPolicies:        policy.GetAllRunPolicies(factory.BuildLister, factory.BuildStatusUpdater),
+		RunPolicies:        policy.GetAllRunPolicies(factory.BuildLister, factory.BuildUpdater, factory.BuildStatusUpdater),
 		BuildStrategy: &typeBasedFactoryStrategy{
 			DockerBuildStrategy: factory.DockerBuildStrategy,
 			SourceBuildStrategy: factory.SourceBuildStrategy,

pkg/build/controller/policy/policy.go

@@ -25,11 +25,11 @@ type RunPolicy interface {
 }
 
 // GetAllRunPolicies returns a set of all run policies.
-func GetAllRunPolicies(lister buildclient.BuildLister, updater buildclient.BuildStatusUpdater) []RunPolicy {
+func GetAllRunPolicies(lister buildclient.BuildLister, updater buildclient.BuildUpdater, statusUpdater buildclient.BuildStatusUpdater) []RunPolicy {
 	return []RunPolicy{
-		&ParallelPolicy{BuildLister: lister, BuildStatusUpdater: updater},
-		&SerialPolicy{BuildLister: lister, BuildStatusUpdater: updater},
-		&SerialLatestOnlyPolicy{BuildLister: lister, BuildStatusUpdater: updater},
+		&ParallelPolicy{BuildLister: lister, BuildStatusUpdater: statusUpdater},
+		&SerialPolicy{BuildLister: lister, BuildStatusUpdater: statusUpdater},
+		&SerialLatestOnlyPolicy{BuildLister: lister, BuildUpdater: updater},
 	}
 }
 

pkg/build/controller/policy/policy_test.go

@@ -25,6 +25,10 @@ func (f *fakeBuildClient) List(namespace string, opts kapi.ListOptions) (*builda
 }
 
 func (f *fakeBuildClient) UpdateStatus(namespace string, build *buildapi.Build) error {
+	return f.Update(namespace, build)
+}
+
+func (f *fakeBuildClient) Update(namespace string, build *buildapi.Build) error {
 	// Make sure every update fails at least once with conflict to ensure build updates are
 	// retried.
 	if f.updateErrCount == 0 {
@@ -67,7 +71,7 @@ func TestForBuild(t *testing.T) {
 		addBuild("build-3", "sample-bc", buildapi.BuildPhaseNew, buildapi.BuildRunPolicySerialLatestOnly),
 	}
 	client := newTestClient(builds)
-	policies := GetAllRunPolicies(client, client)
+	policies := GetAllRunPolicies(client, client, client)
 
 	if policy := ForBuild(&builds[0], policies); policy != nil {
 		if _, ok := policy.(*ParallelPolicy); !ok {

pkg/build/controller/policy/serial_latest_only.go

@@ -22,6 +22,7 @@ import (
 // expect that every commit is built.
 type SerialLatestOnlyPolicy struct {
 	BuildStatusUpdater buildclient.BuildStatusUpdater
+	BuildUpdater       buildclient.BuildUpdater
 	BuildLister        buildclient.BuildLister
 }
 
@@ -79,8 +80,8 @@ func (s *SerialLatestOnlyPolicy) cancelPreviousBuilds(build *buildapi.Build) []e
 	var result = []error{}
 	for _, b := range builds.Items {
 		err := wait.Poll(500*time.Millisecond, 5*time.Second, func() (bool, error) {
-			b.Status.Cancelled = true
-			err := s.BuildStatusUpdater.UpdateStatus(b.Namespace, &b)
+			b.Spec.Cancelled = true
+			err := s.BuildUpdater.Update(b.Namespace, &b)
 			if err != nil && errors.IsConflict(err) {
 				glog.V(5).Infof("Error cancelling build %s/%s: %v (will retry)", b.Namespace, b.Name, err)
 				return false, nil