Prevent unnecessary deployment updates

pkg/apis/serving/v1/revision_defaults.go

@@ -155,6 +155,11 @@ func (*RevisionSpec) applyProbes(container *corev1.Container) {
 		if container.ReadinessProbe.TimeoutSeconds == 0 {
 			container.ReadinessProbe.TimeoutSeconds = 1
 		}
+	} else {
+		// TODO I do not understand above logic, why implies PeriodSeconds the other two values ?
+		container.ReadinessProbe.PeriodSeconds = 10
+		container.ReadinessProbe.FailureThreshold = 3
+		container.ReadinessProbe.TimeoutSeconds = 1
 	}
 }
 

pkg/reconciler/revision/cruds.go

@@ -39,7 +39,7 @@ import (
 func (c *Reconciler) createDeployment(ctx context.Context, rev *v1.Revision) (*appsv1.Deployment, error) {
 	cfgs := config.FromContext(ctx)
 
-	deployment, err := resources.MakeDeployment(rev, cfgs)
+	deployment, err := resources.MakeDeployment(rev, cfgs, nil)
 
 	if err != nil {
 		return nil, fmt.Errorf("failed to make deployment: %w", err)
@@ -66,7 +66,7 @@ func (c *Reconciler) checkAndUpdateDeployment(ctx context.Context, rev *v1.Revis
 	logger := logging.FromContext(ctx)
 	cfgs := config.FromContext(ctx)
 
-	deployment, err := resources.MakeDeployment(rev, cfgs)
+	deployment, err := resources.MakeDeployment(rev, cfgs, have)
 	if err != nil {
 		return nil, fmt.Errorf("failed to update deployment: %w", err)
 	}
@@ -90,6 +90,12 @@ func (c *Reconciler) checkAndUpdateDeployment(ctx context.Context, rev *v1.Revis
 	// Carry over new labels.
 	desiredDeployment.Labels = kmeta.UnionMaps(deployment.Labels, desiredDeployment.Labels)
 
+	debugDiffs, err := kmp.SafeDiff(have.Spec, desiredDeployment.Spec)
+	if err != nil {
+		return nil, err
+	}
+	logger.Infof("Updating deployment %s/%s with diff\n", deployment.Namespace, deployment.Name, debugDiffs)
+
 	d, err := c.kubeclient.AppsV1().Deployments(deployment.Namespace).Update(ctx, desiredDeployment, metav1.UpdateOptions{})
 	if err != nil {
 		return nil, err

pkg/reconciler/revision/resources/deploy.go

@@ -111,8 +111,9 @@ var (
 	userLifecycle = &corev1.Lifecycle{
 		PreStop: &corev1.LifecycleHandler{
 			HTTPGet: &corev1.HTTPGetAction{
-				Port: intstr.FromInt(networking.QueueAdminPort),
-				Path: queue.RequestQueueDrainPath,
+				Port:   intstr.FromInt(networking.QueueAdminPort),
+				Path:   queue.RequestQueueDrainPath,
+				Scheme: corev1.URISchemeHTTP,
 			},
 		},
 	}
@@ -148,8 +149,19 @@ func rewriteUserProbe(p *corev1.Probe, userPort int) {
 	}
 }
 
-func makePodSpec(rev *v1.Revision, cfg *config.Config) (*corev1.PodSpec, error) {
-	queueContainer, err := makeQueueContainer(rev, cfg)
+func makePodSpec(rev *v1.Revision, cfg *config.Config, previous *corev1.PodSpec) (*corev1.PodSpec, error) {
+	var previousQueueContainer *corev1.Container
+	var previousUserContainers []corev1.Container
+	if previous != nil {
+		for _, candidate := range previous.Containers {
+			if candidate.Name == QueueContainerName {
+				previousQueueContainer = &candidate
+			} else {
+				previousUserContainers = append(previousUserContainers, candidate)
+			}
+		}
+	}
+	queueContainer, err := makeQueueContainer(rev, cfg, previousQueueContainer)
 
 	if err != nil {
 		return nil, fmt.Errorf("failed to create queue-proxy container: %w", err)
@@ -178,7 +190,7 @@ func makePodSpec(rev *v1.Revision, cfg *config.Config) (*corev1.PodSpec, error)
 		extraVolumes = append(extraVolumes, certVolume(rev.Namespace+"-"+networking.ServingCertName))
 	}
 
-	podSpec := BuildPodSpec(rev, append(BuildUserContainers(rev), *queueContainer), cfg)
+	podSpec := BuildPodSpec(rev, append(BuildUserContainers(rev, previousUserContainers), *queueContainer), cfg, previous)
 	podSpec.Volumes = append(podSpec.Volumes, extraVolumes...)
 
 	if cfg.Observability.EnableVarLogCollection {
@@ -202,14 +214,21 @@ func makePodSpec(rev *v1.Revision, cfg *config.Config) (*corev1.PodSpec, error)
 }
 
 // BuildUserContainers makes an array of containers from the Revision template.
-func BuildUserContainers(rev *v1.Revision) []corev1.Container {
+func BuildUserContainers(rev *v1.Revision, previous []corev1.Container) []corev1.Container {
 	containers := make([]corev1.Container, 0, len(rev.Spec.PodSpec.Containers))
 	for i := range rev.Spec.PodSpec.Containers {
 		var container corev1.Container
+		var previousContainer *corev1.Container
+		for _, candidate := range previous {
+			if candidate.Name == rev.Spec.PodSpec.Containers[i].Name {
+				previousContainer = &candidate
+				break
+			}
+		}
 		if len(rev.Spec.PodSpec.Containers[i].Ports) != 0 || len(rev.Spec.PodSpec.Containers) == 1 {
-			container = makeServingContainer(*rev.Spec.PodSpec.Containers[i].DeepCopy(), rev)
+			container = makeServingContainer(*rev.Spec.PodSpec.Containers[i].DeepCopy(), rev, previousContainer)
 		} else {
-			container = makeContainer(*rev.Spec.PodSpec.Containers[i].DeepCopy(), rev)
+			container = makeContainer(*rev.Spec.PodSpec.Containers[i].DeepCopy(), rev, previousContainer)
 		}
 		// The below logic is safe because the image digests in Status.ContainerStatus will have been resolved
 		// before this method is called. We check for an empty array here because the method can also be
@@ -224,7 +243,7 @@ func BuildUserContainers(rev *v1.Revision) []corev1.Container {
 	return containers
 }
 
-func makeContainer(container corev1.Container, rev *v1.Revision) corev1.Container {
+func makeContainer(container corev1.Container, rev *v1.Revision, previousContainer *corev1.Container) corev1.Container {
 	// Adding or removing an overwritten corev1.Container field here? Don't forget to
 	// update the fieldmasks / validations in pkg/apis/serving
 	container.Lifecycle = userLifecycle
@@ -236,16 +255,20 @@ func makeContainer(container corev1.Container, rev *v1.Revision) corev1.Containe
 	if container.TerminationMessagePolicy == "" {
 		container.TerminationMessagePolicy = corev1.TerminationMessageFallbackToLogsOnError
 	}
+	if previousContainer != nil {
+		container.ImagePullPolicy = previousContainer.ImagePullPolicy
+		container.TerminationMessagePath = previousContainer.TerminationMessagePath
+	}
 	return container
 }
 
-func makeServingContainer(servingContainer corev1.Container, rev *v1.Revision) corev1.Container {
+func makeServingContainer(servingContainer corev1.Container, rev *v1.Revision, previousContainer *corev1.Container) corev1.Container {
 	userPort := getUserPort(rev)
 	userPortStr := strconv.Itoa(int(userPort))
 	// Replacement is safe as only up to a single port is allowed on the Revision
 	servingContainer.Ports = buildContainerPorts(userPort)
 	servingContainer.Env = append(servingContainer.Env, buildUserPortEnv(userPortStr))
-	container := makeContainer(servingContainer, rev)
+	container := makeContainer(servingContainer, rev, previousContainer)
 	if container.ReadinessProbe != nil {
 		if container.ReadinessProbe.HTTPGet != nil || container.ReadinessProbe.TCPSocket != nil {
 			// HTTP and TCP ReadinessProbes are executed by the queue-proxy directly against the
@@ -260,13 +283,19 @@ func makeServingContainer(servingContainer corev1.Container, rev *v1.Revision) c
 
 // BuildPodSpec creates a PodSpec from the given revision and containers.
 // cfg can be passed as nil if not within revision reconciliation context.
-func BuildPodSpec(rev *v1.Revision, containers []corev1.Container, cfg *config.Config) *corev1.PodSpec {
+func BuildPodSpec(rev *v1.Revision, containers []corev1.Container, cfg *config.Config, previous *corev1.PodSpec) *corev1.PodSpec {
 	pod := rev.Spec.PodSpec.DeepCopy()
 	pod.Containers = containers
 	pod.TerminationGracePeriodSeconds = rev.Spec.TimeoutSeconds
 	if cfg != nil && pod.EnableServiceLinks == nil {
 		pod.EnableServiceLinks = cfg.Defaults.EnableServiceLinks
 	}
+	if previous != nil {
+		pod.DNSPolicy = previous.DNSPolicy
+		pod.RestartPolicy = previous.RestartPolicy
+		pod.SchedulerName = previous.SchedulerName
+		pod.SecurityContext = previous.SecurityContext
+	}
 	return pod
 }
 
@@ -284,6 +313,7 @@ func buildContainerPorts(userPort int32) []corev1.ContainerPort {
 	return []corev1.ContainerPort{{
 		Name:          v1.UserPortName,
 		ContainerPort: userPort,
+		Protocol:      corev1.ProtocolTCP,
 	}}
 }
 
@@ -292,14 +322,16 @@ func buildVarLogSubpathEnvs() []corev1.EnvVar {
 		Name: "K_INTERNAL_POD_NAME",
 		ValueFrom: &corev1.EnvVarSource{
 			FieldRef: &corev1.ObjectFieldSelector{
-				FieldPath: "metadata.name",
+				APIVersion: "v1",
+				FieldPath:  "metadata.name",
 			},
 		},
 	}, {
 		Name: "K_INTERNAL_POD_NAMESPACE",
 		ValueFrom: &corev1.EnvVarSource{
 			FieldRef: &corev1.ObjectFieldSelector{
-				FieldPath: "metadata.namespace",
+				APIVersion: "v1",
+				FieldPath:  "metadata.namespace",
 			},
 		},
 	}}
@@ -313,8 +345,12 @@ func buildUserPortEnv(userPort string) corev1.EnvVar {
 }
 
 // MakeDeployment constructs a K8s Deployment resource from a revision.
-func MakeDeployment(rev *v1.Revision, cfg *config.Config) (*appsv1.Deployment, error) {
-	podSpec, err := makePodSpec(rev, cfg)
+func MakeDeployment(rev *v1.Revision, cfg *config.Config, previous *appsv1.Deployment) (*appsv1.Deployment, error) {
+	var previousPodSpec *corev1.PodSpec
+	if previous != nil {
+		previousPodSpec = &previous.Spec.Template.Spec
+	}
+	podSpec, err := makePodSpec(rev, cfg, previousPodSpec)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create PodSpec: %w", err)
 	}
@@ -340,7 +376,7 @@ func MakeDeployment(rev *v1.Revision, cfg *config.Config) (*appsv1.Deployment, e
 
 	// Slowly but steadily roll the deployment out, to have the least possible impact.
 	maxUnavailable := intstr.FromInt(0)
-	return &appsv1.Deployment{
+	deployment := &appsv1.Deployment{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:            names.Deployment(rev),
 			Namespace:       rev.Namespace,
@@ -366,5 +402,15 @@ func MakeDeployment(rev *v1.Revision, cfg *config.Config) (*appsv1.Deployment, e
 				Spec: *podSpec,
 			},
 		},
-	}, nil
+	}
+
+	if previous != nil {
+		deployment.Spec.RevisionHistoryLimit = previous.Spec.RevisionHistoryLimit
+		deployment.Spec.Strategy.RollingUpdate.MaxSurge = previous.Spec.Strategy.RollingUpdate.MaxSurge
+
+		// Assumption: the template labels should not change because a change would mean a new revision
+		deployment.Spec.Template.ObjectMeta.Labels = previous.Spec.Template.ObjectMeta.Labels
+	}
+
+	return deployment, nil
 }

pkg/reconciler/revision/resources/deploy_test.go

@@ -1308,7 +1308,7 @@ func TestMakePodSpec(t *testing.T) {
 			if test.defaults != nil {
 				cfg.Defaults = test.defaults
 			}
-			got, err := makePodSpec(test.rev, cfg)
+			got, err := makePodSpec(test.rev, cfg, nil)
 			if err != nil {
 				t.Fatal("makePodSpec returned error:", err)
 			}
@@ -1494,15 +1494,15 @@ func TestMakeDeployment(t *testing.T) {
 			cfg := revConfig()
 			cfg.Autoscaler = ac
 			cfg.Deployment = &test.dc
-			podSpec, err := makePodSpec(test.rev, cfg)
+			podSpec, err := makePodSpec(test.rev, cfg, nil)
 			if err != nil {
 				t.Fatal("makePodSpec returned error:", err)
 			}
 			if test.want != nil {
 				test.want.Spec.Template.Spec = *podSpec
 			}
 			// Copy to override
-			got, err := MakeDeployment(test.rev, cfg)
+			got, err := MakeDeployment(test.rev, cfg, nil)
 			if err != nil {
 				t.Fatal("Got unexpected error:", err)
 			}

pkg/reconciler/revision/resources/queue.go

@@ -54,10 +54,12 @@ var (
 	queueHTTPPort = corev1.ContainerPort{
 		Name:          requestQueueHTTPPortName,
 		ContainerPort: networking.BackendHTTPPort,
+		Protocol:      corev1.ProtocolTCP,
 	}
 	queueHTTP2Port = corev1.ContainerPort{
 		Name:          requestQueueHTTPPortName,
 		ContainerPort: networking.BackendHTTP2Port,
+		Protocol:      corev1.ProtocolTCP,
 	}
 	queueHTTPSPort = corev1.ContainerPort{
 		Name:          requestQueueHTTPSPortName,
@@ -67,17 +69,21 @@ var (
 		// Provides health checks and lifecycle hooks.
 		Name:          v1.QueueAdminPortName,
 		ContainerPort: networking.QueueAdminPort,
+		Protocol:      corev1.ProtocolTCP,
 	}, {
 		Name:          v1.AutoscalingQueueMetricsPortName,
 		ContainerPort: networking.AutoscalingQueueMetricsPort,
+		Protocol:      corev1.ProtocolTCP,
 	}, {
 		Name:          v1.UserQueueMetricsPortName,
 		ContainerPort: networking.UserQueueMetricsPort,
+		Protocol:      corev1.ProtocolTCP,
 	}}
 
 	profilingPort = corev1.ContainerPort{
 		Name:          profilingPortName,
 		ContainerPort: profiling.ProfilingPort,
+		Protocol:      corev1.ProtocolTCP,
 	}
 
 	queueSecurityContext = &corev1.SecurityContext{
@@ -180,7 +186,7 @@ func fractionFromPercentage(m map[string]string, key kmap.KeyPriority) (float64,
 }
 
 // makeQueueContainer creates the container spec for the queue sidecar.
-func makeQueueContainer(rev *v1.Revision, cfg *config.Config) (*corev1.Container, error) {
+func makeQueueContainer(rev *v1.Revision, cfg *config.Config, previous *corev1.Container) (*corev1.Container, error) {
 	configName := ""
 	if owner := metav1.GetControllerOf(rev); owner != nil && owner.Kind == "Configuration" {
 		configName = owner.Name
@@ -250,7 +256,9 @@ func makeQueueContainer(rev *v1.Revision, cfg *config.Config) (*corev1.Container
 		httpProbe = container.ReadinessProbe.DeepCopy()
 		httpProbe.ProbeHandler = corev1.ProbeHandler{
 			HTTPGet: &corev1.HTTPGetAction{
-				Port: intstr.FromInt(int(servingPort.ContainerPort)),
+				Port:   intstr.FromInt(int(servingPort.ContainerPort)),
+				Path:   "/",
+				Scheme: "HTTP",
 				HTTPHeaders: []corev1.HTTPHeader{{
 					Name:  netheader.ProbeKey,
 					Value: queue.Name,
@@ -301,14 +309,16 @@ func makeQueueContainer(rev *v1.Revision, cfg *config.Config) (*corev1.Container
 			Name: "SERVING_POD",
 			ValueFrom: &corev1.EnvVarSource{
 				FieldRef: &corev1.ObjectFieldSelector{
-					FieldPath: "metadata.name",
+					APIVersion: "v1",
+					FieldPath:  "metadata.name",
 				},
 			},
 		}, {
 			Name: "SERVING_POD_IP",
 			ValueFrom: &corev1.EnvVarSource{
 				FieldRef: &corev1.ObjectFieldSelector{
-					FieldPath: "status.podIP",
+					APIVersion: "v1",
+					FieldPath:  "status.podIP",
 				},
 			},
 		}, {
@@ -379,6 +389,12 @@ func makeQueueContainer(rev *v1.Revision, cfg *config.Config) (*corev1.Container
 		}},
 	}
 
+	if previous != nil {
+		c.ImagePullPolicy = previous.ImagePullPolicy
+		c.TerminationMessagePath = previous.TerminationMessagePath
+		c.TerminationMessagePolicy = previous.TerminationMessagePolicy
+	}
+
 	return c, nil
 }
 

pkg/reconciler/revision/resources/queue_test.go

@@ -419,7 +419,7 @@ func TestMakeQueueContainer(t *testing.T) {
 					Features: &test.fc,
 				},
 			}
-			got, err := makeQueueContainer(test.rev, cfg)
+			got, err := makeQueueContainer(test.rev, cfg, nil)
 			if err != nil {
 				t.Fatal("makeQueueContainer returned error:", err)
 			}
@@ -554,7 +554,7 @@ func TestMakeQueueContainerWithPercentageAnnotation(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			cfg := revConfig()
 			cfg.Deployment = &test.dc
-			got, err := makeQueueContainer(test.rev, cfg)
+			got, err := makeQueueContainer(test.rev, cfg, nil)
 			if err != nil {
 				t.Fatal("makeQueueContainer returned error:", err)
 			}
@@ -629,7 +629,7 @@ func TestProbeGenerationHTTPDefaults(t *testing.T) {
 		}
 	})
 
-	got, err := makeQueueContainer(rev, revConfig())
+	got, err := makeQueueContainer(rev, revConfig(), nil)
 	if err != nil {
 		t.Fatal("makeQueueContainer returned error")
 	}
@@ -705,7 +705,7 @@ func TestProbeGenerationHTTP(t *testing.T) {
 		}
 	})
 
-	got, err := makeQueueContainer(rev, revConfig())
+	got, err := makeQueueContainer(rev, revConfig(), nil)
 	if err != nil {
 		t.Fatal("makeQueueContainer returned error")
 	}
@@ -890,7 +890,7 @@ func TestTCPProbeGeneration(t *testing.T) {
 			config := revConfig()
 			config.Deployment = &test.dc
 
-			got, err := makeQueueContainer(testRev, config)
+			got, err := makeQueueContainer(testRev, config, nil)
 			if err != nil {
 				t.Fatal("makeQueueContainer returned error")
 			}

pkg/reconciler/revision/table_test.go

@@ -810,7 +810,7 @@ func deploy(t *testing.T, namespace, name string, opts ...interface{}) *appsv1.D
 	// Do this here instead of in `rev` itself to ensure that we populate defaults
 	// before calling MakeDeployment within Reconcile.
 	rev.SetDefaults(context.Background())
-	deployment, err := resources.MakeDeployment(rev, cfg)
+	deployment, err := resources.MakeDeployment(rev, cfg, nil)
 	if err != nil {
 		t.Fatal("failed to create deployment")
 	}