Skip to content

Commit

Permalink
486 runnable output bug (#876)
Browse files Browse the repository at this point in the history 
Runnable stampedObjects without a Succeeded condition should not be treated as suceeded

Change the behaviour of runnable's outputs:
1. outputs come from the latest StampedObject with a succeeded:true status. objects without a suceeded status are ignored.
2. not specifying outputs is legal - supports terminal/throwaway tasks
  • Loading branch information
@squeedee
squeedee authored May 27, 2022
1 parent b53230e commit 8a58a84
Show file tree
Hide file tree
Showing 4 changed files with 492 additions and 240 deletions.
6 changes: 3 additions & 3 deletions pkg/realizer/runnable/realizer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ func (r *runnableRealizer) Realize(ctx context.Context, runnable *v1alpha1.Runna
log.Error(err, "failed to cleanup runnable stamped objects")
}

outputs, evaluatedStampedObject, err := template.GetOutput(allRunnableStampedObjects)
outputs, outputSource, err := template.GetLatestSuccessfulOutput(allRunnableStampedObjects)
if err != nil {
for _, obj := range allRunnableStampedObjects {
log.V(logger.DEBUG).Info("failed to retrieve output from any object", "considered", obj)
Expand All @@ -138,8 +138,8 @@ func (r *runnableRealizer) Realize(ctx context.Context, runnable *v1alpha1.Runna
}
}

if evaluatedStampedObject != nil {
log.V(logger.DEBUG).Info("retrieved output from stamped object", "stamped object", evaluatedStampedObject)
if outputSource != nil {
log.V(logger.DEBUG).Info("retrieved output from stamped object", "stamped object", outputSource)
}

if len(outputs) == 0 {
Expand Down
12 changes: 7 additions & 5 deletions pkg/realizer/runnable/realizer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ var _ = Describe("Realizer", func() {
APIVersion: "test.run/v1alpha1",
},
ObjectMeta: metav1.ObjectMeta{
GenerateName: "my-stamped-resource-",
GenerateName: "my-stamped-resource-",
CreationTimestamp: metav1.Now(),
},
Spec: resources.TestSpec{
Foo: "is a string",
Expand Down Expand Up @@ -465,9 +466,10 @@ var _ = Describe("Realizer", func() {
Template: runtime.RawExtension{
Raw: []byte(D(`{
"apiVersion": "v1",
"kind": "ConfigMap",
"metadata": { "generateName": "my-stamped-resource-" },
"data": { "has": "is a string" }
"kind": "AThing",
"metadata": { "generateName": "my-stamped-resource-", "creationTimestamp": "2021-09-17T17:02:30Z" },
"spec": { "has": "is a string" },
"status": { "conditions": [{"type":"Succeeded", "status":"True"}] }
}`,
)),
},
Expand All @@ -489,7 +491,7 @@ var _ = Describe("Realizer", func() {
It("returns RetrieveOutputError", func() {
_, _, err := rlzr.Realize(ctx, runnable, systemRepo, runnableRepo, discoveryClient)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring(`unable to retrieve outputs from stamped object [my-important-ns/my-stamped-resource-] of type [configmap] for run template [my-template]: failed to evaluate path [data.hasnot]: jsonpath returned empty list: data.hasnot`))
Expect(err.Error()).To(ContainSubstring(`unable to retrieve outputs from stamped object [my-important-ns/my-stamped-resource-] of type [athing] for run template [my-template]: failed to evaluate path [data.hasnot]: jsonpath returned empty list: data.hasnot`))
Expect(reflect.TypeOf(err).String()).To(Equal("errors.RunnableRetrieveOutputError"))
})
})
Expand Down
109 changes: 41 additions & 68 deletions pkg/templates/cluster_run_template.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,91 +26,68 @@ import (
"github.com/vmware-tanzu/cartographer/pkg/eval"
)

func NewRunTemplateModel(template *v1alpha1.ClusterRunTemplate) ClusterRunTemplate {
return &runTemplate{
template: template,
evaluator: eval.EvaluatorBuilder(),
}
}

type Outputs map[string]apiextensionsv1.JSON

type ClusterRunTemplate interface {
GetName() string
GetResourceTemplate() v1alpha1.TemplateSpec
GetOutput(stampedObjects []*unstructured.Unstructured) (Outputs, *unstructured.Unstructured, error)
GetLatestSuccessfulOutput(stampedObjects []*unstructured.Unstructured) (Outputs, *unstructured.Unstructured, error)
}

type runTemplate struct {
template *v1alpha1.ClusterRunTemplate
template *v1alpha1.ClusterRunTemplate
evaluator eval.Evaluator
}

func (t runTemplate) GetOutput(stampedObjects []*unstructured.Unstructured) (Outputs, *unstructured.Unstructured, error) {
var (
updateError error
everyObjectErrored bool
mostRecentlySubmittedSuccesfulTime *time.Time
evaluatedStampedObject *unstructured.Unstructured
)
const SuccessStatusPath = `status.conditions[?(@.type=="Succeeded")].status`

outputs := Outputs{}
// GetLatestSuccessfulOutput returns the most recent condition:Succeeded=True stamped object.
// If no output paths are specified, then you only receive the object and empty outputs.
// If the output path is specified but doesn't match anything in the latest "suceeded" object, then an error is returned
// along with the matched object.
// if the output paths are all satisfied, then the outputs from the latest object, and the object itself, are returned.
func (t *runTemplate) GetLatestSuccessfulOutput(stampedObjects []*unstructured.Unstructured) (Outputs, *unstructured.Unstructured, error) {
latestMatchingObject := t.getLatestSuccessfulObject(stampedObjects)

evaluator := eval.EvaluatorBuilder()
if latestMatchingObject == nil {
return Outputs{}, nil, nil
}

everyObjectErrored = true
outputError, outputs := t.getOutputsOfSingleObject(t.evaluator, *latestMatchingObject)

for _, stampedObject := range stampedObjects {
objectErr, provisionalOutputs := t.getOutputsOfSingleObject(evaluator, *stampedObject)
return outputs, latestMatchingObject, outputError
}

statusPath := `status.conditions[?(@.type=="Succeeded")].status`
status, err := evaluator.EvaluateJsonPath(statusPath, stampedObject.UnstructuredContent())
if err != nil {
updateError = objectErr
continue
}
func (t *runTemplate) getLatestSuccessfulObject(stampedObjects []*unstructured.Unstructured) *unstructured.Unstructured {
var (
latestTime time.Time // zero value is used for comparison
latestMatchingObject *unstructured.Unstructured
)

if status == "True" && objectErr == nil {
objectCreationTimestamp, err := getCreationTimestamp(stampedObject, evaluator)
if err != nil {
continue
}

if mostRecentlySubmittedSuccesfulTime == nil {
mostRecentlySubmittedSuccesfulTime = objectCreationTimestamp
} else if objectCreationTimestamp.After(*mostRecentlySubmittedSuccesfulTime) {
mostRecentlySubmittedSuccesfulTime = objectCreationTimestamp
} else {
continue
}

outputs = provisionalOutputs
evaluatedStampedObject = stampedObject
for _, stampedObject := range stampedObjects {
status, err := t.evaluator.EvaluateJsonPath(SuccessStatusPath, stampedObject.UnstructuredContent())
if !(err == nil && status == "True") {
continue
}

if objectErr != nil {
updateError = objectErr
} else {
everyObjectErrored = false
currentTime := stampedObject.GetCreationTimestamp().Time
if currentTime.After(latestTime) {
latestMatchingObject = stampedObject
latestTime = currentTime
}
}

if everyObjectErrored {
return nil, nil, updateError
}

return outputs, evaluatedStampedObject, nil
return latestMatchingObject
}

func getCreationTimestamp(stampedObject *unstructured.Unstructured, evaluator evaluator) (*time.Time, error) {
creationTimestamp, err := evaluator.EvaluateJsonPath("metadata.creationTimestamp", stampedObject.UnstructuredContent())
if err != nil {
return nil, err
}
creationTimeString, ok := creationTimestamp.(string)
if !ok {
return nil, err
}
creationTime, err := time.Parse(time.RFC3339, creationTimeString)
if err != nil {
return nil, fmt.Errorf("failed to parse creation metadata.creationTimestamp: %w", err)
}
return &creationTime, nil
}

func (t runTemplate) getOutputsOfSingleObject(evaluator eval.Evaluator, stampedObject unstructured.Unstructured) (error, Outputs) {
func (t *runTemplate) getOutputsOfSingleObject(evaluator eval.Evaluator, stampedObject unstructured.Unstructured) (error, Outputs) {
var objectErr error
provisionalOutputs := Outputs{}
for key, path := range t.template.Spec.Outputs {
Expand All @@ -133,15 +110,11 @@ func (t runTemplate) getOutputsOfSingleObject(evaluator eval.Evaluator, stampedO
return objectErr, provisionalOutputs
}

func NewRunTemplateModel(template *v1alpha1.ClusterRunTemplate) ClusterRunTemplate {
return &runTemplate{template: template}
}

func (t runTemplate) GetName() string {
func (t *runTemplate) GetName() string {
return t.template.Name
}

func (t runTemplate) GetResourceTemplate() v1alpha1.TemplateSpec {
func (t *runTemplate) GetResourceTemplate() v1alpha1.TemplateSpec {
return v1alpha1.TemplateSpec{
Template: &t.template.Spec.Template,
}
Expand Down
Loading

0 comments on commit 8a58a84

Please sign in to comment.