Skip to content

[WIP] PoC for multicontainer support #6739

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

Closed
savitaashture wants to merge 7 commits into from
Closed

[WIP] PoC for multicontainer support #6739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0217cb3
[WIP] PoC for multicontainer support
savitaashture Feb 5, 2020
8b7c77c
fix review comments
savitaashture Feb 6, 2020
22a6b25
handle port validation for both single and multicontainer
savitaashture Feb 6, 2020
29b8ab8
fix review comments
savitaashture Feb 6, 2020
68736d7
update update-codegen.sh for ImageDigests
savitaashture Feb 8, 2020
478fb07
fix conflict issue
savitaashture Feb 26, 2020
2f91fb9
Add unit test cases
savitaashture Feb 29, 2020
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
76 changes: 62 additions & 14 deletions pkg/apis/serving/k8s_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,17 @@ func ValidatePodSpec(ps corev1.PodSpec) *apis.FieldError {
errs = errs.Also(ValidateContainer(ps.Containers[0], volumes).
ViaFieldIndex("containers", 0))
default:
errs = errs.Also(apis.ErrMultipleOneOf("containers"))
errs = errs.Also(ValidateMultiContainerPorts(ps.Containers)).ViaField("containers")
for i := range ps.Containers {
// probes are not allowed other than serving container
// ref: https://docs.google.com/document/d/1XjIRnOGaq9UGllkZgYXQHuTQmhbECNAOk6TT6RNfJMw/edit?disco=AAAAEHNSwZU
if len(ps.Containers[i].Ports) == 0 {
errs = errs.Also(ValidateSidecarContainer(ps.Containers[i], volumes).ViaFieldIndex("containers", i))
} else {
errs = errs.Also(ValidateContainer(ps.Containers[i], volumes).ViaFieldIndex("containers", i))
}
}

}
if ps.ServiceAccountName != "" {
for range validation.IsDNS1123Subdomain(ps.ServiceAccountName) {
Expand All @@ -267,7 +277,58 @@ func ValidatePodSpec(ps corev1.PodSpec) *apis.FieldError {
return errs
}

// ValidateMultiContainerPorts validates port when specified multiple containers
func ValidateMultiContainerPorts(containers []corev1.Container) *apis.FieldError {
var (
count int
errs *apis.FieldError
)
for i := range containers {
count += len(containers[i].Ports)
}
if count == 0 {
errs = errs.Also(apis.ErrMissingField(apis.CurrentField))
}
return errs.Also(portValidation(count)).ViaField("ports")
}

// ValidateSidecarContainer validate fields for non serving containers
func ValidateSidecarContainer(container corev1.Container, volumes sets.String) *apis.FieldError {
var errs *apis.FieldError
if container.LivenessProbe != nil {
errs = errs.Also(apis.CheckDisallowedFields(*container.LivenessProbe,
*ProbeMask(&corev1.Probe{})).ViaField("livenessProbe"))
}
if container.ReadinessProbe != nil {
errs = errs.Also(apis.CheckDisallowedFields(*container.ReadinessProbe,
*ProbeMask(&corev1.Probe{})).ViaField("readinessProbe"))
}
return errs.Also(validate(container, volumes))
}

// ValidateContainer validate fields for serving containers
func ValidateContainer(container corev1.Container, volumes sets.String) *apis.FieldError {
var errs *apis.FieldError
errs = errs.Also(portValidation(len(container.Ports))).ViaField("ports")
// Liveness Probes
errs = errs.Also(validateProbe(container.LivenessProbe).ViaField("livenessProbe"))
// Readiness Probes
errs = errs.Also(validateReadinessProbe(container.ReadinessProbe).ViaField("readinessProbe"))
return errs.Also(validate(container, volumes))
}

func portValidation(count int) *apis.FieldError {
if count > 1 {
return &apis.FieldError{
Message: "More than one container port is set",
Paths: []string{apis.CurrentField},
Details: "Only a single port is allowed",
}
}
return nil
}

func validate(container corev1.Container, volumes sets.String) *apis.FieldError {
if equality.Semantic.DeepEqual(container, corev1.Container{}) {
return apis.ErrMissingField(apis.CurrentField)
}
Copy link
Copy Markdown

@dgerd dgerd Feb 5, 2020

Choose a reason for hiding this comment

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

Related to this change, but tangential: we should expand the list of reservedContainerNames in case we need to add more system sidecars later. Perhaps something with a prefix of knative-serving-system-*?

This is potentially breaking, but given that this requires named containers means that the risk of breaking only goes up after this lands.

Copy link
Copy Markdown
Contributor

@markusthoemmes markusthoemmes Feb 11, 2020

Choose a reason for hiding this comment

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

It doesn't require named containers, does it? Aren't we inferring names from the indices? @savitaashture

Copy link
Copy Markdown
Contributor Author

@savitaashture savitaashture Feb 11, 2020

Choose a reason for hiding this comment

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

Yes if user don't provide name for sidecar container we inferring names from the indices
https://github.com/knative/serving/pull/6739/files#diff-daca68b8123c709e8127c6c834c5c023R56

Expand Down Expand Up @@ -296,12 +357,8 @@ func ValidateContainer(container corev1.Container, volumes sets.String) *apis.Fi
}
errs = errs.Also(fe)
}
// Liveness Probes
errs = errs.Also(validateProbe(container.LivenessProbe).ViaField("livenessProbe"))
// Ports
errs = errs.Also(validateContainerPorts(container.Ports).ViaField("ports"))
// Readiness Probes
errs = errs.Also(validateReadinessProbe(container.ReadinessProbe).ViaField("readinessProbe"))
// Resources
errs = errs.Also(validateResources(&container.Resources).ViaField("resources"))
// SecurityContext
Expand Down Expand Up @@ -391,20 +448,11 @@ func validateContainerPorts(ports []corev1.ContainerPort) *apis.FieldError {
if len(ports) == 0 {
return nil
}

var errs *apis.FieldError

// user can set container port which names "user-port" to define application's port.
// Queue-proxy will use it to send requests to application
// if user didn't set any port, it will set default port user-port=8080.
if len(ports) > 1 {
errs = errs.Also(&apis.FieldError{
Message: "More than one container port is set",
Paths: []string{apis.CurrentField},
Details: "Only a single port is allowed",
})
}

userPort := ports[0]

errs = errs.Also(apis.CheckDisallowedFields(userPort, *ContainerPortMask(&userPort)))
Expand Down
81 changes: 79 additions & 2 deletions pkg/apis/serving/k8s_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,15 +150,92 @@ func TestPodSpecValidation(t *testing.T) {
},
want: apis.ErrMissingField("containers"),
}, {
name: "too many containers",
name: "more than one container",
ps: corev1.PodSpec{
Containers: []corev1.Container{{
Image: "busybox",
Ports: []corev1.ContainerPort{{
ContainerPort: 80,
}},
}, {
Image: "helloworld",
}},
},
want: nil,
}, {
name: "probes are not allowed for non serving containers",
ps: corev1.PodSpec{
Containers: []corev1.Container{{
Image: "busybox",
Ports: []corev1.ContainerPort{{
ContainerPort: 80,
}},
}, {
Image: "helloworld",
LivenessProbe: &corev1.Probe{
TimeoutSeconds: 1,
},
ReadinessProbe: &corev1.Probe{
TimeoutSeconds: 1,
},
}},
},
want: &apis.FieldError{
Message: "must not set the field(s)",
Paths: []string{"containers[1].livenessProbe.timeoutSeconds", "containers[1].readinessProbe.timeoutSeconds"},
},
}, {
name: "too many containers with no port",
ps: corev1.PodSpec{
Containers: []corev1.Container{{
Image: "busybox",
}, {
Image: "helloworld",
}},
},
want: apis.ErrMissingField("containers.ports"),
}, {
name: "too many containers with too many port",
ps: corev1.PodSpec{
Containers: []corev1.Container{{
Image: "busybox",
Ports: []corev1.ContainerPort{{
ContainerPort: 80,
}},
}, {
Image: "helloworld",
Ports: []corev1.ContainerPort{{
ContainerPort: 80,
}},
}},
},
want: &apis.FieldError{
Message: "More than one container port is set",
Paths: []string{"containers.ports"},
Details: "Only a single port is allowed",
},
}, {
name: "too many containers with too many port for a single container",
ps: corev1.PodSpec{
Containers: []corev1.Container{{
Image: "busybox",
Ports: []corev1.ContainerPort{{
ContainerPort: 80,
}, {
ContainerPort: 90,
}},
}, {
Image: "helloworld",
Ports: []corev1.ContainerPort{{
ContainerPort: 80,
}},
}},
},
want: apis.ErrMultipleOneOf("containers"),
want: &apis.FieldError{
Message: "More than one container port is set",
Paths: []string{"containers.ports, containers[0].ports"},
Details: "Only a single port is allowed",
},
}, {
name: "extra field",
ps: corev1.PodSpec{
Expand Down
94 changes: 57 additions & 37 deletions pkg/apis/serving/v1/revision_defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package v1

import (
"context"
"strconv"

corev1 "k8s.io/api/core/v1"
"knative.dev/pkg/apis"
Expand Down Expand Up @@ -51,52 +52,71 @@ func (rs *RevisionSpec) SetDefaults(ctx context.Context) {

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

if rs.PodSpec.Containers[idx].Resources.Requests == nil {
rs.PodSpec.Containers[idx].Resources.Requests = corev1.ResourceList{}
}
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 len(rs.PodSpec.Containers) > 1 {
rs.PodSpec.Containers[idx].Name = cfg.Defaults.UserContainerName(ctx) + "-" + strconv.Itoa(idx)
} else {
rs.PodSpec.Containers[idx].Name = cfg.Defaults.UserContainerName(ctx)
}
}
rs.applyDefault(&rs.PodSpec.Containers[idx], cfg)
}
}

if rs.PodSpec.Containers[idx].Resources.Limits == nil {
rs.PodSpec.Containers[idx].Resources.Limits = corev1.ResourceList{}
func (rs *RevisionSpec) applyDefault(container *corev1.Container, cfg *config.Config) {
if container.Resources.Requests == nil {
container.Resources.Requests = corev1.ResourceList{}
}
if _, ok := container.Resources.Requests[corev1.ResourceCPU]; !ok {
if rc := cfg.Defaults.RevisionCPURequest; rc != nil {
container.Resources.Requests[corev1.ResourceCPU] = *rc
}
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 := container.Resources.Requests[corev1.ResourceMemory]; !ok {
if rm := cfg.Defaults.RevisionMemoryRequest; rm != nil {
container.Resources.Requests[corev1.ResourceMemory] = *rm
}
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 container.Resources.Limits == nil {
container.Resources.Limits = corev1.ResourceList{}
}
if _, ok := container.Resources.Limits[corev1.ResourceCPU]; !ok {
if rc := cfg.Defaults.RevisionCPULimit; rc != nil {
container.Resources.Limits[corev1.ResourceCPU] = *rc
}
if rs.PodSpec.Containers[idx].ReadinessProbe == nil {
rs.PodSpec.Containers[idx].ReadinessProbe = &corev1.Probe{}
}
if _, ok := container.Resources.Limits[corev1.ResourceMemory]; !ok {
if rm := cfg.Defaults.RevisionMemoryLimit; rm != nil {
container.Resources.Limits[corev1.ResourceMemory] = *rm
}
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 len(rs.PodSpec.Containers) == 1 {
rs.applyProbes(container)
} else {
// If there are multiple containers then default probes will be applied to the container where user specified PORT
// default probes will not be applied for non serving containers
if len(container.Ports) != 0 {
rs.applyProbes(container)
}
}

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
}
func (*RevisionSpec) applyProbes(container *corev1.Container) {
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
}
}
35 changes: 32 additions & 3 deletions pkg/apis/serving/v1/revision_defaults_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,9 @@ func TestRevisionDefaulting(t *testing.T) {
PodSpec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: "busybox",
Ports: []corev1.ContainerPort{{
ContainerPort: 80,
}},
}, {
Name: "helloworld",
}},
Expand All @@ -319,10 +322,36 @@ func TestRevisionDefaulting(t *testing.T) {
Name: "busybox",
Resources: defaultResources,
ReadinessProbe: defaultProbe,
Ports: []corev1.ContainerPort{{
ContainerPort: 80,
}},
}, {
Name: "helloworld",
Resources: defaultResources,
ReadinessProbe: defaultProbe,
Name: "helloworld",
Resources: defaultResources,
}},
},
},
},
}, {
name: "multiple containers without container name",
in: &Revision{
Spec: RevisionSpec{
PodSpec: corev1.PodSpec{
Containers: []corev1.Container{{}, {}},
},
},
},
want: &Revision{
Spec: RevisionSpec{
TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds),
ContainerConcurrency: ptr.Int64(config.DefaultContainerConcurrency),
PodSpec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: "user-container-0",
Resources: defaultResources,
}, {
Name: "user-container-1",
Resources: defaultResources,
}},
},
},
Expand Down
9 changes: 8 additions & 1 deletion pkg/apis/serving/v1/revision_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,15 @@ func (e LastPinnedParseError) Error() string {
// It is never nil and should be exactly the specified container as guaranteed
// by validation.
func (rs *RevisionSpec) GetContainer() *corev1.Container {
if len(rs.Containers) > 0 {
switch {
case len(rs.Containers) == 1:
return &rs.Containers[0]
case len(rs.Containers) > 1:
for i := range rs.Containers {
if len(rs.Containers[i].Ports) != 0 {
return &rs.Containers[i]
}
}
}
// Should be unreachable post-validation, but here to ease testing.
return &corev1.Container{}
Expand Down
Loading