From f12aca35318520899c62fa6d781225bb8de09b94 Mon Sep 17 00:00:00 2001 From: Michail Kargakis Date: Thu, 4 Feb 2016 18:13:41 +0100 Subject: [PATCH] deployapi: add generation numbers Add generation numbers to deploymentconfigs. Helpful for decoupling latestVersion from oc --- api/swagger-spec/oapi-v1.json | 5 +++++ pkg/api/v1beta3/deep_copy_generated.go | 1 + pkg/deploy/api/deep_copy_generated.go | 1 + pkg/deploy/api/types.go | 2 ++ pkg/deploy/api/v1/conversion_generated.go | 2 ++ pkg/deploy/api/v1/deep_copy_generated.go | 1 + pkg/deploy/api/v1/types.go | 2 ++ pkg/deploy/api/v1beta3/types.go | 2 ++ pkg/deploy/api/validation/validation.go | 20 ++++++++++++----- .../controller/deploymentconfig/controller.go | 13 ++++++++--- pkg/deploy/registry/deployconfig/strategy.go | 22 +++++++++++++++++-- 11 files changed, 60 insertions(+), 11 deletions(-) diff --git a/api/swagger-spec/oapi-v1.json b/api/swagger-spec/oapi-v1.json index 5d8b076987ff..780ca32f6953 100644 --- a/api/swagger-spec/oapi-v1.json +++ b/api/swagger-spec/oapi-v1.json @@ -20531,6 +20531,11 @@ "details": { "$ref": "v1.DeploymentDetails", "description": "Details are the reasons for the update to this deployment config. This could be based on a change made by the user or caused by an automatic trigger" + }, + "observedGeneration": { + "type": "integer", + "format": "int64", + "description": "ObservedGeneration is the most recent generation observed by the controller." } } }, diff --git a/pkg/api/v1beta3/deep_copy_generated.go b/pkg/api/v1beta3/deep_copy_generated.go index ce7aa3675321..9a76b2e25208 100644 --- a/pkg/api/v1beta3/deep_copy_generated.go +++ b/pkg/api/v1beta3/deep_copy_generated.go @@ -1611,6 +1611,7 @@ func deepCopy_v1beta3_DeploymentConfigStatus(in deployapiv1beta3.DeploymentConfi } else { out.Details = nil } + out.ObservedGeneration = in.ObservedGeneration return nil } diff --git a/pkg/deploy/api/deep_copy_generated.go b/pkg/deploy/api/deep_copy_generated.go index 71958ab3d9c8..6417e48c4dd9 100644 --- a/pkg/deploy/api/deep_copy_generated.go +++ b/pkg/deploy/api/deep_copy_generated.go @@ -192,6 +192,7 @@ func DeepCopy_api_DeploymentConfigStatus(in DeploymentConfigStatus, out *Deploym } else { out.Details = nil } + out.ObservedGeneration = in.ObservedGeneration return nil } diff --git a/pkg/deploy/api/types.go b/pkg/deploy/api/types.go index 7d621324abf0..152da0144f51 100644 --- a/pkg/deploy/api/types.go +++ b/pkg/deploy/api/types.go @@ -313,6 +313,8 @@ type DeploymentConfigStatus struct { // Details are the reasons for the update to this deployment config. // This could be based on a change made by the user or caused by an automatic trigger Details *DeploymentDetails + // ObservedGeneration is the most recent generation observed by the controller. + ObservedGeneration int64 } // DeploymentTriggerPolicy describes a policy for a single trigger that results in a new deployment. diff --git a/pkg/deploy/api/v1/conversion_generated.go b/pkg/deploy/api/v1/conversion_generated.go index ef8bac1bc142..73be54eac1f0 100644 --- a/pkg/deploy/api/v1/conversion_generated.go +++ b/pkg/deploy/api/v1/conversion_generated.go @@ -477,6 +477,7 @@ func autoConvert_v1_DeploymentConfigStatus_To_api_DeploymentConfigStatus(in *Dep } else { out.Details = nil } + out.ObservedGeneration = in.ObservedGeneration return nil } @@ -498,6 +499,7 @@ func autoConvert_api_DeploymentConfigStatus_To_v1_DeploymentConfigStatus(in *dep } else { out.Details = nil } + out.ObservedGeneration = in.ObservedGeneration return nil } diff --git a/pkg/deploy/api/v1/deep_copy_generated.go b/pkg/deploy/api/v1/deep_copy_generated.go index af2ca90db53a..457af964fe6d 100644 --- a/pkg/deploy/api/v1/deep_copy_generated.go +++ b/pkg/deploy/api/v1/deep_copy_generated.go @@ -191,6 +191,7 @@ func DeepCopy_v1_DeploymentConfigStatus(in DeploymentConfigStatus, out *Deployme } else { out.Details = nil } + out.ObservedGeneration = in.ObservedGeneration return nil } diff --git a/pkg/deploy/api/v1/types.go b/pkg/deploy/api/v1/types.go index 3022bcf6e36a..18aec38b7686 100644 --- a/pkg/deploy/api/v1/types.go +++ b/pkg/deploy/api/v1/types.go @@ -281,6 +281,8 @@ type DeploymentConfigStatus struct { // Details are the reasons for the update to this deployment config. // This could be based on a change made by the user or caused by an automatic trigger Details *DeploymentDetails `json:"details,omitempty"` + // ObservedGeneration is the most recent generation observed by the controller. + ObservedGeneration int64 `json:"observedGeneration,omitempty"` } // DeploymentTriggerPolicy describes a policy for a single trigger that results in a new deployment. diff --git a/pkg/deploy/api/v1beta3/types.go b/pkg/deploy/api/v1beta3/types.go index a0007860041b..707dfff32fc2 100644 --- a/pkg/deploy/api/v1beta3/types.go +++ b/pkg/deploy/api/v1beta3/types.go @@ -296,6 +296,8 @@ type DeploymentConfigStatus struct { // The reasons for the update to this deployment config. // This could be based on a change made by the user or caused by an automatic trigger Details *DeploymentDetails `json:"details,omitempty"` + // ObservedGeneration is the most recent generation observed by the controller. + ObservedGeneration int64 `json:"observedGeneration,omitempty"` } // DeploymentTriggerPolicy describes a policy for a single trigger that results in a new deployment. diff --git a/pkg/deploy/api/validation/validation.go b/pkg/deploy/api/validation/validation.go index 96a321bf82d8..a128e907dd7c 100644 --- a/pkg/deploy/api/validation/validation.go +++ b/pkg/deploy/api/validation/validation.go @@ -21,30 +21,35 @@ func ValidateDeploymentConfig(config *deployapi.DeploymentConfig) field.ErrorLis allErrs := validation.ValidateObjectMeta(&config.ObjectMeta, true, validation.NameIsDNSSubdomain, field.NewPath("metadata")) // TODO: Refactor to validate spec and status separately + specPath := field.NewPath("spec") for i := range config.Spec.Triggers { - allErrs = append(allErrs, validateTrigger(&config.Spec.Triggers[i], field.NewPath("spec", "triggers").Index(i))...) + allErrs = append(allErrs, validateTrigger(&config.Spec.Triggers[i], specPath.Child("triggers").Index(i))...) } var spec *kapi.PodSpec if config.Spec.Template != nil { spec = &config.Spec.Template.Spec } - specPath := field.NewPath("spec") - allErrs = append(allErrs, validateDeploymentStrategy(&config.Spec.Strategy, spec, field.NewPath("spec", "strategy"))...) + allErrs = append(allErrs, validateDeploymentStrategy(&config.Spec.Strategy, spec, specPath.Child("strategy"))...) if config.Spec.Template == nil { allErrs = append(allErrs, field.Required(specPath.Child("template"), "")) } else { allErrs = append(allErrs, validation.ValidatePodTemplateSpec(config.Spec.Template, specPath.Child("template"))...) } - if config.Status.LatestVersion < 0 { - allErrs = append(allErrs, field.Invalid(field.NewPath("status", "latestVersion"), config.Status.LatestVersion, "latestVersion cannot be negative")) - } if config.Spec.Replicas < 0 { allErrs = append(allErrs, field.Invalid(specPath.Child("replicas"), config.Spec.Replicas, "replicas cannot be negative")) } if len(config.Spec.Selector) == 0 { allErrs = append(allErrs, field.Invalid(specPath.Child("selector"), config.Spec.Selector, "selector cannot be empty")) } + + statusPath := field.NewPath("status") + if config.Status.LatestVersion < 0 { + allErrs = append(allErrs, field.Invalid(statusPath.Child("latestVersion"), config.Status.LatestVersion, "latestVersion cannot be negative")) + } + if config.Status.ObservedGeneration < int64(0) { + allErrs = append(allErrs, field.Invalid(statusPath.Child("observedGeneration"), config.Status.ObservedGeneration, "observedGeneration cannot be negative")) + } return allErrs } @@ -57,6 +62,9 @@ func ValidateDeploymentConfigUpdate(newConfig *deployapi.DeploymentConfig, oldCo } else if newConfig.Status.LatestVersion > (oldConfig.Status.LatestVersion + 1) { allErrs = append(allErrs, field.Invalid(statusPath.Child("latestVersion"), newConfig.Status.LatestVersion, "latestVersion can only be incremented by 1")) } + if newConfig.Status.ObservedGeneration < oldConfig.Status.ObservedGeneration { + allErrs = append(allErrs, field.Invalid(statusPath.Child("observedGeneration"), newConfig.Status.ObservedGeneration, "observedGeneration cannot be decremented")) + } return allErrs } diff --git a/pkg/deploy/controller/deploymentconfig/controller.go b/pkg/deploy/controller/deploymentconfig/controller.go index 97908bac4fb0..e3cfd4ed7cb9 100644 --- a/pkg/deploy/controller/deploymentconfig/controller.go +++ b/pkg/deploy/controller/deploymentconfig/controller.go @@ -136,7 +136,10 @@ func (c *DeploymentConfigController) Handle(config *deployapi.DeploymentConfig) return fmt.Errorf("couldn't create deployment for deployment config %s: %v", deployutil.LabelForDeploymentConfig(config), err) } c.recorder.Eventf(config, kapi.EventTypeNormal, "DeploymentCreated", "Created new deployment %q for version %d", created.Name, config.Status.LatestVersion) - return nil + // Supposedly we have to update the dc status here; will get the separate status call with https://github.com/openshift/origin/pull/6479 + config.Status.ObservedGeneration = int(config.Generation) + _, err = c.osClient.DeploymentConfigs(config.Namespace).Update(config) + return err } // reconcileDeployments reconciles existing deployment replica counts which @@ -219,7 +222,8 @@ func (c *DeploymentConfigController) reconcileDeployments(existingDeployments *k default: oldReplicas := config.Spec.Replicas config.Spec.Replicas = activeReplicas - _, err := c.osClient.DeploymentConfigs(config.Namespace).Update(config) + var err error + config, err = c.osClient.DeploymentConfigs(config.Namespace).Update(config) if err != nil { return err } @@ -260,5 +264,8 @@ func (c *DeploymentConfigController) reconcileDeployments(existingDeployments *k } } } - return nil + // Supposedly we have to update the dc status here; will get the separate status call with https://github.com/openshift/origin/pull/6479 + config.Status.ObservedGeneration = int(config.Generation) + _, err := c.osClient.DeploymentConfigs(config.Namespace).Update(config) + return err } diff --git a/pkg/deploy/registry/deployconfig/strategy.go b/pkg/deploy/registry/deployconfig/strategy.go index aeca26739c99..cd497def5161 100644 --- a/pkg/deploy/registry/deployconfig/strategy.go +++ b/pkg/deploy/registry/deployconfig/strategy.go @@ -2,6 +2,7 @@ package deployconfig import ( "fmt" + "reflect" kapi "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/fields" @@ -39,14 +40,31 @@ func (strategy) AllowUnconditionalUpdate() bool { // PrepareForCreate clears fields that are not allowed to be set by end users on creation. func (strategy) PrepareForCreate(obj runtime.Object) { - _ = obj.(*api.DeploymentConfig) + dc := obj.(*api.DeploymentConfig) // TODO: need to ensure status.latestVersion is not set out of order + dc.Generation = 1 } // PrepareForUpdate clears fields that are not allowed to be set by end users on update. func (strategy) PrepareForUpdate(obj, old runtime.Object) { - _ = obj.(*api.DeploymentConfig) + oldDc := obj.(*api.DeploymentConfig) + newDc := old.(*api.DeploymentConfig) // TODO: need to ensure status.latestVersion is not set out of order + + // Any changes to the spec increment the generation number, any changes to the + // status should reflect the generation number of the corresponding object. We push + // the burden of managing the status onto the clients because we can't (in general) + // know here what version of spec the writer of the status has seen. It may seem like + // we can at first -- since obj contains spec -- but in the future we will probably make + // status its own object, and even if we don't, writes may be the result of a + // read-update-write loop, so the contents of spec may not actually be the spec that + // the controller has *seen*. + // + // TODO: Any changes to a part of the object that represents desired state (labels, + // annotations etc) should also increment the generation. + if !reflect.DeepEqual(oldDc.Spec, newDc.Spec) { + newDc.Generation = oldDc.Generation + 1 + } } // Canonicalize normalizes the object after validation.