Add support for array task results as matrix parameters

[abayer]

Changes

fixes #5925

/kind feature

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

• [x] Has Docs included if any changes are user facing
• [x] Has Tests included if any functionality added or changed
• [x] Follows the commit message standard
• [x] Meets the Tekton contributor standards (including functionality, content, code)
• [x] Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
• [x] Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
• [ ] Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Array task results can now be used as the value for matrix parameters.

[abayer]

/retest

[abayer]

/retest

[abayer]

/hold Until I add an example test. =)

[afrittoli]

Given the size of the change, and considering that without the feature "feeding results to a matrix" is incomplete, I would consider this PR for a v0.44.1. v0.44 is an LTS release, and it would be a pity for it to be this close to having this functionality. wdyt?

[abayer]

/hold cancel

@afrittoli I agree - #6061 may also make sense to backport.

[jerop]

@abayer it seems some changes are needed in the validation: "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: invalid value: parameters of type array only are allowed in matrix: spec.pipelineSpec.tasks[2].matrix[browser]

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernick

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [lbernick]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[abayer]

it seems some changes are needed in the validation: "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: invalid value: parameters of type array only are allowed in matrix: spec.pipelineSpec.tasks[2].matrix[browser]

Oh my, yes, this is a rathole. =) So the problem is that we're checking what the raw parsed type is - which will be a string, not an array, since the value is $(tasks.get-browsers.results.browsers[*]). If we turn off the validation check in validateParametersInTaskMatrix, things do just work (or rather, I thought they did, but in my local test, the controller is panicking in ValidateResolvedTaskResources, so there might be something more busted). Aaaanyway - the validation problem is that we're requiring the param value to be an array, but we don't actually know that the initially-parsed string $(tasks.get-browsers.results.browsers[*]) will end up being an array. And now it also appears that there are other issues with resolved task resources, but I haven't dug into that yet.

[abayer]

Ok, I have a fix for the validation that I'm fairly happy with, but the next problem is that by the time GetNamesOfTaskRuns(pipelineRun.Status.ChildReferences, pipelineTask.Name, pipelineRun.Name, pipelineTask.GetMatrixCombinationsCount()) gets called within ResolvePipelineTask, the array results parameter is still a string, meaning GetMatrixCombinationsCount() ends up returning 0 (because the "string" param has len(param.Value.ArrayVal) == 0). Digging further.

EDIT: ...yup, there's the problem. It's that matrix, as a whole, can't handle dynamically generated params, not specifically that it can't handle array results. It just doesn't know the size of the matrix at PipelineRun reconciliation time and that breaks a bunch of stuff.

With my validation change (basically just saying "ok, this matrix param is valid if either it's ParamTypeArray or it matches the form of $(whatever[*])), PipelineRun parameters can be used just fine, but not results. This is gonna require some serious thinking to figure out since PipelineRun reconciliation wants to know all the PipelineTasks it'll have to execute immediately, and when we're dependent on an earlier PipelineTask to finish before we know what the matrix combination count is, it's impossible to generate all the matrixed PipelineTask names until that earlier PipelineTask has completed.

cc @jerop

[abayer]

I've updated the PR with my fix for the validation issue, and with a unit test tweak to use an array result as a matrix parameter, to make it easier to dig into the problem.

[afrittoli]

Thanks @abayer for the detailed investigation on this.

So it sounds like this:

• this is not something we should backport to v0.44 as it will be a non-trivial change
• we could perhaps detect and have a clearer error message for users that attempt to pass results into parameters, and backport that fix instead

wdyt?

[tekton-robot]

@abayer: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-tekton-pipeline-unit-tests	513a55eb9e8a1b039d03f4372ec84b3b4324c659	link	true	/test tekton-pipeline-unit-tests
pull-tekton-pipeline-integration-tests	513a55eb9e8a1b039d03f4372ec84b3b4324c659	link	true	/test pull-tekton-pipeline-integration-tests
pull-tekton-pipeline-alpha-integration-tests	513a55eb9e8a1b039d03f4372ec84b3b4324c659	link	true	/test pull-tekton-pipeline-alpha-integration-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[abayer]

@afrittoli Yeah, that sounds good. I’ll try to get a PR up tomorrow for the better error message.

[abayer]

/hold

Putting this on hold until we most likely just close it. =)

From a chat this morning with @jerop, I feel like this is a reasonable path forward:

• First, add a check in Pipeline validation to verify that matrix parameters are not just arrays, but that they are not empty arrays. This won't change existing functionality/capabilities, but will get rid of the panic in the reconciler. I created #6071 for this.
• Next, tackle adding support for using PipelineRun parameters as matrix parameter values. This will require making changes to the validation to allow $(params.foo[*]), rather than an explicit YAML array, to be used as a matrix parameter, and will require the PipelineRun reconciler to handle replacing that params reference with the actual value, and better error handling around the possibility of that parameter's value being an empty array.
• Finally, go after using array results as matrix parameter values. Everything relevant to $(params.foo[*]) as matrix parameter value applies here too, but there's the additional problem that, at the time that the PipelineRun reconciler is generating the list of matrixed PipelineTasks it will eventually execute, we don't yet know what $(tasks.someTask.results.anArrayResult[*]) actually will contain, so we can't calculate the matrix combination count, and therefore can't create the PipelineTasks for each matrix combination. I feel like this is going to require some fairly deep changes to how matrix is handled in PipelineRun reconciliation, so yeah, this'll be A Bunch Of Work. =)

[pritidesai]

@abayer @jerop Issue #6023 explains what is going on with array results when the task specification does not include square brackets which is the core identification piece in general but this might be related here.

[tekton-robot]

@abayer: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[pritidesai]

Pipelines WG - @abayer do you think we want to keep this in this milestone or move it the next milestone?

[afrittoli]

@abayer do you think there is still a chance for this to happen in the next milestone or should we move it to the following one?

[abayer]

Moved to next milestone - I need to think about this a bit more, but I'm leaning towards likely closing it in favor of what's described in https://github.com/tektoncd/pipeline/pull/6056#issuecomment-1408936114.

[afrittoli]

This should be unlocked by https://github.com/tektoncd/pipeline/pull/6140

[pritidesai]

@EmmaMunley @jerop is this PR still applicable and needed after all the efforts from @EmmaMunley? Shall we keep this open?

This did not make it to 0.46, moving it to 0.47.

[jerop]

yes, it's still applicable, let's leave it open for now

[EmmaMunley]

@abayer This should be unblocked now. https://github.com/tektoncd/pipeline/pull/6140 has merged and this issue has been resolved: PipelineRun reconciler panics for a matrix parameter with an empty array value #6071

cc: @jerop, @pritidesai, @afrittoli

[lbernick]

@abayer my understanding is that @EmmaMunley is now working on this feature in a separate PR, do you mind if I close this?

[jerop]

closing this PR because @EmmaMunley is addressing it separately, thank you for the initial investigation @abayer!