diff --git a/pkg/reconciler/revision/cruds.go b/pkg/reconciler/revision/cruds.go index bf8f92fe9a25..4ff93eddd123 100644 --- a/pkg/reconciler/revision/cruds.go +++ b/pkg/reconciler/revision/cruds.go @@ -19,6 +19,7 @@ package revision import ( "context" + "github.com/pkg/errors" appsv1 "k8s.io/api/apps/v1" "k8s.io/apimachinery/pkg/api/equality" caching "knative.dev/caching/pkg/apis/caching/v1alpha1" @@ -34,7 +35,7 @@ import ( func (c *Reconciler) createDeployment(ctx context.Context, rev *v1alpha1.Revision) (*appsv1.Deployment, error) { cfgs := config.FromContext(ctx) - deployment := resources.MakeDeployment( + deployment, err := resources.MakeDeployment( rev, cfgs.Logging, cfgs.Tracing, @@ -44,6 +45,10 @@ func (c *Reconciler) createDeployment(ctx context.Context, rev *v1alpha1.Revisio cfgs.Deployment, ) + if err != nil { + return nil, errors.Wrap(err, "failed to make deployment") + } + return c.KubeClientSet.AppsV1().Deployments(deployment.Namespace).Create(deployment) } @@ -51,7 +56,7 @@ func (c *Reconciler) checkAndUpdateDeployment(ctx context.Context, rev *v1alpha1 logger := logging.FromContext(ctx) cfgs := config.FromContext(ctx) - deployment := resources.MakeDeployment( + deployment, err := resources.MakeDeployment( rev, cfgs.Logging, cfgs.Tracing, @@ -61,6 +66,10 @@ func (c *Reconciler) checkAndUpdateDeployment(ctx context.Context, rev *v1alpha1 cfgs.Deployment, ) + if err != nil { + return nil, errors.Wrap(err, "failed to update deployment") + } + // Preserve the current scale of the Deployment. deployment.Spec.Replicas = have.Spec.Replicas diff --git a/pkg/reconciler/revision/resources/deploy.go b/pkg/reconciler/revision/resources/deploy.go index 778cf3466b0c..245a70dc0f2a 100644 --- a/pkg/reconciler/revision/resources/deploy.go +++ b/pkg/reconciler/revision/resources/deploy.go @@ -19,6 +19,7 @@ package resources import ( "strconv" + "github.com/pkg/errors" "knative.dev/pkg/kmeta" "knative.dev/pkg/logging" "knative.dev/pkg/ptr" @@ -108,7 +109,13 @@ func rewriteUserProbe(p *corev1.Probe, userPort int) { } } -func makePodSpec(rev *v1alpha1.Revision, loggingConfig *logging.Config, tracingConfig *tracingconfig.Config, observabilityConfig *metrics.ObservabilityConfig, autoscalerConfig *autoscaler.Config, deploymentConfig *deployment.Config) *corev1.PodSpec { +func makePodSpec(rev *v1alpha1.Revision, loggingConfig *logging.Config, tracingConfig *tracingconfig.Config, observabilityConfig *metrics.ObservabilityConfig, autoscalerConfig *autoscaler.Config, deploymentConfig *deployment.Config) (*corev1.PodSpec, error) { + queueContainer, err := makeQueueContainer(rev, loggingConfig, tracingConfig, observabilityConfig, autoscalerConfig, deploymentConfig) + + if err != nil { + return nil, errors.Wrap(err, "failed to create queue-proxy container") + } + 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 @@ -149,7 +156,7 @@ func makePodSpec(rev *v1alpha1.Revision, loggingConfig *logging.Config, tracingC podSpec := &corev1.PodSpec{ Containers: []corev1.Container{ *userContainer, - *makeQueueContainer(rev, loggingConfig, tracingConfig, observabilityConfig, autoscalerConfig, deploymentConfig), + *queueContainer, }, Volumes: append([]corev1.Volume{varLogVolume}, rev.Spec.Volumes...), ServiceAccountName: rev.Spec.ServiceAccountName, @@ -161,7 +168,7 @@ func makePodSpec(rev *v1alpha1.Revision, loggingConfig *logging.Config, tracingC podSpec.Volumes = append(podSpec.Volumes, internalVolume) } - return podSpec + return podSpec, nil } func getUserPort(rev *v1alpha1.Revision) int32 { @@ -193,7 +200,7 @@ func buildUserPortEnv(userPort string) corev1.EnvVar { // MakeDeployment constructs a K8s Deployment resource from a revision. func MakeDeployment(rev *v1alpha1.Revision, loggingConfig *logging.Config, tracingConfig *tracingconfig.Config, networkConfig *network.Config, observabilityConfig *metrics.ObservabilityConfig, - autoscalerConfig *autoscaler.Config, deploymentConfig *deployment.Config) *appsv1.Deployment { + autoscalerConfig *autoscaler.Config, deploymentConfig *deployment.Config) (*appsv1.Deployment, error) { podTemplateAnnotations := resources.FilterMap(rev.GetAnnotations(), func(k string) bool { return k == serving.RevisionLastPinnedAnnotationKey @@ -221,6 +228,10 @@ func MakeDeployment(rev *v1alpha1.Revision, podTemplateAnnotations[IstioOutboundIPRangeAnnotation] = networkConfig.IstioOutboundIPRanges } } + podSpec, err := makePodSpec(rev, loggingConfig, tracingConfig, observabilityConfig, autoscalerConfig, deploymentConfig) + if err != nil { + return nil, errors.Wrap(err, "failed to create PodSpec") + } return &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ @@ -242,8 +253,8 @@ func MakeDeployment(rev *v1alpha1.Revision, Labels: makeLabels(rev), Annotations: podTemplateAnnotations, }, - Spec: *makePodSpec(rev, loggingConfig, tracingConfig, observabilityConfig, autoscalerConfig, deploymentConfig), + Spec: *podSpec, }, }, - } + }, nil } diff --git a/pkg/reconciler/revision/resources/deploy_test.go b/pkg/reconciler/revision/resources/deploy_test.go index 7ccbea4309f9..2c47b9094e8d 100644 --- a/pkg/reconciler/revision/resources/deploy_test.go +++ b/pkg/reconciler/revision/resources/deploy_test.go @@ -17,6 +17,7 @@ limitations under the License. package resources import ( + "fmt" "testing" "github.com/google/go-cmp/cmp" @@ -159,6 +160,9 @@ var ( }, { Name: "INTERNAL_VOLUME_PATH", Value: internalVolumePath, + }, { + Name: "SERVING_READINESS_PROBE", + Value: fmt.Sprintf(`{"tcpSocket":{"port":%d,"host":"127.0.0.1"}}`, v1alpha1.DefaultUserPort), }}, } @@ -284,6 +288,15 @@ func withReadinessProbe(handler corev1.Handler) containerOption { } } +func withTCPReadinessProbe() containerOption { + return withReadinessProbe(corev1.Handler{ + TCPSocket: &corev1.TCPSocketAction{ + Host: "127.0.0.1", + Port: intstr.FromInt(v1alpha1.DefaultUserPort), + }, + }) +} + func withHTTPReadinessProbe(port int) containerOption { return withReadinessProbe(corev1.Handler{ HTTPGet: &corev1.HTTPGetAction{ @@ -386,6 +399,9 @@ func TestMakePodSpec(t *testing.T) { revision.Spec.GetContainer().Ports = []corev1.ContainerPort{{ ContainerPort: 8888, }} + container(revision.Spec.GetContainer(), + withTCPReadinessProbe(), + ) }, ), lc: &logging.Config{}, @@ -404,7 +420,7 @@ func TestMakePodSpec(t *testing.T) { queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "1"), withEnvVar("USER_PORT", "8888"), - withEnvVar("SERVING_READINESS_PROBE", ""), + withEnvVar("SERVING_READINESS_PROBE", `{"tcpSocket":{"port":8888,"host":"127.0.0.1"}}`), ), }), }, { @@ -419,6 +435,9 @@ func TestMakePodSpec(t *testing.T) { Name: "asdf", MountPath: "/asdf", }} + container(revision.Spec.GetContainer(), + withTCPReadinessProbe(), + ) revision.Spec.Volumes = []corev1.Volume{{ Name: "asdf", VolumeSource: corev1.VolumeSource{ @@ -449,7 +468,7 @@ func TestMakePodSpec(t *testing.T) { queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "1"), withEnvVar("USER_PORT", "8888"), - withEnvVar("SERVING_READINESS_PROBE", ""), + withEnvVar("SERVING_READINESS_PROBE", `{"tcpSocket":{"port":8888,"host":"127.0.0.1"}}`), ), }, withAppendedVolumes(corev1.Volume{ Name: "asdf", @@ -461,18 +480,24 @@ func TestMakePodSpec(t *testing.T) { })), }, { name: "concurrency=1 no owner", - rev: revision(withContainerConcurrency(1)), - lc: &logging.Config{}, - tc: &tracingconfig.Config{}, - oc: &metrics.ObservabilityConfig{}, - ac: &autoscaler.Config{}, - cc: &deployment.Config{}, + rev: revision( + withContainerConcurrency(1), + func(revision *v1alpha1.Revision) { + container(revision.Spec.GetContainer(), + withTCPReadinessProbe(), + ) + }, + ), + lc: &logging.Config{}, + tc: &tracingconfig.Config{}, + oc: &metrics.ObservabilityConfig{}, + ac: &autoscaler.Config{}, + cc: &deployment.Config{}, want: podSpec( []corev1.Container{ userContainer(), queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "1"), - withEnvVar("SERVING_READINESS_PROBE", ""), ), }), }, { @@ -483,6 +508,9 @@ func TestMakePodSpec(t *testing.T) { revision.Status = v1alpha1.RevisionStatus{ ImageDigest: "busybox@sha256:deadbeef", } + container(revision.Spec.GetContainer(), + withTCPReadinessProbe(), + ) }, ), lc: &logging.Config{}, @@ -497,7 +525,6 @@ func TestMakePodSpec(t *testing.T) { }), queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "1"), - withEnvVar("SERVING_READINESS_PROBE", ""), ), }), }, { @@ -505,6 +532,11 @@ func TestMakePodSpec(t *testing.T) { rev: revision( withContainerConcurrency(1), withOwnerReference("parent-config"), + func(revision *v1alpha1.Revision) { + container(revision.Spec.GetContainer(), + withTCPReadinessProbe(), + ) + }, ), lc: &logging.Config{}, tc: &tracingconfig.Config{}, @@ -517,7 +549,6 @@ func TestMakePodSpec(t *testing.T) { queueContainer( withEnvVar("SERVING_CONFIGURATION", "parent-config"), withEnvVar("CONTAINER_CONCURRENCY", "1"), - withEnvVar("SERVING_READINESS_PROBE", ""), ), }), }, { @@ -590,6 +621,7 @@ func TestMakePodSpec(t *testing.T) { name: "with http liveness probe", rev: revision(func(revision *v1alpha1.Revision) { container(revision.Spec.GetContainer(), + withTCPReadinessProbe(), withLivenessProbe(corev1.Handler{ HTTPGet: &corev1.HTTPGetAction{ Path: "/", @@ -618,13 +650,13 @@ func TestMakePodSpec(t *testing.T) { ), queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "0"), - withEnvVar("SERVING_READINESS_PROBE", ""), ), }), }, { name: "with tcp liveness probe", rev: revision(func(revision *v1alpha1.Revision) { container(revision.Spec.GetContainer(), + withTCPReadinessProbe(), withLivenessProbe(corev1.Handler{ TCPSocket: &corev1.TCPSocketAction{}, }), @@ -646,14 +678,18 @@ func TestMakePodSpec(t *testing.T) { ), queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "0"), - withEnvVar("SERVING_READINESS_PROBE", ""), ), }), }, { name: "with /var/log collection", - rev: revision(withContainerConcurrency(1)), - lc: &logging.Config{}, - tc: &tracingconfig.Config{}, + rev: revision(withContainerConcurrency(1), + func(revision *v1alpha1.Revision) { + container(revision.Spec.GetContainer(), + withTCPReadinessProbe(), + ) + }), + lc: &logging.Config{}, + tc: &tracingconfig.Config{}, oc: &metrics.ObservabilityConfig{ EnableVarLogCollection: true, }, @@ -666,7 +702,6 @@ func TestMakePodSpec(t *testing.T) { withEnvVar("CONTAINER_CONCURRENCY", "1"), withEnvVar("ENABLE_VAR_LOG_COLLECTION", "true"), withInternalVolumeMount(), - withEnvVar("SERVING_READINESS_PROBE", ""), ), }, func(podSpec *corev1.PodSpec) { @@ -682,6 +717,7 @@ func TestMakePodSpec(t *testing.T) { revision.Spec.GetContainer().Command = []string{"/bin/bash"} revision.Spec.GetContainer().Args = []string{"-c", "echo Hello world"} container(revision.Spec.GetContainer(), + withTCPReadinessProbe(), withEnvVar("FOO", "bar"), withEnvVar("BAZ", "blah"), ) @@ -733,7 +769,6 @@ func TestMakePodSpec(t *testing.T) { ), queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "1"), - withEnvVar("SERVING_READINESS_PROBE", ""), withEnvVar("SERVING_SERVICE", ""), ), }), @@ -744,7 +779,10 @@ func TestMakePodSpec(t *testing.T) { quantityComparer := cmp.Comparer(func(x, y resource.Quantity) bool { return x.Cmp(y) == 0 }) - got := makePodSpec(test.rev, test.lc, test.tc, test.oc, test.ac, test.cc) + 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) } @@ -761,7 +799,10 @@ func TestMakePodSpec(t *testing.T) { } test.rev.Spec.DeprecatedContainer = nil - got := makePodSpec(test.rev, test.lc, test.tc, test.oc, test.ac, test.cc) + 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) } @@ -769,6 +810,21 @@ func TestMakePodSpec(t *testing.T) { } } +func TestMissingProbeError(t *testing.T) { + _, err := MakeDeployment(defaultRevision, + &logging.Config{}, + &tracingconfig.Config{}, + &network.Config{}, + &metrics.ObservabilityConfig{}, + &autoscaler.Config{}, + &deployment.Config{}, + ) + + if err == nil { + t.Error("expected error from MakeDeployment") + } +} + func TestMakeDeployment(t *testing.T) { tests := []struct { name string @@ -785,6 +841,16 @@ func TestMakeDeployment(t *testing.T) { rev: revision( withoutLabels, withContainerConcurrency(1), + func(revision *v1alpha1.Revision) { + container(revision.Spec.GetContainer(), + withReadinessProbe(corev1.Handler{ + TCPSocket: &corev1.TCPSocketAction{ + Host: "127.0.0.1", + Port: intstr.FromInt(12345), + }, + }), + ) + }, ), lc: &logging.Config{}, tc: &tracingconfig.Config{}, @@ -798,6 +864,16 @@ func TestMakeDeployment(t *testing.T) { rev: revision( withoutLabels, withOwnerReference("parent-config"), + func(revision *v1alpha1.Revision) { + container(revision.Spec.GetContainer(), + withReadinessProbe(corev1.Handler{ + TCPSocket: &corev1.TCPSocketAction{ + Host: "127.0.0.1", + Port: intstr.FromInt(12345), + }, + }), + ) + }, ), lc: &logging.Config{}, tc: &tracingconfig.Config{}, @@ -808,9 +884,21 @@ func TestMakeDeployment(t *testing.T) { want: makeDeployment(), }, { name: "with outbound IP range configured", - rev: revision(withoutLabels), - lc: &logging.Config{}, - tc: &tracingconfig.Config{}, + rev: revision( + withoutLabels, + func(revision *v1alpha1.Revision) { + container(revision.Spec.GetContainer(), + withReadinessProbe(corev1.Handler{ + TCPSocket: &corev1.TCPSocketAction{ + Host: "127.0.0.1", + Port: intstr.FromInt(12345), + }, + }), + ) + }, + ), + lc: &logging.Config{}, + tc: &tracingconfig.Config{}, nc: &network.Config{ IstioOutboundIPRanges: "*", }, @@ -826,6 +914,14 @@ func TestMakeDeployment(t *testing.T) { revision.ObjectMeta.Annotations = map[string]string{ sidecarIstioInjectAnnotation: "false", } + container(revision.Spec.GetContainer(), + withReadinessProbe(corev1.Handler{ + TCPSocket: &corev1.TCPSocketAction{ + Host: "127.0.0.1", + Port: intstr.FromInt(12345), + }, + }), + ) }), lc: &logging.Config{}, tc: &tracingconfig.Config{}, @@ -845,6 +941,14 @@ func TestMakeDeployment(t *testing.T) { revision.ObjectMeta.Annotations = map[string]string{ IstioOutboundIPRangeAnnotation: "10.4.0.0/14,10.7.240.0/20", } + container(revision.Spec.GetContainer(), + withReadinessProbe(corev1.Handler{ + TCPSocket: &corev1.TCPSocketAction{ + Host: "127.0.0.1", + Port: intstr.FromInt(12345), + }, + }), + ) }, ), lc: &logging.Config{}, @@ -864,8 +968,15 @@ func TestMakeDeployment(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { // Tested above so that we can rely on it here for brevity. - test.want.Spec.Template.Spec = *makePodSpec(test.rev, test.lc, test.tc, test.oc, test.ac, test.cc) - got := MakeDeployment(test.rev, test.lc, test.tc, test.nc, test.oc, test.ac, test.cc) + podSpec, err := makePodSpec(test.rev, test.lc, test.tc, test.oc, test.ac, test.cc) + if err != nil { + t.Fatal("makePodSpec returned errror") + } + test.want.Spec.Template.Spec = *podSpec + got, err := MakeDeployment(test.rev, test.lc, test.tc, test.nc, test.oc, test.ac, test.cc) + if err != nil { + t.Fatalf("got unexpected error: %v", err) + } if diff := cmp.Diff(test.want, got, cmpopts.IgnoreUnexported(resource.Quantity{})); diff != "" { t.Errorf("MakeDeployment (-want, +got) = %v", diff) } diff --git a/pkg/reconciler/revision/resources/queue.go b/pkg/reconciler/revision/resources/queue.go index 5e97489cee83..b80158115b5d 100644 --- a/pkg/reconciler/revision/resources/queue.go +++ b/pkg/reconciler/revision/resources/queue.go @@ -21,6 +21,7 @@ import ( "math" "strconv" + "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -181,7 +182,7 @@ func makeQueueProbe(in *corev1.Probe) *corev1.Probe { // makeQueueContainer creates the container spec for the queue sidecar. func makeQueueContainer(rev *v1alpha1.Revision, loggingConfig *logging.Config, tracingConfig *tracingconfig.Config, observabilityConfig *metrics.ObservabilityConfig, - autoscalerConfig *autoscaler.Config, deploymentConfig *deployment.Config) *corev1.Container { + autoscalerConfig *autoscaler.Config, deploymentConfig *deployment.Config) (*corev1.Container, error) { configName := "" if owner := metav1.GetControllerOf(rev); owner != nil && owner.Kind == "Configuration" { configName = owner.Name @@ -218,8 +219,10 @@ func makeQueueContainer(rev *v1alpha1.Revision, loggingConfig *logging.Config, t applyReadinessProbeDefaults(rp, userPort) - // TODO(joshrider) bubble up error instead of squashing it here - probeJSON, _ := readiness.EncodeProbe(rp) + probeJSON, err := readiness.EncodeProbe(rp) + if err != nil { + return nil, errors.Wrap(err, "failed to serialize readiness probe") + } return &corev1.Container{ Name: QueueContainerName, @@ -313,7 +316,7 @@ func makeQueueContainer(rev *v1alpha1.Revision, loggingConfig *logging.Config, t Name: "SERVING_READINESS_PROBE", Value: probeJSON, }}, - } + }, nil } func applyReadinessProbeDefaults(p *corev1.Probe, port int32) { switch { diff --git a/pkg/reconciler/revision/resources/queue_test.go b/pkg/reconciler/revision/resources/queue_test.go index dd5d06ff4616..07d88253d875 100644 --- a/pkg/reconciler/revision/resources/queue_test.go +++ b/pkg/reconciler/revision/resources/queue_test.go @@ -18,6 +18,7 @@ package resources import ( "encoding/json" + "fmt" "sort" "strconv" "testing" @@ -47,22 +48,33 @@ import ( tracingconfig "knative.dev/serving/pkg/tracing/config" ) -var defaultKnativeQReadinessProbe = &corev1.Probe{ - Handler: corev1.Handler{ - Exec: &corev1.ExecAction{ - Command: []string{"/ko-app/queue", "-probe-period", "0"}, +var ( + defaultKnativeQReadinessProbe = &corev1.Probe{ + Handler: corev1.Handler{ + Exec: &corev1.ExecAction{ + Command: []string{"/ko-app/queue", "-probe-period", "0"}, + }, }, - }, - // We want to mark the service as not ready as soon as the - // PreStop handler is called, so we need to check a little - // bit more often than the default. It is a small - // sacrifice for a low rate of 503s. - PeriodSeconds: 1, - // We keep the connection open for a while because we're - // actively probing the user-container on that endpoint and - // thus don't want to be limited by K8s granularity here. - TimeoutSeconds: 10, -} + // We want to mark the service as not ready as soon as the + // PreStop handler is called, so we need to check a little + // bit more often than the default. It is a small + // sacrifice for a low rate of 503s. + PeriodSeconds: 1, + // We keep the connection open for a while because we're + // actively probing the user-container on that endpoint and + // thus don't want to be limited by K8s granularity here. + TimeoutSeconds: 10, + } + testProbe = &corev1.Probe{ + Handler: corev1.Handler{ + TCPSocket: &corev1.TCPSocketAction{ + Host: "127.0.0.1", + }, + }, + } +) + +const testProbeJSONTemplate = `{"tcpSocket":{"port":%d,"host":"127.0.0.1"}}` func TestMakeQueueContainer(t *testing.T) { tests := []struct { @@ -118,7 +130,8 @@ func TestMakeQueueContainer(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: containerName, + ReadinessProbe: testProbe, Ports: []corev1.ContainerPort{{ ContainerPort: 1955, Name: string(networking.ProtocolH2C), @@ -376,14 +389,20 @@ 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: containerName, + ReadinessProbe: testProbe, }}, } } - got := makeQueueContainer(test.rev, test.lc, test.tc, test.oc, test.ac, test.cc) + got, err := makeQueueContainer(test.rev, test.lc, test.tc, test.oc, test.ac, test.cc) + + if err != nil { + t.Fatal("makeQueueContainer returned error") + } + test.want.Env = append(test.want.Env, corev1.EnvVar{ Name: "SERVING_READINESS_PROBE", - Value: probeJSON(test.rev.Spec.GetContainer().ReadinessProbe), + Value: probeJSON(test.rev.Spec.GetContainer()), }) sortEnv(got.Env) sortEnv(test.want.Env) @@ -424,7 +443,8 @@ func TestMakeQueueContainerWithPercentageAnnotation(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: containerName, + ReadinessProbe: testProbe, Resources: corev1.ResourceRequirements{ Limits: corev1.ResourceList{ corev1.ResourceName("memory"): resource.MustParse("2Gi"), @@ -483,7 +503,8 @@ func TestMakeQueueContainerWithPercentageAnnotation(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: containerName, + ReadinessProbe: testProbe, Resources: corev1.ResourceRequirements{ Requests: corev1.ResourceList{ corev1.ResourceName("cpu"): resource.MustParse("50m"), @@ -539,7 +560,8 @@ func TestMakeQueueContainerWithPercentageAnnotation(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: containerName, + ReadinessProbe: testProbe, Resources: corev1.ResourceRequirements{ Requests: corev1.ResourceList{ corev1.ResourceName("cpu"): resource.MustParse("50m"), @@ -594,7 +616,8 @@ func TestMakeQueueContainerWithPercentageAnnotation(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: containerName, + ReadinessProbe: testProbe, Resources: corev1.ResourceRequirements{ Requests: corev1.ResourceList{ corev1.ResourceName("memory"): resource.MustParse("900000Pi"), @@ -634,10 +657,13 @@ func TestMakeQueueContainerWithPercentageAnnotation(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got := makeQueueContainer(test.rev, test.lc, test.tc, test.oc, test.ac, test.cc) + got, err := makeQueueContainer(test.rev, test.lc, test.tc, test.oc, test.ac, test.cc) + if err != nil { + t.Fatal("makeQueueContainer returned error") + } test.want.Env = append(test.want.Env, corev1.EnvVar{ Name: "SERVING_READINESS_PROBE", - Value: probeJSON(test.rev.Spec.GetContainer().ReadinessProbe), + Value: probeJSON(test.rev.Spec.GetContainer()), }) sortEnv(got.Env) sortEnv(test.want.Env) @@ -706,6 +732,11 @@ func TestProbeGenerationHTTPDefaults(t *testing.T) { TimeoutSeconds: 10, } + wantProbeJSON, err := json.Marshal(expectedProbe) + if err != nil { + t.Fatal("failed to marshal expected probe") + } + lc := &logging.Config{} tc := &tracingconfig.Config{} oc := &metrics.ObservabilityConfig{} @@ -727,12 +758,15 @@ func TestProbeGenerationHTTPDefaults(t *testing.T) { }, // These changed based on the Revision and configs passed in. Env: env(map[string]string{ - "SERVING_READINESS_PROBE": probeJSON(expectedProbe), + "SERVING_READINESS_PROBE": string(wantProbeJSON), }), SecurityContext: queueSecurityContext, } - got := makeQueueContainer(rev, lc, tc, oc, ac, cc) + got, err := makeQueueContainer(rev, lc, tc, oc, ac, cc) + if err != nil { + t.Fatal("makeQueueContainer returned error") + } sortEnv(got.Env) if diff := cmp.Diff(want, got, cmpopts.IgnoreUnexported(resource.Quantity{})); diff != "" { t.Errorf("makeQueueContainer(-want, +got) = %v", diff) @@ -792,6 +826,11 @@ func TestProbeGenerationHTTP(t *testing.T) { TimeoutSeconds: 10, } + wantProbeJSON, err := json.Marshal(expectedProbe) + if err != nil { + t.Fatal("failed to marshal expected probe") + } + lc := &logging.Config{} tc := &tracingconfig.Config{} oc := &metrics.ObservabilityConfig{} @@ -812,11 +851,17 @@ func TestProbeGenerationHTTP(t *testing.T) { TimeoutSeconds: 10, }, // These changed based on the Revision and configs passed in. - Env: env(map[string]string{"USER_PORT": strconv.Itoa(userPort), "SERVING_READINESS_PROBE": probeJSON(expectedProbe)}), + Env: env(map[string]string{ + "USER_PORT": strconv.Itoa(userPort), + "SERVING_READINESS_PROBE": string(wantProbeJSON), + }), SecurityContext: queueSecurityContext, } - got := makeQueueContainer(rev, lc, tc, oc, ac, cc) + got, err := makeQueueContainer(rev, lc, tc, oc, ac, cc) + if err != nil { + t.Fatal("makeQueueContainer returned error") + } sortEnv(got.Env) if diff := cmp.Diff(want, got, cmpopts.IgnoreUnexported(resource.Quantity{})); diff != "" { t.Errorf("makeQueueContainer(-want, +got) = %v", diff) @@ -1004,12 +1049,19 @@ func TestTCPProbeGeneration(t *testing.T) { }, Spec: test.rev, } + wantProbeJSON, err := json.Marshal(test.wantProbe) + if err != nil { + t.Fatal("failed to marshal expected probe") + } test.want.Env = append(test.want.Env, corev1.EnvVar{ Name: "SERVING_READINESS_PROBE", - Value: probeJSON(test.wantProbe), + Value: string(wantProbeJSON), }) - got := makeQueueContainer(testRev, lc, tc, oc, ac, cc) + got, err := makeQueueContainer(testRev, lc, tc, oc, ac, cc) + if err != nil { + t.Fatal("makeQueueContainer returned error") + } sortEnv(got.Env) sortEnv(test.want.Env) if diff := cmp.Diff(test.want, got, cmpopts.IgnoreUnexported(resource.Quantity{})); diff != "" { @@ -1044,13 +1096,16 @@ var defaultEnv = map[string]string{ "INTERNAL_VOLUME_PATH": internalVolumePath, } -func probeJSON(probe *corev1.Probe) string { - if probe != nil { - if probeBytes, err := json.Marshal(probe); err == nil { - return string(probeBytes) - } +func probeJSON(container *corev1.Container) string { + if container == nil { + return fmt.Sprintf(testProbeJSONTemplate, v1alpha1.DefaultUserPort) + } + + ports := container.Ports + if len(ports) > 0 && ports[0].ContainerPort != 0 { + return fmt.Sprintf(testProbeJSONTemplate, ports[0].ContainerPort) } - return "" + return fmt.Sprintf(testProbeJSONTemplate, v1alpha1.DefaultUserPort) } func env(overrides map[string]string) []corev1.EnvVar { diff --git a/pkg/reconciler/revision/table_test.go b/pkg/reconciler/revision/table_test.go index 048636dab26a..ec0d210fdd29 100644 --- a/pkg/reconciler/revision/table_test.go +++ b/pkg/reconciler/revision/table_test.go @@ -76,7 +76,7 @@ func TestReconcile(t *testing.T) { WantCreates: []runtime.Object{ // The first reconciliation of a Revision creates the following resources. pa("foo", "first-reconcile"), - deploy("foo", "first-reconcile"), + deploy(t, "foo", "first-reconcile"), image("foo", "first-reconcile"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -99,7 +99,7 @@ func TestReconcile(t *testing.T) { }, WantCreates: []runtime.Object{ // We still see the following creates before the failure is induced. - deploy("foo", "update-status-failure"), + deploy(t, "foo", "update-status-failure"), image("foo", "update-status-failure"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -126,7 +126,7 @@ func TestReconcile(t *testing.T) { WantCreates: []runtime.Object{ // We still see the following creates before the failure is induced. pa("foo", "create-pa-failure"), - deploy("foo", "create-pa-failure"), + deploy(t, "foo", "create-pa-failure"), image("foo", "create-pa-failure"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -153,7 +153,7 @@ func TestReconcile(t *testing.T) { }, WantCreates: []runtime.Object{ // We still see the following creates before the failure is induced. - deploy("foo", "create-user-deploy-failure"), + deploy(t, "foo", "create-user-deploy-failure"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "create-user-deploy-failure", @@ -174,7 +174,7 @@ func TestReconcile(t *testing.T) { Objects: []runtime.Object{ rev("foo", "stable-reconcile", WithLogURL, AllUnknownConditions), pa("foo", "stable-reconcile"), - deploy("foo", "stable-reconcile"), + deploy(t, "foo", "stable-reconcile"), image("foo", "stable-reconcile"), }, // No changes are made to any objects. @@ -191,7 +191,7 @@ func TestReconcile(t *testing.T) { rev.Spec.Containers = nil }), pa("foo", "needs-upgrade"), - deploy("foo", "needs-upgrade"), + deploy(t, "foo", "needs-upgrade"), image("foo", "needs-upgrade"), }, WantPatches: []clientgotesting.PatchActionImpl{{ @@ -210,11 +210,11 @@ func TestReconcile(t *testing.T) { rev("foo", "fix-containers", WithLogURL, AllUnknownConditions), pa("foo", "fix-containers"), - changeContainers(deploy("foo", "fix-containers")), + changeContainers(deploy(t, "foo", "fix-containers")), image("foo", "fix-containers"), }, WantUpdates: []clientgotesting.UpdateActionImpl{{ - Object: deploy("foo", "fix-containers"), + Object: deploy(t, "foo", "fix-containers"), }}, Key: "foo/fix-containers", }, { @@ -228,11 +228,11 @@ func TestReconcile(t *testing.T) { rev("foo", "failure-update-deploy", withK8sServiceName("whateves"), WithLogURL, AllUnknownConditions), pa("foo", "failure-update-deploy"), - changeContainers(deploy("foo", "failure-update-deploy")), + changeContainers(deploy(t, "foo", "failure-update-deploy")), image("foo", "failure-update-deploy"), }, WantUpdates: []clientgotesting.UpdateActionImpl{{ - Object: deploy("foo", "failure-update-deploy"), + Object: deploy(t, "foo", "failure-update-deploy"), }}, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "InternalError", "inducing failure for update deployments"), @@ -249,7 +249,7 @@ func TestReconcile(t *testing.T) { MarkInactive("NoTraffic", "This thing is inactive.")), pa("foo", "stable-deactivation", WithNoTraffic("NoTraffic", "This thing is inactive.")), - deploy("foo", "stable-deactivation"), + deploy(t, "foo", "stable-deactivation"), image("foo", "stable-deactivation"), }, Key: "foo/stable-deactivation", @@ -259,7 +259,7 @@ func TestReconcile(t *testing.T) { rev("foo", "pa-ready", withK8sServiceName("old-stuff"), WithLogURL, AllUnknownConditions), pa("foo", "pa-ready", WithTraffic, WithPAStatusService("new-stuff")), - deploy("foo", "pa-ready"), + deploy(t, "foo", "pa-ready"), image("foo", "pa-ready"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -283,7 +283,7 @@ func TestReconcile(t *testing.T) { pa("foo", "pa-not-ready", WithPAStatusService("its-not-confidential"), WithBufferedTraffic("Something", "This is something longer")), - deploy("foo", "pa-not-ready"), + deploy(t, "foo", "pa-not-ready"), image("foo", "pa-not-ready"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -304,7 +304,7 @@ func TestReconcile(t *testing.T) { withK8sServiceName("something-in-the-way"), WithLogURL, MarkRevisionReady), pa("foo", "pa-inactive", WithNoTraffic("NoTraffic", "This thing is inactive.")), - deploy("foo", "pa-inactive"), + deploy(t, "foo", "pa-inactive"), image("foo", "pa-inactive"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -325,7 +325,7 @@ func TestReconcile(t *testing.T) { pa("foo", "pa-inactive", WithNoTraffic("NoTraffic", "This thing is inactive."), WithPAStatusService("pa-inactive-svc")), - deploy("foo", "pa-inactive"), + deploy(t, "foo", "pa-inactive"), image("foo", "pa-inactive"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -347,7 +347,7 @@ func TestReconcile(t *testing.T) { withK8sServiceName("ill-follow-the-sun"), WithLogURL, MarkRevisionReady), pa("foo", "fix-mutated-pa", WithProtocolType(networking.ProtocolH2C), WithTraffic, WithPAStatusService("fix-mutated-pa")), - deploy("foo", "fix-mutated-pa"), + deploy(t, "foo", "fix-mutated-pa"), image("foo", "fix-mutated-pa"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -370,7 +370,7 @@ func TestReconcile(t *testing.T) { withK8sServiceName("some-old-stuff"), WithLogURL, AllUnknownConditions), pa("foo", "fix-mutated-pa-fail", WithProtocolType(networking.ProtocolH2C)), - deploy("foo", "fix-mutated-pa-fail"), + deploy(t, "foo", "fix-mutated-pa-fail"), image("foo", "fix-mutated-pa-fail"), }, WantErr: true, @@ -395,7 +395,7 @@ func TestReconcile(t *testing.T) { rev("foo", "deploy-timeout", withK8sServiceName("the-taxman"), WithLogURL, MarkActive), pa("foo", "deploy-timeout"), // pa can't be ready since deployment times out. - timeoutDeploy(deploy("foo", "deploy-timeout")), + timeoutDeploy(deploy(t, "foo", "deploy-timeout")), image("foo", "deploy-timeout"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -418,8 +418,8 @@ func TestReconcile(t *testing.T) { rev("foo", "pull-backoff", withK8sServiceName("the-taxman"), WithLogURL, MarkActivating("Deploying", "")), pa("foo", "pull-backoff"), // pa can't be ready since deployment times out. - pod("foo", "pull-backoff", WithWaitingContainer("user-container", "ImagePullBackoff", "can't pull it")), - timeoutDeploy(deploy("foo", "pull-backoff")), + pod(t, "foo", "pull-backoff", WithWaitingContainer("user-container", "ImagePullBackoff", "can't pull it")), + timeoutDeploy(deploy(t, "foo", "pull-backoff")), image("foo", "pull-backoff"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -438,8 +438,8 @@ func TestReconcile(t *testing.T) { rev("foo", "pod-error", withK8sServiceName("a-pod-error"), WithLogURL, AllUnknownConditions, MarkActive), pa("foo", "pod-error"), // PA can't be ready, since no traffic. - pod("foo", "pod-error", WithFailingContainer("user-container", 5, "I failed man!")), - deploy("foo", "pod-error"), + pod(t, "foo", "pod-error", WithFailingContainer("user-container", 5, "I failed man!")), + deploy(t, "foo", "pod-error"), image("foo", "pod-error"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -456,8 +456,8 @@ func TestReconcile(t *testing.T) { rev("foo", "pod-schedule-error", withK8sServiceName("a-pod-schedule-error"), WithLogURL, AllUnknownConditions, MarkActive), pa("foo", "pod-schedule-error"), // PA can't be ready, since no traffic. - pod("foo", "pod-schedule-error", WithUnschedulableContainer("Insufficient energy", "Unschedulable")), - deploy("foo", "pod-schedule-error"), + pod(t, "foo", "pod-schedule-error", WithUnschedulableContainer("Insufficient energy", "Unschedulable")), + deploy(t, "foo", "pod-schedule-error"), image("foo", "pod-schedule-error"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -475,7 +475,7 @@ func TestReconcile(t *testing.T) { Objects: []runtime.Object{ rev("foo", "steady-ready", withK8sServiceName("very-steady"), WithLogURL), pa("foo", "steady-ready", WithTraffic, WithPAStatusService("steadier-even")), - deploy("foo", "steady-ready"), + deploy(t, "foo", "steady-ready"), image("foo", "steady-ready"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -495,7 +495,7 @@ func TestReconcile(t *testing.T) { rev("foo", "missing-owners", withK8sServiceName("lesser-revision"), WithLogURL, MarkRevisionReady), pa("foo", "missing-owners", WithTraffic, WithPodAutoscalerOwnersRemoved), - deploy("foo", "missing-owners"), + deploy(t, "foo", "missing-owners"), image("foo", "missing-owners"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -515,7 +515,7 @@ func TestReconcile(t *testing.T) { rev("foo", "missing-owners", withK8sServiceName("youre-gonna-lose"), WithLogURL, MarkRevisionReady), pa("foo", "missing-owners", WithTraffic), - noOwner(deploy("foo", "missing-owners")), + noOwner(deploy(t, "foo", "missing-owners")), image("foo", "missing-owners"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -608,7 +608,8 @@ func AllUnknownConditions(r *v1alpha1.Revision) { type configOption func(*config.Config) -func deploy(namespace, name string, opts ...interface{}) *appsv1.Deployment { +func deploy(t *testing.T, namespace, name string, opts ...interface{}) *appsv1.Deployment { + t.Helper() cfg := ReconcilerTestConfig() for _, opt := range opts { @@ -628,10 +629,14 @@ func deploy(namespace, name string, opts ...interface{}) *appsv1.Deployment { // Do this here instead of in `rev` itself to ensure that we populate defaults // before calling MakeDeployment within Reconcile. rev.SetDefaults(context.Background()) - return resources.MakeDeployment(rev, cfg.Logging, cfg.Tracing, cfg.Network, + deployment, err := resources.MakeDeployment(rev, cfg.Logging, cfg.Tracing, cfg.Network, cfg.Observability, cfg.Autoscaler, cfg.Deployment, ) + if err != nil { + t.Fatal("failed to create deployment") + } + return deployment } func image(namespace, name string, co ...configOption) *caching.Image { @@ -653,8 +658,9 @@ func pa(namespace, name string, ko ...PodAutoscalerOption) *autoscalingv1alpha1. return k } -func pod(namespace, name string, po ...PodOption) *corev1.Pod { - deploy := deploy(namespace, name) +func pod(t *testing.T, namespace, name string, po ...PodOption) *corev1.Pod { + t.Helper() + deploy := deploy(t, namespace, name) pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{