Revision reconciler always updates the Kubernetes Deployment

[SaschaSchwarze0]

I was hunting down a performance issue that we are seeing when Knative is reconciling all revisions, for example due to the controller being restarted or global syncs.

The problem happens in the checkAndUpdateDeployment. The code there uses MakeDeployment like in the create case to setup the deployment. It will then check if there are any changes compared to the previous version by running this code:

	// If the spec we want is the spec we have, then we're good.
	if equality.Semantic.DeepEqual(have.Spec, deployment.Spec) {
		return have, nil
	}

This check will never return true. The reasons are the following:

1. Defaulting. Knative sets up the deployment and does not set fields at various places which Kubernetes will default to some value. Those differences are always there. Examples are: the Protocol field in ports which gets set to TCP, the TimeoutSeconds/PeriodSeconds/FailureThreshold in the queue proxy probe, maxSurge in the rolling update strategy which gets set to 25, the RevisionHistoryLimit which becomes 10, the APIVersion in an env.valueFrom.FieldRef which gets set to v1, the restart policy that gets set to Always ...
2. Mutations by others. Istio for example mutates in the labels service.istio.io/canonical-name and service.istio.io/canonical-revision in the pod template.

Why is this a problem? Assuming there are just 500 revisions in the system. If every revision reconcile is causing the one Update call, then - with a QPS of 40 (Default 5 multiplied by controller count 8), then only these update calls will need 12.5 seconds. If you have rather 3000 revisions, we're talking about 1m15s. And it's not just about the time. It's also the unnecessary hammering on the API server.

How can this be fixed? I currently have two ideas:

1. Pass the previous version along into MakeDeployment and there we will need to follow two strategies:

   a) is to set defaults where applicable. If you define a TCP probe, just specify that. Kubernetes would unlikely change its default, but specifying the explicit value also does not hurt.
   b) Use the previous version to set fields that Knative does not care and Kubernetes could change, such as the maxSurge or revision limit.

   A sample of that can be found in SaschaSchwarze0@e3fe7f5. There is also code added in checkAndUpdateDeployment that dumps the differences which one can separately apply to see the problem. This part is also explained below in the reproduction steps.

   But, this approach has two disadvantages:

   a) it will require a regular verification and extension assuming Kubernetes eventually adds new fields with defaulting
   b) it will be a large effort to capture every possible field that others could mutate in, it will never be able to handle situations where others mutate changes on top of Knative

2. Pass a target object into MakeDeployment. This would be an empty object for the create case and the current version for an update. All the existing functions that build the deployment details, pod spec, its container details, would need a rewrite to handle an empty object, or update an object.

   This will imo be cleaner, but its complexity and amount of changes grew beyond what I wanted to try out in code I am not familiar with so that I have no spike code to show here.

   This will also not solve situations where webhooks mutate things that Knative sets.

I am happy to discuss this. You can reach me in Knative slack (@sascha). Also happy to help with the implementation.

--

One more thing I'd like to mention: creating an object from scratch and then applying it to the cluster for both the create and update case could be a pattern. I only focused on the revision controller to configure the deployment. I would not be surprised if that pattern (which I consider broken because of above explanations) is used elsewhere. Whether that is a pain or not depends on how many objects of that kind would be present and how often all objects get reconciled.

What version of Knative?

Current

Expected Behavior

Knative only updates Kubernetes deployments when there is a need to do this.

Actual Behavior

Every reconcile of a revision triggers a deployment update.

Steps to Reproduce the Problem

Add the following code before https://github.com/knative/serving/blob/v0.33.0/pkg/reconciler/revision/cruds.go#L93:

	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)

Then let Knative running while you have revisions in the system. You can observe the unnecessary updates in the container logs of the controller.

[psschwei]

For reference, here's the output of one of those logs after a reconcile

  v1.DeploymentSpec{
    Replicas: &0,
    Selector: &{MatchLabels: {"serving.knative.dev/revisionUID": "f7a53897-8006-48a9-b38f-57617fc8a892"}},
    Template: v1.PodTemplateSpec{
      ObjectMeta: {Labels: {"app": "hello-00001", "serving.knative.dev/configuration": "hello", "serving.knative.dev/configurationGeneration": "1", "serving.knative.dev/configurationUID": "819b6139-fde5-40fb-9c9d-30265e7f7663", ...}, Annotations: {"serving.knative.dev/creator": "minikube-user"}},
      Spec: v1.PodSpec{
        Volumes:        nil,
        InitContainers: nil,
        Containers: []v1.Container{
          {
            ... // 3 identical fields
            Args:       nil,
            WorkingDir: "",
            Ports: []v1.ContainerPort{
              {
                Name:          "user-port",
                HostPort:      0,
                ContainerPort: 8080,
-               Protocol:      "TCP",
+               Protocol:      "",
                HostIP:        "",
              },
            },
            EnvFrom: nil,
            Env:     {{Name: "TARGET", Value: "Go Sample v2"}, {Name: "PORT", Value: "8080"}, {Name: "K_REVISION", Value: "hello-00001"}, {Name: "K_CONFIGURATION", Value: "hello"}, ...},
            Resources: v1.ResourceRequirements{
-             Limits:   nil,
+             Limits:   v1.ResourceList{},
-             Requests: nil,
+             Requests: v1.ResourceList{},
            },
            VolumeMounts:   nil,
            VolumeDevices:  nil,
            LivenessProbe:  nil,
            ReadinessProbe: nil,
            StartupProbe:   nil,
            Lifecycle: &v1.Lifecycle{
              PostStart: nil,
              PreStop: &v1.LifecycleHandler{
                Exec: nil,
                HTTPGet: &v1.HTTPGetAction{
                  Path:        "/wait-for-drain",
                  Port:        {IntVal: 8022},
                  Host:        "",
-                 Scheme:      "HTTP",
+                 Scheme:      "",
                  HTTPHeaders: nil,
                },
                TCPSocket: nil,
              },
            },
-           TerminationMessagePath:   "/dev/termination-log",
+           TerminationMessagePath:   "",
            TerminationMessagePolicy: "FallbackToLogsOnError",
-           ImagePullPolicy:          "IfNotPresent",
+           ImagePullPolicy:          "",
            SecurityContext:          nil,
            Stdin:                    false,
            ... // 2 identical fields
          },
          {
            ... // 3 identical fields
            Args:       nil,
            WorkingDir: "",
            Ports: []v1.ContainerPort{
              {
                Name:          "http-queueadm",
                HostPort:      0,
                ContainerPort: 8022,
-               Protocol:      "TCP",
+               Protocol:      "",
                HostIP:        "",
              },
              {
                Name:          "http-autometric",
                HostPort:      0,
                ContainerPort: 9090,
-               Protocol:      "TCP",
+               Protocol:      "",
                HostIP:        "",
              },
              {
                Name:          "http-usermetric",
                HostPort:      0,
                ContainerPort: 9091,
-               Protocol:      "TCP",
+               Protocol:      "",
                HostIP:        "",
              },
              {
                Name:          "queue-port",
                HostPort:      0,
                ContainerPort: 8012,
-               Protocol:      "TCP",
+               Protocol:      "",
                HostIP:        "",
              },
              {
                Name:          "https-port",
                HostPort:      0,
                ContainerPort: 8112,
-               Protocol:      "TCP",
+               Protocol:      "",
                HostIP:        "",
              },
            },
            EnvFrom: nil,
            Env: []v1.EnvVar{
              ... // 8 identical elements
              {Name: "REVISION_RESPONSE_START_TIMEOUT_SECONDS", Value: "0"},
              {Name: "REVISION_IDLE_TIMEOUT_SECONDS", Value: "0"},
              {
                Name:  "SERVING_POD",
                Value: "",
                ValueFrom: &v1.EnvVarSource{
                  FieldRef: &v1.ObjectFieldSelector{
-                   APIVersion: "v1",
+                   APIVersion: "",
                    FieldPath:  "metadata.name",
                  },
                  ResourceFieldRef: nil,
                  ConfigMapKeyRef:  nil,
                  SecretKeyRef:     nil,
                },
              },
              {
                Name:  "SERVING_POD_IP",
                Value: "",
                ValueFrom: &v1.EnvVarSource{
                  FieldRef: &v1.ObjectFieldSelector{
-                   APIVersion: "v1",
+                   APIVersion: "",
                    FieldPath:  "status.podIP",
                  },
                  ResourceFieldRef: nil,
                  ConfigMapKeyRef:  nil,
                  SecretKeyRef:     nil,
                },
              },
              {Name: "SERVING_LOGGING_CONFIG"},
              {Name: "SERVING_LOGGING_LEVEL"},
              ... // 18 identical elements
            },
            Resources:     {Requests: {s"cpu": {i: {...}, s: "25m", Format: "DecimalSI"}}},
            VolumeMounts:  nil,
            VolumeDevices: nil,
            LivenessProbe: nil,
            ReadinessProbe: &v1.Probe{
              ProbeHandler: v1.ProbeHandler{
                Exec: nil,
                HTTPGet: &v1.HTTPGetAction{
-                 Path:        "/",
+                 Path:        "",
                  Port:        {IntVal: 8012},
                  Host:        "",
-                 Scheme:      "HTTP",
+                 Scheme:      "",
                  HTTPHeaders: {{Name: "K-Network-Probe", Value: "queue"}},
                },
                TCPSocket: nil,
                GRPC:      nil,
              },
              InitialDelaySeconds:           0,
-             TimeoutSeconds:                1,
+             TimeoutSeconds:                0,
-             PeriodSeconds:                 10,
+             PeriodSeconds:                 0,
              SuccessThreshold:              1,
-             FailureThreshold:              3,
+             FailureThreshold:              0,
              TerminationGracePeriodSeconds: nil,
            },
            StartupProbe:             nil,
            Lifecycle:                nil,
-           TerminationMessagePath:   "/dev/termination-log",
+           TerminationMessagePath:   "",
-           TerminationMessagePolicy: "File",
+           TerminationMessagePolicy: "",
-           ImagePullPolicy:          "IfNotPresent",
+           ImagePullPolicy:          "",
            SecurityContext:          &{Capabilities: &{Drop: {"all"}}, RunAsNonRoot: &true, ReadOnlyRootFilesystem: &true, AllowPrivilegeEscalation: &false, ...},
            Stdin:                    false,
            ... // 2 identical fields
          },
        },
        EphemeralContainers:           nil,
-       RestartPolicy:                 "Always",
+       RestartPolicy:                 "",
        TerminationGracePeriodSeconds: &300,
        ActiveDeadlineSeconds:         nil,
-       DNSPolicy:                     "ClusterFirst",
+       DNSPolicy:                     "",
        NodeSelector:                  nil,
        ServiceAccountName:            "",
        ... // 5 identical fields
        HostIPC:               false,
        ShareProcessNamespace: nil,
-       SecurityContext:       s"&PodSecurityContext{SELinuxOptions:nil,RunAsUser:nil,RunAsNonRoot:nil,SupplementalGroups:[],FSGroup:nil,RunAsGroup:nil,Sysctls:[]Sysctl{},WindowsOptions:nil,FSGroupChangePolicy:nil,SeccompProfile:nil,}",
+       SecurityContext:       nil,
        ImagePullSecrets:      nil,
        Hostname:              "",
        Subdomain:             "",
        Affinity:              nil,
-       SchedulerName:         "default-scheduler",
+       SchedulerName:         "",
        Tolerations:           nil,
        HostAliases:           nil,
        ... // 11 identical fields
      },
    },
    Strategy: v1.DeploymentStrategy{
      Type: "RollingUpdate",
      RollingUpdate: &v1.RollingUpdateDeployment{
        MaxUnavailable: &{},
-       MaxSurge:       s"25%",
+       MaxSurge:       nil,
      },
    },
    MinReadySeconds:         0,
-   RevisionHistoryLimit:    &10,
+   RevisionHistoryLimit:    nil,
    Paused:                  false,
    ProgressDeadlineSeconds: &600,
  }

Not sure the best way to do so, but this seems like something we should fix if possible.

/triage accepted

[SaschaSchwarze0]

Hi @psschwei, as I said in the description, I would be happy to help with the implementation. But, I also mentioned that I don't know what the preferred design here would be. If I would have enough self-confidence, I would have opened a PR and not an issue. So, what is your expection after above action ?

[SaschaSchwarze0]

/assign psschwei

[skonto]

@SaschaSchwarze0 @psschwei Hi, maybe something similar to this could help here. I havent checked the details yet if defaulting first via K8s, then by knative and then passing the dep obj covers all cases but could be a start.

[SaschaSchwarze0]

@SaschaSchwarze0 @psschwei Hi, maybe something similar to this could help here. I havent checked the details yet if defaulting first via K8s, then by knative and then passing the dep obj covers all cases but could be a start.

@skonto this idea sounds interesting. But, I don't know how this can work. Knative is a Kubernetes client and only has all the client code available. The registry that the Schema has to do defaulting, does not contain a function for Deployments. Which makes sense given SetObjectDefaults_Deployment is part of Kubernetes server code that Knative does not pick up - and which one is not supposed to import.

Even if it would work and we would import this function somehow, then we would import the defaulting code of one Kubernetes version. I don't know how much this has changed between Kube versions, but we might not be fully happy with that behavior.

[SaschaSchwarze0]

I opened a pull request with a different solution where a hash of the client-side built Deployment spec is stored as Deployment annotation to be later able to perform a comparison. This approach even works with mutating webhooks modifying the Deployment in an arbitrary way in addition to the "known updates" that the Kubernetes defaulting does.

[dprotaso]

I started to look at server side apply and I believe it could solve the excess upgrades described in this issue as well as allowing two controllers to coordinate reconciling a single resource (tracked here: knative/pkg#2128).

1.9 release is next week but I think this is worth digging into for Knative 1.10 (something that I can do)

[SaschaSchwarze0]

I started to look at server side apply and I believe it could solve the excess upgrades described in this issue as well as allowing two controllers to coordinate reconciling a single resource (tracked here: knative/pkg#2128).

1.9 release is next week but I think this is worth digging into for Knative 1.10 (something that I can do)

@dprotaso for me to understand ... can you explain me how server-side apply helps to determine on the client side whether an update call to the server should be made or not ? Just to make sure it is clear: I named the issue "Revision reconciler always updates the Kubernetes Deployment". What I really meant was that the reconciler always triggers an update call to the server without that it really updates the object. And the call itself is the problem as it puts unnecessary load on the API server and consumes Knative's "quota" (QPS).