Read BoundPods from etcd instead of ContainerManifestList #1662

smarterclayton · 2014-10-08T22:52:49Z

There are three values that uniquely identify a pod on a host - the configuration source (etcd, file, http), the pod name, and the pod namespace. This change ensures that configuration properly makes those
names unique by changing podFullName to contain both name (currently ID in v1beta1, Name in v1beta3) and namespace.

The Kubelet does not properly handle information requests for pods not in the default namespace at this time.

When complete, this will represent a breaking schema change for etcd. Only the last commit two commits are new, the others are covered.

Extracted from #1600

smarterclayton · 2014-10-08T22:53:37Z

@thockin you may want to take a peek at this prior to it being complete

smarterclayton · 2014-10-08T23:02:25Z

Special note: I made use of Annotations to distinguish config sources (see pkg/kubelet/types.go), may need review for name-spacing implications of properties we set.

smarterclayton · 2014-10-09T17:03:20Z

@thockin I'm contemplating changing the registry key from /registry/host/<machine>/kubelet to something like /registry/node/<machine>/pods - thoughts?

smarterclayton · 2014-10-09T20:32:48Z

@thockin ready for review

smarterclayton · 2014-10-14T03:44:23Z

Rebased

thockin · 2014-10-14T04:04:38Z

Sorry - busy week. On it now.

thockin · 2014-10-14T04:20:15Z

pkg/api/conversion.go

+			out.Spec.Containers = in.Containers
+			out.Spec.Volumes = in.Volumes
+			out.Spec.RestartPolicy = in.RestartPolicy
+			out.ID = in.ID


Maybe I totally missed something - how did we end up with ID and UID at the same time? Or is this just transient? I'm assuming it's just transient?

Just transient - I won't rename ID -> Name until after this happens.

----- Original Message -----

"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"

)

// Codec is the identity codec for this package - it can only convert
itself
// to itself.
var Codec = runtime.CodecFor(Scheme, "")
+
+func init() {

Scheme.AddConversionFuncs(

// Convert ContainerManifest to BoundPod

func(in *ContainerManifest, out *BoundPod, s conversion.Scope) error {

out.Spec.Containers = in.Containers

out.Spec.Volumes = in.Volumes

out.Spec.RestartPolicy = in.RestartPolicy

out.ID = in.ID

Maybe I totally missed something - how did we end up with ID and UID at the
same time? Or is this just transient? I'm assuming it's just transient?

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

thockin · 2014-10-14T04:36:42Z

Annotation: I think what you did is consistent with what we want. However - do we need the annotation, or is it speculative?

Registry key change: SGTM

Otherwise my nits are pretty minor. Since I haven't been paying attention to the in-progress transmogrification, I can only assume that ID will become Name at some point?

smarterclayton · 2014-10-14T20:49:25Z

----- Original Message -----

Annotation: I think what you did is consistent with what we want. However -
do we need the annotation, or is it speculative?

We at a minimum have to identify three unique attributes on each pod - name, namespace, and source of config, and a pod must be unique across all of those (so that you can't have a file source that stomps an apiserver source). It felt more correct to separate those three than to attempt to combine source and namespace and lose the original attribution. Adding values to the end of the BoundPod Namespace attribute felt wrong because that represented a namespace on the server, not a namespace on the Kubelet.

Registry key change: SGTM

Will make that change.

Otherwise my nits are pretty minor. Since I haven't been paying attention to
the in-progress transmogrification, I can only assume that ID will become
Name at some point?

Yes, that's tracked in #1519 and #1600 - this is the last of prereq changes to reduce the scope of #1600.

Reply to this email directly or view it on GitHub:
#1662 (comment)

smarterclayton · 2014-10-14T22:08:24Z

Rebased, renamed the etcd path. Open question on file names and whether we should be enforcing new pattern, old pattern, or redecide.

thockin · 2014-10-15T05:12:51Z

Open question re: "" vs "default.

For filenames I opened a new issue.

Re: annotations, I am not too worried about files and etcd colliding, though I guess they could. Files will be more tightly controlled than random namespaces, anyway, I presume. Either way this is fine, I was just asking.

smarterclayton · 2014-10-15T16:39:25Z

I renamed the file, still waiting on closure of namespace

There are three values that uniquely identify a pod on a host - the configuration source (etcd, file, http), the pod name, and the pod namespace. This change ensures that configuration properly makes those names unique by changing podFullName to contain both name (currently ID in v1beta1, Name in v1beta3) and namespace. The Kubelet does not properly handle information requests for pods not in the default namespace at this time.

Rename ManifestFactory -> BoundPodFactory and change the general structure of the call to focus on BoundPod.

smarterclayton · 2014-10-15T21:41:36Z

Rebased and fixed conflict with another pull, namespace has been punted on and I believe this is ready to merge at @thockin's pleasure on break day.

thockin · 2014-10-15T21:43:06Z

I declare breakage day to be tomorrow. I will merge PRs and watch travis
and jenkins.

On Wed, Oct 15, 2014 at 2:41 PM, Clayton Coleman notifications@github.com
wrote:

Rebased and fixed conflict with another pull, namespace has been punted on
and I believe this is ready to merge at @thockin
https://github.com/thockin's pleasure on break day.

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

smarterclayton · 2014-10-15T21:44:33Z

Set a time so I can clear my schedule for abuse

----- Original Message -----

I declare breakage day to be tomorrow. I will merge PRs and watch travis
and jenkins.

On Wed, Oct 15, 2014 at 2:41 PM, Clayton Coleman notifications@github.com
wrote:

Rebased and fixed conflict with another pull, namespace has been punted on
and I believe this is ready to merge at @thockin
https://github.com/thockin's pleasure on break day.

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

Reply to this email directly or view it on GitHub:
#1662 (comment)

thockin · 2014-10-15T21:46:32Z

I'm hoping for 10:00 am here. Tying off last nits on my PR.

On Wed, Oct 15, 2014 at 2:44 PM, Clayton Coleman notifications@github.com
wrote:

Set a time so I can clear my schedule for abuse

----- Original Message -----

I declare breakage day to be tomorrow. I will merge PRs and watch travis
and jenkins.

On Wed, Oct 15, 2014 at 2:41 PM, Clayton Coleman <
notifications@github.com>
wrote:

Rebased and fixed conflict with another pull, namespace has been
punted on
and I believe this is ready to merge at @thockin
https://github.com/thockin's pleasure on break day.

Reply to this email directly or view it on GitHub
<
#1662 (comment)

.

Reply to this email directly or view it on GitHub:

#1662 (comment)

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

eparis · 2014-10-16T19:04:33Z

I opened #1840 which is just a rebase of this on top of Services v2 and namespaces. One minor conflict. Passed Travis. running e2e against that rebase...

thockin · 2014-10-16T23:58:32Z

Replaced by #1840

…erry-pick-1657-to-release-4.12 [release-4.12] OCPBUGS-17159: Increase service idle max timeout to 100 minutes

smarterclayton force-pushed the bind_pods branch from 31578da to 435727e Compare October 8, 2014 23:01

smarterclayton mentioned this pull request Oct 9, 2014

Implement v1beta3 api #1519

Closed

20 tasks

smarterclayton force-pushed the bind_pods branch from 35f0d93 to 464fa21 Compare October 9, 2014 20:25

smarterclayton changed the title ~~WIP - Read BoundPods from etcd instead of ContainerManifestList~~ Read BoundPods from etcd instead of ContainerManifestList Oct 9, 2014

smarterclayton force-pushed the bind_pods branch from 464fa21 to ea8f2f3 Compare October 9, 2014 20:32

smarterclayton assigned thockin Oct 9, 2014

This was referenced Oct 10, 2014

issue tracking: use BoundPods instead of ContainerManifestList stevendanna/cookbook-r#20

Closed

issue tracking: use BoundPods instead of ContainerManifestList d2iq-archive/kubernetes-mesos#36

Closed

smarterclayton mentioned this pull request Oct 13, 2014

Pass pod labels to containers and to lifecycle hooks #560

Closed

smarterclayton force-pushed the bind_pods branch from ea8f2f3 to c0a98bb Compare October 14, 2014 03:44

thockin reviewed Oct 14, 2014
View reviewed changes

smarterclayton force-pushed the bind_pods branch from cd55708 to f69abf8 Compare October 14, 2014 21:32

smarterclayton force-pushed the bind_pods branch from f69abf8 to eff2219 Compare October 15, 2014 16:39

smarterclayton added 3 commits October 15, 2014 17:37

Write BoundPods to etcd instead of ContainerManifestList

5053a45

Rename ManifestFactory -> BoundPodFactory and change the general structure of the call to focus on BoundPod.

Rename the etcd path for pods to be /registry/nodes/<>/boundpods

2325140

smarterclayton force-pushed the bind_pods branch from eff2219 to 2325140 Compare October 15, 2014 21:41

thockin mentioned this pull request Oct 16, 2014

Rackspace - Switch to CoreOS for standard cluster #1832

Merged

eparis mentioned this pull request Oct 16, 2014

Bind pods #1840

Merged

jbeda force-pushed the master branch from 89ee618 to f61d434 Compare October 16, 2014 23:44

thockin closed this Oct 16, 2014

soltysh pushed a commit to soltysh/kubernetes that referenced this pull request Aug 21, 2023

Merge pull request kubernetes#1662 from openshift-cherrypick-robot/ch…

3b52eeb

…erry-pick-1657-to-release-4.12 [release-4.12] OCPBUGS-17159: Increase service idle max timeout to 100 minutes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read BoundPods from etcd instead of ContainerManifestList #1662

Read BoundPods from etcd instead of ContainerManifestList #1662

smarterclayton commented Oct 8, 2014

smarterclayton commented Oct 8, 2014

smarterclayton commented Oct 8, 2014

smarterclayton commented Oct 9, 2014

smarterclayton commented Oct 9, 2014

smarterclayton commented Oct 14, 2014

thockin commented Oct 14, 2014

thockin Oct 14, 2014

smarterclayton Oct 14, 2014

thockin commented Oct 14, 2014

smarterclayton commented Oct 14, 2014

smarterclayton commented Oct 14, 2014

thockin commented Oct 15, 2014

smarterclayton commented Oct 15, 2014

smarterclayton commented Oct 15, 2014

thockin commented Oct 15, 2014

smarterclayton commented Oct 15, 2014

thockin commented Oct 15, 2014

eparis commented Oct 16, 2014

thockin commented Oct 16, 2014

Read BoundPods from etcd instead of ContainerManifestList #1662

Read BoundPods from etcd instead of ContainerManifestList #1662

Conversation

smarterclayton commented Oct 8, 2014

smarterclayton commented Oct 8, 2014

smarterclayton commented Oct 8, 2014

smarterclayton commented Oct 9, 2014

smarterclayton commented Oct 9, 2014

smarterclayton commented Oct 14, 2014

thockin commented Oct 14, 2014

thockin Oct 14, 2014

Choose a reason for hiding this comment

smarterclayton Oct 14, 2014

Choose a reason for hiding this comment

thockin commented Oct 14, 2014

smarterclayton commented Oct 14, 2014

smarterclayton commented Oct 14, 2014

thockin commented Oct 15, 2014

smarterclayton commented Oct 15, 2014

smarterclayton commented Oct 15, 2014

thockin commented Oct 15, 2014

smarterclayton commented Oct 15, 2014

thockin commented Oct 15, 2014

eparis commented Oct 16, 2014

thockin commented Oct 16, 2014