From 9ed9bd1c4ffe766acb6720326671feb33af4ef3b Mon Sep 17 00:00:00 2001 From: Prashanth Balasubramanian Date: Thu, 18 Jun 2015 12:00:19 -0700 Subject: [PATCH] Add a generation number to the object meta of all objects, and status of rcs --- api/swagger-spec/v1.json | 10 ++++ api/swagger-spec/v1beta3.json | 10 ++++ cmd/integration/integration.go | 6 ++- hack/test-cmd.sh | 9 ++++ pkg/api/deep_copy_generated.go | 2 + pkg/api/types.go | 7 +++ pkg/api/v1/conversion_generated.go | 4 ++ pkg/api/v1/deep_copy_generated.go | 2 + pkg/api/v1/types.go | 8 +++ pkg/api/v1beta3/conversion_generated.go | 4 ++ pkg/api/v1beta3/deep_copy_generated.go | 2 + pkg/api/v1beta3/types.go | 7 +++ pkg/api/validation/validation.go | 4 +- pkg/client/conditions.go | 11 ++++- pkg/controller/controller_utils.go | 18 +++++-- pkg/controller/replication_controller_test.go | 26 ++++++++-- pkg/kubectl/rolling_updater_test.go | 22 +++++++-- pkg/kubectl/scale.go | 5 +- pkg/kubectl/stop_test.go | 6 +-- pkg/registry/controller/etcd/etcd_test.go | 49 +++++++++++++++++++ pkg/registry/controller/rest.go | 21 ++++++++ 21 files changed, 212 insertions(+), 21 deletions(-) diff --git a/api/swagger-spec/v1.json b/api/swagger-spec/v1.json index 1398011cb3e..35391f1bf7b 100644 --- a/api/swagger-spec/v1.json +++ b/api/swagger-spec/v1.json @@ -10828,6 +10828,11 @@ "type": "string", "description": "string that identifies the internal version of this object that can be used by clients to determine when objects have changed; populated by the system, read-only; value must be treated as opaque by clients and passed unmodified back to the server: http://docs.k8s.io/api-conventions.md#concurrency-control-and-consistency" }, + "generation": { + "type": "integer", + "format": "int64", + "description": "a sequence number representing a specific generation of the desired state; populated by the system; read-only" + }, "creationTimestamp": { "type": "string", "description": "RFC 3339 date and time at which the object was created; populated by the system, read-only; null for lists" @@ -12955,6 +12960,11 @@ "type": "integer", "format": "int32", "description": "most recently oberved number of replicas" + }, + "observedGeneration": { + "type": "integer", + "format": "int64", + "description": "reflects the generation of the most recently observed replication controller" } } }, diff --git a/api/swagger-spec/v1beta3.json b/api/swagger-spec/v1beta3.json index 54ef2117bbd..42c02827882 100644 --- a/api/swagger-spec/v1beta3.json +++ b/api/swagger-spec/v1beta3.json @@ -10828,6 +10828,11 @@ "type": "string", "description": "string that identifies the internal version of this object that can be used by clients to determine when objects have changed; populated by the system, read-only; value must be treated as opaque by clients and passed unmodified back to the server: http://docs.k8s.io/api-conventions.md#concurrency-control-and-consistency" }, + "generation": { + "type": "integer", + "format": "int64", + "description": "a sequence number representing a specific generation of the desired state; populated by the system; read-only" + }, "creationTimestamp": { "type": "string", "description": "RFC 3339 date and time at which the object was created; populated by the system, read-only; null for lists" @@ -12957,6 +12962,11 @@ "type": "integer", "format": "int32", "description": "most recently oberved number of replicas" + }, + "observedGeneration": { + "type": "integer", + "format": "int64", + "description": "reflects the generation of the most recently observed replication controller" } } }, diff --git a/cmd/integration/integration.go b/cmd/integration/integration.go index 093c08f614a..7e72a0cff03 100644 --- a/cmd/integration/integration.go +++ b/cmd/integration/integration.go @@ -395,8 +395,10 @@ func runReplicationControllerTest(c *client.Client) { } glog.Infof("Done creating replication controllers") - // Give the controllers some time to actually create the pods - if err := wait.Poll(time.Second, time.Second*30, client.ControllerHasDesiredReplicas(c, updated)); err != nil { + // In practice the controller doesn't need 60s to create a handful of pods, but network latencies on CI + // systems have been observed to vary unpredictably, so give the controller enough time to create pods. + // Our e2e scalability tests will catch controllers that are *actually* slow. + if err := wait.Poll(time.Second, time.Second*60, client.ControllerHasDesiredReplicas(c, updated)); err != nil { glog.Fatalf("FAILED: pods never created %v", err) } diff --git a/hack/test-cmd.sh b/hack/test-cmd.sh index 5a0e47b13e2..d0c99d39c53 100755 --- a/hack/test-cmd.sh +++ b/hack/test-cmd.sh @@ -485,6 +485,15 @@ __EOF__ kube::log::status "Testing kubectl(${version}:replicationcontrollers)" + ### Create and stop controller, make sure it doesn't leak pods + # Pre-condition: no replication controller is running + kube::test::get_object_assert rc "{{range.items}}{{$id_field}}:{{end}}" '' + # Command + kubectl create -f examples/guestbook/frontend-controller.json "${kube_flags[@]}" + kubectl stop rc frontend "${kube_flags[@]}" + # Post-condition: no pods from frontend controller + kube::test::get_object_assert 'pods -l "name=frontend"' "{{range.items}}{{$id_field}}:{{end}}" '' + ### Create replication controller frontend from JSON # Pre-condition: no replication controller is running kube::test::get_object_assert rc "{{range.items}}{{$id_field}}:{{end}}" '' diff --git a/pkg/api/deep_copy_generated.go b/pkg/api/deep_copy_generated.go index 0bce691a3c1..1d45137a1de 100644 --- a/pkg/api/deep_copy_generated.go +++ b/pkg/api/deep_copy_generated.go @@ -991,6 +991,7 @@ func deepCopy_api_ObjectMeta(in ObjectMeta, out *ObjectMeta, c *conversion.Clone out.SelfLink = in.SelfLink out.UID = in.UID out.ResourceVersion = in.ResourceVersion + out.Generation = in.Generation if err := deepCopy_util_Time(in.CreationTimestamp, &out.CreationTimestamp, c); err != nil { return err } @@ -1598,6 +1599,7 @@ func deepCopy_api_ReplicationControllerSpec(in ReplicationControllerSpec, out *R func deepCopy_api_ReplicationControllerStatus(in ReplicationControllerStatus, out *ReplicationControllerStatus, c *conversion.Cloner) error { out.Replicas = in.Replicas + out.ObservedGeneration = in.ObservedGeneration return nil } diff --git a/pkg/api/types.go b/pkg/api/types.go index aad515a7b74..81e4c897217 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -117,6 +117,10 @@ type ObjectMeta struct { // resource or set of resources. Only servers will generate resource versions. ResourceVersion string `json:"resourceVersion,omitempty"` + // A sequence number representing a specific generation of the desired state. + // Currently only implemented by replication controllers. + Generation int64 `json:"generation,omitempty"` + // CreationTimestamp is a timestamp representing the server time when this object was // created. It is not guaranteed to be set in happens-before order across separate operations. // Clients may not set this value. It is represented in RFC3339 form and is in UTC. @@ -998,6 +1002,9 @@ type ReplicationControllerSpec struct { type ReplicationControllerStatus struct { // Replicas is the number of actual replicas. Replicas int `json:"replicas"` + + // ObservedGeneration is the most recent generation observed by the controller. + ObservedGeneration int64 `json:"observedGeneration,omitempty"` } // ReplicationController represents the configuration of a replication controller. diff --git a/pkg/api/v1/conversion_generated.go b/pkg/api/v1/conversion_generated.go index 4f7ceb1ea3c..f97d15032ea 100644 --- a/pkg/api/v1/conversion_generated.go +++ b/pkg/api/v1/conversion_generated.go @@ -1077,6 +1077,7 @@ func convert_api_ObjectMeta_To_v1_ObjectMeta(in *api.ObjectMeta, out *ObjectMeta out.SelfLink = in.SelfLink out.UID = in.UID out.ResourceVersion = in.ResourceVersion + out.Generation = in.Generation if err := s.Convert(&in.CreationTimestamp, &out.CreationTimestamp, 0); err != nil { return err } @@ -1743,6 +1744,7 @@ func convert_api_ReplicationControllerStatus_To_v1_ReplicationControllerStatus(i defaulting.(func(*api.ReplicationControllerStatus))(in) } out.Replicas = in.Replicas + out.ObservedGeneration = in.ObservedGeneration return nil } @@ -3385,6 +3387,7 @@ func convert_v1_ObjectMeta_To_api_ObjectMeta(in *ObjectMeta, out *api.ObjectMeta out.SelfLink = in.SelfLink out.UID = in.UID out.ResourceVersion = in.ResourceVersion + out.Generation = in.Generation if err := s.Convert(&in.CreationTimestamp, &out.CreationTimestamp, 0); err != nil { return err } @@ -4051,6 +4054,7 @@ func convert_v1_ReplicationControllerStatus_To_api_ReplicationControllerStatus(i defaulting.(func(*ReplicationControllerStatus))(in) } out.Replicas = in.Replicas + out.ObservedGeneration = in.ObservedGeneration return nil } diff --git a/pkg/api/v1/deep_copy_generated.go b/pkg/api/v1/deep_copy_generated.go index 2366b4c9106..8160172a712 100644 --- a/pkg/api/v1/deep_copy_generated.go +++ b/pkg/api/v1/deep_copy_generated.go @@ -922,6 +922,7 @@ func deepCopy_v1_ObjectMeta(in ObjectMeta, out *ObjectMeta, c *conversion.Cloner out.SelfLink = in.SelfLink out.UID = in.UID out.ResourceVersion = in.ResourceVersion + out.Generation = in.Generation if err := deepCopy_util_Time(in.CreationTimestamp, &out.CreationTimestamp, c); err != nil { return err } @@ -1534,6 +1535,7 @@ func deepCopy_v1_ReplicationControllerSpec(in ReplicationControllerSpec, out *Re func deepCopy_v1_ReplicationControllerStatus(in ReplicationControllerStatus, out *ReplicationControllerStatus, c *conversion.Cloner) error { out.Replicas = in.Replicas + out.ObservedGeneration = in.ObservedGeneration return nil } diff --git a/pkg/api/v1/types.go b/pkg/api/v1/types.go index cd168e5fc9e..e39456d7054 100644 --- a/pkg/api/v1/types.go +++ b/pkg/api/v1/types.go @@ -115,6 +115,10 @@ type ObjectMeta struct { // resource or set of resources. Only servers will generate resource versions. ResourceVersion string `json:"resourceVersion,omitempty" description:"string that identifies the internal version of this object that can be used by clients to determine when objects have changed; populated by the system, read-only; value must be treated as opaque by clients and passed unmodified back to the server: http://docs.k8s.io/api-conventions.md#concurrency-control-and-consistency"` + // A sequence number representing a specific generation of the desired state. + // Currently only implemented by replication controllers. + Generation int64 `json:"generation,omitempty" description:"a sequence number representing a specific generation of the desired state; populated by the system; read-only"` + // CreationTimestamp is a timestamp representing the server time when this object was // created. It is not guaranteed to be set in happens-before order across separate operations. // Clients may not set this value. It is represented in RFC3339 form and is in UTC. @@ -1001,11 +1005,15 @@ type ReplicationControllerSpec struct { type ReplicationControllerStatus struct { // Replicas is the number of actual replicas. Replicas int `json:"replicas" description:"most recently oberved number of replicas"` + + // ObservedGeneration is the most recent generation observed by the controller. + ObservedGeneration int64 `json:"observedGeneration,omitempty" description:"reflects the generation of the most recently observed replication controller"` } // ReplicationController represents the configuration of a replication controller. type ReplicationController struct { TypeMeta `json:",inline"` + // If the Labels of a ReplicationController are empty, they are defaulted to be the same as the Pod(s) that the replication controller manages. ObjectMeta `json:"metadata,omitempty" description:"standard object metadata; see http://docs.k8s.io/api-conventions.md#metadata"` diff --git a/pkg/api/v1beta3/conversion_generated.go b/pkg/api/v1beta3/conversion_generated.go index 3fdac31ac19..4bd9fbc59bb 100644 --- a/pkg/api/v1beta3/conversion_generated.go +++ b/pkg/api/v1beta3/conversion_generated.go @@ -935,6 +935,7 @@ func convert_api_ObjectMeta_To_v1beta3_ObjectMeta(in *api.ObjectMeta, out *Objec out.SelfLink = in.SelfLink out.UID = in.UID out.ResourceVersion = in.ResourceVersion + out.Generation = in.Generation if err := s.Convert(&in.CreationTimestamp, &out.CreationTimestamp, 0); err != nil { return err } @@ -1563,6 +1564,7 @@ func convert_api_ReplicationControllerStatus_To_v1beta3_ReplicationControllerSta defaulting.(func(*api.ReplicationControllerStatus))(in) } out.Replicas = in.Replicas + out.ObservedGeneration = in.ObservedGeneration return nil } @@ -2997,6 +2999,7 @@ func convert_v1beta3_ObjectMeta_To_api_ObjectMeta(in *ObjectMeta, out *api.Objec out.SelfLink = in.SelfLink out.UID = in.UID out.ResourceVersion = in.ResourceVersion + out.Generation = in.Generation if err := s.Convert(&in.CreationTimestamp, &out.CreationTimestamp, 0); err != nil { return err } @@ -3625,6 +3628,7 @@ func convert_v1beta3_ReplicationControllerStatus_To_api_ReplicationControllerSta defaulting.(func(*ReplicationControllerStatus))(in) } out.Replicas = in.Replicas + out.ObservedGeneration = in.ObservedGeneration return nil } diff --git a/pkg/api/v1beta3/deep_copy_generated.go b/pkg/api/v1beta3/deep_copy_generated.go index ebc9df77517..1394bfa061e 100644 --- a/pkg/api/v1beta3/deep_copy_generated.go +++ b/pkg/api/v1beta3/deep_copy_generated.go @@ -926,6 +926,7 @@ func deepCopy_v1beta3_ObjectMeta(in ObjectMeta, out *ObjectMeta, c *conversion.C out.SelfLink = in.SelfLink out.UID = in.UID out.ResourceVersion = in.ResourceVersion + out.Generation = in.Generation if err := deepCopy_util_Time(in.CreationTimestamp, &out.CreationTimestamp, c); err != nil { return err } @@ -1533,6 +1534,7 @@ func deepCopy_v1beta3_ReplicationControllerSpec(in ReplicationControllerSpec, ou func deepCopy_v1beta3_ReplicationControllerStatus(in ReplicationControllerStatus, out *ReplicationControllerStatus, c *conversion.Cloner) error { out.Replicas = in.Replicas + out.ObservedGeneration = in.ObservedGeneration return nil } diff --git a/pkg/api/v1beta3/types.go b/pkg/api/v1beta3/types.go index 0b457416c68..63323dad7df 100644 --- a/pkg/api/v1beta3/types.go +++ b/pkg/api/v1beta3/types.go @@ -115,6 +115,10 @@ type ObjectMeta struct { // resource or set of resources. Only servers will generate resource versions. ResourceVersion string `json:"resourceVersion,omitempty" description:"string that identifies the internal version of this object that can be used by clients to determine when objects have changed; populated by the system, read-only; value must be treated as opaque by clients and passed unmodified back to the server: http://docs.k8s.io/api-conventions.md#concurrency-control-and-consistency"` + // A sequence number representing a specific generation of the desired state. + // Currently only implemented by replication controllers. + Generation int64 `json:"generation,omitempty" description:"a sequence number representing a specific generation of the desired state; populated by the system; read-only"` + // CreationTimestamp is a timestamp representing the server time when this object was // created. It is not guaranteed to be set in happens-before order across separate operations. // Clients may not set this value. It is represented in RFC3339 form and is in UTC. @@ -1005,6 +1009,9 @@ type ReplicationControllerSpec struct { type ReplicationControllerStatus struct { // Replicas is the number of actual replicas. Replicas int `json:"replicas" description:"most recently oberved number of replicas"` + + // ObservedGeneration is the most recent generation observed by the controller. + ObservedGeneration int64 `json:"observedGeneration,omitempty" description:"reflects the generation of the most recently observed replication controller"` } // ReplicationController represents the configuration of a replication controller. diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 2a5445b75c7..5e84690d49a 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -220,7 +220,9 @@ func ValidateObjectMeta(meta *api.ObjectMeta, requiresNamespace bool, nameFn Val allErrs = append(allErrs, errs.NewFieldInvalid("name", meta.Name, qualifier)) } } - + if meta.Generation < 0 { + allErrs = append(allErrs, errs.NewFieldInvalid("generation", meta.Generation, isNegativeErrorMsg)) + } if requiresNamespace { if len(meta.Namespace) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("namespace")) diff --git a/pkg/client/conditions.go b/pkg/client/conditions.go index 458238dc2b0..c42ed810ca2 100644 --- a/pkg/client/conditions.go +++ b/pkg/client/conditions.go @@ -24,11 +24,20 @@ import ( // ControllerHasDesiredReplicas returns a condition that will be true iff the desired replica count // for a controller's ReplicaSelector equals the Replicas count. func ControllerHasDesiredReplicas(c Interface, controller *api.ReplicationController) wait.ConditionFunc { + + // If we're given a controller where the status lags the spec, it either means that the controller is stale, + // or that the rc manager hasn't noticed the update yet. Polling status.Replicas is not safe in the latter case. + desiredGeneration := controller.Generation + return func() (bool, error) { ctrl, err := c.ReplicationControllers(controller.Namespace).Get(controller.Name) if err != nil { return false, err } - return ctrl.Status.Replicas == ctrl.Spec.Replicas, nil + // There's a chance a concurrent update modifies the Spec.Replicas causing this check to pass, + // or, after this check has passed, a modification causes the rc manager to create more pods. + // This will not be an issue once we've implemented graceful delete for rcs, but till then + // concurrent stop operations on the same rc might have unintended side effects. + return ctrl.Status.ObservedGeneration >= desiredGeneration && ctrl.Status.Replicas == ctrl.Spec.Replicas, nil } } diff --git a/pkg/controller/controller_utils.go b/pkg/controller/controller_utils.go index 53954232dcd..f32403f3c7c 100644 --- a/pkg/controller/controller_utils.go +++ b/pkg/controller/controller_utils.go @@ -304,14 +304,24 @@ func filterActivePods(pods []api.Pod) []*api.Pod { // updateReplicaCount attempts to update the Status.Replicas of the given controller, with a single GET/PUT retry. func updateReplicaCount(rcClient client.ReplicationControllerInterface, controller api.ReplicationController, numReplicas int) (updateErr error) { // This is the steady state. It happens when the rc doesn't have any expectations, since - // we do a periodic relist every 30s. - if controller.Status.Replicas == numReplicas { + // we do a periodic relist every 30s. If the generations differ but the replicas are + // the same, a caller might've resized to the same replica count. + if controller.Status.Replicas == numReplicas && + controller.Generation == controller.Status.ObservedGeneration { return nil } + // Save the generation number we acted on, otherwise we might wrongfully indicate + // that we've seen a spec update when we retry. + // TODO: This can clobber an update if we allow multiple agents to write to the + // same status. + generation := controller.Generation + var getErr error - glog.V(4).Infof("Updating replica count for rc: %v, %d->%d", controller.Name, controller.Status.Replicas, numReplicas) for i, rc := 0, &controller; ; i++ { - rc.Status.Replicas = numReplicas + glog.V(4).Infof("Updating replica count for rc: %v, %d->%d (need %d), sequence No: %v->%v", + controller.Name, controller.Status.Replicas, numReplicas, controller.Spec.Replicas, controller.Status.ObservedGeneration, generation) + + rc.Status = api.ReplicationControllerStatus{Replicas: numReplicas, ObservedGeneration: generation} _, updateErr = rcClient.Update(rc) if updateErr == nil || i >= updateRetries { return updateErr diff --git a/pkg/controller/replication_controller_test.go b/pkg/controller/replication_controller_test.go index f6af8f8dbdd..88ec1c953b7 100644 --- a/pkg/controller/replication_controller_test.go +++ b/pkg/controller/replication_controller_test.go @@ -339,7 +339,7 @@ func TestCreateReplica(t *testing.T) { } } -func TestControllerNoReplicaUpdate(t *testing.T) { +func TestStatusUpdatesWithoutReplicasChange(t *testing.T) { // Setup a fake server to listen for requests, and run the rc manager in steady state fakeHandler := util.FakeHandler{ StatusCode: 200, @@ -365,6 +365,18 @@ func TestControllerNoReplicaUpdate(t *testing.T) { if fakeHandler.RequestReceived != nil { t.Errorf("Unexpected update when pods and rcs are in a steady state") } + + // This response body is just so we don't err out decoding the http response, all + // we care about is the request body sent below. + response := runtime.EncodeOrDie(testapi.Codec(), &api.ReplicationController{}) + fakeHandler.ResponseBody = response + + rc.Generation = rc.Generation + 1 + manager.syncReplicationController(getKey(rc, t)) + + rc.Status.ObservedGeneration = rc.Generation + updatedRc := runtime.EncodeOrDie(testapi.Codec(), rc) + fakeHandler.ValidateRequest(t, testapi.ResourcePath(replicationControllerResourceName(), rc.Namespace, rc.Name), "PUT", &updatedRc) } func TestControllerUpdateReplicas(t *testing.T) { @@ -383,9 +395,12 @@ func TestControllerUpdateReplicas(t *testing.T) { // Status.Replica should update to match number of pods in system, 1 new pod should be created. rc := newReplicationController(5) manager.controllerStore.Store.Add(rc) - rc.Status = api.ReplicationControllerStatus{Replicas: 2} + rc.Status = api.ReplicationControllerStatus{Replicas: 2, ObservedGeneration: 0} + rc.Generation = 1 newPodList(manager.podStore.Store, 4, api.PodRunning, rc) - response := runtime.EncodeOrDie(testapi.Codec(), rc) + + // This response body is just so we don't err out decoding the http response + response := runtime.EncodeOrDie(testapi.Codec(), &api.ReplicationController{}) fakeHandler.ResponseBody = response fakePodControl := FakePodControl{} @@ -393,8 +408,9 @@ func TestControllerUpdateReplicas(t *testing.T) { manager.syncReplicationController(getKey(rc, t)) - // Status.Replicas should go up from 2->4 even though we created 5-4=1 pod - rc.Status = api.ReplicationControllerStatus{Replicas: 4} + // 1. Status.Replicas should go up from 2->4 even though we created 5-4=1 pod. + // 2. Every update to the status should include the Generation of the spec. + rc.Status = api.ReplicationControllerStatus{Replicas: 4, ObservedGeneration: 1} decRc := runtime.EncodeOrDie(testapi.Codec(), rc) fakeHandler.ValidateRequest(t, testapi.ResourcePath(replicationControllerResourceName(), rc.Namespace, rc.Name), "PUT", &decRc) diff --git a/pkg/kubectl/rolling_updater_test.go b/pkg/kubectl/rolling_updater_test.go index ced7e6f3329..79ba4bdd7d5 100644 --- a/pkg/kubectl/rolling_updater_test.go +++ b/pkg/kubectl/rolling_updater_test.go @@ -142,7 +142,9 @@ func TestUpdate(t *testing.T) { []fakeResponse{ // no existing newRc {nil, fmt.Errorf("not found")}, - // 3 gets for each scale + // 4 gets for each scale + {newRc(1, 1), nil}, + {newRc(1, 1), nil}, {newRc(1, 1), nil}, {newRc(1, 1), nil}, {newRc(1, 1), nil}, @@ -164,7 +166,10 @@ Update succeeded. Deleting foo-v1 []fakeResponse{ // no existing newRc {nil, fmt.Errorf("not found")}, - // 3 gets for each scale + // 4 gets for each scale + {newRc(1, 2), nil}, + {newRc(1, 2), nil}, + {newRc(1, 2), nil}, {newRc(1, 2), nil}, {newRc(1, 2), nil}, {newRc(1, 2), nil}, @@ -195,7 +200,10 @@ Update succeeded. Deleting foo-v1 []fakeResponse{ // no existing newRc {nil, fmt.Errorf("not found")}, - // 3 gets for each scale + // 4 gets for each scale + {newRc(1, 2), nil}, + {newRc(1, 2), nil}, + {newRc(1, 2), nil}, {newRc(1, 2), nil}, {newRc(1, 2), nil}, {newRc(1, 2), nil}, @@ -214,6 +222,7 @@ Update succeeded. Deleting foo-v1 {newRc(7, 7), nil}, {newRc(7, 7), nil}, {newRc(7, 7), nil}, + {newRc(7, 7), nil}, // cleanup annotations {newRc(7, 7), nil}, {newRc(7, 7), nil}, @@ -229,7 +238,10 @@ Update succeeded. Deleting foo-v1 []fakeResponse{ // no existing newRc {nil, fmt.Errorf("not found")}, - // 3 gets for each update + // 4 gets for each update + {newRc(1, 2), nil}, + {newRc(1, 2), nil}, + {newRc(1, 2), nil}, {newRc(1, 2), nil}, {newRc(1, 2), nil}, {newRc(1, 2), nil}, @@ -247,6 +259,8 @@ Update succeeded. Deleting foo-v1 // stop oldRc {oldRc(0), nil}, {oldRc(0), nil}, + {oldRc(0), nil}, + {oldRc(0), nil}, // cleanup annotations {newRc(2, 2), nil}, {newRc(2, 2), nil}, diff --git a/pkg/kubectl/scale.go b/pkg/kubectl/scale.go index 8891ea440ec..80064caa957 100644 --- a/pkg/kubectl/scale.go +++ b/pkg/kubectl/scale.go @@ -161,7 +161,10 @@ func (scaler *ReplicationControllerScaler) Scale(namespace, name string, newSize return err } if waitForReplicas != nil { - rc := &api.ReplicationController{ObjectMeta: api.ObjectMeta{Namespace: namespace, Name: name}} + rc, err := scaler.c.GetReplicationController(namespace, name) + if err != nil { + return err + } return wait.Poll(waitForReplicas.Interval, waitForReplicas.Timeout, scaler.c.ControllerHasDesiredReplicas(rc)) } diff --git a/pkg/kubectl/stop_test.go b/pkg/kubectl/stop_test.go index bf04294c9c4..6991817812d 100644 --- a/pkg/kubectl/stop_test.go +++ b/pkg/kubectl/stop_test.go @@ -42,10 +42,10 @@ func TestReplicationControllerStop(t *testing.T) { if s != expected { t.Errorf("expected %s, got %s", expected, s) } - if len(fake.Actions) != 5 { - t.Errorf("unexpected actions: %v, expected 4 actions (get, get, update, get, delete)", fake.Actions) + if len(fake.Actions) != 6 { + t.Errorf("unexpected actions: %v, expected 6 actions (get, get, update, get, get, delete)", fake.Actions) } - for i, action := range []string{"get", "get", "update", "get", "delete"} { + for i, action := range []string{"get", "get", "update", "get", "get", "delete"} { if fake.Actions[i].Action != action+"-replicationController" { t.Errorf("unexpected action: %v, expected %s-replicationController", fake.Actions[i], action) } diff --git a/pkg/registry/controller/etcd/etcd_test.go b/pkg/registry/controller/etcd/etcd_test.go index 6b9fdc15242..325530e6c7d 100644 --- a/pkg/registry/controller/etcd/etcd_test.go +++ b/pkg/registry/controller/etcd/etcd_test.go @@ -280,6 +280,55 @@ func TestEtcdControllerValidatesNamespaceOnUpdate(t *testing.T) { } } +func TestGenerationNumber(t *testing.T) { + storage, _ := newStorage(t) + modifiedSno := validController + modifiedSno.Generation = 100 + modifiedSno.Status.ObservedGeneration = 10 + ctx := api.NewDefaultContext() + rc, err := createController(storage, modifiedSno, t) + ctrl, err := storage.Get(ctx, rc.Name) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + controller, _ := ctrl.(*api.ReplicationController) + + // Generation initialization + if controller.Generation != 1 && controller.Status.ObservedGeneration != 0 { + t.Fatalf("Unexpected generation number %v, status generation %v", controller.Generation, controller.Status.ObservedGeneration) + } + + // Updates to spec should increment the generation number + controller.Spec.Replicas += 1 + storage.Update(ctx, controller) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + ctrl, err = storage.Get(ctx, rc.Name) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + controller, _ = ctrl.(*api.ReplicationController) + if controller.Generation != 2 || controller.Status.ObservedGeneration != 0 { + t.Fatalf("Unexpected generation, spec: %v, status: %v", controller.Generation, controller.Status.ObservedGeneration) + } + + // Updates to status should not increment either spec or status generation numbers + controller.Status.Replicas += 1 + storage.Update(ctx, controller) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + ctrl, err = storage.Get(ctx, rc.Name) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + controller, _ = ctrl.(*api.ReplicationController) + if controller.Generation != 2 || controller.Status.ObservedGeneration != 0 { + t.Fatalf("Unexpected generation number, spec: %v, status: %v", controller.Generation, controller.Status.ObservedGeneration) + } +} + // TestEtcdGetControllerDifferentNamespace ensures same-name controllers in different namespaces do not clash func TestEtcdGetControllerDifferentNamespace(t *testing.T) { storage, fakeClient := newStorage(t) diff --git a/pkg/registry/controller/rest.go b/pkg/registry/controller/rest.go index a89cc07d566..34997067695 100644 --- a/pkg/registry/controller/rest.go +++ b/pkg/registry/controller/rest.go @@ -18,6 +18,7 @@ package controller import ( "fmt" + "reflect" "strconv" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" @@ -47,6 +48,9 @@ func (rcStrategy) NamespaceScoped() bool { func (rcStrategy) PrepareForCreate(obj runtime.Object) { controller := obj.(*api.ReplicationController) controller.Status = api.ReplicationControllerStatus{} + + controller.Generation = 1 + controller.Status.ObservedGeneration = 0 } // PrepareForUpdate clears fields that are not allowed to be set by end users on update. @@ -55,6 +59,23 @@ func (rcStrategy) PrepareForUpdate(obj, old runtime.Object) { //newController := obj.(*api.ReplicationController) //oldController := old.(*api.ReplicationController) //newController.Status = oldController.Status + newController := obj.(*api.ReplicationController) + oldController := old.(*api.ReplicationController) + + // 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(oldController.Spec, newController.Spec) { + newController.Generation = oldController.Generation + 1 + } } // Validate validates a new replication controller.