From c9ca9e234af6d8599f41163ae6445c712e9b11e1 Mon Sep 17 00:00:00 2001 From: Yao Wu Date: Sun, 17 Jun 2018 15:22:04 -0700 Subject: [PATCH 01/10] stabilize 1->0 case --- cmd/activator/main.go | 35 +++--- pkg/apis/serving/v1alpha1/revision_types.go | 8 ++ pkg/controller/configuration/configuration.go | 2 + pkg/controller/revision/resource.go | 4 +- pkg/controller/revision/revision.go | 102 ++++++++++-------- pkg/controller/route/route.go | 77 ++++++++----- 6 files changed, 143 insertions(+), 85 deletions(-) diff --git a/cmd/activator/main.go b/cmd/activator/main.go index 36ae25c214b4..11cba7599a29 100644 --- a/cmd/activator/main.go +++ b/cmd/activator/main.go @@ -16,6 +16,8 @@ package main import ( "flag" "fmt" + "log" + "net" "net/http" "net/http/httputil" "net/url" @@ -31,8 +33,7 @@ import ( ) const ( - maxRetry = 60 - retryInterval = 1 * time.Second + maxRetry = 10 ) type activationHandler struct { @@ -45,22 +46,28 @@ type activationHandler struct { type retryRoundTripper struct{} func (rrt retryRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { - transport := http.DefaultTransport - resp, err := transport.RoundTrip(r) - // TODO: Activator should retry with backoff. - // https://github.com/knative/serving/issues/1229 - i := 1 + transport := http.DefaultTransport.(*http.Transport) + timeout := 500 * time.Millisecond + + i := 0 for ; i < maxRetry; i++ { - if err == nil && resp != nil && resp.StatusCode != 503 { - break + transport.DialContext = (&net.Dialer{ + Timeout: timeout, + KeepAlive: 30 * time.Second, + }).DialContext + resp, err := transport.RoundTrip(r) + if err == nil && resp != nil { + //defer resp.Body.Close() + if resp.StatusCode != 503 { + return resp, nil + } } - resp.Body.Close() - time.Sleep(retryInterval) - resp, err = transport.RoundTrip(r) + timeout = timeout + timeout + log.Printf("Retrying request to %v in %v", r.URL.Host, timeout) + time.Sleep(timeout) } // TODO: add metrics for number of tries and the response code. - glog.Infof("It took %d tries to get response code %d", i, resp.StatusCode) - return resp, nil + return transport.RoundTrip(r) } func (a *activationHandler) handler(w http.ResponseWriter, r *http.Request) { diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index 554f55489b31..7c275057c08f 100644 --- a/pkg/apis/serving/v1alpha1/revision_types.go +++ b/pkg/apis/serving/v1alpha1/revision_types.go @@ -403,6 +403,14 @@ func (rs *RevisionStatus) MarkInactive() { }) } +func (rs *RevisionStatus) MarkDeactivating() { + rs.setCondition(&RevisionCondition{ + Type: RevisionConditionReady, + Status: corev1.ConditionFalse, + Reason: "Deactivating", + }) +} + func (rs *RevisionStatus) MarkContainerMissing(message string) { for _, cond := range []RevisionConditionType{ RevisionConditionContainerHealthy, diff --git a/pkg/controller/configuration/configuration.go b/pkg/controller/configuration/configuration.go index 240d71453449..ad899f443f19 100644 --- a/pkg/controller/configuration/configuration.go +++ b/pkg/controller/configuration/configuration.go @@ -229,6 +229,8 @@ func (c *Controller) createRevision(config *v1alpha1.Configuration, revName stri rev.Labels = make(map[string]string) } rev.Labels[serving.ConfigurationLabelKey] = config.Name + logger.Infof("config here: %v", config) + rev.Labels[serving.RouteLabelKey] = config.Labels[serving.RouteLabelKey] if rev.Annotations == nil { rev.Annotations = make(map[string]string) } diff --git a/pkg/controller/revision/resource.go b/pkg/controller/revision/resource.go index bc783ef69b29..139487e779f6 100644 --- a/pkg/controller/revision/resource.go +++ b/pkg/controller/revision/resource.go @@ -31,7 +31,9 @@ func MakeElaResourceLabels(revision *v1alpha1.Revision) map[string]string { labels[serving.RevisionUID] = string(revision.UID) for k, v := range revision.ObjectMeta.Labels { - labels[k] = v + if k != "serving.knative.dev/route" { + labels[k] = v + } } // If users don't specify an app: label we will automatically // populate it with the revision name to get the benefit of richer diff --git a/pkg/controller/revision/revision.go b/pkg/controller/revision/revision.go index 1be7742a30e6..2d338a13e52b 100644 --- a/pkg/controller/revision/revision.go +++ b/pkg/controller/revision/revision.go @@ -78,7 +78,7 @@ const ( var ( elaPodReplicaCount = int32(1) - elaPodMaxUnavailable = intstr.IntOrString{Type: intstr.Int, IntVal: 1} + elaPodMaxUnavailable = intstr.IntOrString{Type: intstr.Int, IntVal: 0} elaPodMaxSurge = intstr.IntOrString{Type: intstr.Int, IntVal: 1} ) @@ -494,12 +494,26 @@ func (c *Controller) reconcileOnceBuilt(ctx context.Context, rev *v1alpha1.Revis func (c *Controller) deleteK8SResources(ctx context.Context, rev *v1alpha1.Revision, ns string) error { logger := logging.FromContext(ctx) - logger.Info("Deleting the resources for revision") - err := c.deleteDeployment(ctx, rev, ns) + if cond := rev.Status.GetCondition(v1alpha1.RevisionConditionReady); cond != nil { + if cond.Reason != "Inactive" { + if cond.Reason != "Deactivating" { + rev.Status.MarkDeactivating() + if _, err := c.updateStatus(rev); err != nil { + logger.Error("Error updating revision condition to be deactivating", + zap.Error(err)) + return err + } + } + return nil + } + } + + logger.Info("Scale the deployment to 0") + err := c.deactivateDeployment(ctx, rev, ns) if err != nil { - logger.Error("Failed to delete a deployment", zap.Error(err)) + logger.Error("Failed to scale a deployment to 0", zap.Error(err)) } - logger.Info("Deleted deployment") + logger.Info("Scaled the deployment to 0") err = c.deleteAutoscalerDeployment(ctx, rev) if err != nil { @@ -512,21 +526,6 @@ func (c *Controller) deleteK8SResources(ctx context.Context, rev *v1alpha1.Revis logger.Error("Failed to delete autoscaler Service", zap.Error(err)) } logger.Info("Deleted autoscaler Service") - - err = c.deleteService(ctx, rev, ns) - if err != nil { - logger.Error("Failed to delete k8s service", zap.Error(err)) - } - logger.Info("Deleted service") - - // And the deployment is no longer ready, so update that - rev.Status.MarkInactive() - logger.Infof("Updating status with the following conditions %+v", rev.Status.Conditions) - if _, err := c.updateStatus(rev); err != nil { - logger.Error("Error recording inactivation of revision", zap.Error(err)) - return err - } - return nil } @@ -601,6 +600,26 @@ func (c *Controller) createK8SResources(ctx context.Context, rev *v1alpha1.Revis return nil } +func (c *Controller) deactivateDeployment(ctx context.Context, rev *v1alpha1.Revision, ns string) error { + logger := logging.FromContext(ctx) + deploymentName := controller.GetRevisionDeploymentName(rev) + dc := c.KubeClientSet.AppsV1().Deployments(ns) + deployment, err := dc.Get(deploymentName, metav1.GetOptions{}) + if err != nil { + return err + } + + logger.Infof("Deactvaing Deployment %q", deploymentName) + deployment.Spec.Replicas = new(int32) + *deployment.Spec.Replicas = int32(0) + _, err = dc.Update(deployment) + if err != nil { + logger.Errorf("Error deactivating deployment %q: %s", deploymentName, err) + } + logger.Infof("Successfully scaled deployment %s to 0.", deploymentName) + return nil +} + func (c *Controller) deleteDeployment(ctx context.Context, rev *v1alpha1.Revision, ns string) error { logger := logging.FromContext(ctx) deploymentName := controller.GetRevisionDeploymentName(rev) @@ -627,31 +646,30 @@ func (c *Controller) reconcileDeployment(ctx context.Context, rev *v1alpha1.Revi // First, check if deployment exists already. deploymentName := controller.GetRevisionDeploymentName(rev) - if _, err := dc.Get(deploymentName, metav1.GetOptions{}); err != nil { + // Create the deployment. + controllerRef := controller.NewRevisionControllerRef(rev) + // Create a single pod so that it gets created before deployment->RS to try to speed + // things up + podSpec := MakeElaPodSpec(rev, c.controllerConfig) + isCreate := false + _, err := dc.Get(deploymentName, metav1.GetOptions{}) + if err != nil { if !apierrs.IsNotFound(err) { logger.Errorf("deployments.Get for %q failed: %s", deploymentName, err) return err } - logger.Infof("Deployment %q doesn't exist, creating", deploymentName) - } else { - // TODO(mattmoor): Compare the deployments and update if it has changed - // out from under us. - logger.Infof("Found existing deployment %q", deploymentName) - return nil + isCreate = true } - // Create the deployment. - controllerRef := controller.NewRevisionControllerRef(rev) - // Create a single pod so that it gets created before deployment->RS to try to speed - // things up - podSpec := MakeElaPodSpec(rev, c.controllerConfig) deployment := MakeElaDeployment(logger, rev, ns, c.getNetworkConfig()) deployment.OwnerReferences = append(deployment.OwnerReferences, *controllerRef) - + // Set the ProgressDeadlineSeconds + deployment.Spec.ProgressDeadlineSeconds = new(int32) + *deployment.Spec.ProgressDeadlineSeconds = progressDeadlineSeconds deployment.Spec.Template.Spec = *podSpec // Resolve tag image references to digests. - if err := c.resolver.Resolve(deployment); err != nil { + if err = c.resolver.Resolve(deployment); err != nil { logger.Error("Error resolving deployment", zap.Error(err)) rev.Status.MarkContainerMissing(err.Error()) if _, err := c.updateStatus(rev); err != nil { @@ -661,14 +679,14 @@ func (c *Controller) reconcileDeployment(ctx context.Context, rev *v1alpha1.Revi return err } - // Set the ProgressDeadlineSeconds - deployment.Spec.ProgressDeadlineSeconds = new(int32) - *deployment.Spec.ProgressDeadlineSeconds = progressDeadlineSeconds - - logger.Infof("Creating Deployment: %q", deployment.Name) - _, createErr := dc.Create(deployment) - - return createErr + if isCreate { + logger.Infof("Deployment %q doesn't exist, creating", deploymentName) + _, err = dc.Create(deployment) + } else { + logger.Infof("Found existing deployment %q, updating", deploymentName) + _, err = dc.Update(deployment) + } + return err } func (c *Controller) deleteService(ctx context.Context, rev *v1alpha1.Revision, ns string) error { diff --git a/pkg/controller/route/route.go b/pkg/controller/route/route.go index f8431de472ba..385e18591c9d 100644 --- a/pkg/controller/route/route.go +++ b/pkg/controller/route/route.go @@ -295,7 +295,7 @@ func (c *Controller) syncTrafficTargetsAndUpdateRouteStatus(ctx context.Context, // Then create the actual route rules. logger.Info("Creating Istio route rules") - revisionRoutes, err := c.createOrUpdateRouteRules(ctx, route, configMap, revMap) + revisionRoutes, inactiveRev, err := c.createOrUpdateRouteRules(ctx, route, configMap, revMap) if err != nil { logger.Error("Failed to create routes", zap.Error(err)) return nil, err @@ -314,7 +314,27 @@ func (c *Controller) syncTrafficTargetsAndUpdateRouteStatus(ctx context.Context, route.Status.Traffic = traffic } route.Status.Domain = c.routeDomain(route) - return c.updateStatus(ctx, route) + newRoute, err := c.updateStatus(ctx, route) + if err != nil { + logger.Error("Failed to update routes", zap.Error(err)) + return nil, err + } + // After the route rules are updated, mark the revision inactive. In case of deactivation, + // the routerules need to point to activator-service before we deactivate the deployment. + if inactiveRev != "" { + revisionClient := c.ElaClientSet.ServingV1alpha1().Revisions(route.Namespace) + rev, err := revisionClient.Get(inactiveRev, metav1.GetOptions{}) + if err != nil { + logger.Errorf("Failed to fetch the inactive revision %s: %s", inactiveRev, err) + return nil, err + } + rev.Status.MarkInactive() + if _, err = revisionClient.Update(rev); err != nil { + logger.Errorf("Failed to update revision %s to be inactive: %s", inactiveRev, err) + return nil, err + } + } + return newRoute, nil } func (c *Controller) reconcilePlaceholderService(ctx context.Context, route *v1alpha1.Route) error { @@ -673,33 +693,34 @@ func (c *Controller) computeRevisionRoutes( return nil, "", err } - hasRouteRule := true + revisionWeight := tt.Percent cond := rev.Status.GetCondition(v1alpha1.RevisionConditionReady) if enableScaleToZero && cond != nil { // A revision is considered inactive (yet) if it's in - // "Inactive" condition or "Activating" condition. + // "Inactive" condition, "Activating" or "Deactivating" condition. + logger.Infof("cond: %v", cond) if (cond.Reason == "Inactive" && cond.Status == corev1.ConditionFalse) || - (cond.Reason == "Activating" && cond.Status == corev1.ConditionUnknown) { + (cond.Reason == "Activating" && cond.Status == corev1.ConditionUnknown) || + (cond.Reason == "Deactivating" && cond.Status == corev1.ConditionFalse) { // Let inactiveRev be the Reserve revision with the largest traffic weight. if tt.Percent > maxInactivePercent { maxInactivePercent = tt.Percent inactiveRev = rev.Name } totalInactivePercent += tt.Percent - hasRouteRule = false + revisionWeight = 0 } } - if hasRouteRule { - rr := RevisionRoute{ - Name: tt.Name, - RevisionName: rev.Name, - Service: rev.Status.ServiceName, - Namespace: elaNS, - Weight: tt.Percent, - } - ret = append(ret, rr) + rr := RevisionRoute{ + Name: tt.Name, + RevisionName: rev.Name, + Service: rev.Status.ServiceName, + Namespace: elaNS, + Weight: revisionWeight, } + logger.Infof("RevisionRoute: %v", rr) + ret = append(ret, rr) } // TODO: The ideal solution is to append different revision name as headers for each inactive revision. @@ -765,31 +786,31 @@ func (c *Controller) computeEmptyRevisionRoutes( } func (c *Controller) createOrUpdateRouteRules(ctx context.Context, route *v1alpha1.Route, - configMap map[string]*v1alpha1.Configuration, revMap map[string]*v1alpha1.Revision) ([]RevisionRoute, error) { + configMap map[string]*v1alpha1.Configuration, revMap map[string]*v1alpha1.Revision) ([]RevisionRoute, string, error) { logger := logging.FromContext(ctx) // grab a client that's specific to RouteRule. ns := route.Namespace routeClient := c.ElaClientSet.ConfigV1alpha2().RouteRules(ns) if routeClient == nil { logger.Errorf("Failed to create resource client") - return nil, fmt.Errorf("Couldn't get a routeClient") + return nil, "", fmt.Errorf("Couldn't get a routeClient") } revisionRoutes, inactiveRev, err := c.computeRevisionRoutes(ctx, route, configMap, revMap) if err != nil { logger.Errorf("Failed to get routes for %s : %q", route.Name, err) - return nil, err + return nil, "", err } if len(revisionRoutes) == 0 { logger.Errorf("No routes were found for the service %q", route.Name) - return nil, nil + return nil, "", nil } // TODO: remove this once https://github.com/istio/istio/issues/5204 is fixed. emptyRoutes, err := c.computeEmptyRevisionRoutes(ctx, route, configMap, revMap) if err != nil { logger.Errorf("Failed to get empty routes for %s : %q", route.Name, err) - return nil, err + return nil, "", err } revisionRoutes = append(revisionRoutes, emptyRoutes...) // Create route rule for the route domain @@ -797,19 +818,19 @@ func (c *Controller) createOrUpdateRouteRules(ctx context.Context, route *v1alph routeRules, err := routeClient.Get(routeRuleName, metav1.GetOptions{}) if err != nil { if !apierrs.IsNotFound(err) { - return nil, err + return nil, "", err } routeRules = MakeIstioRoutes(route, nil, ns, revisionRoutes, c.routeDomain(route), inactiveRev) if _, err := routeClient.Create(routeRules); err != nil { c.Recorder.Eventf(route, corev1.EventTypeWarning, "CreationFailed", "Failed to create Istio route rule %q: %s", routeRules.Name, err) - return nil, err + return nil, "", err } c.Recorder.Eventf(route, corev1.EventTypeNormal, "Created", "Created Istio route rule %q", routeRules.Name) } else { routeRules.Spec = makeIstioRouteSpec(route, nil, ns, revisionRoutes, c.routeDomain(route), inactiveRev) if _, err := routeClient.Update(routeRules); err != nil { c.Recorder.Eventf(route, corev1.EventTypeWarning, "UpdateFailed", "Failed to update Istio route rule %q: %s", routeRules.Name, err) - return nil, err + return nil, "", err } c.Recorder.Eventf(route, corev1.EventTypeNormal, "Updated", "Updated Istio route rule %q", routeRules.Name) } @@ -823,26 +844,26 @@ func (c *Controller) createOrUpdateRouteRules(ctx context.Context, route *v1alph routeRules, err := routeClient.Get(routeRuleName, metav1.GetOptions{}) if err != nil { if !apierrs.IsNotFound(err) { - return nil, err + return nil, "", err } routeRules = MakeIstioRoutes(route, &tt, ns, revisionRoutes, c.routeDomain(route), inactiveRev) if _, err := routeClient.Create(routeRules); err != nil { c.Recorder.Eventf(route, corev1.EventTypeWarning, "CreationFailed", "Failed to create Istio route rule %q: %s", routeRules.Name, err) - return nil, err + return nil, "", err } c.Recorder.Eventf(route, corev1.EventTypeNormal, "Created", "Created Istio route rule %q", routeRules.Name) } else { routeRules.Spec = makeIstioRouteSpec(route, &tt, ns, revisionRoutes, c.routeDomain(route), inactiveRev) if _, err := routeClient.Update(routeRules); err != nil { - return nil, err + return nil, "", err } c.Recorder.Eventf(route, corev1.EventTypeNormal, "Updated", "Updated Istio route rule %q", routeRules.Name) } } if err := c.removeOutdatedRouteRules(ctx, route); err != nil { - return nil, err + return nil, "", err } - return revisionRoutes, nil + return revisionRoutes, inactiveRev, nil } func (c *Controller) updateStatus(ctx context.Context, route *v1alpha1.Route) (*v1alpha1.Route, error) { From 36bb4391100303ca8cb14790d2b2019e34278936 Mon Sep 17 00:00:00 2001 From: Yao Wu Date: Mon, 18 Jun 2018 13:55:48 -0700 Subject: [PATCH 02/10] update tests --- pkg/controller/configuration/configuration.go | 2 - pkg/controller/revision/autoscaler.go | 4 +- pkg/controller/revision/pod.go | 4 +- pkg/controller/revision/resource.go | 8 +-- pkg/controller/revision/revision.go | 53 ++++++++++++------- pkg/controller/revision/revision_test.go | 9 ++-- pkg/controller/revision/service.go | 2 +- pkg/controller/route/route.go | 2 +- pkg/controller/route/route_test.go | 32 ++++++++--- 9 files changed, 76 insertions(+), 40 deletions(-) diff --git a/pkg/controller/configuration/configuration.go b/pkg/controller/configuration/configuration.go index ad899f443f19..240d71453449 100644 --- a/pkg/controller/configuration/configuration.go +++ b/pkg/controller/configuration/configuration.go @@ -229,8 +229,6 @@ func (c *Controller) createRevision(config *v1alpha1.Configuration, revName stri rev.Labels = make(map[string]string) } rev.Labels[serving.ConfigurationLabelKey] = config.Name - logger.Infof("config here: %v", config) - rev.Labels[serving.RouteLabelKey] = config.Labels[serving.RouteLabelKey] if rev.Annotations == nil { rev.Annotations = make(map[string]string) } diff --git a/pkg/controller/revision/autoscaler.go b/pkg/controller/revision/autoscaler.go index 1a86c5ce1263..dc92ac2d407a 100644 --- a/pkg/controller/revision/autoscaler.go +++ b/pkg/controller/revision/autoscaler.go @@ -78,7 +78,7 @@ func MakeElaAutoscalerDeployment(rev *v1alpha1.Revision, autoscalerImage string) ObjectMeta: metav1.ObjectMeta{ Name: controller.GetRevisionAutoscalerName(rev), Namespace: pkg.GetServingSystemNamespace(), - Labels: MakeElaResourceLabels(rev), + Labels: MakeElaResourceLabels(rev, true), Annotations: MakeElaResourceAnnotations(rev), }, Spec: appsv1.DeploymentSpec{ @@ -181,7 +181,7 @@ func MakeElaAutoscalerService(rev *v1alpha1.Revision) *corev1.Service { // makeElaAutoScalerLabels constructs the labels we will apply to // service and deployment specs for autoscaler. func makeElaAutoScalerLabels(rev *v1alpha1.Revision) map[string]string { - labels := MakeElaResourceLabels(rev) + labels := MakeElaResourceLabels(rev, true) labels[serving.AutoscalerLabelKey] = controller.GetRevisionAutoscalerName(rev) return labels } diff --git a/pkg/controller/revision/pod.go b/pkg/controller/revision/pod.go index d722570629b3..aa7afb282da7 100644 --- a/pkg/controller/revision/pod.go +++ b/pkg/controller/revision/pod.go @@ -224,7 +224,7 @@ func MakeElaDeployment(logger *zap.SugaredLogger, u *v1alpha1.Revision, namespac ObjectMeta: metav1.ObjectMeta{ Name: controller.GetRevisionDeploymentName(u), Namespace: namespace, - Labels: MakeElaResourceLabels(u), + Labels: MakeElaResourceLabels(u, true), Annotations: MakeElaResourceAnnotations(u), }, Spec: appsv1.DeploymentSpec{ @@ -236,7 +236,7 @@ func MakeElaDeployment(logger *zap.SugaredLogger, u *v1alpha1.Revision, namespac }, Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ - Labels: MakeElaResourceLabels(u), + Labels: MakeElaResourceLabels(u, true), Annotations: podTemplateAnnotations, }, }, diff --git a/pkg/controller/revision/resource.go b/pkg/controller/revision/resource.go index 139487e779f6..71ba11bdc453 100644 --- a/pkg/controller/revision/resource.go +++ b/pkg/controller/revision/resource.go @@ -25,13 +25,13 @@ import ( const appLabelKey = "app" // MakeElaResourceLabels constructs the labels we will apply to K8s resources. -func MakeElaResourceLabels(revision *v1alpha1.Revision) map[string]string { +func MakeElaResourceLabels(revision *v1alpha1.Revision, includeRouteLabel bool) map[string]string { labels := make(map[string]string, len(revision.ObjectMeta.Labels)+2) labels[serving.RevisionLabelKey] = revision.Name labels[serving.RevisionUID] = string(revision.UID) for k, v := range revision.ObjectMeta.Labels { - if k != "serving.knative.dev/route" { + if includeRouteLabel || k != serving.RouteLabelKey { labels[k] = v } } @@ -46,7 +46,9 @@ func MakeElaResourceLabels(revision *v1alpha1.Revision) map[string]string { // MakeElaResourceSelector constructs the Selector we will apply to K8s resources. func MakeElaResourceSelector(revision *v1alpha1.Revision) *metav1.LabelSelector { - return &metav1.LabelSelector{MatchLabels: MakeElaResourceLabels(revision)} + // Deployment spec.selector is an immutable field so we need to exclude the route label, + // which could change in a revision. + return &metav1.LabelSelector{MatchLabels: MakeElaResourceLabels(revision, false)} } // MakeElaResourceAnnotations creates the annotations we will apply to diff --git a/pkg/controller/revision/revision.go b/pkg/controller/revision/revision.go index 2d338a13e52b..509698cfe652 100644 --- a/pkg/controller/revision/revision.go +++ b/pkg/controller/revision/revision.go @@ -489,33 +489,50 @@ func (c *Controller) reconcileOnceBuilt(ctx context.Context, rev *v1alpha1.Revis logger.Info("Creating or reconciling resources for revision") return c.createK8SResources(ctx, rev, elaNS) } - return c.deleteK8SResources(ctx, rev, elaNS) + return c.teardownK8SResources(ctx, rev, elaNS) } -func (c *Controller) deleteK8SResources(ctx context.Context, rev *v1alpha1.Revision, ns string) error { +func (c *Controller) teardownK8SResources(ctx context.Context, rev *v1alpha1.Revision, ns string) error { logger := logging.FromContext(ctx) - if cond := rev.Status.GetCondition(v1alpha1.RevisionConditionReady); cond != nil { - if cond.Reason != "Inactive" { - if cond.Reason != "Deactivating" { - rev.Status.MarkDeactivating() - if _, err := c.updateStatus(rev); err != nil { - logger.Error("Error updating revision condition to be deactivating", - zap.Error(err)) - return err + if rev.Spec.ServingState == v1alpha1.RevisionServingStateRetired { + // Delete the k8s deployment and revision service if serving state is Retired. + logger.Info("Deleting the resources for revision") + err := c.deleteDeployment(ctx, rev, ns) + if err != nil { + logger.Error("Failed to delete a deployment", zap.Error(err)) + } + logger.Info("Deleted deployment") + err = c.deleteService(ctx, rev, ns) + if err != nil { + logger.Error("Failed to delete k8s service", zap.Error(err)) + } + logger.Info("Deleted service") + } else { + // Serving state is RevisionServingStateReserve. Delete the revision service and update + // the dpeloyment replicas to be 0. + if cond := rev.Status.GetCondition(v1alpha1.RevisionConditionReady); cond != nil { + if cond.Reason != "Inactive" { + if cond.Reason != "Deactivating" { + rev.Status.MarkDeactivating() + if _, err := c.updateStatus(rev); err != nil { + logger.Error("Error updating revision condition to be deactivating", + zap.Error(err)) + return err + } } + return nil } - return nil } - } - logger.Info("Scale the deployment to 0") - err := c.deactivateDeployment(ctx, rev, ns) - if err != nil { - logger.Error("Failed to scale a deployment to 0", zap.Error(err)) + logger.Info("Scale the deployment to 0") + err := c.deactivateDeployment(ctx, rev, ns) + if err != nil { + logger.Error("Failed to scale a deployment to 0", zap.Error(err)) + } + logger.Info("Scaled the deployment to 0") } - logger.Info("Scaled the deployment to 0") - err = c.deleteAutoscalerDeployment(ctx, rev) + err := c.deleteAutoscalerDeployment(ctx, rev) if err != nil { logger.Error("Failed to delete autoscaler Deployment", zap.Error(err)) } diff --git a/pkg/controller/revision/revision_test.go b/pkg/controller/revision/revision_test.go index a6aeccda5606..138107702dad 100644 --- a/pkg/controller/revision/revision_test.go +++ b/pkg/controller/revision/revision_test.go @@ -1489,10 +1489,11 @@ func TestActiveToReserveRevisionDeletesStuff(t *testing.T) { rev.Spec.ServingState = v1alpha1.RevisionServingStateReserve updateRevision(elaClient, elaInformer, controller, rev) - // Expect the deployment to be gone. - deployment, err := kubeClient.AppsV1().Deployments(testNamespace).Get(expectedDeploymentName, metav1.GetOptions{}) - if err == nil { - t.Fatalf("Expected ela deployment to be missing but it was really here: %v", deployment) + // Expect the deployment to be there. + _, err = kubeClient.AppsV1().Deployments(testNamespace).Get(expectedDeploymentName, metav1.GetOptions{}) + + if err != nil { + t.Fatalf("Expected k8s deployment to be there but it was gone: %s/%s", testNamespace, expectedDeploymentName) } } diff --git a/pkg/controller/revision/service.go b/pkg/controller/revision/service.go index afb1c669aa28..cafbb0fa1f94 100644 --- a/pkg/controller/revision/service.go +++ b/pkg/controller/revision/service.go @@ -37,7 +37,7 @@ func MakeRevisionK8sService(u *v1alpha1.Revision, ns string) *corev1.Service { ObjectMeta: meta_v1.ObjectMeta{ Name: controller.GetElaK8SServiceNameForRevision(u), Namespace: ns, - Labels: MakeElaResourceLabels(u), + Labels: MakeElaResourceLabels(u, true), Annotations: MakeElaResourceAnnotations(u), }, Spec: corev1.ServiceSpec{ diff --git a/pkg/controller/route/route.go b/pkg/controller/route/route.go index 385e18591c9d..b3848eb42527 100644 --- a/pkg/controller/route/route.go +++ b/pkg/controller/route/route.go @@ -320,7 +320,7 @@ func (c *Controller) syncTrafficTargetsAndUpdateRouteStatus(ctx context.Context, return nil, err } // After the route rules are updated, mark the revision inactive. In case of deactivation, - // the routerules need to point to activator-service before we deactivate the deployment. + // the routerules need to point to activator-service before we tear down k8s resources. if inactiveRev != "" { revisionClient := c.ElaClientSet.ServingV1alpha1().Revisions(route.Namespace) rev, err := revisionClient.Get(inactiveRev, metav1.GetOptions{}) diff --git a/pkg/controller/route/route_test.go b/pkg/controller/route/route_test.go index fca631ff9d15..50db8eb1648c 100644 --- a/pkg/controller/route/route_test.go +++ b/pkg/controller/route/route_test.go @@ -429,7 +429,16 @@ func TestCreateRouteForOneReserveRevision(t *testing.T) { }, }, }, - Route: []v1alpha2.DestinationWeight{getActivatorDestinationWeight(100)}, + Route: []v1alpha2.DestinationWeight{ + { + Destination: v1alpha2.IstioService{ + Name: "test-rev-service", + Namespace: testNamespace, + }, + Weight: 0, + }, + getActivatorDestinationWeight(100), + }, AppendHeaders: appendHeaders, } @@ -663,13 +672,22 @@ func TestCreateRouteWithOneTargetReserve(t *testing.T) { }, }, }, - Route: []v1alpha2.DestinationWeight{{ - Destination: v1alpha2.IstioService{ - Name: fmt.Sprintf("%s-service", cfgrev.Name), - Namespace: testNamespace, + Route: []v1alpha2.DestinationWeight{ + { + Destination: v1alpha2.IstioService{ + Name: "p-deadbeef-service", + Namespace: testNamespace, + }, + Weight: 90, }, - Weight: 90, - }, getActivatorDestinationWeight(10)}, + { + Destination: v1alpha2.IstioService{ + Name: "test-rev-service", + Namespace: testNamespace, + }, + Weight: 0, + }, + getActivatorDestinationWeight(10)}, AppendHeaders: appendHeaders, } From 189d5ce199ce857bdd0c15c6a4f2e40fd69f3c8d Mon Sep 17 00:00:00 2001 From: Yao Wu Date: Mon, 18 Jun 2018 14:45:56 -0700 Subject: [PATCH 03/10] minor clean up --- pkg/controller/revision/revision.go | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/pkg/controller/revision/revision.go b/pkg/controller/revision/revision.go index 3abc759a59e1..756290b1c572 100644 --- a/pkg/controller/revision/revision.go +++ b/pkg/controller/revision/revision.go @@ -649,28 +649,17 @@ func (c *Controller) reconcileDeployment(ctx context.Context, rev *v1alpha1.Revi // First, check if deployment exists already. deploymentName := controller.GetRevisionDeploymentName(rev) - // // Create the deployment. - // controllerRef := controller.NewRevisionControllerRef(rev) - // // Create a single pod so that it gets created before deployment->RS to try to speed - // // things up - // podSpec := MakeServingPodSpec(rev, c.controllerConfig) - isCreate := false + deploymentExists := true _, err := dc.Get(deploymentName, metav1.GetOptions{}) if err != nil { if !apierrs.IsNotFound(err) { logger.Errorf("deployments.Get for %q failed: %s", deploymentName, err) return err } - isCreate = true + deploymentExists = false } deployment := MakeServingDeployment(logger, rev, c.getNetworkConfig(), c.controllerConfig) - // deployment.OwnerReferences = append(deployment.OwnerReferences, *controllerRef) - // // Set the ProgressDeadlineSeconds - // deployment.Spec.ProgressDeadlineSeconds = new(int32) - // *deployment.Spec.ProgressDeadlineSeconds = progressDeadlineSeconds - // deployment.Spec.Template.Spec = *podSpec - // Resolve tag image references to digests. if err = c.resolver.Resolve(deployment); err != nil { logger.Error("Error resolving deployment", zap.Error(err)) @@ -682,14 +671,14 @@ func (c *Controller) reconcileDeployment(ctx context.Context, rev *v1alpha1.Revi return err } - if isCreate { - logger.Infof("Deployment %q doesn't exist, creating", deploymentName) - _, err = dc.Create(deployment) - } else { + if deploymentExists { // TODO(mattmoor): Compare the deployments and update if it has changed // out from under us. logger.Infof("Found existing deployment %q, updating", deploymentName) _, err = dc.Update(deployment) + } else { + logger.Infof("Deployment %q doesn't exist, creating", deploymentName) + _, err = dc.Create(deployment) } return err } From 440a361c60e26ace5aa9689a1cb4add5e1ac9ec5 Mon Sep 17 00:00:00 2001 From: Yao Wu Date: Mon, 18 Jun 2018 15:17:25 -0700 Subject: [PATCH 04/10] delete route label from revisions --- pkg/controller/revision/autoscaler.go | 4 ++-- pkg/controller/revision/pod.go | 4 ++-- pkg/controller/revision/resource.go | 6 +++--- pkg/controller/revision/revision_test.go | 5 ++--- pkg/controller/revision/service.go | 2 +- 5 files changed, 10 insertions(+), 11 deletions(-) diff --git a/pkg/controller/revision/autoscaler.go b/pkg/controller/revision/autoscaler.go index eadcbf9ca4f6..451e1b11cc24 100644 --- a/pkg/controller/revision/autoscaler.go +++ b/pkg/controller/revision/autoscaler.go @@ -78,7 +78,7 @@ func MakeServingAutoscalerDeployment(rev *v1alpha1.Revision, autoscalerImage str ObjectMeta: metav1.ObjectMeta{ Name: controller.GetRevisionAutoscalerName(rev), Namespace: pkg.GetServingSystemNamespace(), - Labels: MakeServingResourceLabels(rev, true), + Labels: MakeServingResourceLabels(rev), Annotations: MakeServingResourceAnnotations(rev), OwnerReferences: []metav1.OwnerReference{*controller.NewRevisionControllerRef(rev)}, }, @@ -183,7 +183,7 @@ func MakeServingAutoscalerService(rev *v1alpha1.Revision) *corev1.Service { // makeServingAutoScalerLabels constructs the labels we will apply to // service and deployment specs for autoscaler. func makeServingAutoScalerLabels(rev *v1alpha1.Revision) map[string]string { - labels := MakeServingResourceLabels(rev, true) + labels := MakeServingResourceLabels(rev) labels[serving.AutoscalerLabelKey] = controller.GetRevisionAutoscalerName(rev) return labels } diff --git a/pkg/controller/revision/pod.go b/pkg/controller/revision/pod.go index 33fe5d701113..bc7f38c35aab 100644 --- a/pkg/controller/revision/pod.go +++ b/pkg/controller/revision/pod.go @@ -230,7 +230,7 @@ func MakeServingDeployment(logger *zap.SugaredLogger, rev *v1alpha1.Revision, ObjectMeta: metav1.ObjectMeta{ Name: controller.GetRevisionDeploymentName(rev), Namespace: controller.GetServingNamespaceName(rev.Namespace), - Labels: MakeServingResourceLabels(rev, true), + Labels: MakeServingResourceLabels(rev), Annotations: MakeServingResourceAnnotations(rev), OwnerReferences: []metav1.OwnerReference{*controller.NewRevisionControllerRef(rev)}, }, @@ -244,7 +244,7 @@ func MakeServingDeployment(logger *zap.SugaredLogger, rev *v1alpha1.Revision, ProgressDeadlineSeconds: &progressDeadlineSeconds, Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ - Labels: MakeServingResourceLabels(rev, true), + Labels: MakeServingResourceLabels(rev), Annotations: podTemplateAnnotations, }, Spec: *MakeServingPodSpec(rev, controllerConfig), diff --git a/pkg/controller/revision/resource.go b/pkg/controller/revision/resource.go index 00c43c925f1e..e47d8220f1e7 100644 --- a/pkg/controller/revision/resource.go +++ b/pkg/controller/revision/resource.go @@ -25,13 +25,13 @@ import ( const appLabelKey = "app" // MakeServingResourceLabels constructs the labels we will apply to K8s resources. -func MakeServingResourceLabels(revision *v1alpha1.Revision, includeRouteLabel bool) map[string]string { +func MakeServingResourceLabels(revision *v1alpha1.Revision) map[string]string { labels := make(map[string]string, len(revision.ObjectMeta.Labels)+2) labels[serving.RevisionLabelKey] = revision.Name labels[serving.RevisionUID] = string(revision.UID) for k, v := range revision.ObjectMeta.Labels { - if includeRouteLabel || k != serving.RouteLabelKey { + if k != serving.RouteLabelKey { labels[k] = v } } @@ -48,7 +48,7 @@ func MakeServingResourceLabels(revision *v1alpha1.Revision, includeRouteLabel bo func MakeServingResourceSelector(revision *v1alpha1.Revision) *metav1.LabelSelector { // Deployment spec.selector is an immutable field so we need to exclude the route label, // which could change in a revision. - return &metav1.LabelSelector{MatchLabels: MakeServingResourceLabels(revision, false)} + return &metav1.LabelSelector{MatchLabels: MakeServingResourceLabels(revision)} } // MakeServingResourceAnnotations creates the annotations we will apply to diff --git a/pkg/controller/revision/revision_test.go b/pkg/controller/revision/revision_test.go index f8e35642d0ff..cd2d7e47ed70 100644 --- a/pkg/controller/revision/revision_test.go +++ b/pkg/controller/revision/revision_test.go @@ -78,9 +78,8 @@ func getTestRevision() *v1alpha1.Revision { Name: "test-rev", Namespace: testNamespace, Labels: map[string]string{ - "testLabel1": "foo", - "testLabel2": "bar", - serving.RouteLabelKey: "test-route", + "testLabel1": "foo", + "testLabel2": "bar", }, Annotations: map[string]string{ "testAnnotation": "test", diff --git a/pkg/controller/revision/service.go b/pkg/controller/revision/service.go index 8d8b7cdbec90..66841e007064 100644 --- a/pkg/controller/revision/service.go +++ b/pkg/controller/revision/service.go @@ -37,7 +37,7 @@ func MakeRevisionK8sService(rev *v1alpha1.Revision) *corev1.Service { ObjectMeta: metav1.ObjectMeta{ Name: controller.GetServingK8SServiceNameForRevision(rev), Namespace: controller.GetServingNamespaceName(rev.Namespace), - Labels: MakeServingResourceLabels(rev, true), + Labels: MakeServingResourceLabels(rev), Annotations: MakeServingResourceAnnotations(rev), OwnerReferences: []metav1.OwnerReference{*controller.NewRevisionControllerRef(rev)}, }, From eadba78aa4d008b81775c5cc5b61a7f6c17d3c3e Mon Sep 17 00:00:00 2001 From: Yao Wu Date: Tue, 19 Jun 2018 09:55:31 -0700 Subject: [PATCH 05/10] address cr --- cmd/activator/main.go | 17 ++++++++++------- pkg/controller/revision/revision.go | 8 +++++++- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/cmd/activator/main.go b/cmd/activator/main.go index 07aed2500966..2ba78ef27b63 100644 --- a/cmd/activator/main.go +++ b/cmd/activator/main.go @@ -44,16 +44,16 @@ type activationHandler struct { // retryRoundTripper retries on 503's for up to 60 seconds. The reason is there is // a small delay for k8s to include the ready IP in service. // https://github.com/knative/serving/issues/660#issuecomment-384062553 -type retryRoundTripper struct{} +type retryRoundTripper struct { + transport http.RoundTripper + transport2 http.RoundTripper +} func (rrt retryRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { - var transport http.RoundTripper - - transport = http.DefaultTransport + transport := rrt.transport if r.ProtoMajor == 2 { - transport = h2cutil.NewTransport() + transport = rrt.transport2 } - resp, err := transport.RoundTrip(r) // TODO: Activator should retry with backoff. // https://github.com/knative/serving/issues/1229 @@ -86,7 +86,10 @@ func (a *activationHandler) handler(w http.ResponseWriter, r *http.Request) { Host: fmt.Sprintf("%s:%d", endpoint.FQDN, endpoint.Port), } proxy := httputil.NewSingleHostReverseProxy(target) - proxy.Transport = retryRoundTripper{} + proxy.Transport = retryRoundTripper{ + transport: http.DefaultTransport, + transport2: h2cutil.NewTransport(), + } // TODO: Clear the host to avoid 404's. // https://github.com/elafros/elafros/issues/964 diff --git a/pkg/controller/revision/revision.go b/pkg/controller/revision/revision.go index c29d07f38208..62bb4f58f6e7 100644 --- a/pkg/controller/revision/revision.go +++ b/pkg/controller/revision/revision.go @@ -484,15 +484,17 @@ func (c *Controller) teardownK8SResources(ctx context.Context, rev *v1alpha1.Rev err := c.deleteDeployment(ctx, rev) if err != nil { logger.Error("Failed to delete a deployment", zap.Error(err)) + return err } logger.Info("Deleted deployment") err = c.deleteService(ctx, rev) if err != nil { logger.Error("Failed to delete k8s service", zap.Error(err)) + return err } logger.Info("Deleted service") } else { - // Serving state is RevisionServingStateReserve. Delete the revision service and update + // Serving state is RevisionServingStateReserve. Keep the revision service and update // the deployment replicas to be 0. if cond := rev.Status.GetCondition(v1alpha1.RevisionConditionReady); cond != nil && cond.Reason != "Inactive" { if cond.Reason == "Deactivating" { @@ -511,6 +513,7 @@ func (c *Controller) teardownK8SResources(ctx context.Context, rev *v1alpha1.Rev err := c.deactivateDeployment(ctx, rev) if err != nil { logger.Error("Failed to scale a deployment to 0", zap.Error(err)) + return err } logger.Info("Scaled the deployment to 0") } @@ -518,12 +521,14 @@ func (c *Controller) teardownK8SResources(ctx context.Context, rev *v1alpha1.Rev err := c.deleteAutoscalerDeployment(ctx, rev) if err != nil { logger.Error("Failed to delete autoscaler Deployment", zap.Error(err)) + return err } logger.Info("Deleted autoscaler Deployment") err = c.deleteAutoscalerService(ctx, rev) if err != nil { logger.Error("Failed to delete autoscaler Service", zap.Error(err)) + return err } logger.Info("Deleted autoscaler Service") return nil @@ -615,6 +620,7 @@ func (c *Controller) deactivateDeployment(ctx context.Context, rev *v1alpha1.Rev _, err = dc.Update(deployment) if err != nil { logger.Errorf("Error deactivating deployment %q: %s", deploymentName, err) + return err } logger.Infof("Successfully scaled deployment %s to 0.", deploymentName) return nil From c155a8e28078537a761a6b408bd0717b0658ac57 Mon Sep 17 00:00:00 2001 From: Yao Wu Date: Tue, 19 Jun 2018 11:22:31 -0700 Subject: [PATCH 06/10] address cr comments --- cmd/activator/main.go | 18 +++++++++------ pkg/controller/revision/revision.go | 34 ++++++----------------------- 2 files changed, 18 insertions(+), 34 deletions(-) diff --git a/cmd/activator/main.go b/cmd/activator/main.go index 2ba78ef27b63..e73d480071fc 100644 --- a/cmd/activator/main.go +++ b/cmd/activator/main.go @@ -49,9 +49,16 @@ type retryRoundTripper struct { transport2 http.RoundTripper } -func (rrt retryRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { - transport := rrt.transport - if r.ProtoMajor == 2 { +var defaultRetryTransport http.RoundTripper = &retryRoundTripper{ + transport: http.DefaultTransport, + transport2: h2cutil.NewTransport(), +} + +func (rrt *retryRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { + var transport http.RoundTripper + if r.ProtoMajor == 1 { + transport = rrt.transport + } else { transport = rrt.transport2 } resp, err := transport.RoundTrip(r) @@ -86,10 +93,7 @@ func (a *activationHandler) handler(w http.ResponseWriter, r *http.Request) { Host: fmt.Sprintf("%s:%d", endpoint.FQDN, endpoint.Port), } proxy := httputil.NewSingleHostReverseProxy(target) - proxy.Transport = retryRoundTripper{ - transport: http.DefaultTransport, - transport2: h2cutil.NewTransport(), - } + proxy.Transport = defaultRetryTransport // TODO: Clear the host to avoid 404's. // https://github.com/elafros/elafros/issues/964 diff --git a/pkg/controller/revision/revision.go b/pkg/controller/revision/revision.go index 62bb4f58f6e7..da7d7d16ceda 100644 --- a/pkg/controller/revision/revision.go +++ b/pkg/controller/revision/revision.go @@ -481,23 +481,18 @@ func (c *Controller) teardownK8SResources(ctx context.Context, rev *v1alpha1.Rev if rev.Spec.ServingState == v1alpha1.RevisionServingStateRetired { // Delete the k8s deployment and revision service if serving state is Retired. logger.Info("Deleting the resources for revision") - err := c.deleteDeployment(ctx, rev) - if err != nil { - logger.Error("Failed to delete a deployment", zap.Error(err)) + if err := c.deleteDeployment(ctx, rev); err != nil { return err } - logger.Info("Deleted deployment") - err = c.deleteService(ctx, rev) - if err != nil { - logger.Error("Failed to delete k8s service", zap.Error(err)) + if err := c.deleteService(ctx, rev); err != nil { return err } - logger.Info("Deleted service") } else { // Serving state is RevisionServingStateReserve. Keep the revision service and update // the deployment replicas to be 0. if cond := rev.Status.GetCondition(v1alpha1.RevisionConditionReady); cond != nil && cond.Reason != "Inactive" { if cond.Reason == "Deactivating" { + logger.Info("The revision is in Deactivating condition") return nil } rev.Status.MarkDeactivating() @@ -508,30 +503,14 @@ func (c *Controller) teardownK8SResources(ctx context.Context, rev *v1alpha1.Rev } return err } - - logger.Info("Scaling the deployment to 0") - err := c.deactivateDeployment(ctx, rev) - if err != nil { - logger.Error("Failed to scale a deployment to 0", zap.Error(err)) + if err := c.deactivateDeployment(ctx, rev); err != nil { return err } - logger.Info("Scaled the deployment to 0") } - - err := c.deleteAutoscalerDeployment(ctx, rev) - if err != nil { - logger.Error("Failed to delete autoscaler Deployment", zap.Error(err)) + if err := c.deleteAutoscalerDeployment(ctx, rev); err != nil { return err } - logger.Info("Deleted autoscaler Deployment") - - err = c.deleteAutoscalerService(ctx, rev) - if err != nil { - logger.Error("Failed to delete autoscaler Service", zap.Error(err)) - return err - } - logger.Info("Deleted autoscaler Service") - return nil + return c.deleteAutoscalerService(ctx, rev) } func (c *Controller) createK8SResources(ctx context.Context, rev *v1alpha1.Revision) error { @@ -611,6 +590,7 @@ func (c *Controller) deactivateDeployment(ctx context.Context, rev *v1alpha1.Rev dc := c.KubeClientSet.AppsV1().Deployments(rev.Namespace) deployment, err := dc.Get(deploymentName, metav1.GetOptions{}) if err != nil { + logger.Errorf("Failed to get deployment %q", deploymentName) return err } From 2b7dacace214934bc6dc4841ef2097f358999b8c Mon Sep 17 00:00:00 2001 From: Yao Wu Date: Tue, 19 Jun 2018 14:20:14 -0700 Subject: [PATCH 07/10] add unit test for MarkDeactivating --- pkg/apis/serving/v1alpha1/revision_types_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/apis/serving/v1alpha1/revision_types_test.go b/pkg/apis/serving/v1alpha1/revision_types_test.go index 628625f0a95e..d757c28e5ab3 100644 --- a/pkg/apis/serving/v1alpha1/revision_types_test.go +++ b/pkg/apis/serving/v1alpha1/revision_types_test.go @@ -409,6 +409,14 @@ func TestTypicalFlowWithSuspendResume(t *testing.T) { checkConditionSucceededRevision(r.Status, RevisionConditionContainerHealthy, t) checkConditionSucceededRevision(r.Status, RevisionConditionReady, t) + // Deactivate the revision to simulate scale to zero. + r.Status.MarkDeactivating() + checkConditionSucceededRevision(r.Status, RevisionConditionResourcesAvailable, t) + checkConditionSucceededRevision(r.Status, RevisionConditionContainerHealthy, t) + if got := checkConditionFailedRevision(r.Status, RevisionConditionReady, t); got == nil || got.Reason != "Deactivating" { + t.Errorf("MarkDeactivating = %v, want Deactivating", got) + } + // From a Ready state, make the revision inactive to simulate scale to zero. r.Status.MarkInactive() checkConditionSucceededRevision(r.Status, RevisionConditionResourcesAvailable, t) From a345fa3fcdebccbe4ee2baf45eff678a88eb1345 Mon Sep 17 00:00:00 2001 From: Yao Wu Date: Tue, 19 Jun 2018 14:33:48 -0700 Subject: [PATCH 08/10] add comment and format test as cr requested --- pkg/controller/revision/resource.go | 3 +++ pkg/controller/route/route_test.go | 25 +++++++++++-------------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/pkg/controller/revision/resource.go b/pkg/controller/revision/resource.go index e47d8220f1e7..c80b7fec1823 100644 --- a/pkg/controller/revision/resource.go +++ b/pkg/controller/revision/resource.go @@ -31,6 +31,9 @@ func MakeServingResourceLabels(revision *v1alpha1.Revision) map[string]string { labels[serving.RevisionUID] = string(revision.UID) for k, v := range revision.ObjectMeta.Labels { + // The route for a revision could change, therefore it is excluded in + // revision labels. If the set of labels changed, the deployment could + // end up with multiple replica sets. if k != serving.RouteLabelKey { labels[k] = v } diff --git a/pkg/controller/route/route_test.go b/pkg/controller/route/route_test.go index a2884adef3fd..ed3e46de731a 100644 --- a/pkg/controller/route/route_test.go +++ b/pkg/controller/route/route_test.go @@ -673,22 +673,19 @@ func TestCreateRouteWithOneTargetReserve(t *testing.T) { }, }, }, - Route: []v1alpha2.DestinationWeight{ - { - Destination: v1alpha2.IstioService{ - Name: "p-deadbeef-service", - Namespace: testNamespace, - }, - Weight: 90, + Route: []v1alpha2.DestinationWeight{{ + Destination: v1alpha2.IstioService{ + Name: "p-deadbeef-service", + Namespace: testNamespace, }, - { - Destination: v1alpha2.IstioService{ - Name: "test-rev-service", - Namespace: testNamespace, - }, - Weight: 0, + Weight: 90, + }, { + Destination: v1alpha2.IstioService{ + Name: "test-rev-service", + Namespace: testNamespace, }, - getActivatorDestinationWeight(10)}, + Weight: 0, + }, getActivatorDestinationWeight(10)}, AppendHeaders: appendHeaders, } From 81e4a064242052d3703cde5a87b5561c75be367e Mon Sep 17 00:00:00 2001 From: Yao Wu Date: Wed, 20 Jun 2018 10:58:48 -0700 Subject: [PATCH 09/10] address cr comments --- pkg/controller/revision/resource.go | 8 +++++--- pkg/controller/revision/revision.go | 4 ++++ pkg/controller/revision/revision_test.go | 11 +++++++---- pkg/controller/route/route.go | 5 +++-- pkg/controller/route/route_test.go | 21 ++++++++------------- 5 files changed, 27 insertions(+), 22 deletions(-) diff --git a/pkg/controller/revision/resource.go b/pkg/controller/revision/resource.go index c80b7fec1823..3d81873eef0a 100644 --- a/pkg/controller/revision/resource.go +++ b/pkg/controller/revision/resource.go @@ -31,9 +31,11 @@ func MakeServingResourceLabels(revision *v1alpha1.Revision) map[string]string { labels[serving.RevisionUID] = string(revision.UID) for k, v := range revision.ObjectMeta.Labels { - // The route for a revision could change, therefore it is excluded in - // revision labels. If the set of labels changed, the deployment could - // end up with multiple replica sets. + // TODO: Use a fixed set for revision labels. If the set of labels changed, + // the deployment could end up with multiple replica sets. + // https://github.com/knative/serving/issues/1293 + // The route for a revision could change, therefore it is excluded + // in revision labels as a temporary solution. if k != serving.RouteLabelKey { labels[k] = v } diff --git a/pkg/controller/revision/revision.go b/pkg/controller/revision/revision.go index da7d7d16ceda..3a231ee56aef 100644 --- a/pkg/controller/revision/revision.go +++ b/pkg/controller/revision/revision.go @@ -593,6 +593,10 @@ func (c *Controller) deactivateDeployment(ctx context.Context, rev *v1alpha1.Rev logger.Errorf("Failed to get deployment %q", deploymentName) return err } + if *deployment.Spec.Replicas == 0 { + logger.Infof("Deployment %s is scaled to 0 already.", deploymentName) + return nil + } logger.Infof("Deactivating deployment %q", deploymentName) deployment.Spec.Replicas = new(int32) diff --git a/pkg/controller/revision/revision_test.go b/pkg/controller/revision/revision_test.go index cd2d7e47ed70..e92afb60607a 100644 --- a/pkg/controller/revision/revision_test.go +++ b/pkg/controller/revision/revision_test.go @@ -1478,8 +1478,8 @@ func TestActiveToReserveRevisionDeletesStuff(t *testing.T) { // appropriate. createRevision(elaClient, elaInformer, controller, rev) - expectedDeploymentName := fmt.Sprintf("%s-deployment", rev.Name) - _, err := kubeClient.AppsV1().Deployments(testNamespace).Get(expectedDeploymentName, metav1.GetOptions{}) + deploymentName := fmt.Sprintf("%s-deployment", rev.Name) + _, err := kubeClient.AppsV1().Deployments(testNamespace).Get(deploymentName, metav1.GetOptions{}) if err != nil { t.Fatalf("Couldn't get ela deployment: %v", err) } @@ -1490,10 +1490,13 @@ func TestActiveToReserveRevisionDeletesStuff(t *testing.T) { updateRevision(elaClient, elaInformer, controller, rev) // Expect the deployment to be there. - _, err = kubeClient.AppsV1().Deployments(testNamespace).Get(expectedDeploymentName, metav1.GetOptions{}) + _, err = kubeClient.AppsV1().Deployments(testNamespace).Get(deploymentName, metav1.GetOptions{}) if err != nil { - t.Fatalf("Expected k8s deployment to be there but it was gone: %s/%s", testNamespace, expectedDeploymentName) + if apierrs.IsNotFound(err) { + t.Fatalf("Expected k8s deployment to be there but it was gone: %s/%s", testNamespace, deploymentName) + } + t.Fatalf("There was an error to get the deployment %s while it exists", deploymentName) } } diff --git a/pkg/controller/route/route.go b/pkg/controller/route/route.go index 8a3a53a17adf..6d949f43e80b 100644 --- a/pkg/controller/route/route.go +++ b/pkg/controller/route/route.go @@ -684,9 +684,10 @@ func (c *Controller) computeRevisionRoutes( revisionWeight := tt.Percent cond := rev.Status.GetCondition(v1alpha1.RevisionConditionReady) if enableScaleToZero && cond != nil { - // A revision is considered inactive (yet) if it's in + // A revision is considered inactive if it's in // "Inactive" condition, "Activating" or "Deactivating" condition. - logger.Infof("cond: %v", cond) + // TODO: consilidate revision conditions. + // https://github.com/knative/serving/issues/645 if (cond.Reason == "Inactive" && cond.Status == corev1.ConditionFalse) || (cond.Reason == "Activating" && cond.Status == corev1.ConditionUnknown) || (cond.Reason == "Deactivating" && cond.Status == corev1.ConditionFalse) { diff --git a/pkg/controller/route/route_test.go b/pkg/controller/route/route_test.go index ed3e46de731a..f9e4d874a836 100644 --- a/pkg/controller/route/route_test.go +++ b/pkg/controller/route/route_test.go @@ -430,16 +430,13 @@ func TestCreateRouteForOneReserveRevision(t *testing.T) { }, }, }, - Route: []v1alpha2.DestinationWeight{ - { - Destination: v1alpha2.IstioService{ - Name: "test-rev-service", - Namespace: testNamespace, - }, - Weight: 0, + Route: []v1alpha2.DestinationWeight{{ + Destination: v1alpha2.IstioService{ + Name: "test-rev-service", + Namespace: testNamespace, }, - getActivatorDestinationWeight(100), - }, + Weight: 0, + }, getActivatorDestinationWeight(100)}, AppendHeaders: appendHeaders, } @@ -599,8 +596,7 @@ func TestCreateRouteWithMultipleTargets(t *testing.T) { Namespace: testNamespace, }, Weight: 10, - }, getActivatorDestinationWeight(0)}, - } + }, getActivatorDestinationWeight(0)}} if diff := cmp.Diff(expectedRouteSpec, routerule.Spec); diff != "" { t.Errorf("Unexpected rule spec diff (-want +got): %v", diff) @@ -875,8 +871,7 @@ func TestCreateRouteWithNamedTargets(t *testing.T) { Namespace: testNamespace, }, Weight: 50, - }, getActivatorDestinationWeight(0)}, - }) + }, getActivatorDestinationWeight(0)}}) // Expects authority header to have the traffic target name prefixed to the // domain suffix. Also weights 100% of the traffic to the specified traffic From c23bb92c9b553bf46f9d7a9bc626f296f00c736c Mon Sep 17 00:00:00 2001 From: Yao Wu Date: Wed, 20 Jun 2018 11:05:43 -0700 Subject: [PATCH 10/10] Delete a comment --- pkg/controller/revision/resource.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/controller/revision/resource.go b/pkg/controller/revision/resource.go index 3d81873eef0a..c0ab810d5004 100644 --- a/pkg/controller/revision/resource.go +++ b/pkg/controller/revision/resource.go @@ -51,8 +51,6 @@ func MakeServingResourceLabels(revision *v1alpha1.Revision) map[string]string { // MakeServingResourceSelector constructs the Selector we will apply to K8s resources. func MakeServingResourceSelector(revision *v1alpha1.Revision) *metav1.LabelSelector { - // Deployment spec.selector is an immutable field so we need to exclude the route label, - // which could change in a revision. return &metav1.LabelSelector{MatchLabels: MakeServingResourceLabels(revision)} }