From 8e2347370e8a7b6823a0fc0cc38077cb74593f14 Mon Sep 17 00:00:00 2001 From: Marcin Date: Tue, 8 Nov 2016 12:27:09 +0100 Subject: [PATCH] Add observedGeneration to PodDisruptionBudgetStatus --- pkg/apis/policy/types.go | 27 ++++++++++------- pkg/apis/policy/v1beta1/types.go | 29 +++++++++++-------- pkg/controller/disruption/disruption.go | 4 ++- pkg/controller/disruption/disruption_test.go | 4 ++- pkg/registry/core/pod/etcd/eviction.go | 3 ++ .../poddisruptionbudget/etcd/etcd_test.go | 11 +++++-- .../policy/poddisruptionbudget/strategy.go | 5 ++-- 7 files changed, 54 insertions(+), 29 deletions(-) diff --git a/pkg/apis/policy/types.go b/pkg/apis/policy/types.go index 1255c322cef..b9f298f0e65 100644 --- a/pkg/apis/policy/types.go +++ b/pkg/apis/policy/types.go @@ -40,17 +40,10 @@ type PodDisruptionBudgetSpec struct { // PodDisruptionBudgetStatus represents information about the status of a // PodDisruptionBudget. Status may trail the actual state of a system. type PodDisruptionBudgetStatus struct { - // Number of pod disruptions that are currently allowed. - PodDisruptionsAllowed int32 `json:"disruptionsAllowed"` - - // current number of healthy pods - CurrentHealthy int32 `json:"currentHealthy"` - - // minimum desired number of healthy pods - DesiredHealthy int32 `json:"desiredHealthy"` - - // total number of pods counted by this disruption budget - ExpectedPods int32 `json:"expectedPods"` + // Most recent generation observed when updating this PDB status. PodDisruptionsAllowed and other + // status informatio is valid only if observedGeneration equals to PDB's object generation. + // +optional + ObservedGeneration int64 `json:"observedGeneration,omitempty"` // DisruptedPods contains information about pods whose eviction was // processed by the API server eviction subresource handler but has not @@ -64,6 +57,18 @@ type PodDisruptionBudgetStatus struct { // If everything goes smooth this map should be empty for the most of the time. // Large number of entries in the map may indicate problems with pod deletions. DisruptedPods map[string]unversioned.Time `json:"disruptedPods" protobuf:"bytes,5,rep,name=disruptedPods"` + + // Number of pod disruptions that are currently allowed. + PodDisruptionsAllowed int32 `json:"disruptionsAllowed"` + + // current number of healthy pods + CurrentHealthy int32 `json:"currentHealthy"` + + // minimum desired number of healthy pods + DesiredHealthy int32 `json:"desiredHealthy"` + + // total number of pods counted by this disruption budget + ExpectedPods int32 `json:"expectedPods"` } // +genclient=true diff --git a/pkg/apis/policy/v1beta1/types.go b/pkg/apis/policy/v1beta1/types.go index 46c7e7d1b3d..0e7cff85725 100644 --- a/pkg/apis/policy/v1beta1/types.go +++ b/pkg/apis/policy/v1beta1/types.go @@ -38,17 +38,10 @@ type PodDisruptionBudgetSpec struct { // PodDisruptionBudgetStatus represents information about the status of a // PodDisruptionBudget. Status may trail the actual state of a system. type PodDisruptionBudgetStatus struct { - // Number of pod disruptions that are currently allowed. - PodDisruptionsAllowed int32 `json:"disruptionsAllowed" protobuf:"varint,1,opt,name=disruptionsAllowed"` - - // current number of healthy pods - CurrentHealthy int32 `json:"currentHealthy" protobuf:"varint,2,opt,name=currentHealthy"` - - // minimum desired number of healthy pods - DesiredHealthy int32 `json:"desiredHealthy" protobuf:"varint,3,opt,name=desiredHealthy"` - - // total number of pods counted by this disruption budget - ExpectedPods int32 `json:"expectedPods" protobuf:"varint,4,opt,name=expectedPods"` + // Most recent generation observed when updating this PDB status. PodDisruptionsAllowed and other + // status informatio is valid only if observedGeneration equals to PDB's object generation. + // +optional + ObservedGeneration int64 `json:"observedGeneration,omitempty" protobuf:"varint,1,opt,name=observedGeneration"` // DisruptedPods contains information about pods whose eviction was // processed by the API server eviction subresource handler but has not @@ -61,7 +54,19 @@ type PodDisruptionBudgetStatus struct { // the list automatically by PodDisruptionBudget controller after some time. // If everything goes smooth this map should be empty for the most of the time. // Large number of entries in the map may indicate problems with pod deletions. - DisruptedPods map[string]unversioned.Time `json:"disruptedPods" protobuf:"bytes,5,rep,name=disruptedPods"` + DisruptedPods map[string]unversioned.Time `json:"disruptedPods" protobuf:"bytes,2,rep,name=disruptedPods"` + + // Number of pod disruptions that are currently allowed. + PodDisruptionsAllowed int32 `json:"disruptionsAllowed" protobuf:"varint,3,opt,name=disruptionsAllowed"` + + // current number of healthy pods + CurrentHealthy int32 `json:"currentHealthy" protobuf:"varint,4,opt,name=currentHealthy"` + + // minimum desired number of healthy pods + DesiredHealthy int32 `json:"desiredHealthy" protobuf:"varint,5,opt,name=desiredHealthy"` + + // total number of pods counted by this disruption budget + ExpectedPods int32 `json:"expectedPods" protobuf:"varint,6,opt,name=expectedPods"` } // +genclient=true diff --git a/pkg/controller/disruption/disruption.go b/pkg/controller/disruption/disruption.go index 9c0ddaa176b..45ff073d30c 100644 --- a/pkg/controller/disruption/disruption.go +++ b/pkg/controller/disruption/disruption.go @@ -669,7 +669,8 @@ func (dc *DisruptionController) updatePdbStatus(pdb *policy.PodDisruptionBudget, pdb.Status.DesiredHealthy == desiredHealthy && pdb.Status.ExpectedPods == expectedCount && pdb.Status.PodDisruptionsAllowed == disruptionsAllowed && - reflect.DeepEqual(pdb.Status.DisruptedPods, disruptedPods) { + reflect.DeepEqual(pdb.Status.DisruptedPods, disruptedPods) && + pdb.Status.ObservedGeneration == pdb.Generation { return nil } @@ -685,6 +686,7 @@ func (dc *DisruptionController) updatePdbStatus(pdb *policy.PodDisruptionBudget, ExpectedPods: expectedCount, PodDisruptionsAllowed: disruptionsAllowed, DisruptedPods: disruptedPods, + ObservedGeneration: pdb.Generation, } return dc.getUpdater()(&newPdb) diff --git a/pkg/controller/disruption/disruption_test.go b/pkg/controller/disruption/disruption_test.go index a02d26a21ba..84157304e98 100644 --- a/pkg/controller/disruption/disruption_test.go +++ b/pkg/controller/disruption/disruption_test.go @@ -58,14 +58,16 @@ func (ps *pdbStates) Get(key string) policy.PodDisruptionBudget { func (ps *pdbStates) VerifyPdbStatus(t *testing.T, key string, disruptionsAllowed, currentHealthy, desiredHealthy, expectedPods int32, disruptedPodMap map[string]unversioned.Time) { + actualPDB := ps.Get(key) expectedStatus := policy.PodDisruptionBudgetStatus{ PodDisruptionsAllowed: disruptionsAllowed, CurrentHealthy: currentHealthy, DesiredHealthy: desiredHealthy, ExpectedPods: expectedPods, DisruptedPods: disruptedPodMap, + ObservedGeneration: actualPDB.Generation, } - actualStatus := ps.Get(key).Status + actualStatus := actualPDB.Status if !reflect.DeepEqual(actualStatus, expectedStatus) { debug.PrintStack() t.Fatalf("PDB %q status mismatch. Expected %+v but got %+v.", key, expectedStatus, actualStatus) diff --git a/pkg/registry/core/pod/etcd/eviction.go b/pkg/registry/core/pod/etcd/eviction.go index dbe7aed9dc8..312bdfdf884 100644 --- a/pkg/registry/core/pod/etcd/eviction.go +++ b/pkg/registry/core/pod/etcd/eviction.go @@ -116,6 +116,9 @@ func (r *EvictionREST) Create(ctx api.Context, obj runtime.Object) (runtime.Obje } func (r *EvictionREST) checkAndDecrement(namespace string, podName string, pdb policy.PodDisruptionBudget) (ok bool, err error) { + if pdb.Status.ObservedGeneration != pdb.Generation { + return false, nil + } if pdb.Status.PodDisruptionsAllowed < 0 { return false, fmt.Errorf("pdb disruptions allowed is negative") } diff --git a/pkg/registry/policy/poddisruptionbudget/etcd/etcd_test.go b/pkg/registry/policy/poddisruptionbudget/etcd/etcd_test.go index 18098dc5442..c955cd6c339 100644 --- a/pkg/registry/policy/poddisruptionbudget/etcd/etcd_test.go +++ b/pkg/registry/policy/poddisruptionbudget/etcd/etcd_test.go @@ -91,8 +91,15 @@ func TestStatusUpdate(t *testing.T) { if err := storage.Storage.Create(ctx, key, validPodDisruptionBudget, nil, 0); err != nil { t.Fatalf("unexpected error: %v", err) } + + obj, err := storage.Get(ctx, "foo") + if err != nil { + t.Fatalf("failed to get pdb: %v", err) + } + obtainedPdb := obj.(*policy.PodDisruptionBudget) + update := policy.PodDisruptionBudget{ - ObjectMeta: validPodDisruptionBudget.ObjectMeta, + ObjectMeta: obtainedPdb.ObjectMeta, Spec: policy.PodDisruptionBudgetSpec{ MinAvailable: intstr.FromInt(8), }, @@ -104,7 +111,7 @@ func TestStatusUpdate(t *testing.T) { if _, _, err := statusStorage.Update(ctx, update.Name, rest.DefaultUpdatedObjectInfo(&update, api.Scheme)); err != nil { t.Fatalf("unexpected error: %v", err) } - obj, err := storage.Get(ctx, "foo") + obj, err = storage.Get(ctx, "foo") if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/pkg/registry/policy/poddisruptionbudget/strategy.go b/pkg/registry/policy/poddisruptionbudget/strategy.go index 04e853adc10..0be465ddc4c 100644 --- a/pkg/registry/policy/poddisruptionbudget/strategy.go +++ b/pkg/registry/policy/poddisruptionbudget/strategy.go @@ -91,9 +91,10 @@ func (podDisruptionBudgetStrategy) ValidateUpdate(ctx api.Context, obj, old runt return append(validationErrorList, updateErrorList...) } -// AllowUnconditionalUpdate is the default update policy for PodDisruptionBudget objects. +// AllowUnconditionalUpdate is the default update policy for PodDisruptionBudget objects. Status update should +// only be allowed if version match. func (podDisruptionBudgetStrategy) AllowUnconditionalUpdate() bool { - return true + return false } // PodDisruptionBudgetToSelectableFields returns a field set that represents the object.