Runnable Status is Ready even when stamped object has never been successful

[waciumawanjohi]

Bug description:

When a Runnable is submitted, it will not display output until the created object has a condition Ready with a status True. But while waiting for a healthy child object, the Runnable itself will still have a status Ready with status True.

Steps to reproduce:

Submit these objects:

---
apiVersion: v1
kind: ServiceAccount
metadata:
  name: super-service-account

---
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  name: super-role-binding
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: Role
  name: super-role
subjects:
  - kind: ServiceAccount
    name: super-service-account

---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
  name: super-role
rules:
  - apiGroups: ["*"]
    resources: ["*"]
    verbs: ["*"]

---

apiVersion: carto.run/v1alpha1
kind: ClusterRunTemplate
metadata:
  name: cm
spec:
  outputs:
    val: data.val
  template:
    apiVersion: v1
    kind: ConfigMap
    metadata:
     generateName: cm-
    data:
      val: $(runnable.spec.inputs.some)$

---
apiVersion: carto.run/v1alpha1
kind: Runnable
metadata:
  name: cm
spec:
  serviceAccountName: super-service-account

  runTemplateRef:
    name: cm

  inputs:
    some: string

Expected behavior:

k get -o yaml runnable cm
returns a Runnable with condition Ready == False

Actual behavior:

k get -o yaml runnable cm
returns a Runnable with condition Ready == True

Additional context:

See branch runnable-status-bug for a test that exercises this.

TODO

• fix the issue
• document the status behavior

[martyspiewak]

Just to +1 this, this causes some unexpected behavior at the workload level too if a supplychain is stamping out a runnable because the workload will report that it is Ready before the runnable tasks have run because the runnable itself is reporting that it is Ready.

[jwntrs]

Just to clarify, the desired behaviour for the Ready condition on Runnable would be as follows:
Ready: True -> Latest thing passed
Ready: False -> Latest thing failed
Ready: Unknown -> Something Running

[squeedee]

Current state of the code:

1. There is code for success checks, and it copies forward the latest "success=true" stamped object's outputs to Runnable's Outputs.
2. Unfortunately, it also treats the absence of a success condition on the stamped object as "success=true". This much is clearly a bug
3. The only code which causes our Runnable status Ready: False is if there are no outputs to copy forward - this is probably a bug as well.

Based on the current behavior, Outputs should be read as "LatestSuccessfulOutputs" -

[squeedee]

We can absolutely implement @jwntrs spec, I'll use this policy:

• Output is from the most recent StampedObject where:
  • Success exists
  • Success: True
  • All output paths from the runTemplate can be found
  • • note: Update docs to explain that Outputs is "lastSuccessfulOutput" and that GC can eventually cause these to go away
• Runnable Condition: StampedObjectReady(see open questions)
  • Unknown: StampedObject Success does not exist
  • False: stampedObjects Output path doesn't resolve (regardless of success/fail)
  • False when the stampedObject's Success==False
  • True when the stampedObject's Success==True (and output path resolves)

Open questions about Runnable Status:

1. There is one second level status "RunTemplateReady" - this refers to the RunTemplate, not the StampedObject. Are we ok introducing a "StampedObject" status to reflect where we're drawing our current top level from?
2. What's a satisfactory pattern for the top-level Ready status? perhaps:
   TemplateReady False: Ready False (always)
   TemplateReady True: Ready = StampedObject . (true/false/unkown)

[squeedee]

Additional:

• OutputPathNotSatisfiedCondition becomes a StampedObjectReady Condition
• Yes we introduce StampedObjectReady
• Yes that's a good top-level strategy. but implementing it in ConditionManager might change workload/deliverable behavior --- be careful

[squeedee]

Did #876 and #900 fix this? Check and resolve @squeedee