Ensure pod and manifest have a UUID in apiserver #345

smarterclayton · 2014-07-02T23:22:11Z

Also has replication controller allow apiserver to automatically assign a UUID

Continues #253 and #199

lavalamp · 2014-07-02T23:28:40Z

pkg/registry/controller_registry.go

@@ -96,7 +100,7 @@ func (storage *ControllerRegistryStorage) Update(obj interface{}) (<-chan interf
 	if !ok {
 		return nil, fmt.Errorf("not a replication controller: %#v", obj)
 	}
-	if controller.ID == "" {
+	if len(controller.ID) == 0 {


I don't care which way it is, but only change this if you think it's clearer; don't do it as an optimization... http://stackoverflow.com/questions/18594330/the-best-way-to-test-for-an-empty-string-in-go

Yeah I was just cleaning the file as I went. I saw len() and it seemed more broadly used.

lavalamp · 2014-07-02T23:40:58Z

Aside from a few comments, this LGTM. Thanks for this change, it should go a long ways towards making our IDs sensible!

brendandburns · 2014-07-02T23:46:14Z

Thanks for the PR! LGTM, modulo Daniel's comments.

monnand · 2014-07-03T06:37:51Z

I have a small request: Would you please run the race detector first? Or could we wait for #348 getting merged?

I'm sorry, after fixing data races for two days, I think it might be better to make sure the code could pass the race detector before merging it.

smarterclayton · 2014-07-03T16:40:17Z

@monnand no additional data races reported (other than the existing one in TestWatchControllers)

thockin · 2014-07-03T16:40:52Z

I'm unclear on what exactly JSONBase.ID is supposed to represent. Is it a globally unique ID for each api-managed object? Is it a transaction ID? Something in-between?

I'm also finding it hard to think about this change because I am still mulling the other discussion about the different forms of name and ID. It feels weird that ID is in JSONBase for some objects and not others.

I'm not blocking this change at this point, but I feel like I don't quite get it yet. Something somewhere has not clicked for me :)

brendandburns · 2014-07-03T21:25:25Z

I intended ID to be a unique GUID that identifies a particular type of resource in the system.

So all Pods should have unique ID, all replication controllers should have unique ID (though a replication controller and a pod might have the same id, since they're in different namespaces)

as far as I intended, it is not a transaction ID, except in the case of an ServerOp, and even their it is a GUID, its just a GUID for an object that represents a transaction.

I think its different than the name of a container inside a pod, for example, since that is a semantic construct used for identifying (and possibly for DNS allocation, etc), but will never be used by the API itself to reference to anything.

For a while, I actually had ID (GUID) and Name (semantic identifier) but that actually got too confusing, so I ripped out Name...

thockin · 2014-07-05T03:35:25Z

Should we use JSONBase.ID instead of structure-specific IDs with
structure-specific semantics (e.g. "this ID survives pod moves and
restarts" vs "this ID is changed on every reschedule") ?

Or put another way, the ContainerManifest does not include JSONBase.
Should it?

On Thu, Jul 3, 2014 at 2:25 PM, brendandburns notifications@github.com
wrote:

I intended ID to be a unique GUID that identifies a particular type of
resource in the system.

So all Pods should have unique ID, all replication controllers should have
unique ID (though a replication controller and a pod might have the same
id, since they're in different namespaces)

as far as I intended, it is not a transaction ID, except in the case of an
ServerOp, and even their it is a GUID, its just a GUID for an object that
represents a transaction.

I think its different than the name of a container inside a pod, for
example, since that is a semantic construct used for identifying (and
possibly for DNS allocation, etc), but will never be used by the API itself
to reference to anything.

For a while, I actually had ID (GUID) and Name (semantic identifier) but
that actually got too confusing, so I ripped out Name...

Reply to this email directly or view it on GitHub
#345 (comment)
.

smarterclayton · 2014-07-05T17:35:08Z

This implies to me that ContainerManifest should not have an ID - it was a temporary issue that the presence of a pod uuid resolves, so I marked it as deprecated in the latest commit. Now, if ContainerManifest becomes an API resource as commented in another issue, I think we would want to make it a JSON base.

thockin · 2014-07-07T03:39:35Z

Yeah, we have some confusion between what is a pod and what is a manifest.
From Kubelet's point of view a manifest IS a pod, and it is a resource.
From apiserver's point of view it's just a piece of a larger resource.

Brendan and I were discussing separating the objects based on point-of-view
(API Server vs Kubelet) but I have not done so yet. If
ContainerManifest.ID is deprecated, what do I use to identify a pod on a
kubelet?

Got time to do a hangout this week maybe?

On Sat, Jul 5, 2014 at 10:35 AM, Clayton Coleman notifications@github.com
wrote:

This implies
#345 (comment)
to me that ContainerManifest should not have an ID - it was a temporary
issue that the presence of a pod uuid resolves, so I marked it as
deprecated in the latest commit. Now, if ContainerManifest becomes an API
resource as commented in another issue, I think we would want to make it a
JSON base.

Reply to this email directly or view it on GitHub
#345 (comment)
.

smarterclayton · 2014-07-07T03:53:50Z

Sure - I had started to take a stab at what it would take to abstract the kubelet from a direct etcd dependency in #356 and noted the same thing. Would be good to go over the options in a hangout.

thockin · 2014-07-07T04:05:59Z

You're east-coast time, right?

Tuesday afternoon (your time, after 11:00a California time) is open for
me...

For anyone else, this is not a private meeting, but we would like to focus
on progress as much as possible :)

On Sun, Jul 6, 2014 at 8:53 PM, Clayton Coleman notifications@github.com
wrote:

Sure - I had started to take a stab at what it would take to abstract the
kubelet from a direct etcd dependency in #356
#356 and noted
the same thing. Would be good to go over the options in a hangout.

Reply to this email directly or view it on GitHub
#345 (comment)
.

verdverm · 2014-07-07T05:55:05Z

I'd be down to join a hangout, though I'm not sure how much I have to contribute at this point

smarterclayton · 2014-07-07T13:29:10Z

Let's try 2pm EDT on Tuesday

thockin · 2014-07-07T18:35:36Z

I will post a hangout link on this thread.

Proposed topics:

API structure split: master vs slave
API versioning
IDs and Names

On Mon, Jul 7, 2014 at 6:29 AM, Clayton Coleman notifications@github.com
wrote:

Let's try 2pm EDT on Tuesday

Reply to this email directly or view it on GitHub
#345 (comment)
.

thockin · 2014-07-08T17:53:45Z

https://plus.google.com/hangouts/_/grkrl7elarpergdg6ksepxjclea

I hope this works - never set up a public hangout before.

On Mon, Jul 7, 2014 at 11:35 AM, Tim Hockin thockin@google.com wrote:

I will post a hangout link on this thread.

Proposed topics:

API structure split: master vs slave
API versioning
IDs and Names

On Mon, Jul 7, 2014 at 6:29 AM, Clayton Coleman notifications@github.com
wrote:

Let's try 2pm EDT on Tuesday

Reply to this email directly or view it on GitHub
#345 (comment)
.

verdverm · 2014-07-08T18:01:35Z

Link says the party is over

thockin · 2014-07-08T18:04:22Z

try

https://plus.google.com/hangouts/_/g6eqprge2izsz3xmvubuajky2ua

On Tue, Jul 8, 2014 at 11:01 AM, Tony Worm notifications@github.com wrote:

Link says the party is over

Reply to this email directly or view it on GitHub
#345 (comment)
.

thockin · 2014-07-08T20:15:37Z

Notes from this discussion:

API structure split: master vs slave

We all agreed in general that we should differentiate top-level API
structures between the API server and the Kubelet. Common structures like
ContainerManifest will stay common and shared, but (for example) there
probably needs to be a different "Pod" structure for master and slave.

Google will find someone to work on this issue.

API versioning

We all agreed in general that we need to start versioning the APIs and
having an explicit point in the processing logic where we convert from
versioned external structures to internal structures. We will try to avoid
explicitly versioning internal structures for simplicity, but nobody was
deeply against it, if need be. If we find ourselves with truly
incompatible behavior, we will need to introduce them carefully with
explicit notes of API breakage.

Nobody volunteered to work on this, so Google will find someone if possible

IDs and names

We agreed on the following:

"Pod ID" that exists for the lifetime of the pod in k8s
"Pod Instance ID" that exists for the lifetime of a pod on a host
"Container attempt ID" that is assigned to each run of a container
(docker's ID)

We did not discuss, but previously agreed on:

"Pod namespace" that represents the origin of the pod
"Pod name"
"Container name"

We agreed to not have the following:

A generation number that changes on stop/start cycles.

I'll write this up more fully in PR #349. Clayton is working on this.

Thanks everyone. way faster than email.

Tim

On Mon, Jul 7, 2014 at 11:35 AM, Tim Hockin thockin@google.com wrote:

I will post a hangout link on this thread.

Proposed topics:

API structure split: master vs slave
API versioning
IDs and Names

On Mon, Jul 7, 2014 at 6:29 AM, Clayton Coleman notifications@github.com
wrote:

Let's try 2pm EDT on Tuesday

Reply to this email directly or view it on GitHub
#345 (comment)
.

smarterclayton · 2014-07-10T15:08:37Z

For the purposes of this pull, I think the change here fits within the agreed direction and is merely an incremental step towards it (accordingly I marked ContainerManifest.ID as deprecated). Disagreement? If no could use a final rereview and merge.

thockin · 2014-07-10T19:55:49Z

LGTM - resolve conflicts or whatever is causing "We can’t automatically merge this pull request"

…bc9799ae1

* Fixes implication #2 in docs/identifiers.md * Replication controller lets the apiserver set the pod ID

smarterclayton · 2014-07-11T18:26:33Z

@thockin ptal

Ensure pod and manifest have a UUID in apiserver

Added HTTP Auth and HTTP Digest authentication kubernetes#302

Format Node Component, Scheduling, and API Machinery sections.

…r-scheduler-plugin bug 1874920: Revert "bug 1874919: UPSTREAM: 94423: debugger scheduler plugin"

lavalamp reviewed Jul 2, 2014
View reviewed changes

smarterclayton added 2 commits July 10, 2014 17:06

bump(code.google.com/p/go-uuid/uuid): 3da0bf12e76e1048469b95fb20e29f1…

31e40e1

…bc9799ae1

Ensure pod and manifest always have a UUID

5896ac1

* Fixes implication #2 in docs/identifiers.md * Replication controller lets the apiserver set the pod ID

thockin added a commit that referenced this pull request Jul 11, 2014

Merge pull request #345 from smarterclayton/fix_ids

f532038

Ensure pod and manifest have a UUID in apiserver

thockin merged commit f532038 into kubernetes:master Jul 11, 2014

smarterclayton deleted the fix_ids branch February 11, 2015 02:21

vishh pushed a commit to vishh/kubernetes that referenced this pull request Apr 6, 2016

Merge pull request kubernetes#345 from incredlabs/httpauth

22a0584

Added HTTP Auth and HTTP Digest authentication kubernetes#302

seans3 pushed a commit to seans3/kubernetes that referenced this pull request Apr 10, 2019

Merge pull request kubernetes#345 from kubernetes/dchen1107-patch-8

09289d2

Format Node Component, Scheduling, and API Machinery sections.

smarterclayton pushed a commit to smarterclayton/kubernetes that referenced this pull request Sep 21, 2020

Merge pull request kubernetes#345 from ingvagabund/revert-330-debugge…

032ded0

…r-scheduler-plugin bug 1874920: Revert "bug 1874919: UPSTREAM: 94423: debugger scheduler plugin"

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure pod and manifest have a UUID in apiserver #345

Ensure pod and manifest have a UUID in apiserver #345

smarterclayton commented Jul 2, 2014

lavalamp Jul 2, 2014

smarterclayton Jul 2, 2014

lavalamp commented Jul 2, 2014

brendandburns commented Jul 2, 2014

monnand commented Jul 3, 2014

smarterclayton commented Jul 3, 2014

thockin commented Jul 3, 2014

brendandburns commented Jul 3, 2014

thockin commented Jul 5, 2014

smarterclayton commented Jul 5, 2014

thockin commented Jul 7, 2014

smarterclayton commented Jul 7, 2014

thockin commented Jul 7, 2014

verdverm commented Jul 7, 2014

smarterclayton commented Jul 7, 2014

thockin commented Jul 7, 2014

thockin commented Jul 8, 2014

verdverm commented Jul 8, 2014

thockin commented Jul 8, 2014

thockin commented Jul 8, 2014

smarterclayton commented Jul 10, 2014

thockin commented Jul 10, 2014

smarterclayton commented Jul 11, 2014

Ensure pod and manifest have a UUID in apiserver #345

Ensure pod and manifest have a UUID in apiserver #345

Conversation

smarterclayton commented Jul 2, 2014

lavalamp Jul 2, 2014

Choose a reason for hiding this comment

smarterclayton Jul 2, 2014

Choose a reason for hiding this comment

lavalamp commented Jul 2, 2014

brendandburns commented Jul 2, 2014

monnand commented Jul 3, 2014

smarterclayton commented Jul 3, 2014

thockin commented Jul 3, 2014

brendandburns commented Jul 3, 2014

thockin commented Jul 5, 2014

smarterclayton commented Jul 5, 2014

thockin commented Jul 7, 2014

smarterclayton commented Jul 7, 2014

thockin commented Jul 7, 2014

verdverm commented Jul 7, 2014

smarterclayton commented Jul 7, 2014

thockin commented Jul 7, 2014

thockin commented Jul 8, 2014

verdverm commented Jul 8, 2014

thockin commented Jul 8, 2014

thockin commented Jul 8, 2014

smarterclayton commented Jul 10, 2014

thockin commented Jul 10, 2014

smarterclayton commented Jul 11, 2014