Skip to content

Commit

Permalink
Do not set default values when unsupported multiple containers are de…
Browse files Browse the repository at this point in the history 
…fined (knative#4709)

* Do not set default values when unsupported multiple containers are defined

If multiple containers are defined but one container is returned after
setting default values, inappropriate error happens.

To fix it, this patch stops setting default values when unsupported
multiple containers are defined.

* Add SetDefault to revision validation test

* Set defaults for all containers passed in thorugh revision

* Add defaulting multiple containers to revision default tests

* Add default probe to multi containers test

* Remove defaulting revision when container is not specified
  • Loading branch information
@nak3 @knative-prow-robot
nak3 authored and knative-prow-robot committed Jul 17, 2019
1 parent 936b4b0 commit 6dbec1f
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 73 deletions.
7 changes: 0 additions & 7 deletions pkg/apis/serving/v1beta1/configuration_defaults_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,6 @@ func TestConfigurationDefaulting(t *testing.T) {
Spec: ConfigurationSpec{
Template: RevisionTemplateSpec{
Spec: RevisionSpec{
PodSpec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: config.DefaultUserContainerName,
Resources: defaultResources,
ReadinessProbe: defaultProbe,
}},
},
TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds),
},
},
Expand Down
86 changes: 41 additions & 45 deletions pkg/apis/serving/v1beta1/revision_defaults.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,58 +44,54 @@ func (rs *RevisionSpec) SetDefaults(ctx context.Context) {
rs.TimeoutSeconds = &ts
}

var container corev1.Container
if len(rs.PodSpec.Containers) == 1 {
container = rs.PodSpec.Containers[0]
}
defer func() {
rs.PodSpec.Containers = []corev1.Container{container}
}()

if container.Name == "" {
container.Name = cfg.Defaults.UserContainerName(ctx)
}
for idx, _ := range rs.PodSpec.Containers {
if rs.PodSpec.Containers[idx].Name == "" {
rs.PodSpec.Containers[idx].Name = cfg.Defaults.UserContainerName(ctx)
}

if container.Resources.Requests == nil {
container.Resources.Requests = corev1.ResourceList{}
}
if _, ok := container.Resources.Requests[corev1.ResourceCPU]; !ok {
if rsrc := cfg.Defaults.RevisionCPURequest; rsrc != nil {
container.Resources.Requests[corev1.ResourceCPU] = *rsrc
if rs.PodSpec.Containers[idx].Resources.Requests == nil {
rs.PodSpec.Containers[idx].Resources.Requests = corev1.ResourceList{}
}
}
if _, ok := container.Resources.Requests[corev1.ResourceMemory]; !ok {
if rsrc := cfg.Defaults.RevisionMemoryRequest; rsrc != nil {
container.Resources.Requests[corev1.ResourceMemory] = *rsrc
if _, ok := rs.PodSpec.Containers[idx].Resources.Requests[corev1.ResourceCPU]; !ok {
if rsrc := cfg.Defaults.RevisionCPURequest; rsrc != nil {
rs.PodSpec.Containers[idx].Resources.Requests[corev1.ResourceCPU] = *rsrc
}
}
if _, ok := rs.PodSpec.Containers[idx].Resources.Requests[corev1.ResourceMemory]; !ok {
if rsrc := cfg.Defaults.RevisionMemoryRequest; rsrc != nil {
rs.PodSpec.Containers[idx].Resources.Requests[corev1.ResourceMemory] = *rsrc
}
}
}

if container.Resources.Limits == nil {
container.Resources.Limits = corev1.ResourceList{}
}
if _, ok := container.Resources.Limits[corev1.ResourceCPU]; !ok {
if rsrc := cfg.Defaults.RevisionCPULimit; rsrc != nil {
container.Resources.Limits[corev1.ResourceCPU] = *rsrc
if rs.PodSpec.Containers[idx].Resources.Limits == nil {
rs.PodSpec.Containers[idx].Resources.Limits = corev1.ResourceList{}
}
}
if _, ok := container.Resources.Limits[corev1.ResourceMemory]; !ok {
if rsrc := cfg.Defaults.RevisionMemoryLimit; rsrc != nil {
container.Resources.Limits[corev1.ResourceMemory] = *rsrc
if _, ok := rs.PodSpec.Containers[idx].Resources.Limits[corev1.ResourceCPU]; !ok {
if rsrc := cfg.Defaults.RevisionCPULimit; rsrc != nil {
rs.PodSpec.Containers[idx].Resources.Limits[corev1.ResourceCPU] = *rsrc
}
}
if _, ok := rs.PodSpec.Containers[idx].Resources.Limits[corev1.ResourceMemory]; !ok {
if rsrc := cfg.Defaults.RevisionMemoryLimit; rsrc != nil {
rs.PodSpec.Containers[idx].Resources.Limits[corev1.ResourceMemory] = *rsrc
}
}
if rs.PodSpec.Containers[idx].ReadinessProbe == nil {
rs.PodSpec.Containers[idx].ReadinessProbe = &corev1.Probe{}
}
if rs.PodSpec.Containers[idx].ReadinessProbe.TCPSocket == nil &&
rs.PodSpec.Containers[idx].ReadinessProbe.HTTPGet == nil &&
rs.PodSpec.Containers[idx].ReadinessProbe.Exec == nil {
rs.PodSpec.Containers[idx].ReadinessProbe.TCPSocket = &corev1.TCPSocketAction{}
}
}
if container.ReadinessProbe == nil {
container.ReadinessProbe = &corev1.Probe{}
}
if container.ReadinessProbe.TCPSocket == nil && container.ReadinessProbe.HTTPGet == nil && container.ReadinessProbe.Exec == nil {
container.ReadinessProbe.TCPSocket = &corev1.TCPSocketAction{}
}

if container.ReadinessProbe.SuccessThreshold == 0 {
container.ReadinessProbe.SuccessThreshold = 1
}
if rs.PodSpec.Containers[idx].ReadinessProbe.SuccessThreshold == 0 {
rs.PodSpec.Containers[idx].ReadinessProbe.SuccessThreshold = 1
}

vms := container.VolumeMounts
for i := range vms {
vms[i].ReadOnly = true
vms := rs.PodSpec.Containers[idx].VolumeMounts
for i := range vms {
vms[i].ReadOnly = true
}
}
}
48 changes: 34 additions & 14 deletions pkg/apis/serving/v1beta1/revision_defaults_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,21 +55,10 @@ func TestRevisionDefaulting(t *testing.T) {
}{{
name: "empty",
in: &Revision{},
want: &Revision{
Spec: RevisionSpec{
TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds),
PodSpec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: config.DefaultUserContainerName,
Resources: defaultResources,
ReadinessProbe: defaultProbe,
}},
},
},
},
want: &Revision{Spec: RevisionSpec{TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds)}},
}, {
name: "with context",
in: &Revision{},
in: &Revision{Spec: RevisionSpec{PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}}},
wc: func(ctx context.Context) context.Context {
s := config.NewStore(logtesting.TestLogger(t))
s.OnConfigChanged(&corev1.ConfigMap{
Expand Down Expand Up @@ -209,7 +198,9 @@ func TestRevisionDefaulting(t *testing.T) {
}, {
name: "partially initialized",
in: &Revision{
Spec: RevisionSpec{},
Spec: RevisionSpec{
PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}},
},
},
want: &Revision{
Spec: RevisionSpec{
Expand All @@ -223,6 +214,35 @@ func TestRevisionDefaulting(t *testing.T) {
},
},
},
}, {
name: "multiple containers",
in: &Revision{
Spec: RevisionSpec{
PodSpec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: "busybox",
}, {
Name: "helloworld",
}},
},
},
},
want: &Revision{
Spec: RevisionSpec{
TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds),
PodSpec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: "busybox",
Resources: defaultResources,
ReadinessProbe: defaultProbe,
}, {
Name: "helloworld",
Resources: defaultResources,
ReadinessProbe: defaultProbe,
}},
},
},
},
}}

for _, test := range tests {
Expand Down
7 changes: 0 additions & 7 deletions pkg/apis/serving/v1beta1/service_defaults_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,6 @@ func TestServiceDefaulting(t *testing.T) {
ConfigurationSpec: ConfigurationSpec{
Template: RevisionTemplateSpec{
Spec: RevisionSpec{
PodSpec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: config.DefaultUserContainerName,
Resources: defaultResources,
ReadinessProbe: defaultProbe,
}},
},
TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds),
},
},
Expand Down

0 comments on commit 6dbec1f

Please sign in to comment.