From 0b6263d10be7de10a86f569666cdbdbce9f2039c Mon Sep 17 00:00:00 2001 From: Yao Wu Date: Thu, 21 Jun 2018 14:08:22 -0700 Subject: [PATCH 1/7] Keep revision service and deployment when scale to 0 --- pkg/controller/revision/revision.go | 130 +++++++++++++++++------ pkg/controller/revision/revision_test.go | 23 ++-- pkg/controller/route/route.go | 82 -------------- pkg/controller/route/route_test.go | 109 +------------------ 4 files changed, 110 insertions(+), 234 deletions(-) diff --git a/pkg/controller/revision/revision.go b/pkg/controller/revision/revision.go index c39c50aafaf2..4292fa6d4432 100644 --- a/pkg/controller/revision/revision.go +++ b/pkg/controller/revision/revision.go @@ -81,7 +81,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} foregroundDeletion = metav1.DeletePropagationForeground fgDeleteOptions = &metav1.DeleteOptions{ @@ -324,7 +324,7 @@ func (c *Controller) Reconcile(key string) error { return c.createK8SResources(ctx, rev) case v1alpha1.RevisionServingStateReserve: - return c.deleteK8SResources(ctx, rev) + return c.teardownK8SResources(ctx, rev) // TODO(mattmoor): Nothing sets this state, and it should be removed. case v1alpha1.RevisionServingStateRetired: @@ -449,48 +449,107 @@ func (c *Controller) SyncEndpoints(endpoint *corev1.Endpoints) { return } +// teardownK8SResources deletes autoscaler resources, and deletes the revision service and deployment. +// It is used when the revision serving state is Retired. func (c *Controller) deleteK8SResources(ctx context.Context, rev *v1alpha1.Revision) error { logger := logging.FromContext(ctx) logger.Info("Deleting the resources for revision") - err := c.deleteDeployment(ctx, rev) - if err != nil { + if err := c.deleteDeployment(ctx, rev); err != nil { logger.Error("Failed to delete a deployment", zap.Error(err)) + return err } - logger.Info("Deleted deployment") - err = c.deleteAutoscalerDeployment(ctx, rev) - if err != nil { + if err := c.deleteService(ctx, rev); err != nil { + logger.Error("Failed to delete k8s service", zap.Error(err)) + return err + } + + if err := c.deleteAutoscalerResources(ctx, rev); err != nil { + logger.Error("Failed to delete autoscaler resources", zap.Error(err)) + return err + } + + // 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 +} + +// teardownK8SResources deletes autoscaler resources, but keeps the revision service and deployment. +// It is used when the revision serving state is Reserve. +func (c *Controller) teardownK8SResources(ctx context.Context, rev *v1alpha1.Revision) error { + logger := logging.FromContext(ctx) + logger.Info("Scale the deployment to 0 for the revision") + if err := c.deactivateDeployment(ctx, rev); err != nil { + logger.Error("Failed to deactivate a deployment", zap.Error(err)) + return err + } + + if err := c.deleteAutoscalerResources(ctx, rev); err != nil { + logger.Error("Failed to delete autoscaler resources", zap.Error(err)) + return err + } + + // 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 +} + +func (c *Controller) deleteAutoscalerResources(ctx context.Context, rev *v1alpha1.Revision) error { + logger := logging.FromContext(ctx) + if err := c.deleteAutoscalerDeployment(ctx, rev); 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 { + if err := c.deleteAutoscalerService(ctx, rev); err != nil { logger.Error("Failed to delete autoscaler Service", zap.Error(err)) + return err } - logger.Info("Deleted autoscaler Service") if c.controllerConfig.AutoscaleEnableVerticalPodAutoscaling.Get() { if err := c.deleteVpa(ctx, rev); err != nil { logger.Error("Failed to delete VPA", zap.Error(err)) + return err } - logger.Info("Deleted VPA") } + return nil +} - err = c.deleteService(ctx, rev) +func (c *Controller) deactivateDeployment(ctx context.Context, rev *v1alpha1.Revision) error { + logger := logging.FromContext(ctx) + deploymentName := controller.GetRevisionDeploymentName(rev) + dc := c.KubeClientSet.AppsV1().Deployments(rev.Namespace) + deployment, err := dc.Get(deploymentName, metav1.GetOptions{}) if err != nil { - logger.Error("Failed to delete k8s service", zap.Error(err)) + 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.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)) + logger.Infof("Deactivating 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) return err } - + logger.Infof("Successfully scaled deployment %s to 0.", deploymentName) return nil } @@ -585,24 +644,19 @@ 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 { + 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 } - 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 + deploymentExists = false } - // Create the deployment. deployment := MakeServingDeployment(logger, rev, c.getNetworkConfig(), c.controllerConfig) - // 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 { @@ -612,10 +666,16 @@ func (c *Controller) reconcileDeployment(ctx context.Context, rev *v1alpha1.Revi return err } - logger.Infof("Creating Deployment: %q", deployment.Name) - _, createErr := dc.Create(deployment) - - return createErr + 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 } func (c *Controller) deleteService(ctx context.Context, rev *v1alpha1.Revision) error { diff --git a/pkg/controller/revision/revision_test.go b/pkg/controller/revision/revision_test.go index 92308cfa8b3f..e2c4f936ab88 100644 --- a/pkg/controller/revision/revision_test.go +++ b/pkg/controller/revision/revision_test.go @@ -80,9 +80,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", @@ -1532,7 +1531,7 @@ func TestActiveToRetiredRevisionDeletesStuff(t *testing.T) { } } -func TestActiveToReserveRevisionDeletesStuff(t *testing.T) { +func TestActiveToReserveRevisionDeactivateDeployment(t *testing.T) { kubeClient, _, elaClient, _, controller, _, _, elaInformer, _, _ := newTestController(t) rev := getTestRevision() @@ -1540,8 +1539,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) } @@ -1551,10 +1550,14 @@ 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(deploymentName, metav1.GetOptions{}) + + if err != nil { + 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 61c0e3a5f289..5060ded21d9b 100644 --- a/pkg/controller/route/route.go +++ b/pkg/controller/route/route.go @@ -274,13 +274,6 @@ func (c *Controller) syncTrafficTargetsAndUpdateRouteStatus(ctx context.Context, return nil, err } - if err := c.deleteLabelForOutsideOfGivenRevisions(ctx, route, revMap); err != nil { - return nil, err - } - if err := c.setLabelForGivenRevisions(ctx, route, revMap); err != nil { - return nil, err - } - // Then create the actual route rules. logger.Info("Creating Istio route rules") revisionRoutes, err := c.createOrUpdateRouteRules(ctx, route, configMap, revMap) @@ -509,51 +502,6 @@ func (c *Controller) setLabelForGivenConfigurations( return nil } -func (c *Controller) setLabelForGivenRevisions( - ctx context.Context, route *v1alpha1.Route, revMap map[string]*v1alpha1.Revision) error { - logger := logging.FromContext(ctx) - revisionClient := c.ServingClientSet.ServingV1alpha1().Revisions(route.Namespace) - - // Validate revision if it already has a route label - for _, rev := range revMap { - if routeName, ok := rev.Labels[serving.RouteLabelKey]; ok { - if routeName != route.Name { - errMsg := fmt.Sprintf("Revision %q is already in use by %q, and cannot be used by %q", - rev.Name, routeName, route.Name) - c.Recorder.Event(route, corev1.EventTypeWarning, "RevisionInUse", errMsg) - logger.Error(errMsg) - return errors.New(errMsg) - } - } - } - - for _, rev := range revMap { - if rev.Labels != nil { - if _, ok := rev.Labels[serving.RouteLabelKey]; ok { - continue - } - } - // Fetch the latest version of the revision to label, to narrow the window for - // optimistic concurrency failures. - latestRev, err := revisionClient.Get(rev.Name, metav1.GetOptions{}) - if err != nil { - return err - } - if latestRev.Labels == nil { - latestRev.Labels = make(map[string]string) - } else if _, ok := latestRev.Labels[serving.RouteLabelKey]; ok { - continue - } - latestRev.Labels[serving.RouteLabelKey] = route.Name - if _, err := revisionClient.Update(latestRev); err != nil { - logger.Errorf("Failed to add route label to Revision %s: %s", rev.Name, err) - return err - } - } - - return nil -} - func (c *Controller) deleteLabelForOutsideOfGivenConfigurations( ctx context.Context, route *v1alpha1.Route, configMap map[string]*v1alpha1.Configuration) error { logger := logging.FromContext(ctx) @@ -584,36 +532,6 @@ func (c *Controller) deleteLabelForOutsideOfGivenConfigurations( return nil } -func (c *Controller) deleteLabelForOutsideOfGivenRevisions( - ctx context.Context, route *v1alpha1.Route, revMap map[string]*v1alpha1.Revision) error { - logger := logging.FromContext(ctx) - revClient := c.ServingClientSet.ServingV1alpha1().Revisions(route.Namespace) - - oldRevList, err := revClient.List( - metav1.ListOptions{ - LabelSelector: fmt.Sprintf("%s=%s", serving.RouteLabelKey, route.Name), - }, - ) - if err != nil { - logger.Errorf("Failed to fetch revisions with label '%s=%s': %s", - serving.RouteLabelKey, route.Name, err) - return err - } - - // Delete label for newly removed revisions as traffic target. - for _, rev := range oldRevList.Items { - if _, ok := revMap[rev.Name]; !ok { - delete(rev.Labels, serving.RouteLabelKey) - if _, err := revClient.Update(&rev); err != nil { - logger.Errorf("Failed to remove route label from Revision %s: %s", rev.Name, err) - return err - } - } - } - - return nil -} - // computeRevisionRoutes computes RevisionRoute for a route object. If there is one or more inactive revisions and enableScaleToZero // is true, a route rule with the activator service as the destination will be added. It returns the revision routes, the inactive // revision name to which the activator should forward requests to, and error if there is any. diff --git a/pkg/controller/route/route_test.go b/pkg/controller/route/route_test.go index 13b9cd9d8619..87a36bc3c6e7 100644 --- a/pkg/controller/route/route_test.go +++ b/pkg/controller/route/route_test.go @@ -1015,105 +1015,6 @@ func TestSetLabelToConfigurationDirectlyConfigured(t *testing.T) { } } -func TestSetLabelToRevisionDirectlyConfigured(t *testing.T) { - _, elaClient, controller, _, elaInformer, _ := newTestController(t) - config := getTestConfiguration() - rev := getTestRevisionForConfig(config) - route := getTestRouteWithTrafficTargets( - []v1alpha1.TrafficTarget{{ - ConfigurationName: config.Name, - Percent: 100, - }}, - ) - - elaClient.ServingV1alpha1().Configurations(testNamespace).Create(config) - elaClient.ServingV1alpha1().Revisions(testNamespace).Create(rev) - elaClient.ServingV1alpha1().Routes(testNamespace).Create(route) - // Since updateRouteEvent looks in the lister, we need to add it to the informer - elaInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) - controller.updateRouteEvent(KeyOrDie(route)) - - rev, err := elaClient.ServingV1alpha1().Revisions(testNamespace).Get(rev.Name, metav1.GetOptions{}) - if err != nil { - t.Fatalf("error getting revision: %v", err) - } - - // Revision should not have route label as the revision is not marked as the Config's latest ready revision - expectedLabels := map[string]string{ - serving.ConfigurationLabelKey: config.Name, - } - - if diff := cmp.Diff(expectedLabels, rev.Labels); diff != "" { - t.Errorf("Unexpected label diff (-want +got): %v", diff) - } - - // Mark the revision as the Config's latest ready revision - config.Status.LatestReadyRevisionName = rev.Name - - elaClient.ServingV1alpha1().Configurations(testNamespace).Update(config) - controller.updateRouteEvent(KeyOrDie(route)) - - rev, err = elaClient.ServingV1alpha1().Revisions(testNamespace).Get(rev.Name, metav1.GetOptions{}) - if err != nil { - t.Fatalf("error getting revision: %v", err) - } - - // Revision should have the route label - expectedLabels = map[string]string{ - serving.ConfigurationLabelKey: config.Name, - serving.RouteLabelKey: route.Name, - } - - if diff := cmp.Diff(expectedLabels, rev.Labels); diff != "" { - t.Errorf("Unexpected label diff (-want +got): %v", diff) - } -} - -func TestSetLabelToConfigurationAndRevisionIndirectlyConfigured(t *testing.T) { - _, elaClient, controller, _, elaInformer, _ := newTestController(t) - config := getTestConfiguration() - rev := getTestRevisionForConfig(config) - route := getTestRouteWithTrafficTargets( - []v1alpha1.TrafficTarget{{ - RevisionName: rev.Name, - Percent: 100, - }}, - ) - - elaClient.ServingV1alpha1().Configurations(testNamespace).Create(config) - elaClient.ServingV1alpha1().Revisions(testNamespace).Create(rev) - elaClient.ServingV1alpha1().Routes(testNamespace).Create(route) - // Since updateRouteEvent looks in the lister, we need to add it to the informer - elaInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) - controller.updateRouteEvent(KeyOrDie(route)) - - config, err := elaClient.ServingV1alpha1().Configurations(testNamespace).Get(config.Name, metav1.GetOptions{}) - if err != nil { - t.Fatalf("error getting config: %v", err) - } - - // Configuration should be labeled for this route - expectedLabels := map[string]string{serving.RouteLabelKey: route.Name} - if diff := cmp.Diff(expectedLabels, config.Labels); diff != "" { - t.Errorf("Unexpected label in configuration diff (-want +got): %v", diff) - } - - rev, err = elaClient.ServingV1alpha1().Revisions(testNamespace).Get(rev.Name, metav1.GetOptions{}) - if err != nil { - t.Fatalf("error getting revision: %v", err) - } - - // Revision should have the route label - expectedLabels = map[string]string{ - serving.ConfigurationLabelKey: config.Name, - serving.RouteLabelKey: route.Name, - } - - if diff := cmp.Diff(expectedLabels, rev.Labels); diff != "" { - t.Errorf("Unexpected label in revision diff (-want +got): %v", diff) - } -} - func TestCreateRouteWithInvalidConfigurationShouldReturnError(t *testing.T) { _, elaClient, controller, _, elaInformer, _ := newTestController(t) config := getTestConfiguration() @@ -1250,7 +1151,7 @@ func TestCreateRouteConfigurationMissingCondition(t *testing.T) { } } -func TestSetLabelNotChangeConfigurationAndRevisionLabelIfLabelExists(t *testing.T) { +func TestSetLabelNotChangeConfigurationLabelIfLabelExists(t *testing.T) { _, elaClient, controller, _, elaInformer, _ := newTestController(t) config := getTestConfiguration() rev := getTestRevisionForConfig(config) @@ -1264,10 +1165,6 @@ func TestSetLabelNotChangeConfigurationAndRevisionLabelIfLabelExists(t *testing. // by function setLabelForGivenConfigurations. config.Labels = map[string]string{serving.RouteLabelKey: route.Name} - // Set revision's route label with route name to make sure revision's label will not be set - // by function setLabelForGivenRevisions. - rev.Labels[serving.RouteLabelKey] = route.Name - elaClient.ServingV1alpha1().Configurations(testNamespace).Create(config) elaClient.ServingV1alpha1().Revisions(testNamespace).Create(rev) elaClient.ServingV1alpha1().Routes(testNamespace).Create(route) @@ -1293,15 +1190,13 @@ func TestSetLabelNotChangeConfigurationAndRevisionLabelIfLabelExists(t *testing. controller.updateRouteEvent(route.Namespace + "/" + route.Name) } -func TestDeleteLabelOfConfigurationAndRevisionWhenUnconfigured(t *testing.T) { +func TestDeleteLabelOfConfigurationWhenUnconfigured(t *testing.T) { _, elaClient, controller, _, elaInformer, _ := newTestController(t) route := getTestRouteWithTrafficTargets([]v1alpha1.TrafficTarget{}) config := getTestConfiguration() // Set a route label in configuration which is expected to be deleted. config.Labels = map[string]string{serving.RouteLabelKey: route.Name} rev := getTestRevisionForConfig(config) - // Set a route label in revision which is expected to be deleted. - rev.Labels[serving.RouteLabelKey] = route.Name elaClient.ServingV1alpha1().Configurations(testNamespace).Create(config) elaClient.ServingV1alpha1().Revisions(testNamespace).Create(rev) From 946db6c3d0a46207f8e98e92e71252792dc1a7fd Mon Sep 17 00:00:00 2001 From: Yao Wu Date: Thu, 21 Jun 2018 14:17:45 -0700 Subject: [PATCH 2/7] minor change --- pkg/controller/revision/revision.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/pkg/controller/revision/revision.go b/pkg/controller/revision/revision.go index 4292fa6d4432..4f46b91c62e6 100644 --- a/pkg/controller/revision/revision.go +++ b/pkg/controller/revision/revision.go @@ -645,8 +645,7 @@ func (c *Controller) reconcileDeployment(ctx context.Context, rev *v1alpha1.Revi deploymentName := controller.GetRevisionDeploymentName(rev) deploymentExists := true - _, err := dc.Get(deploymentName, metav1.GetOptions{}) - if err != nil { + if _, err := dc.Get(deploymentName, metav1.GetOptions{}); err != nil { if !apierrs.IsNotFound(err) { logger.Errorf("deployments.Get for %q failed: %s", deploymentName, err) return err @@ -656,12 +655,12 @@ func (c *Controller) reconcileDeployment(ctx context.Context, rev *v1alpha1.Revi deployment := MakeServingDeployment(logger, rev, c.getNetworkConfig(), c.controllerConfig) // 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 { - logger.Error("Error recording resolution problem", zap.Error(err)) - return err + if _, updateErr := c.updateStatus(rev); updateErr != nil { + logger.Error("Error recording resolution problem", zap.Error(updateErr)) + return updateErr } return err } From 1594fe467dd9d7f754bab6475ff9efdc77dd1a5e Mon Sep 17 00:00:00 2001 From: Yao Wu Date: Thu, 21 Jun 2018 14:24:34 -0700 Subject: [PATCH 3/7] minor fix --- pkg/controller/revision/revision.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/controller/revision/revision.go b/pkg/controller/revision/revision.go index 4f46b91c62e6..dec0406a4ebb 100644 --- a/pkg/controller/revision/revision.go +++ b/pkg/controller/revision/revision.go @@ -665,6 +665,7 @@ func (c *Controller) reconcileDeployment(ctx context.Context, rev *v1alpha1.Revi return err } + var err error if deploymentExists { // TODO(mattmoor): Compare the deployments and update if it has changed // out from under us. From c01b33f9846b584ba0e0232dfd165715c7ad875b Mon Sep 17 00:00:00 2001 From: Yao Wu Date: Thu, 21 Jun 2018 15:36:44 -0700 Subject: [PATCH 4/7] address cr comments --- pkg/controller/revision/revision.go | 52 ++++++++++++++--------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/pkg/controller/revision/revision.go b/pkg/controller/revision/revision.go index dec0406a4ebb..5e54e5be145e 100644 --- a/pkg/controller/revision/revision.go +++ b/pkg/controller/revision/revision.go @@ -484,9 +484,9 @@ func (c *Controller) deleteK8SResources(ctx context.Context, rev *v1alpha1.Revis // It is used when the revision serving state is Reserve. func (c *Controller) teardownK8SResources(ctx context.Context, rev *v1alpha1.Revision) error { logger := logging.FromContext(ctx) - logger.Info("Scale the deployment to 0 for the revision") - if err := c.deactivateDeployment(ctx, rev); err != nil { - logger.Error("Failed to deactivate a deployment", zap.Error(err)) + logger.Info("Scaling the deployment to 0 for the revision") + if err := c.scaleRevisionResourcesToZero(ctx, rev); err != nil { + logger.Error("Failed to scale the deployment to 0", zap.Error(err)) return err } @@ -527,7 +527,7 @@ func (c *Controller) deleteAutoscalerResources(ctx context.Context, rev *v1alpha return nil } -func (c *Controller) deactivateDeployment(ctx context.Context, rev *v1alpha1.Revision) error { +func (c *Controller) scaleRevisionResourcesToZero(ctx context.Context, rev *v1alpha1.Revision) error { logger := logging.FromContext(ctx) deploymentName := controller.GetRevisionDeploymentName(rev) dc := c.KubeClientSet.AppsV1().Deployments(rev.Namespace) @@ -541,12 +541,12 @@ func (c *Controller) deactivateDeployment(ctx context.Context, rev *v1alpha1.Rev return nil } - logger.Infof("Deactivating deployment %q", deploymentName) + logger.Infof("Setting deployment %q to 0", 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.Errorf("Error scaling deployment %q to 0: %s", deploymentName, err) return err } logger.Infof("Successfully scaled deployment %s to 0.", deploymentName) @@ -644,18 +644,9 @@ func (c *Controller) reconcileDeployment(ctx context.Context, rev *v1alpha1.Revi // First, check if deployment exists already. deploymentName := controller.GetRevisionDeploymentName(rev) - deploymentExists := true - if _, err := dc.Get(deploymentName, metav1.GetOptions{}); err != nil { - if !apierrs.IsNotFound(err) { - logger.Errorf("deployments.Get for %q failed: %s", deploymentName, err) - return err - } - deploymentExists = false - } - - deployment := MakeServingDeployment(logger, rev, c.getNetworkConfig(), c.controllerConfig) + desiredDeployment := MakeServingDeployment(logger, rev, c.getNetworkConfig(), c.controllerConfig) // Resolve tag image references to digests. - if err := c.resolver.Resolve(deployment); err != nil { + if err := c.resolver.Resolve(desiredDeployment); err != nil { logger.Error("Error resolving deployment", zap.Error(err)) rev.Status.MarkContainerMissing(err.Error()) if _, updateErr := c.updateStatus(rev); updateErr != nil { @@ -665,17 +656,26 @@ func (c *Controller) reconcileDeployment(ctx context.Context, rev *v1alpha1.Revi return err } - var err error - 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 { + if existingDeployment, err := dc.Get(deploymentName, metav1.GetOptions{}); 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) - _, err = dc.Create(deployment) + _, err := dc.Create(desiredDeployment) + return err + } else { + logger.Infof("Found existing deployment %q", deploymentName) + // TODO(mattmoor): Compare the deployments and update if it has changed + // out from under us. So far the deployment could only be updated for replicas field. + if *existingDeployment.Spec.Replicas == *desiredDeployment.Spec.Replicas { + logger.Infof("The existing deployment %q replicas count %d is expected", deploymentName, *existingDeployment.Spec.Replicas) + return nil + } + logger.Infof("Updating deployment %q", deploymentName) + _, err := dc.Update(desiredDeployment) + return err } - return err } func (c *Controller) deleteService(ctx context.Context, rev *v1alpha1.Revision) error { From 1a663e2c54716271cdf17b0fd9e349c22340d858 Mon Sep 17 00:00:00 2001 From: Yao Wu Date: Thu, 21 Jun 2018 15:42:29 -0700 Subject: [PATCH 5/7] minor change --- pkg/controller/revision/revision.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/revision/revision.go b/pkg/controller/revision/revision.go index 5e54e5be145e..21ec2ef19528 100644 --- a/pkg/controller/revision/revision.go +++ b/pkg/controller/revision/revision.go @@ -541,7 +541,7 @@ func (c *Controller) scaleRevisionResourcesToZero(ctx context.Context, rev *v1al return nil } - logger.Infof("Setting deployment %q to 0", deploymentName) + logger.Infof("Setting deployment %q replicas to 0", deploymentName) deployment.Spec.Replicas = new(int32) *deployment.Spec.Replicas = int32(0) _, err = dc.Update(deployment) From 4cc56f0a6f3834af33dc960fbce5b668af5d9830 Mon Sep 17 00:00:00 2001 From: Yao Wu Date: Thu, 21 Jun 2018 15:46:14 -0700 Subject: [PATCH 6/7] rename functions --- pkg/controller/revision/revision.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/controller/revision/revision.go b/pkg/controller/revision/revision.go index 21ec2ef19528..b78eff76712d 100644 --- a/pkg/controller/revision/revision.go +++ b/pkg/controller/revision/revision.go @@ -324,7 +324,7 @@ func (c *Controller) Reconcile(key string) error { return c.createK8SResources(ctx, rev) case v1alpha1.RevisionServingStateReserve: - return c.teardownK8SResources(ctx, rev) + return c.scaleRevisionResourcesToZero(ctx, rev) // TODO(mattmoor): Nothing sets this state, and it should be removed. case v1alpha1.RevisionServingStateRetired: @@ -449,7 +449,7 @@ func (c *Controller) SyncEndpoints(endpoint *corev1.Endpoints) { return } -// teardownK8SResources deletes autoscaler resources, and deletes the revision service and deployment. +// deleteK8SResources deletes autoscaler resources, and deletes the revision service and deployment. // It is used when the revision serving state is Retired. func (c *Controller) deleteK8SResources(ctx context.Context, rev *v1alpha1.Revision) error { logger := logging.FromContext(ctx) @@ -480,12 +480,12 @@ func (c *Controller) deleteK8SResources(ctx context.Context, rev *v1alpha1.Revis return nil } -// teardownK8SResources deletes autoscaler resources, but keeps the revision service and deployment. +// scaleRevisionResourcesToZero deletes autoscaler resources, but keeps the revision service and deployment. // It is used when the revision serving state is Reserve. -func (c *Controller) teardownK8SResources(ctx context.Context, rev *v1alpha1.Revision) error { +func (c *Controller) scaleRevisionResourcesToZero(ctx context.Context, rev *v1alpha1.Revision) error { logger := logging.FromContext(ctx) logger.Info("Scaling the deployment to 0 for the revision") - if err := c.scaleRevisionResourcesToZero(ctx, rev); err != nil { + if err := c.scaleDeploymentToZero(ctx, rev); err != nil { logger.Error("Failed to scale the deployment to 0", zap.Error(err)) return err } @@ -527,7 +527,7 @@ func (c *Controller) deleteAutoscalerResources(ctx context.Context, rev *v1alpha return nil } -func (c *Controller) scaleRevisionResourcesToZero(ctx context.Context, rev *v1alpha1.Revision) error { +func (c *Controller) scaleDeploymentToZero(ctx context.Context, rev *v1alpha1.Revision) error { logger := logging.FromContext(ctx) deploymentName := controller.GetRevisionDeploymentName(rev) dc := c.KubeClientSet.AppsV1().Deployments(rev.Namespace) From 313f770f67b08f97561c483e815812ec3de57f61 Mon Sep 17 00:00:00 2001 From: Yao Wu Date: Tue, 26 Jun 2018 11:16:59 -0700 Subject: [PATCH 7/7] keep autoscaler deployment --- pkg/controller/revision/revision.go | 33 ++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/pkg/controller/revision/revision.go b/pkg/controller/revision/revision.go index b78eff76712d..dcd00429309f 100644 --- a/pkg/controller/revision/revision.go +++ b/pkg/controller/revision/revision.go @@ -485,7 +485,7 @@ func (c *Controller) deleteK8SResources(ctx context.Context, rev *v1alpha1.Revis func (c *Controller) scaleRevisionResourcesToZero(ctx context.Context, rev *v1alpha1.Revision) error { logger := logging.FromContext(ctx) logger.Info("Scaling the deployment to 0 for the revision") - if err := c.scaleDeploymentToZero(ctx, rev); err != nil { + if err := c.scaleRevisionDeploymentToZero(ctx, rev); err != nil { logger.Error("Failed to scale the deployment to 0", zap.Error(err)) return err } @@ -508,7 +508,7 @@ func (c *Controller) scaleRevisionResourcesToZero(ctx context.Context, rev *v1al func (c *Controller) deleteAutoscalerResources(ctx context.Context, rev *v1alpha1.Revision) error { logger := logging.FromContext(ctx) - if err := c.deleteAutoscalerDeployment(ctx, rev); err != nil { + if err := c.scaleAutoscalerDeploymentToZero(ctx, rev); err != nil { logger.Error("Failed to delete autoscaler Deployment", zap.Error(err)) return err } @@ -527,7 +527,34 @@ func (c *Controller) deleteAutoscalerResources(ctx context.Context, rev *v1alpha return nil } -func (c *Controller) scaleDeploymentToZero(ctx context.Context, rev *v1alpha1.Revision) error { +func (c *Controller) scaleAutoscalerDeploymentToZero(ctx context.Context, rev *v1alpha1.Revision) error { + logger := logging.FromContext(ctx) + deploymentName := controller.GetRevisionAutoscalerName(rev) + + dc := c.KubeClientSet.AppsV1().Deployments(pkg.GetServingSystemNamespace()) + deployment, err := dc.Get(deploymentName, metav1.GetOptions{}) + if err != nil { + 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("Setting deployment %q replicas to 0", deploymentName) + deployment.Spec.Replicas = new(int32) + *deployment.Spec.Replicas = int32(0) + _, err = dc.Update(deployment) + if err != nil { + logger.Errorf("Error scaling deployment %q to 0: %s", deploymentName, err) + return err + } + logger.Infof("Successfully scaled deployment %s to 0.", deploymentName) + return nil +} + +func (c *Controller) scaleRevisionDeploymentToZero(ctx context.Context, rev *v1alpha1.Revision) error { logger := logging.FromContext(ctx) deploymentName := controller.GetRevisionDeploymentName(rev) dc := c.KubeClientSet.AppsV1().Deployments(rev.Namespace)