Fix kubectl stop rc with sequence numbers #9739

bprashanth · 2015-06-12T20:49:33Z

Introduces the concept of sequence numbers for rcs fixing the behavior of kubectl stop rc in the process.
@lavalamp @bgrant0607 #9147

Still need to think of a good description string and look at why the conversion generating script doesn't autogenerate for rc.Spec changes (https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/api/v1/conversion.go#L30)

smarterclayton · 2015-06-12T22:01:43Z

Is sequenceNumber the right name? I assume this a pattern we'll use in the future for other reasons.

lavalamp · 2015-06-12T22:06:28Z

pkg/registry/controller/rest.go

@@ -47,14 +48,26 @@ func (rcStrategy) NamespaceScoped() bool {
 func (rcStrategy) PrepareForCreate(obj runtime.Object) {
 	controller := obj.(*api.ReplicationController)
 	controller.Status = api.ReplicationControllerStatus{}
+	controller.Spec.SequenceNumber = 0
+	controller.Spec.SequenceNumber = 0


oh, i meant status

Arguably this should not be here at all; it looks like defaulting, so it should go in defaulting code, but 0 is the default for a number anyway so you don't really need to do anything?

Hmmm, the intent here is to override the specification in rc.json, if any

oh-- gotcha. I think it's not important to do that though-- does it cause any harm if it starts out with seq 42?

Why allow it? I think people might be tempted to update if we allow initialization (the only use case i can think of is cloning all the keys by getting and reputting to a diffferent apiserver). I'm fine defaulting the SequenceNumber to the answer to life the universe and everything, if that's what you meant :)

lavalamp · 2015-06-12T22:13:14Z

@smarterclayton Yeah, it's a general pattern. SequenceNumber is the most obvious name, but that doesn't mean there's not a better name possible :)

I think the interesting debate to have about it is:
a) Does it go in metadata? (IMO: yes)
b) Do we implement it for all objects? (IMO: eventually, yes-- but it doesn't have to work everywhere immediately)
c) Does this imply that there should only be a single writer of obj.Status? (IMO: Yes! This should force us to clarify our rapidly-increasing-in-complexity web of object readers & writers.)

lavalamp · 2015-06-12T22:16:47Z

The basic idea, of course, is to give you some idea about what version of the object is being used by the object's actuator, so that you can--in a generic way--tell when the system has quiesced.

@bgrant0607 I've forgotten why we can't just have the actuator just reiterate the resourceVersion that it's acting on? Is this only because updates to status also increase the resourceVersion, or is there another reason, too?

lavalamp · 2015-06-12T22:22:01Z

pkg/api/types.go

@@ -998,14 +998,16 @@ type ReplicationControllerSpec struct {
 	// insufficient replicas are detected. Internally, this takes precedence over a
 	// TemplateRef.
 	// Must be set before converting to a v1beta1 or v1beta2 API object.
-	Template *PodTemplateSpec `json:"template,omitempty"`
+	Template       *PodTemplateSpec `json:"template,omitempty"`
+	SequenceNumber int              `json:"sequenceNumber"`


int64-- no ints in the API, they are variable-width.

k8s-bot · 2015-06-12T22:22:35Z

GCE e2e build/test passed for commit 379916efde76ee47ffb825884aea63416dcc940f.

lavalamp · 2015-06-12T22:24:42Z

pkg/registry/controller/rest.go

+	// Any changes to the spec increment the sequence number, any changes to the
+	// status reflect the sequence number of the corresponding spec
+	if !reflect.DeepEqual(oldController.Spec, newController.Spec) {
+		newController.Spec.SequenceNumber += 1


must be newController.Spec.SequenceNumber = oldController.Spec.SequenceNumber + 1 otherwise it's gameable.

sure that's easy enough to do, but so is the resource version right (this is the logic i've mostly applied when thinking about what to validate etc)? if someone tries to game it, all bets are off

resourceVersion is generated by etcd & isn't gameable.

I mean if someone doesn't re-get but just iterates through all the rvs
I agree it's different in that etcd will also reject if it's different

That's a different kind of gaming. We can't ultimately stop a client that's determined to get it wrong. But we can ignore the seq # they pass in :)

lavalamp · 2015-06-12T22:29:51Z

And adding to my earlier list--
d) Some objects may be dependent on other objects. E.g., arguably endpoints should be using the sequence number from the service object.

...now that I think about it, endpoints should really be a subresource of services, or part of service.status.

bprashanth · 2015-06-12T22:59:13Z

I've forgotten why we can't just have the actuator just reiterate the resourceVersion that it's acting on? Is this only because updates to status also increase the resourceVersion, or is there another reason, too?

I can't think into the future enough to say it's safe to compare the resource version, even of a single resource, against itself. can we assume a future version of the same resource will have a higher resource version even if its from a different member/shard? The sequence number going up property is useful when a client wants to know the controller has seen an update, but doesn't care if it's seen many updates after that.

smarterclayton · 2015-06-12T23:29:27Z

On Jun 12, 2015, at 6:13 PM, Daniel Smith notifications@github.com wrote:

@smarterclayton Yeah, it's a general pattern. SequenceNumber is the most obvious name, but that doesn't mean there's not a better name possible :)

I think the interesting debate to have about it is:
a) Does it go in metadata? (IMO: yes)

Agree, unless we have a need to subdivide the object into multiple sets, but it's likely those objects probably need a different pattern anyway.
b) Do we implemented for all objects? (IMO: eventually, yes-- but it doesn't have to work everywhere immediately)

Or an object which doesn't have an obvious actuator just doesn't expose it in status. For instance, a service isn't actuated so much as propagated.
c) Does this imply that there should only be a single writer of obj.Status? (IMO: Yes! This should force us to clarify our rapidly-increasing-in-complexity web of object readers & writers.)

—
Reply to this email directly or view it on GitHub.

lavalamp · 2015-06-12T23:35:13Z

The sequence number going up property is useful when a client wants to know the controller has seen an update, but doesn't care if it's seen many updates after that.

Ah, yes, thank you-- I forgot about that part. We want to allow >=, not just ==.

derekwaynecarr · 2015-06-13T02:05:14Z

pkg/api/v1/types.go

 }

 // ReplicationControllerStatus represents the current status of a replication
 // controller.
 type ReplicationControllerStatus struct {
 	// Replicas is the number of actual replicas.
-	Replicas int `json:"replicas" description:"most recently oberved number of replicas"`
+	Replicas       int `json:"replicas" description:"most recently oberved number of replicas"`
+	SequenceNumber int `json:"sequenceNumber" description:""`


missing descriptions in swagger

I figured people will change what i write anyway, so I'd just leave it blank till someone says: write this :)

bprashanth · 2015-06-15T19:17:58Z

PTAL, moved the Status.SequenceNumber code into the controller manager and added some more unittests

k8s-bot · 2015-06-15T19:35:01Z

GCE e2e build/test passed for commit cc767d3e32c4eb06f62d63eca2abe8a6157cdead.

k8s-bot · 2015-06-15T19:47:41Z

GCE e2e build/test passed for commit 915d36b0f848225ec6d8acde693e4b36f8a74bba.

k8s-bot · 2015-06-15T22:06:00Z

GCE e2e build/test passed for commit 7bb39e4a2a2fb3f703087850454d7f847c3266cf.

lavalamp · 2015-06-15T23:05:44Z

pkg/controller/controller_utils.go

 	for i, rc := 0, &controller; ; i++ {
-		rc.Status.Replicas = numReplicas
+


nit: extra blank line

lavalamp · 2015-06-15T23:15:23Z

I think this looks pretty good. I'd like to nail down whether sequenceNumber should go in metadata or in spec before we merge it, though.

smarterclayton · 2015-06-15T23:19:16Z

I haven't thought of a concrete reason to need two sequence numbers that couldn't be solved in spec directly (container sequence, for instance). If we add this to metadata we probably want to define it for objects which don't have clear specs (something like a user record, or an event). Also, probably need to clarify whether it applies to spec for things like nodes (answer is probably yes, but spec is a bit less clear for that type).

On Jun 15, 2015, at 7:15 PM, Daniel Smith notifications@github.com wrote:

I think this looks pretty good. I'd like to nail down whether sequenceNumber should go in metadata or in spec before we merge it, though.

—
Reply to this email directly or view it on GitHub.

0xmichalis · 2015-06-16T13:15:48Z

pkg/api/types.go

@@ -998,14 +998,16 @@ type ReplicationControllerSpec struct {
 	// insufficient replicas are detected. Internally, this takes precedence over a
 	// TemplateRef.
 	// Must be set before converting to a v1beta1 or v1beta2 API object.
-	Template *PodTemplateSpec `json:"template,omitempty"`
+	Template       *PodTemplateSpec `json:"template,omitempty"`
+	SequenceNumber int64            `json:"sequenceNumber"`


@bprashanth mind adding a comment for this, since we are under api?

bgrant0607 · 2015-06-18T00:19:29Z

See also #7328

bprashanth · 2015-06-18T22:29:51Z

'Pologies for the delay, got sidetracked debugging an unrelated e2e failure to confirm it's not this change. Might still update some of the comments/descriptions but my latest set of commits has a Generation embedded in the object. PTAL.

k8s-bot · 2015-06-18T22:44:29Z

GCE e2e build/test passed for commit f75736f48e4553ccf218482756986b2bdd391252.

k8s-bot · 2015-06-19T07:15:37Z

GCE e2e build/test passed for commit 52f559bb6c7f9229746b1e7da871c2a3dd5a8dd7.

k8s-bot · 2015-06-19T07:19:03Z

GCE e2e build/test passed for commit b69e033a80d2c6462679a87e0673857426bbb667.

k8s-bot · 2015-06-19T16:05:32Z

GCE e2e build/test passed for commit 79760a54a00839cb3ae2bdd96c4473b487fed9a7.

k8s-bot · 2015-06-19T18:41:00Z

GCE e2e build/test passed for commit 66edecf930f16439ada47e29b9f32f62e1330024.

bgrant0607 · 2015-06-19T19:24:35Z

pkg/api/v1/types.go

@@ -1001,11 +1004,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:"generation,omitempty" description:"reflects the generation of the most recently observed replication controller"`


json: observedGeneration

bgrant0607 · 2015-06-19T19:29:51Z

Looks pretty good. Just a couple minor comments.

bgrant0607 · 2015-06-19T19:31:47Z

Please regenerate the swagger so that shippable can run.
hack/update-swagger-spec.sh

bprashanth · 2015-06-19T20:37:34Z

I did run update-swagger a few times (and the new commits ended up triggering k8s bot as well), I'll need to dig into why that doesn't seem to have worked

bprashanth · 2015-06-19T20:53:01Z

PTAL, I'll figure out the swagger bug later, somehow verify followed by updated seems to have cleared some state and it worked.

k8s-bot · 2015-06-19T21:09:12Z

GCE e2e build/test passed for commit 1b31fc8b6c14151a231aab3872a95ffc8d2fd405.

… of rcs

bprashanth · 2015-06-19T21:36:31Z

Shippable failure is a flake because n/w latencies are really high for some reason (took ~20s for a watch in my last run). Bumped up an integration test timeout that doesn't really need to be that short anyway with my last commit.

k8s-bot · 2015-06-19T21:51:08Z

GCE e2e build/test passed for commit 9ed9bd1.

bgrant0607 · 2015-06-19T22:18:24Z

LGTM

Fix kubectl stop rc with sequence numbers

googlebot added the cla: yes label Jun 12, 2015

lavalamp reviewed Jun 12, 2015
View reviewed changes

derekwaynecarr reviewed Jun 13, 2015
View reviewed changes

davidopp assigned lavalamp Jun 14, 2015

bprashanth force-pushed the sno branch from 379916e to cc767d3 Compare June 15, 2015 19:14

bprashanth force-pushed the sno branch from 915d36b to 7bb39e4 Compare June 15, 2015 21:49

lavalamp reviewed Jun 15, 2015
View reviewed changes

pkg/controller/controller_utils.go

for i, rc := 0, &controller; ; i++ {

rc.Status.Replicas = numReplicas

Copy link

Member

lavalamp Jun 15, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra blank line

0xmichalis reviewed Jun 16, 2015
View reviewed changes

bprashanth mentioned this pull request Jun 17, 2015

kubectl stop rc fails to delete all replicas #9979

Closed

bgrant0607 mentioned this pull request Jun 18, 2015

Create per-object sequence number and report last value seen in status of each object #7328

Open

bprashanth force-pushed the sno branch 2 times, most recently from 52f559b to b69e033 Compare June 19, 2015 07:01

bprashanth force-pushed the sno branch from 79760a5 to 66edecf Compare June 19, 2015 18:21

bgrant0607 reviewed Jun 19, 2015
View reviewed changes

bprashanth force-pushed the sno branch from 66edecf to 1b31fc8 Compare June 19, 2015 20:51

Add a generation number to the object meta of all objects, and status…

9ed9bd1

… of rcs

bprashanth force-pushed the sno branch from 1b31fc8 to 9ed9bd1 Compare June 19, 2015 21:32

bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2015

bgrant0607 added this to the v1.0 milestone Jun 19, 2015

satnam6502 added a commit that referenced this pull request Jun 19, 2015

Merge pull request #9739 from bprashanth/sno

b964f3c

Fix kubectl stop rc with sequence numbers

satnam6502 merged commit b964f3c into kubernetes:master Jun 19, 2015

bprashanth mentioned this pull request Aug 11, 2015

Kubectl delete should handle stopping an rc while it's starting replicas #9147

Closed

bprashanth unassigned lavalamp Aug 12, 2015

bprashanth deleted the sno branch October 26, 2015 00:41

		for i, rc := 0, &controller; ; i++ {
		rc.Status.Replicas = numReplicas

Fix kubectl stop rc with sequence numbers #9739

Fix kubectl stop rc with sequence numbers #9739

Conversation

bprashanth commented Jun 12, 2015

smarterclayton commented Jun 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Jun 12, 2015

lavalamp commented Jun 12, 2015

Choose a reason for hiding this comment

k8s-bot commented Jun 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Jun 12, 2015

bprashanth commented Jun 12, 2015

smarterclayton commented Jun 12, 2015

lavalamp commented Jun 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bprashanth commented Jun 15, 2015

k8s-bot commented Jun 15, 2015

k8s-bot commented Jun 15, 2015

k8s-bot commented Jun 15, 2015

Choose a reason for hiding this comment

lavalamp commented Jun 15, 2015

smarterclayton commented Jun 15, 2015

Choose a reason for hiding this comment

bgrant0607 commented Jun 18, 2015

bprashanth commented Jun 18, 2015

k8s-bot commented Jun 18, 2015

k8s-bot commented Jun 19, 2015

k8s-bot commented Jun 19, 2015

k8s-bot commented Jun 19, 2015

k8s-bot commented Jun 19, 2015

Choose a reason for hiding this comment

bgrant0607 commented Jun 19, 2015

bgrant0607 commented Jun 19, 2015

bprashanth commented Jun 19, 2015

bprashanth commented Jun 19, 2015

k8s-bot commented Jun 19, 2015

bprashanth commented Jun 19, 2015

k8s-bot commented Jun 19, 2015

bgrant0607 commented Jun 19, 2015