TEP-0147 Dynamic Matrix #1081

pritidesai · 2023-10-06T04:58:41Z

Proposing a new section strategy under matrix which allows to specify the explicit list of combinations dynamically.

tektoncd/pipeline#7170

/kind tep

pritidesai · 2023-10-09T16:18:03Z

/assign @afrittoli
/assign @dibyom

teps/0147-dynamic-matrix.md

dibyom

The problem statement makes sense to me. I was curious about why the JSON syntax is a bit different from regular implicit combinations.

teps/0147-dynamic-matrix.md

afrittoli

Thanks @pritidesai - this will be a nice addition to the matrix functionality,
Do you see this as being part of the scope for matrix beta?

/approve

tekton-robot · 2023-10-16T20:53:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli

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:

~~teps/OWNERS~~ [afrittoli]

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

pritidesai · 2023-10-17T00:15:30Z

Thanks @pritidesai - this will be a nice addition to the matrix functionality, Do you see this as being part of the scope for matrix beta?

/approve

Thanks @afrittoli, yes this is being part of the scope for matrix beta. As an end users, we will be able to cater many more use cases using matrix, mainly building multiple artifacts (multiple images or multiple jar files) as part of a single pipelineRun which is essential from auditing perspective.

pritidesai · 2023-10-18T05:06:50Z

Hey @dibyom, please take a look, its ready for one more round of review, thanks 🙏

dibyom · 2023-10-18T16:29:20Z

Thanks @pritidesai. I think we should capture the future work/alternatives around using object params, CEL etc in the TEP itself. Otherwise, LGTM!

pritidesai · 2023-10-18T18:40:39Z

Thanks @pritidesai. I think we should capture the future work/alternatives around using object params, CEL etc in the TEP itself. Otherwise, LGTM!

I have CEL described as a future work in one of the paragraphs, I am adding a dedicated section for future work as it might be easier to read to include nested params and CEL support. Thanks!

Proposing a new section for matrix which allows to specify the explicit list of combinations dyanmically Signed-off-by: Priti Desai <[email protected]>

pritidesai · 2023-10-18T22:09:18Z

Done adding two additional sections on alternatives and future work, thanks!

jerop · 2023-10-19T18:05:59Z

teps/0147-dynamic-matrix.md

+ params:
+ - name: matrix-strategy
+ value: $(tasks.read-matrix-strategy.results.matrix-strategy)
+ taskRef:
+ name: kaniko
+ workspaces:
+ - name: source
+ workspace: shared-workspace
+ matrix:
+ strategy: $(params.matrix-strategy)


It's not clear to me why the result is passed to a pt.param then passed to pt.matrix.strategy, I'd have expected the result to be consumed directly in pt.matrix.strategy just like they are in pt.when for when expressions

Suggested change

params:

- name: matrix-strategy

value: $(tasks.read-matrix-strategy.results.matrix-strategy)

taskRef:

name: kaniko

workspaces:

- name: source

workspace: shared-workspace

matrix:

strategy: $(params.matrix-strategy)

taskRef:

name: kaniko

workspaces:

- name: source

workspace: shared-workspace

matrix:

strategy: $(tasks.read-matrix-strategy.results.matrix-strategy)

Yes, I think we can support both results and params replacements for this new strategy

yup, its up to the users how they would like to design their pipelines, both params and results are supported.

jerop

now that matrix is in beta, will this feature be an alpha feature gated by its own feature flag?

pritidesai · 2023-10-19T21:14:43Z

now that matrix is in beta, will this feature be an alpha feature gated by its own feature flag?

Matrix as a mechanism and specifications have gone through a promotion process. Matrix specifications using params and include are promoted recently. The process to fan out a given pipelineTask has been part of many pipelines releases. This proposal is not changing the way fan out functionality is implemented. This proposal is reusing existing matrix functionality and extending a syntax to support dynamism. I do not think this requires an alpha feature gate. What is your expectation?

jerop · 2023-10-19T23:23:32Z

I meant that matrix.strategy is a new field and can be gated by a feature flag as a new feature so that we have some room to gather feedback and make changes if needed

Yongxuanzhang · 2023-10-20T14:59:01Z

teps/0147-dynamic-matrix.md

+ params:
+ - name: matrix-strategy
+ value: $(tasks.read-matrix-strategy.results.matrix-strategy)
+ taskRef:
+ name: kaniko
+ workspaces:
+ - name: source
+ workspace: shared-workspace
+ matrix:
+ strategy: $(params.matrix-strategy)


Yes, I think we can support both results and params replacements for this new strategy

Yongxuanzhang · 2023-10-20T15:04:28Z

teps/0147-dynamic-matrix.md

+We propose adding a field - `strategy` of type `string` - within the `matrix` section. This allows pipeline authors to
+maintain a single pipeline in a catalog and allow the users to specify the list of explicit combinations for each
+instance dynamically through `pipelineRun` or a trigger template or a task result.


It might be a bit confusing for users who are familiar with github actions, it uses strategy.matrix but we are proposing matrix.strategy.

And if we look at the example you linked:

strategy: matrix: ${{ fromJSON(needs.job1.outputs.matrix) }}

Users can just add an expression string for the matrix. It is also doable for use. We can support:

matrix: ${params.matrix_passed_in}

we can implement unmarshalljson to support this.
Would this be considered an alternative?

yup, it could be confusing to the users directly coming in from Github actions but we have designed it such that it aligns with the pipelineTask struct.

We have introduced matrix at the taskRef level in the pipelineTask struct. All matrix related fields are part of pipelineTask.matrix.

Supporting an expression language is listed as a future action item. As a project, Tekton pipelines can decide which all expressions languages we would like to support and design accordingly. We have a CEL being adopted in when expressions. We can support it here as well in matrix.strategy if we have a use case.

I'm just wondering if we could have a better naming than strategy? The new field is designed to accept dynamic matrix to pass in. I don't see a clear connection between the word strategy and its purpose.

pritidesai · 2023-10-20T18:39:54Z

I meant that matrix.strategy is a new field and can be gated by a feature flag as a new feature so that we have some room to gather feedback and make changes if needed

yes, my earlier response was in context of matrix.strategy.

matrix.strategy is introducing dynamism using the same underlined beta technology. If we think and expect any changes in the syntax in future, we can certainly introduce an additional feature flag.

pritidesai · 2023-10-20T18:47:01Z

We have a control in place to limit the number of combinations generated so we should not be worried about creating unlimited combinations using matrix.strategy. Also, matrix.strategy follows the same syntax as matrix.include except the name. But once again, if the community would like to introduce this behind a feature flag, as a TEP author I am open to that request. I do not see any obvious issue with the syntax but open to introduce it behind a feature flag.

As a project, we might need a further discussion on how supplemental changes to an an existing beta feature should be handled.

jerop · 2023-10-20T18:52:05Z

thanks for the clarifications @pritidesai -- agreed, let's discuss enhancements to beta and stable features (not brand new features) at the next API WG then we can update TEP-0138 to reflect what we agree on

cc @JeromeJu @tektoncd/core-maintainers

JeromeJu · 2023-10-23T16:51:39Z

/assign
Thanks for the discussion. Happy to get looped in for the feature flag case.

pritidesai · 2023-10-23T17:38:28Z

Thanks @JeromeJu 👍

This proposal is introducing a new field in a struct or an additional API under matrix. matrix is driven by enable-api-fields. We identified this as something TEP reviewers need to decide wether to introduce such API behind a feature flag. We need to find a better solution if introducing an additional flag may be by introducing some meaningful values to a feature flag compared to disabled/enabled/true/false etc

JeromeJu · 2023-10-24T16:27:53Z

teps/0147-dynamic-matrix.md

+a stringified JSON payload in a form of `{"include": a list of combinations}` and creates an equivalent number of
+instances based on the length of the specified list.
+
+When `matrix.strategy` is specified, no other `matrix` fields will be allowed i.e. `matrix.params` and `matrix.include`


It sounds like the strategy feature is mutually exclusive to the existing { params, include}. Wondering if this is fully compatible if we are going to put both the existing matrix feature and the new matrix-strategy feature under enable-api-fields ?

yes, strategy is mutually exclusive from the existing params and include from the usage perspective. From the functionality perspective, it is tightly coupled with include and follows the similar syntax.

The existing matrix feature is guarded behind enable-api-fields and strategy will be guarded behind enable-api-fields as well.

Is that what you are asking?

JeromeJu · 2023-10-24T16:28:04Z

Thanks @pritidesai for the discussion at the WG 🙏
I understand the concern of dependencies of multiple flags used on a feature/enhancement. But if we move forward with the way of not introducing a new Per-feature flag for this, could you help me to understand the following questions:

If we stick with the enable-api-fields flag (as it is for matrix), then matrix.strategy would work as an enhancement rather than a new feature, will it inherit the stability level beta from matrix also?
Is this an enhancement considered as part of the process of matrix being promoted to stable?

pritidesai · 2023-10-25T04:19:37Z

Thanks @pritidesai for the discussion at the WG 🙏 I understand the concern of dependencies of multiple flags used on a feature/enhancement. But if we move forward with the way of not introducing a new Per-feature flag for this, could you help me to understand the following questions:

If we stick with the enable-api-fields flag (as it is for matrix), then matrix.strategy would work as an enhancement rather than a new feature, will it inherit the stability level beta from matrix also?

yes, it would inherit the stability level beta from matrix but I think it is possible to add an extra check to make sure matrix.strategy only works when enable-api-fields set to alpha.

https://github.com/tektoncd/pipeline/blob/main/pkg/apis/pipeline/v1/pipeline_validation.go#L240

Is this an enhancement considered as part of the process of matrix being promoted to stable?

matrix was just promoted to beta without this feature. This feature and any other feature we introduce as part of matrix will all qualify for promotion from beta to stable at the same time.

pritidesai · 2023-10-26T16:20:21Z

Thank you everyone for all the reviews 🙏 Please vote on your preferred solution so that we can make progress with this proposal:

Option 1: Introduce a new feature flag to support matrix.strategy
Option 2: Introduce matrix.strategy as an alpha feature using the same enable-api-fields flag
Option 3: Allow using matrix.strategy with enable-api-fields set to beta

Yongxuanzhang · 2023-10-26T16:34:49Z

Thank you everyone for all the reviews 🙏 Please vote on your preferred solution so that we can make progress with this proposal:

Option 1: Introduce a new feature flag to support matrix.strategy Option 2: Introduce matrix.strategy as an alpha feature using the same enable-api-fields flag Option 3: Allow using matrix.strategy with enable-api-fields set to beta

I prefer option 1 since it leaves room for changes. But for sure it will add more complexity and option 3 may be the most simple solution. I think we just need to make the trade-off here.

JeromeJu · 2023-10-26T20:36:53Z

Thanks for the listing out the options!

Thinking out loud. Some context that might be helpful to taken into consideration when voting:

since the default enable-api-fields value will remain beta, matrix is actually "opt-in" unless cluster operators specifically change their config to stable.

Option 1: Introduce a new feature flag to support matrix.strategy
Probably works the best for cluster operators who are reluctant to opt-in new alpha features. But my concern here is how dependencies of the new enable-strategy flag on enable-api-fields is going to affect the validations(is that something we want to avoid)? If there is an automagical way to just enable matrix features with enable-strategy flag turned on that would be great?

Option 2: Introduce matrix.strategy as an alpha feature using the same enable-api-fields flag
In this case, cluster operators who wants matrix.strategy would also need to enable all other alpha features.

Option 3: Allow using matrix.strategy with enable-api-fields set to beta
We'd consider this as an enhancement by adding a new field IIUC.

I would tend to vote for option 1 if we could have the validation implementations(not necessary in the TEP) sorted out in some way 🤔 .

pritidesai · 2023-10-27T17:27:05Z

since the default enable-api-fields value will remain beta, matrix is actually "opt-in" unless cluster operators specifically change their config to stable.

This would only apply to v1 but not v1beta1?

The disadvantage of option 1 is to the maintenance overhead of the new feature flag. Also, we are not introducing just one new feature for matrix, we have at least two new features apart from matrix.strategy:

displayName at the matrix level - Configurable displayName for a Matrix TaskRun pipeline#7082
max-parallel for matrix - max-parallel in matrix pipeline#7078

Should we introduce an additional flags for each of these features?

JeromeJu · 2023-10-27T17:29:22Z

Also, we are not introducing just one new feature for matrix, we have at least two new features apart from matrix.strategy:
Should we introduce an additional flags for each of these features?

Ahh 🤔 that's a good and yet tricky question. Can I check if they'd all be introducing new API fields? I am trying to figure out what could be regarded as enhancement and what should be regarded as a new feature. And the enhancement part could possibly just stick with the existing validation.

Looking at matrix.displayname, not sure if might fall under the displayname feature IIUC?
Might be wrong, feel free to correct me but both of these look like enhancement.

pritidesai · 2023-10-28T06:24:02Z

Can I check if they'd all be introducing new API fields?

I am anticipating a new API spec/field for both 😞

Looking at tektoncd/pipeline#7082 (comment), not sure if might fall under the displayname feature IIUC?

I consider it as a matrix enhancement/feature rather than the displayName introduced by TEP-0047 since the issue described in tektoncd/pipeline#7082 is very specific to matrix. At the same time, I will leave it open for the community to chime in here.

tekton-robot · 2023-10-30T15:35:56Z

@pritidesai: 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.

vdemeester · 2023-12-14T13:11:41Z

teps/0147-dynamic-matrix.md

+ matrix:
+ include:
+ - name: build-1
+ params:
+ - name: IMAGE
+ value: $(params.image-1)
+ - name: DOCKERFILE
+ value: $(params.dockerfile-1)
+ - name: build-2
+ params:
+ - name: IMAGE
+ value: $(params.image-2)
+ - name: DOCKERFILE
+ value: $(params.dockerfile-2)
+ - name: build-3
+ params:
+ - name: IMAGE
+ value: $(params.image-3)
+ - name: DOCKERFILE
+ value: $(params.dockerfile-3)


Couldn't this be replaced by an implicit param ?

matrix: include: params: foo value: ["$(params.image-1):$(params.dockerfile-1)", "$(params.image-1):$(params.dockerfile-1)", "$(params.image-1):$(params.dockerfile-1)"]

And then a Task that split that ?

vdemeester · 2023-12-14T13:12:35Z

teps/0147-dynamic-matrix.md

+Now, running a shared `pipeline` with `matrix` where the values for each instance of fanned out task is dynamically specified
+at the runtime is not possible with the existing `matrix` syntax. In this proposal, we would like to extend `matrix`
+syntax to provide an option to dynamically specify a list of explicit combinations


It probably is possible, it's just a bit convoluted 🙃

vdemeester · 2024-01-26T09:50:41Z

/assign

JeromeJu · 2024-03-18T16:06:41Z

[WG] Race condition for TEP# here spotted by @ericzzzzzzz thanks!

chitrangpatel · 2024-05-13T16:07:21Z

API WG: Any new updates here @pritidesai?

tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Oct 6, 2023

tekton-robot requested review from dlorenc and mnuttall October 6, 2023 04:58

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 6, 2023

pritidesai force-pushed the 0147-dynamic-matrix branch 2 times, most recently from d68baad to 7ea1903 Compare October 6, 2023 05:02

pritidesai mentioned this pull request Oct 6, 2023

TEP-0147: introducing matrix.strategy tektoncd/pipeline#7180

Draft

7 tasks

tekton-robot assigned afrittoli and dibyom Oct 9, 2023

aaron-prindle reviewed Oct 9, 2023

View reviewed changes

teps/0147-dynamic-matrix.md Show resolved Hide resolved

pritidesai force-pushed the 0147-dynamic-matrix branch 2 times, most recently from 351b457 to 18ede4d Compare October 12, 2023 20:57

dibyom reviewed Oct 13, 2023

View reviewed changes

teps/0147-dynamic-matrix.md Show resolved Hide resolved

teps/0147-dynamic-matrix.md Show resolved Hide resolved

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2023

pritidesai force-pushed the 0147-dynamic-matrix branch from 18ede4d to eaf1428 Compare October 16, 2023 17:44

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2023

afrittoli reviewed Oct 16, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2023

pritidesai force-pushed the 0147-dynamic-matrix branch from eaf1428 to 560864c Compare October 18, 2023 18:57

TEP-0147 Dynamic Matrix

33d6cf8

Proposing a new section for matrix which allows to specify the explicit list of combinations dyanmically Signed-off-by: Priti Desai <[email protected]>

pritidesai force-pushed the 0147-dynamic-matrix branch from 560864c to 33d6cf8 Compare October 18, 2023 19:21

jerop reviewed Oct 19, 2023

View reviewed changes

Yongxuanzhang reviewed Oct 20, 2023

View reviewed changes

tekton-robot assigned JeromeJu Oct 23, 2023

JeromeJu reviewed Oct 24, 2023

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2023

dibyom mentioned this pull request Nov 20, 2023

Promote to GA: TEP-090 Matrix tektoncd/pipeline#7399

Open

vdemeester reviewed Dec 14, 2023

View reviewed changes

tekton-robot assigned vdemeester Jan 26, 2024

TEP-0147 Dynamic Matrix #1081

Are you sure you want to change the base?

TEP-0147 Dynamic Matrix #1081

Conversation

pritidesai commented Oct 6, 2023

pritidesai commented Oct 9, 2023

dibyom left a comment

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

tekton-robot commented Oct 16, 2023

pritidesai commented Oct 17, 2023

pritidesai commented Oct 18, 2023

dibyom commented Oct 18, 2023

pritidesai commented Oct 18, 2023

pritidesai commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerop left a comment

Choose a reason for hiding this comment

pritidesai commented Oct 19, 2023

jerop commented Oct 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritidesai Oct 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritidesai commented Oct 20, 2023

pritidesai commented Oct 20, 2023

jerop commented Oct 20, 2023 • edited

JeromeJu commented Oct 23, 2023

pritidesai commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeromeJu commented Oct 24, 2023 • edited

pritidesai commented Oct 25, 2023

pritidesai commented Oct 26, 2023

Yongxuanzhang commented Oct 26, 2023

JeromeJu commented Oct 26, 2023 • edited

pritidesai commented Oct 27, 2023

JeromeJu commented Oct 27, 2023 • edited

pritidesai commented Oct 28, 2023

tekton-robot commented Oct 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester commented Jan 26, 2024

JeromeJu commented Mar 18, 2024 • edited

chitrangpatel commented May 13, 2024

pritidesai Oct 20, 2023 •

edited

jerop commented Oct 20, 2023 •

edited

JeromeJu commented Oct 24, 2023 •

edited

JeromeJu commented Oct 26, 2023 •

edited

JeromeJu commented Oct 27, 2023 •

edited

JeromeJu commented Mar 18, 2024 •

edited