Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read BoundPods from etcd instead of ContainerManifestList #1662

Closed
wants to merge 3 commits into from
Closed

Read BoundPods from etcd instead of ContainerManifestList #1662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad403fd
Read BoundPods from etcd instead of ContainerManifestList
smarterclayton Oct 8, 2014
5053a45
Write BoundPods to etcd instead of ContainerManifestList
smarterclayton Oct 9, 2014
2325140
Rename the etcd path for pods to be /registry/nodes/<>/boundpods
smarterclayton Oct 14, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions pkg/api/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,64 @@ limitations under the License.
package api

import (
"github.com/GoogleCloudPlatform/kubernetes/pkg/conversion"
"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
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

out.UID = in.UUID
return nil
},
func(in *BoundPod, out *ContainerManifest, s conversion.Scope) error {
out.Containers = in.Spec.Containers
out.Volumes = in.Spec.Volumes
out.RestartPolicy = in.Spec.RestartPolicy
out.Version = "v1beta2"
out.ID = in.ID
out.UUID = in.UID
return nil
},
func(in *ContainerManifestList, out *BoundPods, s conversion.Scope) error {
if err := s.Convert(&in.Items, &out.Items, 0); err != nil {
return err
}
for i := range out.Items {
item := &out.Items[i]
item.ResourceVersion = in.ResourceVersion
}
return nil
},
func(in *BoundPods, out *ContainerManifestList, s conversion.Scope) error {
if err := s.Convert(&in.Items, &out.Items, 0); err != nil {
return err
}
out.ResourceVersion = in.ResourceVersion
return nil
},

// Convert Pod to BoundPod
func(in *Pod, out *BoundPod, s conversion.Scope) error {
if err := s.Convert(&in.DesiredState.Manifest, out, 0); err != nil {
return err
}
// Only copy a subset of fields, and override manifest attributes with the pod
// metadata
out.UID = in.UID
out.ID = in.ID
out.Namespace = in.Namespace
out.CreationTimestamp = in.CreationTimestamp
return nil
},
)
}
4 changes: 4 additions & 0 deletions pkg/api/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@ func init() {
&Binding{},
&Event{},
&EventList{},
&ContainerManifest{},
&ContainerManifestList{},
&BoundPod{},
&BoundPods{},
)
}
Expand All @@ -61,5 +63,7 @@ func (*ServerOp) IsAnAPIObject() {}
func (*ServerOpList) IsAnAPIObject() {}
func (*Event) IsAnAPIObject() {}
func (*EventList) IsAnAPIObject() {}
func (*ContainerManifest) IsAnAPIObject() {}
func (*ContainerManifestList) IsAnAPIObject() {}
func (*BoundPod) IsAnAPIObject() {}
func (*BoundPods) IsAnAPIObject() {}
4 changes: 4 additions & 0 deletions pkg/api/serialization_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,10 @@ func TestTypes(t *testing.T) {
t.Errorf("Couldn't make a %v? %v", kind, err)
continue
}
if _, err := runtime.FindTypeMeta(item); err != nil {
t.Logf("%s is not a TypeMeta and cannot be round tripped: %v", kind, err)
continue
}
runTest(t, v1beta1.Codec, item)
runTest(t, v1beta2.Codec, item)
runTest(t, api.Codec, item)
Expand Down
4 changes: 4 additions & 0 deletions pkg/api/v1beta1/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ func init() {
&ServerOpList{},
&Event{},
&EventList{},
&ContainerManifest{},
&ContainerManifestList{},
&BoundPod{},
&BoundPods{},
)
}
Expand All @@ -63,5 +65,7 @@ func (*ServerOp) IsAnAPIObject() {}
func (*ServerOpList) IsAnAPIObject() {}
func (*Event) IsAnAPIObject() {}
func (*EventList) IsAnAPIObject() {}
func (*ContainerManifest) IsAnAPIObject() {}
func (*ContainerManifestList) IsAnAPIObject() {}
func (*BoundPod) IsAnAPIObject() {}
func (*BoundPods) IsAnAPIObject() {}
4 changes: 4 additions & 0 deletions pkg/api/v1beta2/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ func init() {
&ServerOpList{},
&Event{},
&EventList{},
&ContainerManifest{},
&ContainerManifestList{},
&BoundPod{},
&BoundPods{},
)
}
Expand All @@ -63,5 +65,7 @@ func (*ServerOp) IsAnAPIObject() {}
func (*ServerOpList) IsAnAPIObject() {}
func (*Event) IsAnAPIObject() {}
func (*EventList) IsAnAPIObject() {}
func (*ContainerManifest) IsAnAPIObject() {}
func (*ContainerManifestList) IsAnAPIObject() {}
func (*BoundPod) IsAnAPIObject() {}
func (*BoundPods) IsAnAPIObject() {}
22 changes: 22 additions & 0 deletions pkg/api/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,3 +437,25 @@ func ValidateReadOnlyPersistentDisks(volumes []api.Volume) errs.ErrorList {
}
return allErrs
}

// ValidateBoundPod tests if required fields on a bound pod are set.
func ValidateBoundPod(pod *api.BoundPod) (errors []error) {
if !util.IsDNSSubdomain(pod.ID) {
errors = append(errors, errs.NewFieldInvalid("id", pod.ID))
}
if !util.IsDNSSubdomain(pod.Namespace) {
Copy link
Member

Choose a reason for hiding this comment

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

I thought namespace could be empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All pods are required to have a namespace ("default" should be set at create) - the lack of a namespace is not allowed.

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

@@ -433,3 +433,25 @@ func ValidateReadOnlyPersistentDisks(volumes
[]api.Volume) errs.ErrorList {
}
return allErrs
}
+
+// ValidateBoundPod tests if required fields on a bound pod are set.
+func ValidateBoundPod(pod *api.BoundPod) (errors []error) {

  • if !util.IsDNSSubdomain(pod.ID) {
  •   errors = append(errors, errs.NewFieldInvalid("id", pod.ID))
  • }
  • if !util.IsDNSSubdomain(pod.Namespace) {

I thought namespace could be empty?

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

Copy link
Member

Choose a reason for hiding this comment

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

Except in the other PR there was discussion that "" == "default" - Do we need to hold this up until that lands?

Copy link
Member

Choose a reason for hiding this comment

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

BTW, I agree that "" == default, I thought that was what we had decided, or I would have argued for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that should block this pull

On Oct 15, 2014, at 1:05 AM, Tim Hockin notifications@github.com wrote:

In pkg/api/validation/validation.go:

@@ -433,3 +433,25 @@ func ValidateReadOnlyPersistentDisks(volumes []api.Volume) errs.ErrorList {
}
return allErrs
}
+
+// ValidateBoundPod tests if required fields on a bound pod are set.
+func ValidateBoundPod(pod *api.BoundPod) (errors []error) {

  • if !util.IsDNSSubdomain(pod.ID) {
  •   errors = append(errors, errs.NewFieldInvalid("id", pod.ID))
  • }
  • if !util.IsDNSSubdomain(pod.Namespace) {
    Except in the other PR there was discussion that "" == "default" - Do we need to hold this up until that lands?

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

errors = append(errors, errs.NewFieldInvalid("namespace", pod.Namespace))
}
containerManifest := &api.ContainerManifest{
Version: "v1beta2",
ID: pod.ID,
UUID: pod.UID,
Containers: pod.Spec.Containers,
Volumes: pod.Spec.Volumes,
RestartPolicy: pod.Spec.RestartPolicy,
}
if errs := ValidateManifest(containerManifest); len(errs) != 0 {
errors = append(errors, errs...)
}
return errors
}
18 changes: 18 additions & 0 deletions pkg/api/validation/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -823,3 +823,21 @@ func TestValidateReplicationController(t *testing.T) {
}
}
}

func TestValidateBoundPodNoName(t *testing.T) {
errorCases := map[string]api.BoundPod{
// manifest is tested in api/validation_test.go, ensure it is invoked
"empty version": {TypeMeta: api.TypeMeta{ID: "test"}, Spec: api.PodSpec{Containers: []api.Container{{Name: ""}}}},

// Name
"zero-length name": {TypeMeta: api.TypeMeta{ID: ""}},
"name > 255 characters": {TypeMeta: api.TypeMeta{ID: strings.Repeat("a", 256)}},
"name not a DNS subdomain": {TypeMeta: api.TypeMeta{ID: "a.b.c."}},
"name with underscore": {TypeMeta: api.TypeMeta{ID: "a_b_c"}},
}
for k, v := range errorCases {
if errs := ValidateBoundPod(&v); len(errs) == 0 {
t.Errorf("expected failure for %s", k)
}
}
}
8 changes: 5 additions & 3 deletions pkg/cloudprovider/controller/minioncontroller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,12 @@ import (
)

func NewTestEtcdRegistry(client tools.EtcdClient) *etcdregistry.Registry {
registry := etcdregistry.NewRegistry(tools.EtcdHelper{client, latest.Codec, tools.RuntimeVersionAdapter{latest.ResourceVersioner}},
&pod.BasicManifestFactory{
registry := etcdregistry.NewRegistry(
tools.EtcdHelper{client, latest.Codec, tools.RuntimeVersionAdapter{latest.ResourceVersioner}},
&pod.BasicBoundPodFactory{
ServiceRegistry: &registrytest.ServiceRegistry{},
})
},
)
return registry
}

Expand Down
6 changes: 3 additions & 3 deletions pkg/constraint/constraint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ import (
"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
)

// Allowed returns true if manifests is a collection of manifests
// Allowed returns true if pods is a collection of bound pods
// which can run without conflict on a single minion.
func Allowed(manifests []api.ContainerManifest) bool {
return !PortsConflict(manifests)
func Allowed(pods []api.BoundPod) bool {
return !PortsConflict(pods)
}
36 changes: 18 additions & 18 deletions pkg/constraint/constraint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,69 +30,69 @@ func containerWithHostPorts(ports ...int) api.Container {
return c
}

func manifestWithContainers(containers ...api.Container) api.ContainerManifest {
m := api.ContainerManifest{}
func podWithContainers(containers ...api.Container) api.BoundPod {
m := api.BoundPod{}
for _, c := range containers {
m.Containers = append(m.Containers, c)
m.Spec.Containers = append(m.Spec.Containers, c)
}
return m
}

func TestAllowed(t *testing.T) {
table := []struct {
allowed bool
manifests []api.ContainerManifest
allowed bool
pods []api.BoundPod
}{
{
allowed: true,
manifests: []api.ContainerManifest{
manifestWithContainers(
pods: []api.BoundPod{
podWithContainers(
containerWithHostPorts(1, 2, 3),
containerWithHostPorts(4, 5, 6),
),
manifestWithContainers(
podWithContainers(
containerWithHostPorts(7, 8, 9),
containerWithHostPorts(10, 11, 12),
),
},
},
{
allowed: true,
manifests: []api.ContainerManifest{
manifestWithContainers(
pods: []api.BoundPod{
podWithContainers(
containerWithHostPorts(0, 0),
containerWithHostPorts(0, 0),
),
manifestWithContainers(
podWithContainers(
containerWithHostPorts(0, 0),
containerWithHostPorts(0, 0),
),
},
},
{
allowed: false,
manifests: []api.ContainerManifest{
manifestWithContainers(
pods: []api.BoundPod{
podWithContainers(
containerWithHostPorts(3, 3),
),
},
},
{
allowed: false,
manifests: []api.ContainerManifest{
manifestWithContainers(
pods: []api.BoundPod{
podWithContainers(
containerWithHostPorts(6),
),
manifestWithContainers(
podWithContainers(
containerWithHostPorts(6),
),
},
},
}

for _, item := range table {
if e, a := item.allowed, Allowed(item.manifests); e != a {
t.Errorf("Expected %v, got %v: \n%v\v", e, a, item.manifests)
if e, a := item.allowed, Allowed(item.pods); e != a {
t.Errorf("Expected %v, got %v: \n%v\v", e, a, item.pods)
}
}
}
6 changes: 3 additions & 3 deletions pkg/constraint/ports.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ import (

// PortsConflict returns true iff two containers attempt to expose
// the same host port.
func PortsConflict(manifests []api.ContainerManifest) bool {
func PortsConflict(pods []api.BoundPod) bool {
hostPorts := map[int]struct{}{}
for _, manifest := range manifests {
for _, container := range manifest.Containers {
for _, pod := range pods {
for _, container := range pod.Spec.Containers {
for _, port := range container.Ports {
if port.HostPort == 0 {
continue
Expand Down
Loading