Replica Controller Name Annotation in Pods #5842

simon3z · 2015-03-24T10:59:45Z

As discussed with @bgrant0607 in #3676 these patches are adding a pod annotation ("replicationControllerName") to reference the replica controller that generated a pod.

The PR consists of two patches:

adds the annotation to the generated pods
considers the pods annotation when counting the replicas

2 probably needs some discussion.

In fact today it is possible that a user-created pod (if matching the selector) could be counted as part of a replica controller (with the side effect of e.g. another pod being killed).

We could discuss if that's a bug or a feature. In my opinion it is a bug (a user-initiated pod should successfully start and not affect a replication controller pod).

Considering the replication controller annotation when counting the pods (patch 2) makes sure that a replication controller only looks after pods that it generated and it is managing.

If we go down this road we may consider some attribute more "official" than an annotation.

Update: I forgot to mention that patch 2 on a running cluster is dangerous (not even backward compatible), probably all the replication controller will spawn new containers (effectively doubling the replicas).

For this reason I think that this annotation/attribute is vital to be introduced ASAP, because if we don't tag the pods spawned by a replication controller now, we'll never be able to introduce it later.

googlebot · 2015-03-24T10:59:46Z

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

simon3z · 2015-03-26T00:28:48Z

@bgrant0607 before I address @ravigadde's comments do you want to consider regular attributes (instead of annotations)? I mentioned in the pull request why I think it could be preferable.

Should we queue this behind the pod templates? (The reference will be to the template instead of the RC)

bgrant0607 · 2015-03-27T21:37:38Z

It is a feature that replication controllers do not own the pods they create and also can manage pods they do not create. I am strongly opposed to changing those properties.

Also, annotations are not used for filtering -- that would be labels.

I'll look at the rest.

bgrant0607 · 2015-03-27T21:43:43Z

pkg/controller/replication_controller.go

@@ -60,6 +60,8 @@ type RealPodControl struct {
 // Time period of main replication controller sync loop
 const DefaultSyncPeriod = 10 * time.Second

+const RCNAME_ANNOTATION = "replicationControllerName"


Do you care more about the replication controller, or the pod template used to create the pod?

Any annotation created by the system should be namespaced. I believe we've used kubernetes.io/ elsewhere.

bgrant0607 · 2015-03-28T18:34:18Z

More explanation about the replication controller design can be found here:
https://github.com/GoogleCloudPlatform/kubernetes/blob/master/docs/replication-controller.md

simon3z · 2015-04-20T16:13:25Z

in the case that the replication controller is deleted and a new one created, the pods adopted by the new controller won't be annotated with the name of the new controller, either.

@bgrant0607 do you think it would be acceptable if the replication controller would keep the annotation updated? I think your problem with it is that at every syncReplicationController we would have to check/update the annotation.

Anyway it seems that the real name of the annotation should be kubernetes.io/created-by because unless we do some proper work to keep it updated... it's not really a reference to the replication controller.

I experimented with some json in the annotation, e.g.

apiVersion: v1beta3
items:
- apiVersion: v1beta3
  kind: Pod
  metadata:
    annotations:
      kubernetes.io/created-by: '{"name":"frontend","uid":"2aaccc3f-e775-11e4-b55a-525400c903c1"}'

Now that we realized that it's actually a created-by field (unless you agree on keeping it updated and in that case we'll name it maybe kubernetes.io/replication-controller) ... do you think we could move it to the actual metadata instead of having an annotation?

bgrant0607 · 2015-04-21T00:37:22Z

The annotation is similar to, but not exactly the same as, the existence dependence discussed here: #1535 (comment)

Assuming we want the relationship to be represented regardless of deletion behavior, then I suggest we keep the annotation. Like "description" annotations, it shouldn't be a direct metadata field if it has no semantic purpose. Besides, not all pods will be created by other API objects.

The only case in which either the annotation or dependence would need to be updated would be the case where the pods were to be orphaned and then put under control of a new replication controller. Assuming that we want to support re-parenting in this scenario, we'll need a new subresource, similar to /binding, in which case the annotation could be updated as well.

Given that the current default behavior of ReplicationController is to orphan pods on delete and that we don't have existence dependencies or the subresource, I recommend going with the created-by annotation for now.

It's also worth thinking about what will happen when we split pod templates out as separate resources #5012.

simon3z · 2015-04-21T11:39:22Z

Assuming that we want to support re-parenting in this scenario

@bgrant0607 I thought that given comment #5842 (comment) it was certain that we wanted to support it. Or are the two cases slightly different (user-created and previous-controller-created)?

Is there any case where two controllers may overlap? E.g. an rc with selector frontend and an rc with selector backend could be counting some pre-existing containers that are both labeled frontend and backend (maybe with these two labels it's not making sense but you get the gist of it).
I don't think that it's an interesting case to analyze but I just want to make sure I am not missing something.

Given that the current default behavior of ReplicationController is to orphan pods on delete and that we don't have existence dependencies or the subresource, I recommend going with the created-by annotation for now.

@bgrant0607 is the existence dependencies / subresource something that is actively worked on? It seems that in the long term those are your preferred solutions. How much work would it be? If it's doable I could work on one of them.

Also, for created-by are you ok with a jasonzied ObjectReference? (as shown in #5842 (comment))

bgrant0607 · 2015-04-21T21:38:01Z

Yes, I do want to support reparenting, in general.

No, it is not legal for 2 controllers to overlap. We've tried a couple times to validate that, but there are some complications.

No, existence dependencies and the subresource are not actively being worked on yet. I'm not sure how much work they'd be. The subresource should be straightforward. Either a garbage-collection agent needs to be created in order to implement existence dependencies, or that functionality needs to be added to all existing object managers, so that's harder.

Yes, for created-by, a JSON-ized ObjectReference is fine.

Signed-off-by: Federico Simoncelli <fsimonce@redhat.com>

simon3z · 2015-04-22T21:26:00Z

@bgrant0607 I think the patch should be fine now (no idea why shippable breaks)

bgrant0607 · 2015-04-23T03:18:01Z

Thanks. LGTM.

Replica Controller Name Annotation in Pods

lavalamp · 2015-04-24T23:12:30Z

pkg/controller/replication_controller.go

+		return
+	}
+
+	createdByRefJson, err := json.Marshal(createdByRef)


@simon3z @bgrant0607 This serializes our internal, unversioned object. These annotations will not be readable as soon as that changes!

This should use some codec's Encode function.

It's a bit awkward, because the field doesn't go through conversion, so the versioned rep isn't really appropriate, either. I do want to get rid of the json tags on the internal rep., however, which would also break this.

I suppose using the latest stable API version would be better. Really, the RC controller shouldn't be using the internal rep. at all.

@lavalamp Did you file an issue to fix this? How did you discover it?

Noticed while reviewing #6866. I asked for a TODO there, but I'll file an issue, too.

Also, now that you got me actually thinking about it, this is gonna be a nightmare for any sort of schema updating script-- it's going to have to understand the annotations to be correct.

Filed #7322.

googlebot added the cla: no label Mar 24, 2015

saad-ali assigned bgrant0607 Mar 24, 2015

bgrant0607 reviewed Mar 27, 2015
View reviewed changes

bgrant0607 mentioned this pull request Apr 10, 2015

Consistently support graceful and immediate termination for all objects #1535

Closed

replication: annotate replicated pods with controller name

253ce4b

Signed-off-by: Federico Simoncelli <fsimonce@redhat.com>

simon3z force-pushed the rc-annotation branch from b9f646f to 253ce4b Compare April 22, 2015 15:27

bgrant0607 added lgtm "Looks good to me", indicates that a PR is ready to be merged. cla: yes and removed cla: no labels Apr 23, 2015

bgrant0607 added a commit that referenced this pull request Apr 23, 2015

Merge pull request #5842 from simon3z/rc-annotation

73322af

Replica Controller Name Annotation in Pods

bgrant0607 merged commit 73322af into kubernetes:master Apr 23, 2015

simon3z mentioned this pull request Apr 24, 2015

kubernetes: handle replication controller relationship ManageIQ/manageiq#2743

Merged

simon3z deleted the rc-annotation branch April 24, 2015 17:06

lavalamp reviewed Apr 24, 2015
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replica Controller Name Annotation in Pods #5842

Replica Controller Name Annotation in Pods #5842

simon3z commented Mar 24, 2015

googlebot commented Mar 24, 2015

simon3z commented Mar 26, 2015

bgrant0607 commented Mar 27, 2015

bgrant0607 Mar 27, 2015

bgrant0607 commented Mar 28, 2015

simon3z commented Apr 20, 2015

bgrant0607 commented Apr 21, 2015

simon3z commented Apr 21, 2015

bgrant0607 commented Apr 21, 2015

simon3z commented Apr 22, 2015

bgrant0607 commented Apr 23, 2015

lavalamp Apr 24, 2015

bgrant0607 Apr 24, 2015

bgrant0607 Apr 24, 2015

bgrant0607 Apr 24, 2015

lavalamp Apr 25, 2015

lavalamp Apr 25, 2015

lavalamp Apr 25, 2015

Replica Controller Name Annotation in Pods #5842

Replica Controller Name Annotation in Pods #5842

Conversation

simon3z commented Mar 24, 2015

googlebot commented Mar 24, 2015

simon3z commented Mar 26, 2015

bgrant0607 commented Mar 27, 2015

Choose a reason for hiding this comment

bgrant0607 commented Mar 28, 2015

simon3z commented Apr 20, 2015

bgrant0607 commented Apr 21, 2015

simon3z commented Apr 21, 2015

bgrant0607 commented Apr 21, 2015

simon3z commented Apr 22, 2015

bgrant0607 commented Apr 23, 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

Choose a reason for hiding this comment