From 85602fd5428efe4f6e08b7a540d06425ea6f4e2c Mon Sep 17 00:00:00 2001 From: Di Xu Date: Sun, 13 Aug 2017 20:10:25 +0800 Subject: [PATCH] CollisionCount should have type int32 across controllers that use it for collision avoidance --- pkg/apis/apps/fuzzer/fuzzer.go | 2 +- pkg/apis/apps/types.go | 2 +- pkg/apis/apps/v1beta2/conversion.go | 4 ++-- pkg/apis/apps/validation/validation_test.go | 9 +++++---- pkg/apis/extensions/types.go | 4 ++-- pkg/apis/extensions/validation/validation.go | 8 ++++---- .../extensions/validation/validation_test.go | 19 +++++++++++-------- pkg/controller/controller_utils.go | 4 ++-- pkg/controller/controller_utils_test.go | 13 ++++++++----- pkg/controller/daemon/update.go | 2 +- pkg/controller/deployment/sync.go | 4 ++-- .../deployment/util/replicaset_util.go | 2 +- staging/src/k8s.io/api/apps/v1beta1/types.go | 4 ++-- staging/src/k8s.io/api/apps/v1beta2/types.go | 6 +++--- .../k8s.io/api/extensions/v1beta1/types.go | 4 ++-- test/e2e/apps/deployment.go | 2 +- 16 files changed, 48 insertions(+), 41 deletions(-) diff --git a/pkg/apis/apps/fuzzer/fuzzer.go b/pkg/apis/apps/fuzzer/fuzzer.go index beb97dc1684..8013a273b01 100644 --- a/pkg/apis/apps/fuzzer/fuzzer.go +++ b/pkg/apis/apps/fuzzer/fuzzer.go @@ -44,7 +44,7 @@ var Funcs = func(codecs runtimeserializer.CodecFactory) []interface{} { s.Status.ObservedGeneration = new(int64) } if s.Status.CollisionCount == nil { - s.Status.CollisionCount = new(int64) + s.Status.CollisionCount = new(int32) } }, } diff --git a/pkg/apis/apps/types.go b/pkg/apis/apps/types.go index 9841c2ab4f6..5e2b010c730 100644 --- a/pkg/apis/apps/types.go +++ b/pkg/apis/apps/types.go @@ -192,7 +192,7 @@ type StatefulSetStatus struct { // uses this field as a collision avoidance mechanism when it needs to create the name for the // newest ControllerRevision. // +optional - CollisionCount *int64 + CollisionCount *int32 } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/pkg/apis/apps/v1beta2/conversion.go b/pkg/apis/apps/v1beta2/conversion.go index 01eb4528920..2e130eb2cd9 100644 --- a/pkg/apis/apps/v1beta2/conversion.go +++ b/pkg/apis/apps/v1beta2/conversion.go @@ -244,7 +244,7 @@ func Convert_v1beta2_StatefulSetStatus_To_apps_StatefulSetStatus(in *appsv1beta2 out.CurrentRevision = in.CurrentRevision out.UpdateRevision = in.UpdateRevision if in.CollisionCount != nil { - out.CollisionCount = new(int64) + out.CollisionCount = new(int32) *out.CollisionCount = *in.CollisionCount } return nil @@ -261,7 +261,7 @@ func Convert_apps_StatefulSetStatus_To_v1beta2_StatefulSetStatus(in *apps.Statef out.CurrentRevision = in.CurrentRevision out.UpdateRevision = in.UpdateRevision if in.CollisionCount != nil { - out.CollisionCount = new(int64) + out.CollisionCount = new(int32) *out.CollisionCount = *in.CollisionCount } return nil diff --git a/pkg/apis/apps/validation/validation_test.go b/pkg/apis/apps/validation/validation_test.go index 9071fa189f0..aeee6b391c8 100644 --- a/pkg/apis/apps/validation/validation_test.go +++ b/pkg/apis/apps/validation/validation_test.go @@ -302,7 +302,8 @@ func TestValidateStatefulSet(t *testing.T) { } func TestValidateStatefulSetStatus(t *testing.T) { - minusOne := int64(-1) + observedGenerationMinusOne := int64(-1) + collisionCountMinusOne := int32(-1) tests := []struct { name string replicas int32 @@ -310,7 +311,7 @@ func TestValidateStatefulSetStatus(t *testing.T) { currentReplicas int32 updatedReplicas int32 observedGeneration *int64 - collisionCount *int64 + collisionCount *int32 expectedErr bool }{ { @@ -359,7 +360,7 @@ func TestValidateStatefulSetStatus(t *testing.T) { readyReplicas: 3, currentReplicas: 2, updatedReplicas: 1, - observedGeneration: &minusOne, + observedGeneration: &observedGenerationMinusOne, expectedErr: true, }, { @@ -368,7 +369,7 @@ func TestValidateStatefulSetStatus(t *testing.T) { readyReplicas: 3, currentReplicas: 2, updatedReplicas: 1, - collisionCount: &minusOne, + collisionCount: &collisionCountMinusOne, expectedErr: true, }, { diff --git a/pkg/apis/extensions/types.go b/pkg/apis/extensions/types.go index 8e60c6f9ed9..55511f96f13 100644 --- a/pkg/apis/extensions/types.go +++ b/pkg/apis/extensions/types.go @@ -345,7 +345,7 @@ type DeploymentStatus struct { // field as a collision avoidance mechanism when it needs to create the name for the // newest ReplicaSet. // +optional - CollisionCount *int64 + CollisionCount *int32 } type DeploymentConditionType string @@ -519,7 +519,7 @@ type DaemonSetStatus struct { // uses this field as a collision avoidance mechanism when it needs to // create the name for the newest ControllerRevision. // +optional - CollisionCount *int64 + CollisionCount *int32 } // +genclient diff --git a/pkg/apis/extensions/validation/validation.go b/pkg/apis/extensions/validation/validation.go index d145fd421fb..6696d0b195e 100644 --- a/pkg/apis/extensions/validation/validation.go +++ b/pkg/apis/extensions/validation/validation.go @@ -95,7 +95,7 @@ func ValidateDaemonSetStatusUpdate(ds, oldDS *extensions.DaemonSet) field.ErrorL allErrs := apivalidation.ValidateObjectMetaUpdate(&ds.ObjectMeta, &oldDS.ObjectMeta, field.NewPath("metadata")) allErrs = append(allErrs, validateDaemonSetStatus(&ds.Status, field.NewPath("status"))...) if isDecremented(ds.Status.CollisionCount, oldDS.Status.CollisionCount) { - value := int64(0) + value := int32(0) if ds.Status.CollisionCount != nil { value = *ds.Status.CollisionCount } @@ -311,7 +311,7 @@ func ValidateDeploymentStatus(status *extensions.DeploymentStatus, fldPath *fiel allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(status.AvailableReplicas), fldPath.Child("availableReplicas"))...) allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(status.UnavailableReplicas), fldPath.Child("unavailableReplicas"))...) if status.CollisionCount != nil { - allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(*status.CollisionCount, fldPath.Child("collisionCount"))...) + allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(*status.CollisionCount), fldPath.Child("collisionCount"))...) } msg := "cannot be greater than status.replicas" if status.UpdatedReplicas > status.Replicas { @@ -342,7 +342,7 @@ func ValidateDeploymentStatusUpdate(update, old *extensions.Deployment) field.Er fldPath := field.NewPath("status") allErrs = append(allErrs, ValidateDeploymentStatus(&update.Status, fldPath)...) if isDecremented(update.Status.CollisionCount, old.Status.CollisionCount) { - value := int64(0) + value := int32(0) if update.Status.CollisionCount != nil { value = *update.Status.CollisionCount } @@ -352,7 +352,7 @@ func ValidateDeploymentStatusUpdate(update, old *extensions.Deployment) field.Er } // TODO: Move in "k8s.io/kubernetes/pkg/api/validation" -func isDecremented(update, old *int64) bool { +func isDecremented(update, old *int32) bool { if update == nil && old != nil { return true } diff --git a/pkg/apis/extensions/validation/validation_test.go b/pkg/apis/extensions/validation/validation_test.go index 5e6c6df8ee2..0afd286dcca 100644 --- a/pkg/apis/extensions/validation/validation_test.go +++ b/pkg/apis/extensions/validation/validation_test.go @@ -1238,6 +1238,7 @@ func int64p(i int) *int64 { } func TestValidateDeploymentStatus(t *testing.T) { + collisionCount := int32(-3) tests := []struct { name string @@ -1246,7 +1247,7 @@ func TestValidateDeploymentStatus(t *testing.T) { readyReplicas int32 availableReplicas int32 observedGeneration int64 - collisionCount *int64 + collisionCount *int32 expectedErr bool }{ @@ -1347,7 +1348,7 @@ func TestValidateDeploymentStatus(t *testing.T) { name: "invalid collisionCount", replicas: 3, observedGeneration: 1, - collisionCount: int64p(-3), + collisionCount: &collisionCount, expectedErr: true, }, } @@ -1371,6 +1372,8 @@ func TestValidateDeploymentStatus(t *testing.T) { } func TestValidateDeploymentStatusUpdate(t *testing.T) { + collisionCount := int32(1) + otherCollisionCount := int32(2) tests := []struct { name string @@ -1384,24 +1387,24 @@ func TestValidateDeploymentStatusUpdate(t *testing.T) { CollisionCount: nil, }, to: extensions.DeploymentStatus{ - CollisionCount: int64p(1), + CollisionCount: &collisionCount, }, expectedErr: false, }, { name: "stable: valid update", from: extensions.DeploymentStatus{ - CollisionCount: int64p(1), + CollisionCount: &collisionCount, }, to: extensions.DeploymentStatus{ - CollisionCount: int64p(1), + CollisionCount: &collisionCount, }, expectedErr: false, }, { name: "unset: invalid update", from: extensions.DeploymentStatus{ - CollisionCount: int64p(1), + CollisionCount: &collisionCount, }, to: extensions.DeploymentStatus{ CollisionCount: nil, @@ -1411,10 +1414,10 @@ func TestValidateDeploymentStatusUpdate(t *testing.T) { { name: "decrease: invalid update", from: extensions.DeploymentStatus{ - CollisionCount: int64p(2), + CollisionCount: &otherCollisionCount, }, to: extensions.DeploymentStatus{ - CollisionCount: int64p(1), + CollisionCount: &collisionCount, }, expectedErr: true, }, diff --git a/pkg/controller/controller_utils.go b/pkg/controller/controller_utils.go index d331ccf3f6e..f0ec6943bf2 100644 --- a/pkg/controller/controller_utils.go +++ b/pkg/controller/controller_utils.go @@ -1030,14 +1030,14 @@ func WaitForCacheSync(controllerName string, stopCh <-chan struct{}, cacheSyncs } // ComputeHash returns a hash value calculated from pod template and a collisionCount to avoid hash collision -func ComputeHash(template *v1.PodTemplateSpec, collisionCount *int64) uint32 { +func ComputeHash(template *v1.PodTemplateSpec, collisionCount *int32) uint32 { podTemplateSpecHasher := fnv.New32a() hashutil.DeepHashObject(podTemplateSpecHasher, *template) // Add collisionCount in the hash if it exists. if collisionCount != nil { collisionCountBytes := make([]byte, 8) - binary.LittleEndian.PutUint64(collisionCountBytes, uint64(*collisionCount)) + binary.LittleEndian.PutUint32(collisionCountBytes, uint32(*collisionCount)) podTemplateSpecHasher.Write(collisionCountBytes) } diff --git a/pkg/controller/controller_utils_test.go b/pkg/controller/controller_utils_test.go index 0b3842c5383..a72359585fa 100644 --- a/pkg/controller/controller_utils_test.go +++ b/pkg/controller/controller_utils_test.go @@ -453,23 +453,26 @@ func int64P(num int64) *int64 { } func TestComputeHash(t *testing.T) { + collisionCount := int32(1) + otherCollisionCount := int32(2) + maxCollisionCount := int32(math.MaxInt32) tests := []struct { name string template *v1.PodTemplateSpec - collisionCount *int64 - otherCollisionCount *int64 + collisionCount *int32 + otherCollisionCount *int32 }{ { name: "simple", template: &v1.PodTemplateSpec{}, - collisionCount: int64P(1), - otherCollisionCount: int64P(2), + collisionCount: &collisionCount, + otherCollisionCount: &otherCollisionCount, }, { name: "using math.MaxInt64", template: &v1.PodTemplateSpec{}, collisionCount: nil, - otherCollisionCount: int64P(int64(math.MaxInt64)), + otherCollisionCount: &maxCollisionCount, }, } diff --git a/pkg/controller/daemon/update.go b/pkg/controller/daemon/update.go index 72486594f0e..49ff90ce787 100644 --- a/pkg/controller/daemon/update.go +++ b/pkg/controller/daemon/update.go @@ -368,7 +368,7 @@ func (dsc *DaemonSetsController) snapshot(ds *extensions.DaemonSet, revision int return nil, getErr } if currDS.Status.CollisionCount == nil { - currDS.Status.CollisionCount = new(int64) + currDS.Status.CollisionCount = new(int32) } *currDS.Status.CollisionCount++ _, updateErr := dsc.kubeClient.ExtensionsV1beta1().DaemonSets(ds.Namespace).UpdateStatus(currDS) diff --git a/pkg/controller/deployment/sync.go b/pkg/controller/deployment/sync.go index a1a85a5a8f7..3731f21d329 100644 --- a/pkg/controller/deployment/sync.go +++ b/pkg/controller/deployment/sync.go @@ -160,7 +160,7 @@ func (dc *DeploymentController) rsAndPodsWithHashKeySynced(d *extensions.Deploym // 1. Add hash label to the rs's pod template, and make sure the controller sees this update so that no orphaned pods will be created // 2. Add hash label to all pods this rs owns, wait until replicaset controller reports rs.Status.FullyLabeledReplicas equal to the desired number of replicas // 3. Add hash label to the rs's label and selector -func (dc *DeploymentController) addHashKeyToRSAndPods(rs *extensions.ReplicaSet, podList *v1.PodList, collisionCount *int64) (*extensions.ReplicaSet, error) { +func (dc *DeploymentController) addHashKeyToRSAndPods(rs *extensions.ReplicaSet, podList *v1.PodList, collisionCount *int32) (*extensions.ReplicaSet, error) { // If the rs already has the new hash label in its selector, it's done syncing if labelsutil.SelectorHasLabel(rs.Spec.Selector, extensions.DefaultDeploymentUniqueLabelKey) { return rs, nil @@ -349,7 +349,7 @@ func (dc *DeploymentController) getNewReplicaSet(d *extensions.Deployment, rsLis // and requeue the Deployment. if !isEqual { if d.Status.CollisionCount == nil { - d.Status.CollisionCount = new(int64) + d.Status.CollisionCount = new(int32) } preCollisionCount := *d.Status.CollisionCount *d.Status.CollisionCount++ diff --git a/pkg/controller/deployment/util/replicaset_util.go b/pkg/controller/deployment/util/replicaset_util.go index f2a2fad1cf2..0b7c0d36917 100644 --- a/pkg/controller/deployment/util/replicaset_util.go +++ b/pkg/controller/deployment/util/replicaset_util.go @@ -70,7 +70,7 @@ func UpdateRSWithRetries(rsClient unversionedextensions.ReplicaSetInterface, rsL } // GetReplicaSetHash returns the pod template hash of a ReplicaSet's pod template space -func GetReplicaSetHash(rs *extensions.ReplicaSet, uniquifier *int64) (string, error) { +func GetReplicaSetHash(rs *extensions.ReplicaSet, uniquifier *int32) (string, error) { template, err := scheme.Scheme.DeepCopy(rs.Spec.Template) if err != nil { return "", err diff --git a/staging/src/k8s.io/api/apps/v1beta1/types.go b/staging/src/k8s.io/api/apps/v1beta1/types.go index 6ff3f8fdee1..36ac1e42296 100644 --- a/staging/src/k8s.io/api/apps/v1beta1/types.go +++ b/staging/src/k8s.io/api/apps/v1beta1/types.go @@ -244,7 +244,7 @@ type StatefulSetStatus struct { // uses this field as a collision avoidance mechanism when it needs to create the name for the // newest ControllerRevision. // +optional - CollisionCount *int64 `json:"collisionCount,omitempty" protobuf:"varint,9,opt,name=collisionCount"` + CollisionCount *int32 `json:"collisionCount,omitempty" protobuf:"varint,9,opt,name=collisionCount"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object @@ -445,7 +445,7 @@ type DeploymentStatus struct { // field as a collision avoidance mechanism when it needs to create the name for the // newest ReplicaSet. // +optional - CollisionCount *int64 `json:"collisionCount,omitempty" protobuf:"varint,8,opt,name=collisionCount"` + CollisionCount *int32 `json:"collisionCount,omitempty" protobuf:"varint,8,opt,name=collisionCount"` } type DeploymentConditionType string diff --git a/staging/src/k8s.io/api/apps/v1beta2/types.go b/staging/src/k8s.io/api/apps/v1beta2/types.go index d040d3de2a0..f1bebdccfa7 100644 --- a/staging/src/k8s.io/api/apps/v1beta2/types.go +++ b/staging/src/k8s.io/api/apps/v1beta2/types.go @@ -251,7 +251,7 @@ type StatefulSetStatus struct { // uses this field as a collision avoidance mechanism when it needs to create the name for the // newest ControllerRevision. // +optional - CollisionCount *int64 `json:"collisionCount,omitempty" protobuf:"varint,9,opt,name=collisionCount"` + CollisionCount *int32 `json:"collisionCount,omitempty" protobuf:"varint,9,opt,name=collisionCount"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object @@ -425,7 +425,7 @@ type DeploymentStatus struct { // field as a collision avoidance mechanism when it needs to create the name for the // newest ReplicaSet. // +optional - CollisionCount *int64 `json:"collisionCount,omitempty" protobuf:"varint,8,opt,name=collisionCount"` + CollisionCount *int32 `json:"collisionCount,omitempty" protobuf:"varint,8,opt,name=collisionCount"` } type DeploymentConditionType string @@ -598,7 +598,7 @@ type DaemonSetStatus struct { // uses this field as a collision avoidance mechanism when it needs to // create the name for the newest ControllerRevision. // +optional - CollisionCount *int64 `json:"collisionCount,omitempty" protobuf:"varint,9,opt,name=collisionCount"` + CollisionCount *int32 `json:"collisionCount,omitempty" protobuf:"varint,9,opt,name=collisionCount"` } // +genclient diff --git a/staging/src/k8s.io/api/extensions/v1beta1/types.go b/staging/src/k8s.io/api/extensions/v1beta1/types.go index 98bd22488c5..11300661fb1 100644 --- a/staging/src/k8s.io/api/extensions/v1beta1/types.go +++ b/staging/src/k8s.io/api/extensions/v1beta1/types.go @@ -345,7 +345,7 @@ type DeploymentStatus struct { // field as a collision avoidance mechanism when it needs to create the name for the // newest ReplicaSet. // +optional - CollisionCount *int64 `json:"collisionCount,omitempty" protobuf:"varint,8,opt,name=collisionCount"` + CollisionCount *int32 `json:"collisionCount,omitempty" protobuf:"varint,8,opt,name=collisionCount"` } type DeploymentConditionType string @@ -524,7 +524,7 @@ type DaemonSetStatus struct { // uses this field as a collision avoidance mechanism when it needs to // create the name for the newest ControllerRevision. // +optional - CollisionCount *int64 `json:"collisionCount,omitempty" protobuf:"varint,9,opt,name=collisionCount"` + CollisionCount *int32 `json:"collisionCount,omitempty" protobuf:"varint,9,opt,name=collisionCount"` } // +genclient diff --git a/test/e2e/apps/deployment.go b/test/e2e/apps/deployment.go index 7ba00e19978..79e723ef1f7 100644 --- a/test/e2e/apps/deployment.go +++ b/test/e2e/apps/deployment.go @@ -1449,7 +1449,7 @@ func testDeploymentHashCollisionAvoidance(f *framework.Framework) { return false, nil } framework.Logf(spew.Sprintf("deployment status: %#v", d.Status)) - return d.Status.CollisionCount != nil && *d.Status.CollisionCount == int64(1), nil + return d.Status.CollisionCount != nil && *d.Status.CollisionCount == int32(1), nil }); err != nil { framework.Failf("Failed to increment collision counter for deployment %q: %v", deploymentName, err) }