Sidecar container's status isn't propagated to Kservice

[seongpyoHong]

In what area(s)?

/area API

What version of Knative?

0.11.x

Expected Behavior

When I use multiple containers, one with no ports is not ready, Kservice also becomes Ready=False.

Actual Behavior

I use multiple containers, one with no ports is not ready.

$ kgp
NAME                                                       READY   STATUS             RESTARTS          AGE
failed-multi-container-00001-deployment-75675ff7fd-lswwl   2/3     CrashLoopBackOff   299 (4m54s ago)   25h

However, Kservice's Ready is True

$  k get ksvc
NAME                     URL                         LATESTCREATED                  LATESTREADY                    READY   REASON
failed-multi-container   https://<internal-domain>   failed-multi-container-00001   failed-multi-container-00001   True

Steps to Reproduce the Problem

Use this template.

apiVersion: serving.knative.dev/v1
kind: Service
metadata:
  name: failed-multi-container
spec:
  template:
    spec:
      containerConcurrency: 0
      containers:
      - image: <always-failed-container>
        name: failed-container
      - image: <success-container>
        name: success-container
        ports:
        - containerPort: 17123
          protocol: TCP
        readinessProbe:
          successThreshold: 1
          tcpSocket:
            port: 0
      timeoutSeconds: 300
  traffic:
  - latestRevision: true
    percent: 100

Question

Is there a reason to consider only containers with ports when calculating ContainerHealth Condition in Revision?

serving/pkg/reconciler/revision/reconcile_resources.go

Lines 102 to 117 in 40ebfb6

	for _, status := range pod.Status.ContainerStatuses {
	if status.Name == rev.Spec.GetContainer().Name {
	if t := status.LastTerminationState.Terminated; t != nil {
	logger.Infof("marking exiting with: %d/%s", t.ExitCode, t.Message)
	if t.ExitCode == 0 && t.Message == "" {
	// In cases where there is no error message, we should still provide some exit message in the status
	rev.Status.MarkContainerHealthyFalse(v1.ExitCodeReason(t.ExitCode),
	v1.RevisionContainerExitingMessage("container exited with no error"))
	} else {
	rev.Status.MarkContainerHealthyFalse(v1.ExitCodeReason(t.ExitCode), v1.RevisionContainerExitingMessage(t.Message))
	}
	} else if w := status.State.Waiting; w != nil && hasDeploymentTimedOut(deployment) {
	logger.Infof("marking resources unavailable with: %s: %s", w.Reason, w.Message)
	rev.Status.MarkResourcesAvailableFalse(w.Reason, w.Message)
	}
	break

I think checking the status of all containers that are not queue-proxy will solve this problem.

[dprotaso]

/triage accepted

[seongpyoHong]

@dprotaso Can I work on it if you don't mind?

[seongpyoHong]

@dprotaso I've found something strange.

When I reproduce the above situation, the Kservice is Ready=Unknown for a while (not sure how long, but maybe a few minutes) after it is created. After a few minutes, the KService changes to Ready=True.

When we check the status of the child resources, we see the following differences.

When KService Ready=Unknown

• Revision's ResourceAvailable and ContainerHealthy Conditions are Unknown(Reason: Deploying)
• PA's ScaleTargetInitialized is Unknow

When KService Ready=True

• Revision's ResourceAvailable and ContainerHealthy Conditions are True
• PA's ScaleTargetInitialized is True

Question

In code,

serving/pkg/apis/serving/v1/revision_lifecycle.go

Lines 190 to 198 in 7c92928

	// Don't mark the resources available, if deployment status already determined
	// it isn't so.
	resUnavailable := rs.GetCondition(RevisionConditionResourcesAvailable).IsFalse()
	if ps.IsScaleTargetInitialized() && !resUnavailable {
	// Precondition for PA being initialized is SKS being active and
	// that implies that |service.endpoints| > 0.
	rs.MarkResourcesAvailableTrue()
	rs.MarkContainerHealthyTrue()
	}

It seems that the PA's ScaleTargetInitialized needs to be True to make the Revision's ResourceAvailable, ContainerHealthy Conditions True, which in turn makes the Kservice Ready=True. (Please let me know if I'm wrong)

The Kservice I'm having issues with (which I forgot to explain above) is using hpa. Checking the hpa controller code, it appears that the PA's ScaleTargetInitialized is only changed to True if the SKS is Ready=True.

serving/pkg/reconciler/autoscaling/hpa/hpa.go

Lines 93 to 105 in 7c92928

	if !sks.IsReady() {
	pa.Status.MarkSKSNotReady("SKS Services are not ready yet")
	} else {
	pa.Status.MarkSKSReady()
	// If a min-scale value has been set, we don't want to mark the scale target
	// as initialized until the current replicas are >= the min-scale value.
	if !pa.Status.IsScaleTargetInitialized() {
	ms := activeThreshold(ctx, pa)
	if hpa.Status.CurrentReplicas >= int32(ms) {
	pa.Status.MarkScaleTargetInitialized()
	}
	}
	}

However, SKS is always Ready=False, regardless of KService's Ready.

  conditions:
  - lastTransitionTime: "2023-10-27T05:54:52Z"
    message: Revision is backed by Activator
    reason: ActivatorEndpointsPopulated
    severity: Info
    status: "True"
    type: ActivatorEndpointsPopulated
  - lastTransitionTime: "2023-10-27T05:54:52Z"
    message: K8s Service is not ready
    reason: NoHealthyBackends
    status: Unknown
    type: EndpointsPopulated
  - lastTransitionTime: "2023-10-27T05:54:52Z"
    message: K8s Service is not ready
    reason: NoHealthyBackends
    status: Unknown
    type: Ready
  observedGeneration: 1
  privateServiceName: failed-multi-container-00001-private
  serviceName: failed-multi-container-00001

Is there an expected reason for this behavior?

[seongpyoHong]

@dprotaso sorry for bothering you. Can you check above situation?

[dprotaso]

Hey @seongpyoHong - do you mind retesting your scenario with the latest serving release that came out two weeks ago (https://github.com/knative/serving/releases/tag/knative-v1.13.1)

The PR in that release #14795 includes a fix to how we propagate status conditions.

In my testing with your example - I see the Knative Service is in a 'failed' state and it remains like that.

[dprotaso]

I confirmed this is still a problem.