Proposal: v1beta3 API overhaul #1225

smarterclayton · 2014-09-09T04:22:43Z

Speculative change for some items from #1200:

Metadata that applies to resources is split from APIMetadata on non-resource API objects
Metadata is a sub field of objects
Labels are on Metadata
Pod current and desired state is split
Name is used for pods instead of ID (UID is part of metadata, Name is part of resource) for idempotent creation Idempotent creation #148 and Revisit identifiers spec #1216
PodTemplate is a Metadata carrying object as per Separate the pod template from replicationController #170
- If replicationController gave way to a generic "replicator", then pod template could be both a nested struct and a real resource.
A ScheduledPod and ScheduledPodList is what the kubelet sees as per Proposal: Isolate kubelet from etcd #860
Non-schema'd k/v values on Metadata as Config (better names welcome) as per Document annotations and guidelines regarding when to use annotations vs labels #1201

Not changed yet:

ReplicationControllers
Services

EDIT: Moving to higher bandwidth document here: https://docs.google.com/document/d/1E_HNsbyocweD9zJHXbz5Xb9XPwBqvDnLHhGeBq15FlI/edit?usp=sharing

smarterclayton · 2014-09-09T04:27:42Z

Things I like about this

Metadata can be completely omitted when POSTing a naive resource (version and kind could be inferred from an endpoint)
Everything in APIMetadata is clearly useful to every api endpoint, everything in Metadata is clearly useful to every stored resource.

thockin · 2014-09-09T04:48:21Z

I just sent a bunch of comments, but I don't see them in Github at all.
Did you get them?

On Mon, Sep 8, 2014 at 9:27 PM, Clayton Coleman notifications@github.com
wrote:

Things I like about this

Metadata can be completely omitted when POSTing a naive resource
(version and kind could be inferred from an endpoint)

Everything in MetadataBase is clearly useful to every api endpoint,
everything in Metadata is clearly useful to every stored resource.

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

smarterclayton · 2014-09-09T04:49:56Z

No, this seems like the problem from the other day. They eventually shown up.

On Sep 9, 2014, at 12:48 AM, Tim Hockin notifications@github.com wrote:

I just sent a bunch of comments, but I don't see them in Github at all.
Did you get them?

On Mon, Sep 8, 2014 at 9:27 PM, Clayton Coleman notifications@github.com
wrote:

Things I like about this

Metadata can be completely omitted when POSTing a naive resource
(version and kind could be inferred from an endpoint)

Everything in MetadataBase is clearly useful to every api endpoint,
everything in Metadata is clearly useful to every stored resource.

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

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

thockin · 2014-09-09T04:51:42Z

Ugh. I'll keep an eye on it. I spent like 6 hours on comments, they were
the best comments you ever saw. I promise. :)

If they don't show up I'll do it again in the morning.

On Mon, Sep 8, 2014 at 9:50 PM, Clayton Coleman notifications@github.com
wrote:

No, this seems like the problem from the other day. They eventually shown
up.

On Sep 9, 2014, at 12:48 AM, Tim Hockin notifications@github.com
wrote:

I just sent a bunch of comments, but I don't see them in Github at all.
Did you get them?

On Mon, Sep 8, 2014 at 9:27 PM, Clayton Coleman <
notifications@github.com>
wrote:

Things I like about this

Metadata can be completely omitted when POSTing a naive resource
(version and kind could be inferred from an endpoint)

Everything in MetadataBase is clearly useful to every api endpoint,
everything in Metadata is clearly useful to every stored resource.

Reply to this email directly or view it on GitHub
<
https://github.com/GoogleCloudPlatform/kubernetes/pull/1225#issuecomment-54922921>

.

Reply to this email directly or view it on GitHub.

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

smarterclayton · 2014-09-09T04:55:27Z

They just ended up on the commit and then I pushed a new commit.

On Sep 9, 2014, at 12:51 AM, Tim Hockin notifications@github.com wrote:

Ugh. I'll keep an eye on it. I spent like 6 hours on comments, they were
the best comments you ever saw. I promise. :)

If they don't show up I'll do it again in the morning.

On Mon, Sep 8, 2014 at 9:50 PM, Clayton Coleman notifications@github.com
wrote:

No, this seems like the problem from the other day. They eventually shown
up.

On Sep 9, 2014, at 12:48 AM, Tim Hockin notifications@github.com
wrote:

I just sent a bunch of comments, but I don't see them in Github at all.
Did you get them?

On Mon, Sep 8, 2014 at 9:27 PM, Clayton Coleman <
notifications@github.com>
wrote:

Things I like about this

Metadata can be completely omitted when POSTing a naive resource
(version and kind could be inferred from an endpoint)

Everything in MetadataBase is clearly useful to every api endpoint,
everything in Metadata is clearly useful to every stored resource.

Reply to this email directly or view it on GitHub
<
https://github.com/GoogleCloudPlatform/kubernetes/pull/1225#issuecomment-54922921>

.

Reply to this email directly or view it on GitHub.

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

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

thockin · 2014-09-09T04:55:39Z

pkg/api/v1beta3_metadata/types.go

+
+// APIMetadata is shared by all objects sent to, or returned via the API.
+type APIMetadata struct {
+	// Kind is a string value representing the REST resource this object represents.


Could I beg for vertical whitespace between these?

lavalamp · 2014-09-09T04:56:45Z

Can we split off v1beta2 first?

thockin · 2014-09-09T04:57:00Z

pkg/api/v1beta3_metadata/types.go

+}
+
+// PodDesiredState is the state a pod should have when running on a host.
+type PodDesiredState struct {


Can we use a name like PodSpec or something? This is the specification for a pod.

+1 on PodSpec

thockin · 2014-09-09T04:59:02Z

And what is the protocol here.

Do we a) fork a new v1beta3 with big changes and stabilize that or b) make
all these changes to the internal API, and when that is stable fork v1beta3?

On Mon, Sep 8, 2014 at 9:56 PM, Daniel Smith notifications@github.com
wrote:

Can we split off v1beta2 first?

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

thockin · 2014-09-09T05:01:44Z

Overall I like this a lot - I think it helps comprehension enormously.

smarterclayton · 2014-09-09T05:08:42Z

Yeah, this is labelled as 3 to allow that

On Sep 9, 2014, at 12:56 AM, Daniel Smith notifications@github.com wrote:

Can we split off v1beta2 first?

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

dchen1107 · 2014-09-09T05:19:11Z

pkg/api/v1beta3_metadata/types.go

+type PodStatus string
+
+// These are the valid statuses of pods.
+const (


Can we have an error state here? I am wondering how to distinguish it from PodWaiting without such state.

A permanent error means the pod would be in PodTerminated.

Ok, here is one example. A pod has 3 containers with RestartPolicyNever configed. Container A is terminated with exit code -1, Container B, C are still running happily. In today's system, we reset the PodStatus back to PodWaiting, which is obviously wrong to me. But which other state Pod should be? It is a permanent error, but shouldn't put the entire pod to PodTerminated.

Without Error state at pod level, I am ok to leave pod at PodRunning state with error state for Container itself.

IMO, that pod should be reported as being in "PodRunning". If users need per-container info they'll have to look inside the PodInfo field.

@bgrant0607

Like what I proposed above, I can go with PodRunning. But I don't think

(sorry I cannot type today)
s/ is a good/ is not a good/

It seems impossible to present status from three containers in a single field without losing something. And anyway, if your policy is NeverRestart, and one of your containers has exited, who's to say that's an error? If that was an error state, maybe NeverRestart isn't the right policy-- you need something that fails the whole pod if one of the containers exits.

I think it is reasonable to say that pod state is a "worst of" container states. That's what internal 'let is moving to, and it seems sane.

I'm opposed to adding more kinds of statuses here. More nuanced information needs to go elsewhere.

I agree a Running pod should never revert to Waiting. It seems like that would be easy to ensure if we saved the status.

I don't know how to interpret "Worst of".

Transitions:

Waiting->Running: We could/should install all images, then start all containers. Once images are installed, I think we should transition to Running/Active. At that point, we're not waiting. The containers may be broken, but that's not waiting.

Running->Terminated: All containers need to be permanently dead to reach Terminated, IMO. One container running is Running. The restart policy needs to be taken into account here to avoid race conditions: all containers temporarily being down shouldn't transition a pod whose containers should always restart to Terminated.

Meta-question: Is this the right place to be discussing this? Isn't there a more appropriate PR or issue?

Let's keep the discussion of what PodStates codes exist off this issue.

dchen1107 · 2014-09-09T16:51:13Z

pkg/api/v1beta3_metadata/types.go

+// Event is the representation of an event logged to etcd backends.
+type Event struct {
+	Event     string             `json:"event,omitempty"`
+	Manifest  *ContainerManifest `json:"manifest,omitempty"`


Shouldn't ContainerManifest is deprecated entirely here?

Recommend only storing the UID anyway, not the whole thing.

lavalamp · 2014-09-09T17:18:57Z

I really want to dig into this PR later today. For now,

@thockin

Do we a) fork a new v1beta3 with big changes and stabilize that or b) make all these changes to the internal API, and when that is stable fork v1beta3?

I think the steps are:

a. Copy what we have now to v1beta2. fix up links in v1beta1's conversion package.
b. Make these changes in pkg/api, and in new package v1beta3 at the same time.
c. Write to/from conversion functions in pkg/api/v1beta2 that preserves as much of the content as possible.
d. In pkg/api/serialization_test.go, make sure each serialization path (there will be three now) is tested with the fuzzer. (Note that fuzzing may need to be customized in the cases where there information is legitimately lost between two versions, not just moved.)
e. Freeze pkg/api/v1beta3, and any further minor changes need conversion functions in pkg/api/v1beta3.
f. misc: apiserver needs to be fixed to serve all three versions of the api (can be done in parallel).
g. misc: runtime.EmbeddedObject needs to be fixed to not use runtime.DefaultScheme.
h. misc: we may want to consider making a central package that imports all the versions and sets up the three "runtime.Scheme" objects. Not sure what to call it. This will let us avoid some verbose import statements.

lavalamp · 2014-09-09T17:50:38Z

pkg/api/v1beta3_metadata/types.go

+// ContainerManifest corresponds to the Container Manifest format, documented at:
+// https://developers.google.com/compute/docs/containers/container_vms#container_manifest
+// This is used as the representation of Kubernetes workloads.
+// DEPRECATED: This structure is no longer referenced by other constructs.


Deprecation notices should only appear in the v1beta2 package - v1beta3 will just omit the things.

Only here as a comment

smarterclayton · 2014-09-10T01:21:33Z

Highly speculative update - rename ReplicationController to PodReplicator? We've posited a generic replicator, as well as removing Controller from things that aren't an actual rectification process.

bgrant0607 · 2014-09-26T22:50:23Z

LGTM. I'd like to get this merged. We can always tweak this afterward if needed.

smarterclayton · 2014-09-26T23:04:27Z

Will finish up any remaining feedback for Monday morning.

On Sep 26, 2014, at 6:50 PM, bgrant0607 notifications@github.com wrote:

LGTM. I'd like to get this merged. We can always tweak this afterward if needed.

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

bgrant0607 · 2014-09-27T00:19:40Z

pkg/api/v1beta3/types.go

+type PodSpec struct {
+	Volumes       []Volume      `yaml:"volumes" json:"volumes"`
+	Containers    []Container   `yaml:"containers" json:"containers"`
+	RestartPolicy RestartPolicy `json:"restartpolicy,omitempty" yaml:"restartpolicy,omitempty"`


The json should be restartPolicy.

bgrant0607 · 2014-09-27T07:08:10Z

As per the decision in #1458, please update all string constants from Java-style camelCase to Go-style CamelCase. Acronyms, such as protocols, TCP, UDP, HTTP, should be all uppercase, as specified by #1477. The comment of LivenessProbe.Type should be updated to reflect that.

Known cases to address: PodStatus, Status.Status, StatusReasons, CauseTypes

* Separate metadata from objects * Treat lists differently from resources * Add UID and Annotations on Metadata * Introduce BoundPod(s) as distinct from Pod to represent pods scheduled onto a host * Use "spec" instead of "state" and "status" instead of "currentState" * Identify current status of objects consistently * Use "condition" as the string label for substatus * All string constants should be PublicGoCapitalized * Rename Minion -> Node * Rename ServerOp -> Operation * Remove ContainerManifest * Add api-conventions.md document * Replace PodTemplateSpec in ReplCtrl with ObjectReference

smarterclayton · 2014-09-29T16:18:05Z

This has now been updated with all of the current discussion. I specifically did not address ContainerState being renamed - I think we should settle that as a follow on. This pull can land with the proposed API, and we can then begin breaking up the work that constitutes storing it, converting to and from it, exposing it as an API, and plumbing it through a client (each of which has new challenges since we're doing renames, reconstanting, changing response values, etc).

smarterclayton · 2014-09-29T16:24:38Z

I think this is ready for final review and merge with the caveats above.

dchen1107 · 2014-09-29T17:00:41Z

LGTM

@smarterclayton Agreed on settling ContainerState in a separate PR. Since I introduced it at the first place, once we converged on it, I will address this particular issue crossing all versions.

bgrant0607 · 2014-09-30T00:15:11Z

Recent updates LGTM. Is there any reason to not merge this now and fix oversights later?

smarterclayton · 2014-09-30T00:58:54Z

No, should have no effect on running code

On Sep 29, 2014, at 8:15 PM, bgrant0607 notifications@github.com wrote:

Recent updates LGTM. Is there any reason to not merge this now and fix oversights later?

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

thockin · 2014-09-30T03:46:55Z

pkg/api/v1beta3/types.go

+// Service is a named abstraction of software service (for example, mysql) consisting of local port
+// (for example 3306) that the proxy listens on, and the selector that determines which pods
+// will answer requests sent through the proxy.
+type Service struct {


Cross-referencing this with the ip-per-service PR, I have a question. In ip-per-service, the master has to assign a few fields (the IP in particular) to a service upon creation. They must be ignored on input.

Do you see those fields a) in ServiceSpec, b) in Service, c) in ServiceData, d) in BoundService ?

Sounds like ServiceStatus, similar to how Host is set in PodStatus, no?

Or is your question what the proxy should watch (replacement for endpoints?)?

On Sep 29, 2014, at 11:47 PM, Tim Hockin notifications@github.com wrote:

In pkg/api/v1beta3/types.go:

// ContainerPort is the name of the port on the container to direct traffic to. // Optional, if unspecified use the first port on the container. ContainerPort util.IntOrString `json:"containerPort,omitempty" yaml:"containerPort,omitempty"`

}

-func (*Service) IsAnAPIObject() {}
+// Service is a named abstraction of software service (for example, mysql) consisting of local port
+// (for example 3306) that the proxy listens on, and the selector that determines which pods
+// will answer requests sent through the proxy.
+type Service struct {
Cross-referencing this with the ip-per-service PR, I have a question. In ip-per-service, the master has to assign a few fields (the IP in particular) to a service upon creation. They must be ignored on input.

Do you see those fields a) in ServiceSpec, b) in Service, c) in ServiceData, d) in BoundService ?

Seems like ServiceStatus if users can never choose them. If the users can set them or leave it to the server to set a default, maybe we use Spec for user intent and Status for effective state? I feel like the Bound* are more special cases where things are transformed into other atomic units but that's more based on our current pattern.
—
Reply to this email directly or view it on GitHub.

BoundPod is really just a way of encapsulating a PodSpec + metadata, so the analogy here doesn't really hold, but I was grasping.

I guess *Status sounds close enough. Once this API change lands I will have to figure out how to rebase ip-per-service onto it.

thockin · 2014-09-30T03:48:24Z

in before 200 comments!

LGTM, ok to merge in-hours.

bgrant0607 · 2014-09-30T07:17:35Z

pkg/api/v1beta3/types.go

-	HostDirectory *HostDirectory `yaml:"hostDir" json:"hostDir"`
+	// TODO(jonesdl) We need to restrict who can use host directory mounts and who can/can not
+	// mount host directories as read/write.
+	HostDirectory *HostDirectory `json:"hostDir" yaml:"hostDir"`


Just stumbled across #992. We should just resolve it one way or the other in v1beta3. Any compelling arguments against changing Dir to Directory in hostDir and emptyDir?

@thockin @lavalamp @dchen1107 @satnam6502

I'm fine with changing emptyDir to EmptyDirectory or even just "empty".

hostDir is interesting - it doesn't have to be a directory at all - it can be a host file. Maybe it should just become hostMount or fromHost or something.

I don't think v1beta3 needs to block on either of these - I think we can make a compatible change here.

How about hostPath and emptyDirectory?

That's good, but I think we can make that change in a compatible way, so
v1b3 doesn't need to block.

On Tue, Sep 30, 2014 at 9:53 AM, bgrant0607 notifications@github.com
wrote:

In pkg/api/v1beta3/types.go:

@@ -86,16 +129,16 @@ type VolumeSource struct {
// HostDirectory represents a pre-existing directory on the host machine that is directly
// exposed to the container. This is generally used for system agents or other privileged
// things that are allowed to see the host machine. Most containers will NOT need this.

// TODO(jonesdl) We need to restrict who can use host directory mounts and

// who can/can not mount host directories as read/write.

HostDirectory *HostDirectory yaml:"hostDir" json:"hostDir"

// TODO(jonesdl) We need to restrict who can use host directory mounts and who can/can not

// mount host directories as read/write.

HostDirectory *HostDirectory json:"hostDir" yaml:"hostDir"

How about hostPath and emptyDirectory?

Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/1225/files#r18230436
.

Any other comments?

On Sep 30, 2014, at 1:01 PM, Tim Hockin notifications@github.com wrote:

In pkg/api/v1beta3/types.go:

@@ -86,16 +129,16 @@ type VolumeSource struct {
// HostDirectory represents a pre-existing directory on the host machine that is directly
// exposed to the container. This is generally used for system agents or other privileged
// things that are allowed to see the host machine. Most containers will NOT need this.

// TODO(jonesdl) We need to restrict who can use host directory mounts and

// who can/can not mount host directories as read/write.

HostDirectory *HostDirectory yaml:"hostDir" json:"hostDir"

// TODO(jonesdl) We need to restrict who can use host directory mounts and who can/can not

// mount host directories as read/write.

HostDirectory *HostDirectory json:"hostDir" yaml:"hostDir"
That's good, but I think we can make that change in a compatible way, so v1b3 doesn't need to block.
…
—
Reply to this email directly or view it on GitHub.

Proposal: v1beta3 API overhaul

Bug 1999325: Backport 107821 and 107831

smarterclayton force-pushed the v1beta3_proposals branch from c534e1c to 707a527 Compare September 9, 2014 04:33

thockin reviewed Sep 9, 2014
View reviewed changes

dchen1107 reviewed Sep 9, 2014
View reviewed changes

bgrant0607 added the area/api Indicates an issue on api area. label Sep 9, 2014

dchen1107 reviewed Sep 9, 2014
View reviewed changes

smarterclayton force-pushed the v1beta3_proposals branch from 657c4bd to e629bb8 Compare September 9, 2014 17:24

lavalamp reviewed Sep 9, 2014
View reviewed changes

bgrant0607 reviewed Sep 27, 2014
View reviewed changes

This was referenced Sep 27, 2014

Proposal: don't return JSONBase in LIST responses #1223

Closed

api server paths are case sensitive #726

Closed

smarterclayton force-pushed the v1beta3_proposals branch from 917e694 to 46e41cc Compare September 29, 2014 16:05

smarterclayton force-pushed the v1beta3_proposals branch from 46e41cc to c0247d9 Compare September 29, 2014 16:10

bgrant0607 mentioned this pull request Sep 30, 2014

Complete atomic update code for all objects and create #656

Closed

thockin reviewed Sep 30, 2014
View reviewed changes

bgrant0607 mentioned this pull request Sep 30, 2014

Specifying ID twice is sort of tedious #294

Closed

bgrant0607 reviewed Sep 30, 2014
View reviewed changes

thockin added a commit that referenced this pull request Sep 30, 2014

Merge pull request #1225 from smarterclayton/v1beta3_proposals

88bf01b

Proposal: v1beta3 API overhaul

thockin merged commit 88bf01b into kubernetes:master Sep 30, 2014

smarterclayton mentioned this pull request Sep 30, 2014

Implement v1beta3 api #1519

Closed

20 tasks

bgrant0607 mentioned this pull request Oct 2, 2014

Directory renamed to Dir to match json property #1525

Merged

This was referenced Oct 31, 2014

BoundPods should have field -- ListMeta, instead ObjectMeta. #2089

Closed

BoundPods should have field -- ListMeta, instead ObjectMeta #2088

Closed

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

bgrant0607 mentioned this pull request Aug 15, 2015

Create ReplicaSet #3024

Closed

bgrant0607 mentioned this pull request Apr 10, 2018

Initial addition of label and annotation recommendations kubernetes/website#7695

Closed

soltysh pushed a commit to soltysh/kubernetes that referenced this pull request Apr 15, 2022

Merge pull request kubernetes#1225 from rphillips/backports/108756

e7218b0

Bug 1999325: Backport 107821 and 107831

Proposal: v1beta3 API overhaul #1225

Proposal: v1beta3 API overhaul #1225

Conversation

smarterclayton commented Sep 9, 2014

smarterclayton commented Sep 9, 2014

thockin commented Sep 9, 2014

smarterclayton commented Sep 9, 2014

thockin commented Sep 9, 2014

smarterclayton commented Sep 9, 2014

Choose a reason for hiding this comment

lavalamp commented Sep 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Sep 9, 2014

thockin commented Sep 9, 2014

smarterclayton commented Sep 9, 2014

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

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 Sep 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Sep 10, 2014

bgrant0607 commented Sep 26, 2014

smarterclayton commented Sep 26, 2014

Choose a reason for hiding this comment

bgrant0607 commented Sep 27, 2014

smarterclayton commented Sep 29, 2014

smarterclayton commented Sep 29, 2014

dchen1107 commented Sep 29, 2014

bgrant0607 commented Sep 30, 2014

smarterclayton commented Sep 30, 2014

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

thockin commented Sep 30, 2014

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