From 0217cb33769ade7250eb2d4de8246bef41a429b3 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Wed, 5 Feb 2020 20:35:07 +0530 Subject: [PATCH 1/7] [WIP] PoC for multicontainer support --- pkg/apis/serving/k8s_validation.go | 59 +++++++++- pkg/apis/serving/v1/revision_defaults.go | 96 ++++++++++------ pkg/apis/serving/v1/revision_types.go | 7 ++ .../serving/v1alpha1/revision_conversion.go | 6 +- .../serving/v1alpha1/revision_lifecycle.go | 8 +- pkg/apis/serving/v1alpha1/revision_types.go | 7 ++ pkg/reconciler/revision/cruds.go | 4 +- .../revision/reconcile_resources.go | 19 ++- pkg/reconciler/revision/resources/deploy.go | 108 +++++++++++------- .../revision/resources/imagecache.go | 17 ++- pkg/reconciler/revision/revision.go | 2 +- 11 files changed, 232 insertions(+), 101 deletions(-) diff --git a/pkg/apis/serving/k8s_validation.go b/pkg/apis/serving/k8s_validation.go index 09be210985cc..b5ce59c85997 100644 --- a/pkg/apis/serving/k8s_validation.go +++ b/pkg/apis/serving/k8s_validation.go @@ -257,7 +257,16 @@ 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 = ValidateMultiContainerPorts(ps.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(ValidateMultiContainer(&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) { @@ -267,7 +276,51 @@ func ValidatePodSpec(ps corev1.PodSpec) *apis.FieldError { return errs } +//ValidateMultiContainerPorts validates port when specified multiple containers +func ValidateMultiContainerPorts(containers []corev1.Container) *apis.FieldError { + cPort := []int32{} + var errs *apis.FieldError + for i := range containers { + for j := range containers[i].Ports { + cPort = append(cPort, containers[i].Ports[j].ContainerPort) + } + } + if len(cPort) == 0 { + errs = errs.Also(apis.ErrMissingField("containers.ports.containerPort")) + } + if len(cPort) > 1 { + errs = errs.Also(apis.ErrMultipleOneOf("containers.ports.containerPort")) + } + return errs +} + +//ValidateMultiContainer validate fields for non serving containers +func ValidateMultiContainer(containers *corev1.Container, volumes sets.String) *apis.FieldError { + var errs *apis.FieldError + validateMultiContainerProbe := func(p *corev1.Probe) *apis.FieldError { + if p != nil { + return apis.CheckDisallowedFields(*p, *ProbeMask(&corev1.Probe{})) + } + return nil + } + errs = errs.Also(validateMultiContainerProbe(containers.LivenessProbe).ViaField("livenessProbe")) + errs = errs.Also(validateMultiContainerProbe(containers.ReadinessProbe).ViaField("readinessProbe")) + errs = errs.Also(validate(*containers, volumes)) + return errs +} + +//ValidateContainer validate fields for serving containers func ValidateContainer(container corev1.Container, volumes sets.String) *apis.FieldError { + var errs *apis.FieldError + // Liveness Probes + errs = errs.Also(validateProbe(container.LivenessProbe).ViaField("livenessProbe")) + // Readiness Probes + errs = errs.Also(validateReadinessProbe(container.ReadinessProbe).ViaField("readinessProbe")) + errs = errs.Also(validate(container, volumes)) + return errs +} + +func validate(container corev1.Container, volumes sets.String) *apis.FieldError { if equality.Semantic.DeepEqual(container, corev1.Container{}) { return apis.ErrMissingField(apis.CurrentField) } @@ -296,12 +349,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 diff --git a/pkg/apis/serving/v1/revision_defaults.go b/pkg/apis/serving/v1/revision_defaults.go index 01df5b626204..464106c43fe4 100644 --- a/pkg/apis/serving/v1/revision_defaults.go +++ b/pkg/apis/serving/v1/revision_defaults.go @@ -18,6 +18,7 @@ package v1 import ( "context" + "strconv" corev1 "k8s.io/api/core/v1" "knative.dev/pkg/apis" @@ -50,53 +51,74 @@ 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 len(rs.PodSpec.Containers) > 1 { + if rs.PodSpec.Containers[idx].Name == "" { + rs.PodSpec.Containers[idx].Name = cfg.Defaults.UserContainerName(ctx) + "-" + strconv.Itoa(idx) } - } - 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 + } else { + if rs.PodSpec.Containers[idx].Name == "" { + 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 } } diff --git a/pkg/apis/serving/v1/revision_types.go b/pkg/apis/serving/v1/revision_types.go index 7b5765a251a1..2a212d5c5894 100644 --- a/pkg/apis/serving/v1/revision_types.go +++ b/pkg/apis/serving/v1/revision_types.go @@ -136,6 +136,13 @@ type RevisionStatus struct { // may be empty if the image comes from a registry listed to skip resolution. // +optional ImageDigest string `json:"imageDigest,omitempty"` + // ImageDigests holds the resolved digest for the image specified + // within .Spec.Container.Image. The digest is resolved during the creation + // of Revision. This will be filled if there are multiple container specified. + // ImageDigests holds the digest for all the .Spec.Container.Image which are non serving. + // ref: https://docs.google.com/document/d/1XjIRnOGaq9UGllkZgYXQHuTQmhbECNAOk6TT6RNfJMw/edit?disco=AAAAEHNSwZg + // +optional + ImageDigests map[string]string `json:"imageDigests,omitempty"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/pkg/apis/serving/v1alpha1/revision_conversion.go b/pkg/apis/serving/v1alpha1/revision_conversion.go index c6314d33f402..1e8f21536207 100644 --- a/pkg/apis/serving/v1alpha1/revision_conversion.go +++ b/pkg/apis/serving/v1alpha1/revision_conversion.go @@ -62,10 +62,8 @@ func (source *RevisionSpec) ConvertTo(ctx context.Context, sink *v1.RevisionSpec Volumes: source.Volumes, ImagePullSecrets: source.ImagePullSecrets, } - case len(source.Containers) == 1: + case len(source.Containers) != 0: sink.PodSpec = source.PodSpec - case len(source.Containers) > 1: - return apis.ErrMultipleOneOf("containers") default: return apis.ErrMissingOneOf("container", "containers") } @@ -82,6 +80,7 @@ func (source *RevisionStatus) ConvertTo(ctx context.Context, sink *v1.RevisionSt sink.ServiceName = source.ServiceName sink.LogURL = source.LogURL sink.ImageDigest = source.ImageDigest + sink.ImageDigests = source.ImageDigests } // ConvertFrom implements apis.Convertible @@ -114,4 +113,5 @@ func (sink *RevisionStatus) ConvertFrom(ctx context.Context, source v1.RevisionS sink.ServiceName = source.ServiceName sink.LogURL = source.LogURL sink.ImageDigest = source.ImageDigest + sink.ImageDigests = source.ImageDigests } diff --git a/pkg/apis/serving/v1alpha1/revision_lifecycle.go b/pkg/apis/serving/v1alpha1/revision_lifecycle.go index 7c333281e4da..e1d54b7a161c 100644 --- a/pkg/apis/serving/v1alpha1/revision_lifecycle.go +++ b/pkg/apis/serving/v1alpha1/revision_lifecycle.go @@ -76,7 +76,13 @@ func (rs *RevisionSpec) GetContainer() *corev1.Container { if rs.DeprecatedContainer != nil { return rs.DeprecatedContainer } - if len(rs.Containers) > 0 { + if len(rs.Containers) > 1 { + for i := range rs.Containers { + if len(rs.Containers[i].Ports) != 0 { + return &rs.Containers[i] + } + } + } else { return &rs.Containers[0] } // Should be unreachable post-validation, but here to ease testing. diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index e3c4ce8f943e..b3ed4b6d9532 100644 --- a/pkg/apis/serving/v1alpha1/revision_types.go +++ b/pkg/apis/serving/v1alpha1/revision_types.go @@ -192,6 +192,13 @@ type RevisionStatus struct { // may be empty if the image comes from a registry listed to skip resolution. // +optional ImageDigest string `json:"imageDigest,omitempty"` + // ImageDigests holds the resolved digest for the image specified + // within .Spec.Container.Image. The digest is resolved during the creation + // of Revision. This will be filled if there are multiple container specified. + // ImageDigests holds the digest for all the .Spec.Container.Image which are non serving. + // ref: https://docs.google.com/document/d/1XjIRnOGaq9UGllkZgYXQHuTQmhbECNAOk6TT6RNfJMw/edit?disco=AAAAEHNSwZg + // +optional + ImageDigests map[string]string `json:"imageDigests,omitempty"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/pkg/reconciler/revision/cruds.go b/pkg/reconciler/revision/cruds.go index 6100ed2910b0..3610cedc88db 100644 --- a/pkg/reconciler/revision/cruds.go +++ b/pkg/reconciler/revision/cruds.go @@ -109,8 +109,8 @@ func (c *Reconciler) checkAndUpdateDeployment(ctx context.Context, rev *v1.Revis return d, nil } -func (c *Reconciler) createImageCache(ctx context.Context, rev *v1.Revision) (*caching.Image, error) { - image := resources.MakeImageCache(rev) +func (c *Reconciler) createImageCache(rev *v1alpha1.Revision, containerName string) (*caching.Image, error) { + image := resources.MakeImageCache(rev, containerName) return c.cachingclient.CachingV1alpha1().Images(image.Namespace).Create(image) } diff --git a/pkg/reconciler/revision/reconcile_resources.go b/pkg/reconciler/revision/reconcile_resources.go index 13360eae0cd6..be2d6a28e1d6 100644 --- a/pkg/reconciler/revision/reconcile_resources.go +++ b/pkg/reconciler/revision/reconcile_resources.go @@ -113,18 +113,17 @@ func (c *Reconciler) reconcileImageCache(ctx context.Context, rev *v1.Revision) logger := logging.FromContext(ctx) ns := rev.Namespace - imageName := resourcenames.ImageCache(rev) - _, err := c.imageLister.Images(ns).Get(imageName) - if apierrs.IsNotFound(err) { - _, err := c.createImageCache(ctx, rev) - if err != nil { - return fmt.Errorf("failed to create image cache %q: %w", imageName, err) + for i := range rev.Spec.Containers { + imageName := resourcenames.ImageCache(rev) + "-" + rev.Spec.Containers[i].Name + if _, err := c.imageLister.Images(ns).Get(imageName); apierrs.IsNotFound(err) { + if _, err := c.createImageCache(rev, rev.Spec.Containers[i].Name); err != nil { + return fmt.Errorf("failed to create image cache %q: %w", imageName, err) + } + logger.Infof("Created image cache %q", imageName) + } else if err != nil { + return fmt.Errorf("failed to get image cache %q: %w", imageName, err) } - logger.Infof("Created image cache %q", imageName) - } else if err != nil { - return fmt.Errorf("failed to get image cache %q: %w", imageName, err) } - return nil } diff --git a/pkg/reconciler/revision/resources/deploy.go b/pkg/reconciler/revision/resources/deploy.go index 1b64963cc6cc..47404fbb3b6d 100644 --- a/pkg/reconciler/revision/resources/deploy.go +++ b/pkg/reconciler/revision/resources/deploy.go @@ -137,53 +137,45 @@ func rewriteUserProbe(p *corev1.Probe, userPort int) { func makePodSpec(rev *v1.Revision, loggingConfig *logging.Config, tracingConfig *tracingconfig.Config, observabilityConfig *metrics.ObservabilityConfig, autoscalerConfig *autoscalerconfig.Config, deploymentConfig *deployment.Config) (*corev1.PodSpec, error) { queueContainer, err := makeQueueContainer(rev, loggingConfig, tracingConfig, observabilityConfig, autoscalerConfig, deploymentConfig) - if err != nil { return nil, fmt.Errorf("failed to create queue-proxy container: %w", err) } - userContainer := rev.Spec.GetContainer().DeepCopy() - // Adding or removing an overwritten corev1.Container field here? Don't forget to - // update the fieldmasks / validations in pkg/apis/serving - - userContainer.VolumeMounts = append(userContainer.VolumeMounts, varLogVolumeMount) - userContainer.Lifecycle = userLifecycle - userPort := getUserPort(rev) - userPortInt := int(userPort) - userPortStr := strconv.Itoa(userPortInt) - // Replacement is safe as only up to a single port is allowed on the Revision - userContainer.Ports = buildContainerPorts(userPort) - userContainer.Env = append(userContainer.Env, buildUserPortEnv(userPortStr)) - userContainer.Env = append(userContainer.Env, getKnativeEnvVar(rev)...) - // Explicitly disable stdin and tty allocation - userContainer.Stdin = false - userContainer.TTY = false - - // Prefer imageDigest from revision if available - if rev.Status.ImageDigest != "" { - userContainer.Image = rev.Status.ImageDigest + containers := []corev1.Container{ + *queueContainer, } - if userContainer.TerminationMessagePolicy == "" { - userContainer.TerminationMessagePolicy = corev1.TerminationMessageFallbackToLogsOnError + // closure function for serving container + servingContainer := func() { + userContainer := createServingContainer(rev) + // Prefer imageDigest from revision if available + if rev.Status.ImageDigest != "" { + userContainer.Image = rev.Status.ImageDigest + } + containers = appendContainer(containers, *userContainer) } - if userContainer.ReadinessProbe != nil { - if userContainer.ReadinessProbe.HTTPGet != nil || userContainer.ReadinessProbe.TCPSocket != nil { - // HTTP and TCP ReadinessProbes are executed by the queue-proxy directly against the - // user-container instead of via kubelet. - userContainer.ReadinessProbe = nil + // No change in functional behavior if there is one container. + if len(rev.Spec.PodSpec.Containers) == 1 { + servingContainer() + } else { + for i := range rev.Spec.PodSpec.Containers { + if len(rev.Spec.PodSpec.Containers[i].Ports) != 0 { + servingContainer() + } else { + multiContainers := makeContainer(rev.Spec.PodSpec.Containers[i].DeepCopy(), rev) + // Prefer imageDigest from revision if available + if len(rev.Status.ImageDigests) != 0 { + if v, ok := rev.Status.ImageDigests[multiContainers.Name]; ok { + multiContainers.Image = v + } + } + containers = appendContainer(containers, *multiContainers) + } } } - - // If the client provides probes, we should fill in the port for them. - rewriteUserProbe(userContainer.LivenessProbe, userPortInt) - podSpec := &corev1.PodSpec{ - Containers: []corev1.Container{ - *userContainer, - *queueContainer, - }, + Containers: containers, Volumes: append([]corev1.Volume{varLogVolume}, rev.Spec.Volumes...), ServiceAccountName: rev.Spec.ServiceAccountName, TerminationGracePeriodSeconds: rev.Spec.TimeoutSeconds, @@ -194,12 +186,50 @@ func makePodSpec(rev *v1.Revision, loggingConfig *logging.Config, tracingConfig if observabilityConfig.EnableVarLogCollection { podSpec.Volumes = append(podSpec.Volumes, internalVolume) } + return podSpec, nil +} - if autoscalerConfig.EnableGracefulScaledown { - podSpec.Volumes = append(podSpec.Volumes, labelVolume) +func appendContainer(old []corev1.Container, new corev1.Container) []corev1.Container { + for key := range old { + if old[key].Name == new.Name { + // need to check with more negetive scenarios + return old + } } + return append(old, new) +} - return podSpec, nil +func makeContainer(userContainer *corev1.Container, rev *v1alpha1.Revision) *corev1.Container { + userContainer.VolumeMounts = append(userContainer.VolumeMounts, varLogVolumeMount) + userContainer.Lifecycle = userLifecycle + userContainer.Env = append(userContainer.Env, getKnativeEnvVar(rev)...) + // Explicitly disable stdin and tty allocation + userContainer.Stdin = false + userContainer.TTY = false + if userContainer.TerminationMessagePolicy == "" { + userContainer.TerminationMessagePolicy = corev1.TerminationMessageFallbackToLogsOnError + } + return userContainer +} + +func createServingContainer(rev *v1alpha1.Revision) *corev1.Container { + userContainer := makeContainer(rev.Spec.GetContainer().DeepCopy(), rev) + userPort := getUserPort(rev) + userPortInt := int(userPort) + userPortStr := strconv.Itoa(userPortInt) + // Replacement is safe as only up to a single port is allowed on the Revision + userContainer.Ports = buildContainerPorts(userPort) + userContainer.Env = append(userContainer.Env, buildUserPortEnv(userPortStr)) + if userContainer.ReadinessProbe != nil { + if userContainer.ReadinessProbe.HTTPGet != nil || userContainer.ReadinessProbe.TCPSocket != nil { + // HTTP and TCP ReadinessProbes are executed by the queue-proxy directly against the + // user-container instead of via kubelet. + userContainer.ReadinessProbe = nil + } + } + // If the client provides probes, we should fill in the port for them. + rewriteUserProbe(userContainer.LivenessProbe, userPortInt) + return userContainer } func getUserPort(rev *v1.Revision) int32 { diff --git a/pkg/reconciler/revision/resources/imagecache.go b/pkg/reconciler/revision/resources/imagecache.go index fd90667a7bac..33aea3c2e388 100644 --- a/pkg/reconciler/revision/resources/imagecache.go +++ b/pkg/reconciler/revision/resources/imagecache.go @@ -28,15 +28,26 @@ import ( ) // MakeImageCache makes an caching.Image resources from a revision. -func MakeImageCache(rev *v1.Revision) *caching.Image { - image := rev.Status.ImageDigest +func MakeImageCache(rev *v1alpha1.Revision, containerName string) *caching.Image { + var image string + if len(rev.Spec.Containers) == 1 { + image = rev.Status.ImageDigest + } else if len(rev.Spec.Containers) > 1 { + for i := range rev.Spec.Containers { + if len(rev.Spec.Containers[i].Ports) != 0 { + image = rev.Status.ImageDigest + } else { + image = rev.Status.ImageDigests[containerName] + } + } + } if image == "" { image = rev.Spec.GetContainer().Image } img := &caching.Image{ ObjectMeta: metav1.ObjectMeta{ - Name: names.ImageCache(rev), + Name: names.ImageCache(rev) + "-" + containerName, Namespace: rev.Namespace, Labels: makeLabels(rev), Annotations: resources.FilterMap(rev.GetAnnotations(), func(k string) bool { diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 73bc952a45ba..4866f3c26014 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -64,7 +64,7 @@ var _ revisionreconciler.Interface = (*Reconciler)(nil) func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) error { // The image digest has already been resolved. - if rev.Status.ImageDigest != "" { + if rev.Status.ImageDigest != "" && len(rev.Status.ImageDigests) == len(rev.Spec.Containers)-1 { return nil } From 8b7c77c946fa0f8cd2dad14b785c40aa2952fde4 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Thu, 6 Feb 2020 12:50:45 +0530 Subject: [PATCH 2/7] fix review comments --- pkg/apis/serving/k8s_validation.go | 30 +++++++++---------- pkg/apis/serving/v1/revision_defaults.go | 9 +++--- pkg/apis/serving/v1/revision_types.go | 3 +- pkg/apis/serving/v1alpha1/revision_types.go | 3 +- .../revision/resources/imagecache.go | 13 +------- pkg/reconciler/revision/revision.go | 3 +- 6 files changed, 24 insertions(+), 37 deletions(-) diff --git a/pkg/apis/serving/k8s_validation.go b/pkg/apis/serving/k8s_validation.go index b5ce59c85997..25e3e7f66a75 100644 --- a/pkg/apis/serving/k8s_validation.go +++ b/pkg/apis/serving/k8s_validation.go @@ -262,7 +262,7 @@ func ValidatePodSpec(ps corev1.PodSpec) *apis.FieldError { // 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(ValidateMultiContainer(&ps.Containers[i], volumes).ViaFieldIndex("containers", i)) + errs = errs.Also(ValidateSidecarContainer(&ps.Containers[i], volumes).ViaFieldIndex("containers", i)) } else { errs = errs.Also(ValidateContainer(ps.Containers[i], volumes).ViaFieldIndex("containers", i)) } @@ -278,24 +278,26 @@ func ValidatePodSpec(ps corev1.PodSpec) *apis.FieldError { //ValidateMultiContainerPorts validates port when specified multiple containers func ValidateMultiContainerPorts(containers []corev1.Container) *apis.FieldError { - cPort := []int32{} - var errs *apis.FieldError + var ( + count int + errs *apis.FieldError + ) for i := range containers { - for j := range containers[i].Ports { - cPort = append(cPort, containers[i].Ports[j].ContainerPort) + for range containers[i].Ports { + count++ } } - if len(cPort) == 0 { + if count == 0 { errs = errs.Also(apis.ErrMissingField("containers.ports.containerPort")) } - if len(cPort) > 1 { + if count > 1 { errs = errs.Also(apis.ErrMultipleOneOf("containers.ports.containerPort")) } return errs } -//ValidateMultiContainer validate fields for non serving containers -func ValidateMultiContainer(containers *corev1.Container, volumes sets.String) *apis.FieldError { +//ValidateSidecarContainer validate fields for non serving containers +func ValidateSidecarContainer(container *corev1.Container, volumes sets.String) *apis.FieldError { var errs *apis.FieldError validateMultiContainerProbe := func(p *corev1.Probe) *apis.FieldError { if p != nil { @@ -303,10 +305,9 @@ func ValidateMultiContainer(containers *corev1.Container, volumes sets.String) * } return nil } - errs = errs.Also(validateMultiContainerProbe(containers.LivenessProbe).ViaField("livenessProbe")) - errs = errs.Also(validateMultiContainerProbe(containers.ReadinessProbe).ViaField("readinessProbe")) - errs = errs.Also(validate(*containers, volumes)) - return errs + errs = errs.Also(validateMultiContainerProbe(container.LivenessProbe).ViaField("livenessProbe")) + errs = errs.Also(validateMultiContainerProbe(container.ReadinessProbe).ViaField("readinessProbe")) + return errs.Also(validate(*container, volumes)) } //ValidateContainer validate fields for serving containers @@ -316,8 +317,7 @@ func ValidateContainer(container corev1.Container, volumes sets.String) *apis.Fi errs = errs.Also(validateProbe(container.LivenessProbe).ViaField("livenessProbe")) // Readiness Probes errs = errs.Also(validateReadinessProbe(container.ReadinessProbe).ViaField("readinessProbe")) - errs = errs.Also(validate(container, volumes)) - return errs + return errs.Also(validate(container, volumes)) } func validate(container corev1.Container, volumes sets.String) *apis.FieldError { diff --git a/pkg/apis/serving/v1/revision_defaults.go b/pkg/apis/serving/v1/revision_defaults.go index 464106c43fe4..77ffe91b8a53 100644 --- a/pkg/apis/serving/v1/revision_defaults.go +++ b/pkg/apis/serving/v1/revision_defaults.go @@ -51,12 +51,11 @@ func (rs *RevisionSpec) SetDefaults(ctx context.Context) { } for idx := range rs.PodSpec.Containers { - if len(rs.PodSpec.Containers) > 1 { - if rs.PodSpec.Containers[idx].Name == "" { + if rs.PodSpec.Containers[idx].Name == "" { + if len(rs.PodSpec.Containers) > 1 { rs.PodSpec.Containers[idx].Name = cfg.Defaults.UserContainerName(ctx) + "-" + strconv.Itoa(idx) - } - } else { - if rs.PodSpec.Containers[idx].Name == "" { + + } else { rs.PodSpec.Containers[idx].Name = cfg.Defaults.UserContainerName(ctx) } } diff --git a/pkg/apis/serving/v1/revision_types.go b/pkg/apis/serving/v1/revision_types.go index 2a212d5c5894..97de29553e34 100644 --- a/pkg/apis/serving/v1/revision_types.go +++ b/pkg/apis/serving/v1/revision_types.go @@ -138,8 +138,7 @@ type RevisionStatus struct { ImageDigest string `json:"imageDigest,omitempty"` // ImageDigests holds the resolved digest for the image specified // within .Spec.Container.Image. The digest is resolved during the creation - // of Revision. This will be filled if there are multiple container specified. - // ImageDigests holds the digest for all the .Spec.Container.Image which are non serving. + // of Revision. ImageDigests holds the digest for all the .Spec.Container.Image both serving and non serving. // ref: https://docs.google.com/document/d/1XjIRnOGaq9UGllkZgYXQHuTQmhbECNAOk6TT6RNfJMw/edit?disco=AAAAEHNSwZg // +optional ImageDigests map[string]string `json:"imageDigests,omitempty"` diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index b3ed4b6d9532..23d041823f15 100644 --- a/pkg/apis/serving/v1alpha1/revision_types.go +++ b/pkg/apis/serving/v1alpha1/revision_types.go @@ -194,8 +194,7 @@ type RevisionStatus struct { ImageDigest string `json:"imageDigest,omitempty"` // ImageDigests holds the resolved digest for the image specified // within .Spec.Container.Image. The digest is resolved during the creation - // of Revision. This will be filled if there are multiple container specified. - // ImageDigests holds the digest for all the .Spec.Container.Image which are non serving. + // of Revision. ImageDigests holds the digest for all the .Spec.Container.Image both serving and non serving. // ref: https://docs.google.com/document/d/1XjIRnOGaq9UGllkZgYXQHuTQmhbECNAOk6TT6RNfJMw/edit?disco=AAAAEHNSwZg // +optional ImageDigests map[string]string `json:"imageDigests,omitempty"` diff --git a/pkg/reconciler/revision/resources/imagecache.go b/pkg/reconciler/revision/resources/imagecache.go index 33aea3c2e388..2082e08a9bb4 100644 --- a/pkg/reconciler/revision/resources/imagecache.go +++ b/pkg/reconciler/revision/resources/imagecache.go @@ -29,18 +29,7 @@ import ( // MakeImageCache makes an caching.Image resources from a revision. func MakeImageCache(rev *v1alpha1.Revision, containerName string) *caching.Image { - var image string - if len(rev.Spec.Containers) == 1 { - image = rev.Status.ImageDigest - } else if len(rev.Spec.Containers) > 1 { - for i := range rev.Spec.Containers { - if len(rev.Spec.Containers[i].Ports) != 0 { - image = rev.Status.ImageDigest - } else { - image = rev.Status.ImageDigests[containerName] - } - } - } + image := rev.Status.ImageDigests[containerName] if image == "" { image = rev.Spec.GetContainer().Image } diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 4866f3c26014..3fb3e454d22e 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -64,7 +64,7 @@ var _ revisionreconciler.Interface = (*Reconciler)(nil) func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) error { // The image digest has already been resolved. - if rev.Status.ImageDigest != "" && len(rev.Status.ImageDigests) == len(rev.Spec.Containers)-1 { + if rev.Status.ImageDigest != "" && len(rev.Status.ImageDigests) == len(rev.Spec.Containers) { return nil } @@ -86,6 +86,7 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro v1.RevisionContainerMissingMessage( rev.Spec.GetContainer().Image, err.Error())) return err + } rev.Status.ImageDigest = digest From 22a6b253f1c02ba4269c16c8fbe9fdb330baab85 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Thu, 6 Feb 2020 17:18:31 +0530 Subject: [PATCH 3/7] handle port validation for both single and multicontainer --- pkg/apis/serving/k8s_validation.go | 31 +++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/pkg/apis/serving/k8s_validation.go b/pkg/apis/serving/k8s_validation.go index 25e3e7f66a75..67577301bdad 100644 --- a/pkg/apis/serving/k8s_validation.go +++ b/pkg/apis/serving/k8s_validation.go @@ -254,10 +254,11 @@ func ValidatePodSpec(ps corev1.PodSpec) *apis.FieldError { case 0: errs = errs.Also(apis.ErrMissingField("containers")) case 1: + errs = errs.Also(portValidation(len(ps.Containers[0].Ports))).ViaField("containers.ports") errs = errs.Also(ValidateContainer(ps.Containers[0], volumes). ViaFieldIndex("containers", 0)) default: - errs = ValidateMultiContainerPorts(ps.Containers) + errs = errs.Also(ValidateMultiContainerPorts(ps.Containers)).ViaField("containers.ports") for i := range ps.Containers { // probes are not allowed other than serving container // ref: https://docs.google.com/document/d/1XjIRnOGaq9UGllkZgYXQHuTQmhbECNAOk6TT6RNfJMw/edit?disco=AAAAEHNSwZU @@ -288,12 +289,9 @@ func ValidateMultiContainerPorts(containers []corev1.Container) *apis.FieldError } } if count == 0 { - errs = errs.Also(apis.ErrMissingField("containers.ports.containerPort")) + errs = errs.Also(apis.ErrMissingField(apis.CurrentField)) } - if count > 1 { - errs = errs.Also(apis.ErrMultipleOneOf("containers.ports.containerPort")) - } - return errs + return errs.Also(portValidation(count)) } //ValidateSidecarContainer validate fields for non serving containers @@ -320,6 +318,18 @@ func ValidateContainer(container corev1.Container, volumes sets.String) *apis.Fi return errs.Also(validate(container, volumes)) } +func portValidation(count int) *apis.FieldError { + var errs *apis.FieldError + if count > 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", + }) + } + return errs +} + func validate(container corev1.Container, volumes sets.String) *apis.FieldError { if equality.Semantic.DeepEqual(container, corev1.Container{}) { return apis.ErrMissingField(apis.CurrentField) @@ -440,20 +450,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))) From 29b8ab861f834dc917341b9734dacb5fdb085744 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Thu, 6 Feb 2020 19:43:06 +0530 Subject: [PATCH 4/7] fix review comments --- pkg/apis/serving/k8s_validation.go | 27 ++++--- .../revision/reconcile_resources.go | 7 +- pkg/reconciler/revision/resources/deploy.go | 77 ++++++++----------- .../revision/resources/imagecache.go | 2 +- 4 files changed, 50 insertions(+), 63 deletions(-) diff --git a/pkg/apis/serving/k8s_validation.go b/pkg/apis/serving/k8s_validation.go index 67577301bdad..3616a89f602b 100644 --- a/pkg/apis/serving/k8s_validation.go +++ b/pkg/apis/serving/k8s_validation.go @@ -263,7 +263,7 @@ func ValidatePodSpec(ps corev1.PodSpec) *apis.FieldError { // 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)) + errs = errs.Also(ValidateSidecarContainer(ps.Containers[i], volumes).ViaFieldIndex("containers", i)) } else { errs = errs.Also(ValidateContainer(ps.Containers[i], volumes).ViaFieldIndex("containers", i)) } @@ -295,17 +295,17 @@ func ValidateMultiContainerPorts(containers []corev1.Container) *apis.FieldError } //ValidateSidecarContainer validate fields for non serving containers -func ValidateSidecarContainer(container *corev1.Container, volumes sets.String) *apis.FieldError { +func ValidateSidecarContainer(container corev1.Container, volumes sets.String) *apis.FieldError { var errs *apis.FieldError - validateMultiContainerProbe := func(p *corev1.Probe) *apis.FieldError { - if p != nil { - return apis.CheckDisallowedFields(*p, *ProbeMask(&corev1.Probe{})) - } - return nil + 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")) } - errs = errs.Also(validateMultiContainerProbe(container.LivenessProbe).ViaField("livenessProbe")) - errs = errs.Also(validateMultiContainerProbe(container.ReadinessProbe).ViaField("readinessProbe")) - return errs.Also(validate(*container, volumes)) + return errs.Also(validate(container, volumes)) } //ValidateContainer validate fields for serving containers @@ -319,15 +319,14 @@ func ValidateContainer(container corev1.Container, volumes sets.String) *apis.Fi } func portValidation(count int) *apis.FieldError { - var errs *apis.FieldError if count > 1 { - errs = errs.Also(&apis.FieldError{ + return &apis.FieldError{ Message: "More than one container port is set", Paths: []string{apis.CurrentField}, Details: "Only a single port is allowed", - }) + } } - return errs + return nil } func validate(container corev1.Container, volumes sets.String) *apis.FieldError { diff --git a/pkg/reconciler/revision/reconcile_resources.go b/pkg/reconciler/revision/reconcile_resources.go index be2d6a28e1d6..b68b8693c8ab 100644 --- a/pkg/reconciler/revision/reconcile_resources.go +++ b/pkg/reconciler/revision/reconcile_resources.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/api/equality" apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "knative.dev/pkg/kmeta" "knative.dev/pkg/logging" "knative.dev/pkg/logging/logkey" v1 "knative.dev/serving/pkg/apis/serving/v1" @@ -113,10 +114,10 @@ func (c *Reconciler) reconcileImageCache(ctx context.Context, rev *v1.Revision) logger := logging.FromContext(ctx) ns := rev.Namespace - for i := range rev.Spec.Containers { - imageName := resourcenames.ImageCache(rev) + "-" + rev.Spec.Containers[i].Name + for _, container := range rev.Spec.Containers { + imageName := kmeta.ChildName(resourcenames.ImageCache(rev), container.Name) if _, err := c.imageLister.Images(ns).Get(imageName); apierrs.IsNotFound(err) { - if _, err := c.createImageCache(rev, rev.Spec.Containers[i].Name); err != nil { + if _, err := c.createImageCache(rev, container.Name); err != nil { return fmt.Errorf("failed to create image cache %q: %w", imageName, err) } logger.Infof("Created image cache %q", imageName) diff --git a/pkg/reconciler/revision/resources/deploy.go b/pkg/reconciler/revision/resources/deploy.go index 47404fbb3b6d..fe7c7fcbb48f 100644 --- a/pkg/reconciler/revision/resources/deploy.go +++ b/pkg/reconciler/revision/resources/deploy.go @@ -144,34 +144,21 @@ func makePodSpec(rev *v1.Revision, loggingConfig *logging.Config, tracingConfig containers := []corev1.Container{ *queueContainer, } - - // closure function for serving container - servingContainer := func() { - userContainer := createServingContainer(rev) - // Prefer imageDigest from revision if available - if rev.Status.ImageDigest != "" { - userContainer.Image = rev.Status.ImageDigest - } - containers = appendContainer(containers, *userContainer) - } - - // No change in functional behavior if there is one container. - if len(rev.Spec.PodSpec.Containers) == 1 { - servingContainer() - } else { - for i := range rev.Spec.PodSpec.Containers { - if len(rev.Spec.PodSpec.Containers[i].Ports) != 0 { - servingContainer() - } else { - multiContainers := makeContainer(rev.Spec.PodSpec.Containers[i].DeepCopy(), rev) - // Prefer imageDigest from revision if available - if len(rev.Status.ImageDigests) != 0 { - if v, ok := rev.Status.ImageDigests[multiContainers.Name]; ok { - multiContainers.Image = v - } - } - containers = appendContainer(containers, *multiContainers) + for i := range rev.Spec.PodSpec.Containers { + if len(rev.Spec.PodSpec.Containers[i].Ports) != 0 || len(rev.Spec.PodSpec.Containers) == 1 { + servingContainer := makeServingContainer(rev.Spec.GetContainer().DeepCopy(), rev) + // Prefer imageDigest from revision if available + if rev.Status.ImageDigest != "" { + servingContainer.Image = rev.Status.ImageDigest + } + containers = appendContainer(containers, servingContainer) + } else { + multiContainers := makeContainer(rev.Spec.PodSpec.Containers[i].DeepCopy(), rev) + // Prefer imageDigest from revision if available + if v, ok := rev.Status.ImageDigests[multiContainers.Name]; ok { + multiContainers.Image = v } + containers = appendContainer(containers, multiContainers) } } podSpec := &corev1.PodSpec{ @@ -199,37 +186,37 @@ func appendContainer(old []corev1.Container, new corev1.Container) []corev1.Cont return append(old, new) } -func makeContainer(userContainer *corev1.Container, rev *v1alpha1.Revision) *corev1.Container { - userContainer.VolumeMounts = append(userContainer.VolumeMounts, varLogVolumeMount) - userContainer.Lifecycle = userLifecycle - userContainer.Env = append(userContainer.Env, getKnativeEnvVar(rev)...) +func makeContainer(container *corev1.Container, rev *v1alpha1.Revision) corev1.Container { + container.VolumeMounts = append(container.VolumeMounts, varLogVolumeMount) + container.Lifecycle = userLifecycle + container.Env = append(container.Env, getKnativeEnvVar(rev)...) // Explicitly disable stdin and tty allocation - userContainer.Stdin = false - userContainer.TTY = false - if userContainer.TerminationMessagePolicy == "" { - userContainer.TerminationMessagePolicy = corev1.TerminationMessageFallbackToLogsOnError + container.Stdin = false + container.TTY = false + if container.TerminationMessagePolicy == "" { + container.TerminationMessagePolicy = corev1.TerminationMessageFallbackToLogsOnError } - return userContainer + return *container } -func createServingContainer(rev *v1alpha1.Revision) *corev1.Container { - userContainer := makeContainer(rev.Spec.GetContainer().DeepCopy(), rev) +func makeServingContainer(servingContainer *corev1.Container, rev *v1alpha1.Revision) corev1.Container { + container := makeContainer(servingContainer, rev) userPort := getUserPort(rev) userPortInt := int(userPort) userPortStr := strconv.Itoa(userPortInt) // Replacement is safe as only up to a single port is allowed on the Revision - userContainer.Ports = buildContainerPorts(userPort) - userContainer.Env = append(userContainer.Env, buildUserPortEnv(userPortStr)) - if userContainer.ReadinessProbe != nil { - if userContainer.ReadinessProbe.HTTPGet != nil || userContainer.ReadinessProbe.TCPSocket != nil { + container.Ports = buildContainerPorts(userPort) + container.Env = append(container.Env, buildUserPortEnv(userPortStr)) + if container.ReadinessProbe != nil { + if container.ReadinessProbe.HTTPGet != nil || container.ReadinessProbe.TCPSocket != nil { // HTTP and TCP ReadinessProbes are executed by the queue-proxy directly against the // user-container instead of via kubelet. - userContainer.ReadinessProbe = nil + container.ReadinessProbe = nil } } // If the client provides probes, we should fill in the port for them. - rewriteUserProbe(userContainer.LivenessProbe, userPortInt) - return userContainer + rewriteUserProbe(container.LivenessProbe, userPortInt) + return container } func getUserPort(rev *v1.Revision) int32 { diff --git a/pkg/reconciler/revision/resources/imagecache.go b/pkg/reconciler/revision/resources/imagecache.go index 2082e08a9bb4..b45b9d3b0a8f 100644 --- a/pkg/reconciler/revision/resources/imagecache.go +++ b/pkg/reconciler/revision/resources/imagecache.go @@ -36,7 +36,7 @@ func MakeImageCache(rev *v1alpha1.Revision, containerName string) *caching.Image img := &caching.Image{ ObjectMeta: metav1.ObjectMeta{ - Name: names.ImageCache(rev) + "-" + containerName, + Name: kmeta.ChildName(names.ImageCache(rev), containerName), Namespace: rev.Namespace, Labels: makeLabels(rev), Annotations: resources.FilterMap(rev.GetAnnotations(), func(k string) bool { From 68736d732c4a89906d0f8b7a32e57eae3e2b6f36 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Sat, 8 Feb 2020 12:51:07 +0530 Subject: [PATCH 5/7] update update-codegen.sh for ImageDigests --- pkg/apis/serving/k8s_validation.go | 10 ++++------ pkg/apis/serving/v1/zz_generated.deepcopy.go | 7 +++++++ pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go | 7 +++++++ pkg/reconciler/revision/resources/deploy.go | 1 - 4 files changed, 18 insertions(+), 7 deletions(-) diff --git a/pkg/apis/serving/k8s_validation.go b/pkg/apis/serving/k8s_validation.go index 3616a89f602b..bd8f97f9a88d 100644 --- a/pkg/apis/serving/k8s_validation.go +++ b/pkg/apis/serving/k8s_validation.go @@ -277,16 +277,14 @@ func ValidatePodSpec(ps corev1.PodSpec) *apis.FieldError { return errs } -//ValidateMultiContainerPorts validates port when specified multiple containers +// ValidateMultiContainerPorts validates port when specified multiple containers func ValidateMultiContainerPorts(containers []corev1.Container) *apis.FieldError { var ( count int errs *apis.FieldError ) for i := range containers { - for range containers[i].Ports { - count++ - } + count += len(containers[i].Ports) } if count == 0 { errs = errs.Also(apis.ErrMissingField(apis.CurrentField)) @@ -294,7 +292,7 @@ func ValidateMultiContainerPorts(containers []corev1.Container) *apis.FieldError return errs.Also(portValidation(count)) } -//ValidateSidecarContainer validate fields for non serving containers +// 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 { @@ -308,7 +306,7 @@ func ValidateSidecarContainer(container corev1.Container, volumes sets.String) * return errs.Also(validate(container, volumes)) } -//ValidateContainer validate fields for serving containers +// ValidateContainer validate fields for serving containers func ValidateContainer(container corev1.Container, volumes sets.String) *apis.FieldError { var errs *apis.FieldError // Liveness Probes diff --git a/pkg/apis/serving/v1/zz_generated.deepcopy.go b/pkg/apis/serving/v1/zz_generated.deepcopy.go index bfff38f19787..ae7f28d4e883 100644 --- a/pkg/apis/serving/v1/zz_generated.deepcopy.go +++ b/pkg/apis/serving/v1/zz_generated.deepcopy.go @@ -230,6 +230,13 @@ func (in *RevisionSpec) DeepCopy() *RevisionSpec { func (in *RevisionStatus) DeepCopyInto(out *RevisionStatus) { *out = *in in.Status.DeepCopyInto(&out.Status) + if in.ImageDigests != nil { + in, out := &in.ImageDigests, &out.ImageDigests + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } return } diff --git a/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go index 8d347d821b62..92bff3f318a8 100644 --- a/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go @@ -316,6 +316,13 @@ func (in *RevisionSpec) DeepCopy() *RevisionSpec { func (in *RevisionStatus) DeepCopyInto(out *RevisionStatus) { *out = *in in.Status.DeepCopyInto(&out.Status) + if in.ImageDigests != nil { + in, out := &in.ImageDigests, &out.ImageDigests + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } return } diff --git a/pkg/reconciler/revision/resources/deploy.go b/pkg/reconciler/revision/resources/deploy.go index fe7c7fcbb48f..eee40c6dd0ad 100644 --- a/pkg/reconciler/revision/resources/deploy.go +++ b/pkg/reconciler/revision/resources/deploy.go @@ -179,7 +179,6 @@ func makePodSpec(rev *v1.Revision, loggingConfig *logging.Config, tracingConfig func appendContainer(old []corev1.Container, new corev1.Container) []corev1.Container { for key := range old { if old[key].Name == new.Name { - // need to check with more negetive scenarios return old } } From 478fb074ce2ae604377b761f71a303b0808c0f83 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Wed, 26 Feb 2020 17:08:48 +0530 Subject: [PATCH 6/7] fix conflict issue --- pkg/reconciler/revision/cruds.go | 2 +- pkg/reconciler/revision/resources/deploy.go | 4 +-- .../revision/resources/imagecache.go | 2 +- pkg/reconciler/revision/revision.go | 27 ++++++++++++------- 4 files changed, 21 insertions(+), 14 deletions(-) diff --git a/pkg/reconciler/revision/cruds.go b/pkg/reconciler/revision/cruds.go index 3610cedc88db..9e9f042ac8ac 100644 --- a/pkg/reconciler/revision/cruds.go +++ b/pkg/reconciler/revision/cruds.go @@ -109,7 +109,7 @@ func (c *Reconciler) checkAndUpdateDeployment(ctx context.Context, rev *v1.Revis return d, nil } -func (c *Reconciler) createImageCache(rev *v1alpha1.Revision, containerName string) (*caching.Image, error) { +func (c *Reconciler) createImageCache(rev *v1.Revision, containerName string) (*caching.Image, error) { image := resources.MakeImageCache(rev, containerName) return c.cachingclient.CachingV1alpha1().Images(image.Namespace).Create(image) diff --git a/pkg/reconciler/revision/resources/deploy.go b/pkg/reconciler/revision/resources/deploy.go index eee40c6dd0ad..aa85011afb65 100644 --- a/pkg/reconciler/revision/resources/deploy.go +++ b/pkg/reconciler/revision/resources/deploy.go @@ -185,7 +185,7 @@ func appendContainer(old []corev1.Container, new corev1.Container) []corev1.Cont return append(old, new) } -func makeContainer(container *corev1.Container, rev *v1alpha1.Revision) corev1.Container { +func makeContainer(container *corev1.Container, rev *v1.Revision) corev1.Container { container.VolumeMounts = append(container.VolumeMounts, varLogVolumeMount) container.Lifecycle = userLifecycle container.Env = append(container.Env, getKnativeEnvVar(rev)...) @@ -198,7 +198,7 @@ func makeContainer(container *corev1.Container, rev *v1alpha1.Revision) corev1.C return *container } -func makeServingContainer(servingContainer *corev1.Container, rev *v1alpha1.Revision) corev1.Container { +func makeServingContainer(servingContainer *corev1.Container, rev *v1.Revision) corev1.Container { container := makeContainer(servingContainer, rev) userPort := getUserPort(rev) userPortInt := int(userPort) diff --git a/pkg/reconciler/revision/resources/imagecache.go b/pkg/reconciler/revision/resources/imagecache.go index b45b9d3b0a8f..8eff64c84e8e 100644 --- a/pkg/reconciler/revision/resources/imagecache.go +++ b/pkg/reconciler/revision/resources/imagecache.go @@ -28,7 +28,7 @@ import ( ) // MakeImageCache makes an caching.Image resources from a revision. -func MakeImageCache(rev *v1alpha1.Revision, containerName string) *caching.Image { +func MakeImageCache(rev *v1.Revision, containerName string) *caching.Image { image := rev.Status.ImageDigests[containerName] if image == "" { image = rev.Spec.GetContainer().Image diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 3fb3e454d22e..cad73451194f 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -78,18 +78,25 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro ServiceAccountName: rev.Spec.ServiceAccountName, ImagePullSecrets: imagePullSecrets, } - digest, err := c.resolver.Resolve(rev.Spec.GetContainer().Image, - opt, cfgs.Deployment.RegistriesSkippingTagResolving) - if err != nil { - err = fmt.Errorf("failed to resolve image to digest: %w", err) - rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, - v1.RevisionContainerMissingMessage( - rev.Spec.GetContainer().Image, err.Error())) - return err + if rev.Status.ImageDigests == nil { + rev.Status.ImageDigests = make(map[string]string, len(rev.Spec.Containers)) + } + for _, container := range rev.Spec.Containers { + digest, err := c.resolver.Resolve(container.Image, + opt, cfgs.Deployment.RegistriesSkippingTagResolving) + if err != nil { + err = fmt.Errorf("failed to resolve image to digest: %w", err) + rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, + v1.RevisionContainerMissingMessage( + container.Image, err.Error())) + return err + } + if len(rev.Spec.Containers) == 1 || len(container.Ports) != 0 { + rev.Status.ImageDigest = digest + } + rev.Status.ImageDigests[container.Name] = digest } - - rev.Status.ImageDigest = digest return nil } From 2f91fb9ebf33192026de76e36a08ff1ff18cfc14 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Sat, 29 Feb 2020 21:18:46 +0530 Subject: [PATCH 7/7] Add unit test cases --- pkg/apis/serving/k8s_validation.go | 7 +- pkg/apis/serving/k8s_validation_test.go | 81 +++++++- pkg/apis/serving/v1/revision_defaults.go | 1 - pkg/apis/serving/v1/revision_defaults_test.go | 35 +++- pkg/apis/serving/v1/revision_helpers.go | 9 +- .../serving/v1/revision_lifecycle_test.go | 6 + .../serving/v1/revision_validation_test.go | 41 +++- .../v1alpha1/revision_conversion_test.go | 5 +- .../serving/v1alpha1/revision_lifecycle.go | 7 +- .../v1alpha1/revision_lifecycle_test.go | 24 ++- .../serving/v1beta1/revision_defaults_test.go | 35 +++- .../v1beta1/revision_validation_test.go | 41 +++- pkg/reconciler/revision/resources/deploy.go | 21 +- .../revision/resources/deploy_test.go | 180 +++++++++++++----- .../revision/resources/imagecache_test.go | 10 +- .../revision/resources/queue_test.go | 24 +-- pkg/reconciler/revision/revision_test.go | 2 +- pkg/reconciler/revision/table_test.go | 53 +++--- pkg/testing/v1/revision.go | 7 + 19 files changed, 469 insertions(+), 120 deletions(-) diff --git a/pkg/apis/serving/k8s_validation.go b/pkg/apis/serving/k8s_validation.go index bd8f97f9a88d..3590d58e0cda 100644 --- a/pkg/apis/serving/k8s_validation.go +++ b/pkg/apis/serving/k8s_validation.go @@ -254,11 +254,10 @@ func ValidatePodSpec(ps corev1.PodSpec) *apis.FieldError { case 0: errs = errs.Also(apis.ErrMissingField("containers")) case 1: - errs = errs.Also(portValidation(len(ps.Containers[0].Ports))).ViaField("containers.ports") errs = errs.Also(ValidateContainer(ps.Containers[0], volumes). ViaFieldIndex("containers", 0)) default: - errs = errs.Also(ValidateMultiContainerPorts(ps.Containers)).ViaField("containers.ports") + 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 @@ -268,6 +267,7 @@ func ValidatePodSpec(ps corev1.PodSpec) *apis.FieldError { errs = errs.Also(ValidateContainer(ps.Containers[i], volumes).ViaFieldIndex("containers", i)) } } + } if ps.ServiceAccountName != "" { for range validation.IsDNS1123Subdomain(ps.ServiceAccountName) { @@ -289,7 +289,7 @@ func ValidateMultiContainerPorts(containers []corev1.Container) *apis.FieldError if count == 0 { errs = errs.Also(apis.ErrMissingField(apis.CurrentField)) } - return errs.Also(portValidation(count)) + return errs.Also(portValidation(count)).ViaField("ports") } // ValidateSidecarContainer validate fields for non serving containers @@ -309,6 +309,7 @@ func ValidateSidecarContainer(container corev1.Container, volumes sets.String) * // 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 diff --git a/pkg/apis/serving/k8s_validation_test.go b/pkg/apis/serving/k8s_validation_test.go index ba1c7a991bd7..e8fe05ca03cc 100644 --- a/pkg/apis/serving/k8s_validation_test.go +++ b/pkg/apis/serving/k8s_validation_test.go @@ -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{ diff --git a/pkg/apis/serving/v1/revision_defaults.go b/pkg/apis/serving/v1/revision_defaults.go index 77ffe91b8a53..2d272011f3c7 100644 --- a/pkg/apis/serving/v1/revision_defaults.go +++ b/pkg/apis/serving/v1/revision_defaults.go @@ -54,7 +54,6 @@ func (rs *RevisionSpec) SetDefaults(ctx context.Context) { if rs.PodSpec.Containers[idx].Name == "" { 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) } diff --git a/pkg/apis/serving/v1/revision_defaults_test.go b/pkg/apis/serving/v1/revision_defaults_test.go index 162cd8451d5d..12feb6620fd6 100644 --- a/pkg/apis/serving/v1/revision_defaults_test.go +++ b/pkg/apis/serving/v1/revision_defaults_test.go @@ -304,6 +304,9 @@ func TestRevisionDefaulting(t *testing.T) { PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ Name: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, }, { Name: "helloworld", }}, @@ -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, }}, }, }, diff --git a/pkg/apis/serving/v1/revision_helpers.go b/pkg/apis/serving/v1/revision_helpers.go index f49762e12496..36571fe22d53 100644 --- a/pkg/apis/serving/v1/revision_helpers.go +++ b/pkg/apis/serving/v1/revision_helpers.go @@ -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{} diff --git a/pkg/apis/serving/v1/revision_lifecycle_test.go b/pkg/apis/serving/v1/revision_lifecycle_test.go index 6deb24478df2..37d0e8519109 100644 --- a/pkg/apis/serving/v1/revision_lifecycle_test.go +++ b/pkg/apis/serving/v1/revision_lifecycle_test.go @@ -773,6 +773,9 @@ func TestGetContainer(t *testing.T) { Containers: []corev1.Container{{ Name: "firstContainer", Image: "firstImage", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, }, { Name: "secondContainer", Image: "secondImage", @@ -782,6 +785,9 @@ func TestGetContainer(t *testing.T) { want: &corev1.Container{ Name: "firstContainer", Image: "firstImage", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, }, }} for _, tc := range cases { diff --git a/pkg/apis/serving/v1/revision_validation_test.go b/pkg/apis/serving/v1/revision_validation_test.go index 4cdd94864ad5..fc09d859c555 100644 --- a/pkg/apis/serving/v1/revision_validation_test.go +++ b/pkg/apis/serving/v1/revision_validation_test.go @@ -373,7 +373,7 @@ func TestRevisionSpecValidation(t *testing.T) { }, want: apis.ErrMissingField("containers"), }, { - name: "too many containers", + name: "too many containers without container port", rs: &RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ @@ -383,7 +383,44 @@ func TestRevisionSpecValidation(t *testing.T) { }}, }, }, - want: apis.ErrMultipleOneOf("containers"), + want: apis.ErrMissingField("containers.ports"), + }, { + name: "too many containers with one container port", + rs: &RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, + }, { + Image: "helloworld", + }}, + }, + }, + want: nil, + }, { + name: "too many containers with multiple container port", + rs: &RevisionSpec{ + PodSpec: 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: "exceed max timeout", rs: &RevisionSpec{ diff --git a/pkg/apis/serving/v1alpha1/revision_conversion_test.go b/pkg/apis/serving/v1alpha1/revision_conversion_test.go index 87a2fa553069..58a591a78962 100644 --- a/pkg/apis/serving/v1alpha1/revision_conversion_test.go +++ b/pkg/apis/serving/v1alpha1/revision_conversion_test.go @@ -231,6 +231,9 @@ func TestRevisionConversionError(t *testing.T) { TimeoutSeconds: ptr.Int64(18), ContainerConcurrency: ptr.Int64(53), }, + DeprecatedContainer: &corev1.Container{ + Image: "busybox", + }, }, Status: RevisionStatus{ Status: duckv1.Status{ @@ -244,7 +247,7 @@ func TestRevisionConversionError(t *testing.T) { LogURL: "http://logger.io", }, }, - want: apis.ErrMultipleOneOf("containers"), + want: apis.ErrMultipleOneOf("container, containers"), }, { name: "no containers in podspec", in: &Revision{ diff --git a/pkg/apis/serving/v1alpha1/revision_lifecycle.go b/pkg/apis/serving/v1alpha1/revision_lifecycle.go index e1d54b7a161c..c961c2613576 100644 --- a/pkg/apis/serving/v1alpha1/revision_lifecycle.go +++ b/pkg/apis/serving/v1alpha1/revision_lifecycle.go @@ -76,14 +76,15 @@ func (rs *RevisionSpec) GetContainer() *corev1.Container { if rs.DeprecatedContainer != nil { return rs.DeprecatedContainer } - if len(rs.Containers) > 1 { + 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] } } - } else { - return &rs.Containers[0] } // Should be unreachable post-validation, but here to ease testing. return &corev1.Container{} diff --git a/pkg/apis/serving/v1alpha1/revision_lifecycle_test.go b/pkg/apis/serving/v1alpha1/revision_lifecycle_test.go index a819e5b37973..c272b2a36f30 100644 --- a/pkg/apis/serving/v1alpha1/revision_lifecycle_test.go +++ b/pkg/apis/serving/v1alpha1/revision_lifecycle_test.go @@ -820,13 +820,16 @@ func TestGetContainer(t *testing.T) { Image: "foo", }, }, { - name: "get first container info even after passing multiple", + name: "get serving container info even if specified multiple containers", status: RevisionSpec{ RevisionSpec: v1.RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ Name: "firstContainer", Image: "firstImage", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, }, { Name: "secondContainer", Image: "secondImage", @@ -837,7 +840,26 @@ func TestGetContainer(t *testing.T) { want: &corev1.Container{ Name: "firstContainer", Image: "firstImage", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, + }, + }, { + name: "get empty container if specified multiple containers without container port", + status: RevisionSpec{ + RevisionSpec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "firstContainer", + Image: "firstImage", + }, { + Name: "secondContainer", + Image: "secondImage", + }}, + }, + }, }, + want: &corev1.Container{}, }} for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/pkg/apis/serving/v1beta1/revision_defaults_test.go b/pkg/apis/serving/v1beta1/revision_defaults_test.go index 7666f37b2d18..01456a3d5193 100644 --- a/pkg/apis/serving/v1beta1/revision_defaults_test.go +++ b/pkg/apis/serving/v1beta1/revision_defaults_test.go @@ -259,6 +259,9 @@ func TestRevisionDefaulting(t *testing.T) { PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ Name: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, }, { Name: "helloworld", }}, @@ -274,10 +277,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: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{}, {}}, + }, + }, + }, + want: &Revision{ + Spec: v1.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, }}, }, }, diff --git a/pkg/apis/serving/v1beta1/revision_validation_test.go b/pkg/apis/serving/v1beta1/revision_validation_test.go index f8971547e5b4..cd7737a074fb 100644 --- a/pkg/apis/serving/v1beta1/revision_validation_test.go +++ b/pkg/apis/serving/v1beta1/revision_validation_test.go @@ -375,7 +375,7 @@ func TestRevisionSpecValidation(t *testing.T) { }, want: apis.ErrMissingField("containers"), }, { - name: "too many containers", + name: "too many containers without container port", rs: &v1.RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ @@ -385,7 +385,44 @@ func TestRevisionSpecValidation(t *testing.T) { }}, }, }, - want: apis.ErrMultipleOneOf("containers"), + want: apis.ErrMissingField("containers.ports"), + }, { + name: "too many containers with one container port", + rs: &v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, + }, { + Image: "helloworld", + }}, + }, + }, + want: nil, + }, { + name: "too many containers with multiple container port", + rs: &v1.RevisionSpec{ + PodSpec: 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: "exceed max timeout", rs: &v1.RevisionSpec{ diff --git a/pkg/reconciler/revision/resources/deploy.go b/pkg/reconciler/revision/resources/deploy.go index aa85011afb65..7271dd423b21 100644 --- a/pkg/reconciler/revision/resources/deploy.go +++ b/pkg/reconciler/revision/resources/deploy.go @@ -141,9 +141,7 @@ func makePodSpec(rev *v1.Revision, loggingConfig *logging.Config, tracingConfig return nil, fmt.Errorf("failed to create queue-proxy container: %w", err) } - containers := []corev1.Container{ - *queueContainer, - } + containers := []corev1.Container{} for i := range rev.Spec.PodSpec.Containers { if len(rev.Spec.PodSpec.Containers[i].Ports) != 0 || len(rev.Spec.PodSpec.Containers) == 1 { servingContainer := makeServingContainer(rev.Spec.GetContainer().DeepCopy(), rev) @@ -151,16 +149,17 @@ func makePodSpec(rev *v1.Revision, loggingConfig *logging.Config, tracingConfig if rev.Status.ImageDigest != "" { servingContainer.Image = rev.Status.ImageDigest } - containers = appendContainer(containers, servingContainer) + containers = append(containers, servingContainer) } else { multiContainers := makeContainer(rev.Spec.PodSpec.Containers[i].DeepCopy(), rev) // Prefer imageDigest from revision if available if v, ok := rev.Status.ImageDigests[multiContainers.Name]; ok { multiContainers.Image = v } - containers = appendContainer(containers, multiContainers) + containers = append(containers, multiContainers) } } + containers = append(containers, *queueContainer) podSpec := &corev1.PodSpec{ Containers: containers, Volumes: append([]corev1.Volume{varLogVolume}, rev.Spec.Volumes...), @@ -173,16 +172,10 @@ func makePodSpec(rev *v1.Revision, loggingConfig *logging.Config, tracingConfig if observabilityConfig.EnableVarLogCollection { podSpec.Volumes = append(podSpec.Volumes, internalVolume) } - return podSpec, nil -} - -func appendContainer(old []corev1.Container, new corev1.Container) []corev1.Container { - for key := range old { - if old[key].Name == new.Name { - return old - } + if autoscalerConfig.EnableGracefulScaledown { + podSpec.Volumes = append(podSpec.Volumes, labelVolume) } - return append(old, new) + return podSpec, nil } func makeContainer(container *corev1.Container, rev *v1.Revision) corev1.Container { diff --git a/pkg/reconciler/revision/resources/deploy_test.go b/pkg/reconciler/revision/resources/deploy_test.go index 8c185fc43055..f371006d2914 100644 --- a/pkg/reconciler/revision/resources/deploy_test.go +++ b/pkg/reconciler/revision/resources/deploy_test.go @@ -43,10 +43,11 @@ import ( ) var ( - containerName = "my-container-name" + servingContainerName = "user-container-0" + sidecarContainerName = "user-container-1" sidecarIstioInjectAnnotation = "sidecar.istio.io/inject" - defaultUserContainer = &corev1.Container{ - Name: containerName, + defaultServingContainer = &corev1.Container{ + Name: servingContainerName, Image: "busybox", Ports: buildContainerPorts(v1.DefaultUserPort), VolumeMounts: []corev1.VolumeMount{varLogVolumeMount}, @@ -55,9 +56,29 @@ var ( Stdin: false, TTY: false, Env: []corev1.EnvVar{{ + Name: "K_REVISION", + Value: "bar", + }, { + Name: "K_CONFIGURATION", + Value: "cfg", + }, { + Name: "K_SERVICE", + Value: "svc", + }, { Name: "PORT", Value: "8080", - }, { + }}, + } + + defaultSidecarContainer = &corev1.Container{ + Name: sidecarContainerName, + Image: "ubuntu", + VolumeMounts: []corev1.VolumeMount{varLogVolumeMount}, + Lifecycle: userLifecycle, + TerminationMessagePolicy: corev1.TerminationMessageFallbackToLogsOnError, + Stdin: false, + TTY: false, + Env: []corev1.EnvVar{{ Name: "K_REVISION", Value: "bar", }, { @@ -152,7 +173,7 @@ var ( Value: metrics.Domain(), }, { Name: "USER_CONTAINER_NAME", - Value: containerName, + Value: servingContainerName, }, { Name: "ENABLE_VAR_LOG_COLLECTION", Value: "false", @@ -238,12 +259,39 @@ var ( TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, Image: "busybox", }}, }, }, } + defaultRevisionWithMultipleContainers = &v1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "foo", + Name: "bar", + UID: "1234", + Labels: map[string]string{ + serving.ConfigurationLabelKey: "cfg", + serving.ServiceLabelKey: "svc", + serving.RouteLabelKey: "im-a-route", + }, + }, + Spec: v1.RevisionSpec{ + TimeoutSeconds: ptr.Int64(45), + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: servingContainerName, + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Name: sidecarContainerName, + Image: "ubuntu", + }}, + }, + }, + } ) func refInt64(num int64) *int64 { @@ -262,8 +310,12 @@ func container(container *corev1.Container, opts ...containerOption) corev1.Cont return *container } -func userContainer(opts ...containerOption) corev1.Container { - return container(defaultUserContainer.DeepCopy(), opts...) +func servingContainer(opts ...containerOption) corev1.Container { + return container(defaultServingContainer.DeepCopy(), opts...) +} + +func sidecarContainer(opts ...containerOption) corev1.Container { + return container(defaultSidecarContainer.DeepCopy(), opts...) } func queueContainer(opts ...containerOption) corev1.Container { @@ -286,6 +338,12 @@ func withEnvVar(name, value string) containerOption { } } +func withImage(image string) containerOption { + return func(container *corev1.Container) { + container.Image = image + } +} + func withInternalVolumeMount() containerOption { return func(container *corev1.Container) { container.VolumeMounts = append(container.VolumeMounts, internalVolumeMount) @@ -375,6 +433,14 @@ func revision(opts ...revisionOption) *v1.Revision { return revision } +func revisionWithMultiContainer(opts ...revisionOption) *v1.Revision { + revision := defaultRevisionWithMultipleContainers.DeepCopy() + for _, option := range opts { + option(revision) + } + return revision +} + func withContainerConcurrency(cc int64) revisionOption { return func(revision *v1.Revision) { revision.Spec.ContainerConcurrency = &cc @@ -427,7 +493,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer( + servingContainer( func(container *corev1.Container) { container.Ports[0].ContainerPort = 8888 }, @@ -471,7 +537,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer( + servingContainer( func(container *corev1.Container) { container.Ports[0].ContainerPort = 8888 }, @@ -511,7 +577,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer(), + servingContainer(), queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "1"), ), @@ -536,9 +602,9 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer(func(container *corev1.Container) { - container.Image = "busybox@sha256:deadbeef" - }), + servingContainer( + withImage("busybox@sha256:deadbeef"), + ), queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "1"), ), @@ -561,7 +627,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer(), + servingContainer(), queueContainer( withEnvVar("SERVING_CONFIGURATION", "parent-config"), withEnvVar("CONTAINER_CONCURRENCY", "1"), @@ -581,7 +647,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer(), + servingContainer(), queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "0"), withEnvVar("SERVING_READINESS_PROBE", `{"httpGet":{"path":"/","port":8080,"host":"127.0.0.1","scheme":"HTTP","httpHeaders":[{"name":"K-Kubelet-Probe","value":"queue"}]}}`), @@ -606,7 +672,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer(), + servingContainer(), queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "0"), withEnvVar("SERVING_READINESS_PROBE", `{"tcpSocket":{"port":8080,"host":"127.0.0.1"}}`), @@ -626,7 +692,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer( + servingContainer( withExecReadinessProbe([]string{"echo", "hello"})), queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "0"), @@ -652,7 +718,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer( + servingContainer( withLivenessProbe(corev1.Handler{ HTTPGet: &corev1.HTTPGetAction{ Path: "/", @@ -685,7 +751,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer( + servingContainer( withLivenessProbe(corev1.Handler{ TCPSocket: &corev1.TCPSocketAction{ Port: intstr.FromInt(v1.DefaultUserPort), @@ -713,7 +779,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer(), + servingContainer(), queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "1"), withEnvVar("ENABLE_VAR_LOG_COLLECTION", "true"), @@ -740,7 +806,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer(), + servingContainer(), queueContainer( withEnvVar("DOWNWARD_API_LABELS_PATH", fmt.Sprintf("%s/%s", podInfoVolumePath, metadataLabelsPath)), withPodLabelsVolumeMount(), @@ -783,7 +849,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer( + servingContainer( func(container *corev1.Container) { container.Command = []string{"/bin/bash"} container.Args = []string{"-c", "echo Hello world"} @@ -814,6 +880,50 @@ func TestMakePodSpec(t *testing.T) { withEnvVar("SERVING_SERVICE", ""), ), }), + }, { + name: "user-defined user port, queue proxy have PORT env with multiple container", + rev: revisionWithMultiContainer( + withContainerConcurrency(1), + func(revision *v1.Revision) { + revision.Spec.GetContainer().Ports = []corev1.ContainerPort{{ + ContainerPort: 9999, + }} + revision.Status = v1.RevisionStatus{ + ImageDigest: "busybox@sha256:abcdef", + ImageDigests: map[string]string{ + servingContainerName: "busybox@sha256:abcdef", + sidecarContainerName: "ubuntu@sha256:ghijkl", + }, + } + container(revision.Spec.GetContainer(), + withTCPReadinessProbe(), + ) + }, + ), + lc: &logging.Config{}, + tc: &tracingconfig.Config{}, + oc: &metrics.ObservabilityConfig{}, + ac: &autoscalerconfig.Config{}, + cc: &deployment.Config{}, + want: podSpec( + []corev1.Container{ + servingContainer( + func(container *corev1.Container) { + container.Ports[0].ContainerPort = 9999 + }, + withEnvVar("PORT", "9999"), + withImage("busybox@sha256:abcdef"), + ), + + sidecarContainer( + withImage("ubuntu@sha256:ghijkl"), + ), + queueContainer( + withEnvVar("CONTAINER_CONCURRENCY", "1"), + withEnvVar("USER_PORT", "9999"), + withEnvVar("SERVING_READINESS_PROBE", `{"tcpSocket":{"port":9999,"host":"127.0.0.1"}}`), + ), + }), }} for _, test := range tests { @@ -823,21 +933,7 @@ func TestMakePodSpec(t *testing.T) { }) got, err := makePodSpec(test.rev, test.lc, test.tc, test.oc, test.ac, test.cc) if err != nil { - t.Fatal("makePodSpec returned errror") - } - if diff := cmp.Diff(test.want, got, quantityComparer); diff != "" { - t.Errorf("makePodSpec (-want, +got) = %v", diff) - } - }) - - t.Run(test.name+"(podspec)", func(t *testing.T) { - quantityComparer := cmp.Comparer(func(x, y resource.Quantity) bool { - return x.Cmp(y) == 0 - }) - - got, err := makePodSpec(test.rev, test.lc, test.tc, test.oc, test.ac, test.cc) - if err != nil { - t.Fatal("makePodSpec returned errror") + t.Fatalf("makePodSpec returned error: %v", err) } if diff := cmp.Diff(test.want, got, quantityComparer); diff != "" { t.Errorf("makePodSpec (-want, +got) = %v", diff) @@ -847,16 +943,14 @@ func TestMakePodSpec(t *testing.T) { } func TestMissingProbeError(t *testing.T) { - _, err := MakeDeployment(defaultRevision, + if _, err := MakeDeployment(defaultRevision, &logging.Config{}, &tracingconfig.Config{}, &network.Config{}, &metrics.ObservabilityConfig{}, &autoscalerconfig.Config{}, &deployment.Config{}, - ) - - if err == nil { + ); err == nil { t.Error("expected error from MakeDeployment") } } @@ -950,7 +1044,7 @@ func TestMakeDeployment(t *testing.T) { // Tested above so that we can rely on it here for brevity. podSpec, err := makePodSpec(test.rev, test.lc, test.tc, test.oc, test.ac, test.cc) if err != nil { - t.Fatal("makePodSpec returned errror") + t.Fatalf("makePodSpec returned error: %v", err) } test.want.Spec.Template.Spec = *podSpec got, err := MakeDeployment(test.rev, test.lc, test.tc, test.nc, test.oc, test.ac, test.cc) diff --git a/pkg/reconciler/revision/resources/imagecache_test.go b/pkg/reconciler/revision/resources/imagecache_test.go index 966a23959974..55b7aada4ec9 100644 --- a/pkg/reconciler/revision/resources/imagecache_test.go +++ b/pkg/reconciler/revision/resources/imagecache_test.go @@ -55,13 +55,15 @@ func TestMakeImageCache(t *testing.T) { }, }, Status: v1.RevisionStatus{ - ImageDigest: "busybox@sha256:deadbeef", + ImageDigests: map[string]string{ + "bar": "busybox@sha256:deadbeef", + }, }, }, want: &caching.Image{ ObjectMeta: metav1.ObjectMeta{ Namespace: "foo", - Name: "bar-cache", + Name: "bar-cachebar", Labels: map[string]string{ serving.RevisionLabelKey: "bar", serving.RevisionUID: "1234", @@ -104,7 +106,7 @@ func TestMakeImageCache(t *testing.T) { want: &caching.Image{ ObjectMeta: metav1.ObjectMeta{ Namespace: "foo", - Name: "bar-cache", + Name: "bar-cachebar", Labels: map[string]string{ serving.RevisionLabelKey: "bar", serving.RevisionUID: "1234", @@ -129,7 +131,7 @@ func TestMakeImageCache(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got := MakeImageCache(test.rev) + got := MakeImageCache(test.rev, test.rev.Name) if diff := cmp.Diff(test.want, got); diff != "" { t.Errorf("MakeImageCache (-want, +got) = %v", diff) } diff --git a/pkg/reconciler/revision/resources/queue_test.go b/pkg/reconciler/revision/resources/queue_test.go index 646bea971edb..aaa123e803c3 100644 --- a/pkg/reconciler/revision/resources/queue_test.go +++ b/pkg/reconciler/revision/resources/queue_test.go @@ -125,7 +125,7 @@ func TestMakeQueueContainer(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, ReadinessProbe: testProbe, Ports: []corev1.ContainerPort{{ ContainerPort: 1955, @@ -406,7 +406,7 @@ func TestMakeQueueContainer(t *testing.T) { if len(test.rev.Spec.PodSpec.Containers) == 0 { test.rev.Spec.PodSpec = corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, ReadinessProbe: testProbe, }}, } @@ -459,7 +459,7 @@ func TestMakeQueueContainerWithPercentageAnnotation(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, ReadinessProbe: testProbe, Resources: corev1.ResourceRequirements{ Limits: corev1.ResourceList{ @@ -517,7 +517,7 @@ func TestMakeQueueContainerWithPercentageAnnotation(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, ReadinessProbe: testProbe, Resources: corev1.ResourceRequirements{ Requests: corev1.ResourceList{ @@ -572,7 +572,7 @@ func TestMakeQueueContainerWithPercentageAnnotation(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, ReadinessProbe: testProbe, Resources: corev1.ResourceRequirements{ Requests: corev1.ResourceList{ @@ -626,7 +626,7 @@ func TestMakeQueueContainerWithPercentageAnnotation(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, ReadinessProbe: testProbe, Resources: corev1.ResourceRequirements{ Requests: corev1.ResourceList{ @@ -707,7 +707,7 @@ func TestProbeGenerationHTTPDefaults(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, ReadinessProbe: &corev1.Probe{ Handler: corev1.Handler{ HTTPGet: &corev1.HTTPGetAction{ @@ -795,7 +795,7 @@ func TestProbeGenerationHTTP(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, Ports: []corev1.ContainerPort{{ ContainerPort: int32(userPort), }}, @@ -897,7 +897,7 @@ func TestTCPProbeGeneration(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, Ports: []corev1.ContainerPort{{ ContainerPort: int32(userPort), }}, @@ -936,7 +936,7 @@ func TestTCPProbeGeneration(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, ReadinessProbe: &corev1.Probe{ Handler: corev1.Handler{ TCPSocket: &corev1.TCPSocketAction{}, @@ -994,7 +994,7 @@ func TestTCPProbeGeneration(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, Ports: []corev1.ContainerPort{{ ContainerPort: int32(userPort), }}, @@ -1090,7 +1090,7 @@ var defaultEnv = map[string]string{ "SYSTEM_NAMESPACE": system.Namespace(), "METRICS_DOMAIN": metrics.Domain(), "QUEUE_SERVING_PORT": "8012", - "USER_CONTAINER_NAME": containerName, + "USER_CONTAINER_NAME": servingContainerName, "ENABLE_VAR_LOG_COLLECTION": "false", "VAR_LOG_VOLUME_NAME": varLogVolumeName, "INTERNAL_VOLUME_PATH": internalVolumePath, diff --git a/pkg/reconciler/revision/revision_test.go b/pkg/reconciler/revision/revision_test.go index 05a24ddafd0a..759eecd6f7f0 100644 --- a/pkg/reconciler/revision/revision_test.go +++ b/pkg/reconciler/revision/revision_test.go @@ -231,7 +231,7 @@ func addResourcesToInformers(t *testing.T, ctx context.Context, rev *v1.Revision fakepainformer.Get(ctx).Informer().GetIndexer().Add(pa) } - imageName := resourcenames.ImageCache(rev) + imageName := kmeta.ChildName(resourcenames.ImageCache(rev), rev.Spec.GetContainer().Name) image, err := fakecachingclient.Get(ctx).CachingV1alpha1().Images(rev.Namespace).Get(imageName, metav1.GetOptions{}) if err != nil { t.Errorf("Caching.Images.Get(%v) = %v", imageName, err) diff --git a/pkg/reconciler/revision/table_test.go b/pkg/reconciler/revision/table_test.go index 9655b7abf344..2a99f9b9ee98 100644 --- a/pkg/reconciler/revision/table_test.go +++ b/pkg/reconciler/revision/table_test.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" clientgotesting "k8s.io/client-go/testing" + "knative.dev/caching/pkg/apis/caching/v1alpha1" caching "knative.dev/caching/pkg/apis/caching/v1alpha1" cachingclient "knative.dev/caching/pkg/client/injection/client" kubeclient "knative.dev/pkg/client/injection/kube/client" @@ -84,7 +85,7 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "first-reconcile", // The first reconciliation Populates the following status properties. - WithLogURL, AllUnknownConditions, MarkDeploying("Deploying")), + WithLogURL, WithImageDigests, AllUnknownConditions, MarkDeploying("Deploying")), }}, Key: "foo/first-reconcile", }, { @@ -107,7 +108,7 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "update-status-failure", // Despite failure, the following status properties are set. - WithLogURL, AllUnknownConditions, MarkDeploying("Deploying")), + WithLogURL, WithImageDigests, AllUnknownConditions, MarkDeploying("Deploying")), }}, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for %q: %v", @@ -134,7 +135,7 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "create-pa-failure", // Despite failure, the following status properties are set. - WithLogURL, WithInitRevConditions, + WithLogURL, WithImageDigests, WithInitRevConditions, MarkDeploying("Deploying")), }}, WantEvents: []string{ @@ -160,7 +161,7 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "create-user-deploy-failure", // Despite failure, the following status properties are set. - WithLogURL, WithInitRevConditions, + WithLogURL, WithImageDigests, WithInitRevConditions, MarkDeploying("Deploying")), }}, WantEvents: []string{ @@ -175,7 +176,7 @@ func TestReconcile(t *testing.T) { // state (immediately post-creation), and verify that no changes // are necessary. Objects: []runtime.Object{ - rev("foo", "stable-reconcile", WithLogURL, AllUnknownConditions), + rev("foo", "stable-reconcile", WithLogURL, WithImageDigests, AllUnknownConditions), pa("foo", "stable-reconcile", WithReachability(asv1a1.ReachabilityUnknown)), deploy(t, "foo", "stable-reconcile"), @@ -189,7 +190,7 @@ func TestReconcile(t *testing.T) { // with our desired spec. Objects: []runtime.Object{ rev("foo", "fix-containers", - WithLogURL, AllUnknownConditions), + WithLogURL, WithImageDigests, AllUnknownConditions), pa("foo", "fix-containers", WithReachability(asv1a1.ReachabilityUnknown)), changeContainers(deploy(t, "foo", "fix-containers")), image("foo", "fix-containers"), @@ -207,7 +208,7 @@ func TestReconcile(t *testing.T) { }, Objects: []runtime.Object{ rev("foo", "failure-update-deploy", - withK8sServiceName("whateves"), WithLogURL, AllUnknownConditions), + withK8sServiceName("whateves"), WithLogURL, WithImageDigests, AllUnknownConditions), pa("foo", "failure-update-deploy"), changeContainers(deploy(t, "foo", "failure-update-deploy")), image("foo", "failure-update-deploy"), @@ -227,7 +228,7 @@ func TestReconcile(t *testing.T) { // state (port-Reserve), and verify that no changes are necessary. Objects: []runtime.Object{ rev("foo", "stable-deactivation", - WithLogURL, MarkRevisionReady, + WithLogURL, WithImageDigests, MarkRevisionReady, MarkInactive("NoTraffic", "This thing is inactive.")), pa("foo", "stable-deactivation", WithNoTraffic("NoTraffic", "This thing is inactive.")), @@ -246,7 +247,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pa-ready", withK8sServiceName("new-stuff"), - WithLogURL, + WithLogURL, WithImageDigests, // When the endpoint and pa are ready, then we will see the // Revision become ready. MarkRevisionReady), @@ -270,7 +271,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pa-not-ready", - WithLogURL, MarkRevisionReady, + WithLogURL, WithImageDigests, MarkRevisionReady, withK8sServiceName("its-not-confidential"), // When we reconcile a ready state and our pa is in an activating // state, we should see the following mutation. @@ -294,7 +295,7 @@ func TestReconcile(t *testing.T) { WithLogURL, MarkRevisionReady, // When we reconcile an "all ready" revision when the PA // is inactive, we should see the following change. - MarkInactive("NoTraffic", "This thing is inactive.")), + MarkInactive("NoTraffic", "This thing is inactive."), WithImageDigests), }}, Key: "foo/pa-inactive", }, { @@ -312,7 +313,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pa-inactive", - WithLogURL, MarkRevisionReady, + WithLogURL, WithImageDigests, MarkRevisionReady, withK8sServiceName("pa-inactive-svc"), // When we reconcile an "all ready" revision when the PA // is inactive, we should see the following change. @@ -334,7 +335,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "fix-mutated-pa", - WithLogURL, AllUnknownConditions, + WithLogURL, WithImageDigests, AllUnknownConditions, // When our reconciliation has to change the service // we should see the following mutations to status. withK8sServiceName("fix-mutated-pa"), WithLogURL, MarkRevisionReady), @@ -350,7 +351,7 @@ func TestReconcile(t *testing.T) { Objects: []runtime.Object{ rev("foo", "fix-mutated-pa-fail", withK8sServiceName("some-old-stuff"), - WithLogURL, AllUnknownConditions), + WithLogURL, WithImageDigests, AllUnknownConditions), pa("foo", "fix-mutated-pa-fail", WithProtocolType(networking.ProtocolH2C), WithReachability(asv1a1.ReachabilityUnknown)), deploy(t, "foo", "fix-mutated-pa-fail"), image("foo", "fix-mutated-pa-fail"), @@ -382,7 +383,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "deploy-timeout", - WithLogURL, AllUnknownConditions, + WithLogURL, WithImageDigests, AllUnknownConditions, // When the revision is reconciled after a Deployment has // timed out, we should see it marked with the PDE state. MarkProgressDeadlineExceeded("I timed out!")), @@ -403,7 +404,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "deploy-replica-failure", - WithLogURL, AllUnknownConditions, + WithLogURL, WithImageDigests, AllUnknownConditions, // When the revision is reconciled after a Deployment has // timed out, we should see it marked with the FailedCreate state. MarkResourcesUnavailable("FailedCreate", "I replica failed!")), @@ -422,7 +423,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pull-backoff", - WithLogURL, AllUnknownConditions, + WithLogURL, WithImageDigests, AllUnknownConditions, MarkResourcesUnavailable("ImagePullBackoff", "can't pull it")), }}, WantUpdates: []clientgotesting.UpdateActionImpl{{ @@ -445,7 +446,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pod-error", - WithLogURL, AllUnknownConditions, MarkContainerExiting(5, v1.RevisionContainerExitingMessage("I failed man!"))), + WithLogURL, WithImageDigests, AllUnknownConditions, MarkContainerExiting(5, v1.RevisionContainerExitingMessage("I failed man!"))), }}, Key: "foo/pod-error", }, { @@ -463,7 +464,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pod-schedule-error", - WithLogURL, AllUnknownConditions, MarkResourcesUnavailable("Insufficient energy", "Unschedulable")), + WithLogURL, WithImageDigests, AllUnknownConditions, MarkResourcesUnavailable("Insufficient energy", "Unschedulable")), }}, Key: "foo/pod-schedule-error", }, { @@ -480,7 +481,7 @@ func TestReconcile(t *testing.T) { image("foo", "steady-ready"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: rev("foo", "steady-ready", withK8sServiceName("steadier-even"), WithLogURL, + Object: rev("foo", "steady-ready", withK8sServiceName("steadier-even"), WithLogURL, WithImageDigests, // All resources are ready to go, we should see the revision being // marked ready MarkRevisionReady), @@ -500,7 +501,7 @@ func TestReconcile(t *testing.T) { image("foo", "missing-owners"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: rev("foo", "missing-owners", withK8sServiceName("lesser-revision"), WithLogURL, + Object: rev("foo", "missing-owners", withK8sServiceName("lesser-revision"), WithLogURL, WithImageDigests, MarkRevisionReady, // When we're missing the OwnerRef for PodAutoscaler we see this update. MarkResourceNotOwned("PodAutoscaler", "missing-owners")), @@ -520,7 +521,7 @@ func TestReconcile(t *testing.T) { image("foo", "missing-owners"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: rev("foo", "missing-owners", withK8sServiceName("youre-gonna-lose"), WithLogURL, + Object: rev("foo", "missing-owners", withK8sServiceName("youre-gonna-lose"), WithLogURL, WithImageDigests, MarkRevisionReady, // When we're missing the OwnerRef for Deployment we see this update. MarkResourceNotOwned("Deployment", "missing-owners-deployment")), @@ -543,7 +544,7 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "image-pull-secrets", WithImagePullSecrets("foo-secret"), - WithLogURL, AllUnknownConditions, MarkDeploying("Deploying")), + WithLogURL, WithImageDigests, AllUnknownConditions, MarkDeploying("Deploying")), }}, Key: "foo/image-pull-secrets", }} @@ -697,7 +698,11 @@ func image(namespace, name string, co ...configOption) *caching.Image { opt(config) } - return resources.MakeImageCache(rev(namespace, name)) + var image = &v1alpha1.Image{} + for _, container := range rev(namespace, name).Spec.Containers { + image = resources.MakeImageCache(rev(namespace, name), container.Name) + } + return image } func pa(namespace, name string, ko ...PodAutoscalerOption) *asv1a1.PodAutoscaler { diff --git a/pkg/testing/v1/revision.go b/pkg/testing/v1/revision.go index 40a5594704f4..5f437d1a018a 100644 --- a/pkg/testing/v1/revision.go +++ b/pkg/testing/v1/revision.go @@ -74,6 +74,13 @@ func WithLogURL(r *v1.Revision) { r.Status.LogURL = "http://logger.io/test-uid" } +// WithImageDigests sets the .Status.LogURL to the expected value. +func WithImageDigests(r *v1.Revision) { + r.Status.ImageDigests = map[string]string{ + "user-container": "", + } +} + // WithCreationTimestamp sets the Revision's timestamp to the provided time. // TODO(mattmoor): Ideally this could be a more generic Option and use meta.Accessor, // but unfortunately Go's type system cannot support that.