diff --git a/pkg/reconciler/revision/reconcile_resources.go b/pkg/reconciler/revision/reconcile_resources.go index f82fea2668d9..2abece393358 100644 --- a/pkg/reconciler/revision/reconcile_resources.go +++ b/pkg/reconciler/revision/reconcile_resources.go @@ -20,15 +20,13 @@ import ( "context" "fmt" - "go.uber.org/zap" - "github.com/knative/pkg/logging" "github.com/knative/pkg/logging/logkey" kpav1alpha1 "github.com/knative/serving/pkg/apis/autoscaling/v1alpha1" "github.com/knative/serving/pkg/apis/serving/v1alpha1" "github.com/knative/serving/pkg/reconciler/revision/resources" resourcenames "github.com/knative/serving/pkg/reconciler/revision/resources/names" - + "go.uber.org/zap" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" apierrs "k8s.io/apimachinery/pkg/api/errors" @@ -89,6 +87,9 @@ func (c *Reconciler) reconcileDeployment(ctx context.Context, rev *v1alpha1.Revi if t := status.LastTerminationState.Terminated; t != nil { logger.Infof("%s marking exiting with: %d/%s", rev.Name, t.ExitCode, t.Message) rev.Status.MarkContainerExiting(t.ExitCode, t.Message) + } else if w := status.State.Waiting; w != nil && hasDeploymentTimedOut(deployment) { + logger.Infof("%s marking resources unavailable with: %s: %s", rev.Name, w.Reason, w.Message) + rev.Status.MarkResourcesUnavailable(w.Reason, w.Message) } break } diff --git a/pkg/reconciler/revision/table_test.go b/pkg/reconciler/revision/table_test.go index c60b24d18ef2..6ea8599ac6bc 100644 --- a/pkg/reconciler/revision/table_test.go +++ b/pkg/reconciler/revision/table_test.go @@ -25,6 +25,7 @@ import ( "github.com/knative/pkg/controller" "github.com/knative/pkg/logging" logtesting "github.com/knative/pkg/logging/testing" + . "github.com/knative/pkg/reconciler/testing" autoscalingv1alpha1 "github.com/knative/serving/pkg/apis/autoscaling/v1alpha1" "github.com/knative/serving/pkg/apis/networking" "github.com/knative/serving/pkg/apis/serving/v1alpha1" @@ -35,14 +36,12 @@ import ( "github.com/knative/serving/pkg/reconciler" "github.com/knative/serving/pkg/reconciler/revision/config" "github.com/knative/serving/pkg/reconciler/revision/resources" + . "github.com/knative/serving/pkg/reconciler/testing" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" clientgotesting "k8s.io/client-go/testing" - - . "github.com/knative/pkg/reconciler/testing" - . "github.com/knative/serving/pkg/reconciler/testing" ) // This is heavily based on the way the OpenShift Ingress controller tests its reconciliation method. @@ -432,6 +431,23 @@ func TestReconcile(t *testing.T) { "deploy-timeout"), }, Key: "foo/deploy-timeout", + }, { + Name: "surface ImagePullBackoff", + // Test the propagation of ImagePullBackoff from user container. + Objects: []runtime.Object{ + rev("foo", "pull-backoff", + withK8sServiceName("the-taxman"), WithLogURL, MarkActivating("Deploying", "")), + kpa("foo", "pull-backoff"), // KPA 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")), + image("foo", "pull-backoff"), + }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: rev("foo", "pull-backoff", + WithLogURL, AllUnknownConditions, + MarkResourcesUnavailable("ImagePullBackoff", "can't pull it")), + }}, + Key: "foo/pull-backoff", }, { Name: "surface pod errors", // Test the propagation of the termination state of a Pod into the revision. diff --git a/pkg/reconciler/testing/functional.go b/pkg/reconciler/testing/functional.go index dc71f7dacc9d..9977204729da 100644 --- a/pkg/reconciler/testing/functional.go +++ b/pkg/reconciler/testing/functional.go @@ -1011,7 +1011,7 @@ func WithFailingContainer(name string, exitCode int, message string) PodOption { } } -// WithUnschedulableContainer sets the .Status.Conditionss on the pod to +// WithUnschedulableContainer sets the .Status.Conditions on the pod to // include `PodScheduled` status to `False` with the given message and reason. func WithUnschedulableContainer(reason, message string) PodOption { return func(pod *corev1.Pod) { @@ -1024,6 +1024,22 @@ func WithUnschedulableContainer(reason, message string) PodOption { } } +// WithWaitingContainer sets the .Status.ContainerStatuses on the pod to +// include a container named accordingly to wait with the given state. +func WithWaitingContainer(name, reason, message string) PodOption { + return func(pod *corev1.Pod) { + pod.Status.ContainerStatuses = []corev1.ContainerStatus{{ + Name: name, + State: corev1.ContainerState{ + Waiting: &corev1.ContainerStateWaiting{ + Reason: reason, + Message: message, + }, + }, + }} + } +} + // ClusterIngressOption enables further configuration of the Cluster Ingress. type ClusterIngressOption func(*netv1alpha1.ClusterIngress) diff --git a/test/e2e/image_pull_error_test.go b/test/e2e/image_pull_error_test.go new file mode 100644 index 000000000000..a8e843d45df9 --- /dev/null +++ b/test/e2e/image_pull_error_test.go @@ -0,0 +1,110 @@ +// +build e2e + +/* +Copyright 2019 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package e2e + +import ( + "fmt" + "strings" + "testing" + + "github.com/knative/serving/pkg/apis/serving/v1alpha1" + serviceresourcenames "github.com/knative/serving/pkg/reconciler/service/resources/names" + v1alpha1testing "github.com/knative/serving/pkg/reconciler/testing" + "github.com/knative/serving/test" +) + +func TestImagePullError(t *testing.T) { + clients := Setup(t) + const ( + backoffMsg = "Back-off pulling image" + backoffReason = "ImagePullBackOff" + daemonMsg = "Error response from daemon: manifest for" + daemonReason = "ErrImagePull" + ) + names := test.ResourceNames{ + Service: test.ObjectNameForTest(t), + // TODO: Replace this when sha256 is broken. + Image: "ubuntu@sha256:0000000000000000000000000000000000000000000000000000000000000000", + } + + defer test.TearDown(clients, names) + test.CleanupOnInterrupt(func() { test.TearDown(clients, names) }) + + t.Logf("Creating a new Service %s", names.Image) + var ( + svc *v1alpha1.Service + err error + ) + if svc, err = createLatestService(t, clients, names); err != nil { + t.Fatalf("Failed to create Service %s: %v", names.Service, err) + } + + names.Config = serviceresourcenames.Configuration(svc) + + err = test.WaitForServiceState(clients.ServingClient, names.Service, func(r *v1alpha1.Service) (bool, error) { + cond := r.Status.GetCondition(v1alpha1.ConfigurationConditionReady) + if cond != nil && !cond.IsUnknown() { + if cond.IsFalse() { + if strings.Contains(cond.Message, backoffMsg) || + strings.Contains(cond.Message, daemonMsg) { + return true, nil + } + } + t.Logf("Reason: %s ; Message: %s ; Status: %s", cond.Reason, cond.Message, cond.Status) + return true, fmt.Errorf("the service %s was not marked with expected error condition, but with (Reason=\"%s\", Message=\"%s\", Status=\"%s\")", + names.Service, cond.Reason, cond.Message, cond.Status) + } + return false, nil + }, "ContainerUnpullable") + + if err != nil { + t.Fatalf("Failed to validate service state: %s", err) + } + + revisionName, err := revisionFromConfiguration(clients, names.Config) + if err != nil { + t.Fatalf("Failed to get revision from configuration %s: %v", names.Config, err) + } + + t.Log("When the images are not pulled, the revision should have error status.") + err = test.CheckRevisionState(clients.ServingClient, revisionName, func(r *v1alpha1.Revision) (bool, error) { + cond := r.Status.GetCondition(v1alpha1.RevisionConditionReady) + if cond != nil { + if (cond.Reason == backoffReason && strings.Contains(cond.Message, backoffMsg)) || + (cond.Reason == daemonReason && strings.Contains(cond.Message, daemonMsg)) { + return true, nil + } + return true, fmt.Errorf("the revision %s was not marked with expected error condition, but with (Reason=%q, Message=%q)", + revisionName, cond.Reason, cond.Message) + } + return false, nil + }) + + if err != nil { + t.Fatalf("Failed to validate revision state: %s", err) + } +} + +// Wrote our own thing so that we can pass in an image by digest. +// knative/pkg/test.ImagePath currently assumes there's a tag, which fails to parse. +func createLatestService(t *testing.T, clients *test.Clients, names test.ResourceNames) (*v1alpha1.Service, error) { + opt := v1alpha1testing.WithInlineConfigSpec(*test.ConfigurationSpec(names.Image, &test.Options{})) + service := v1alpha1testing.ServiceWithoutNamespace(names.Service, opt) + test.LogResourceObject(t, test.ResourceObjects{Service: service}) + return clients.ServingClient.Services.Create(service) +}