From 85d5269d6204527e174345b9cf34c26ae3a12d75 Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Sun, 12 Dec 2021 14:49:43 -0800 Subject: [PATCH 1/7] Merge pipeline design doc --- docs/design-docs/03-pipeline-merge.md | 405 ++++++++++++++++++++++++++ 1 file changed, 405 insertions(+) create mode 100644 docs/design-docs/03-pipeline-merge.md diff --git a/docs/design-docs/03-pipeline-merge.md b/docs/design-docs/03-pipeline-merge.md new file mode 100644 index 0000000000..c3f8545119 --- /dev/null +++ b/docs/design-docs/03-pipeline-merge.md @@ -0,0 +1,405 @@ +# Pipeline Merge + +* Author(s): Phani Teja Marupaka, phanimarupaka +* Approver: Sunil Arora, Mike Borozdin + +## Why + +Currently, `kpt pkg update` doesn't merge the `pipeline` section in the Kptfile as expected. +The fact that the `pipeline` section is a non-associative list with no defined function identity and ordering makes +it very difficult to merge with upstream counterparts. This is forcing users to use +setters and discouraging them from declaring other functions in the `pipeline` as +they will be deleted during the `kpt pkg update`. Merging the pipeline correctly +will reduce huge amounts of friction in declaring new functions. This will encourage +users to declare more functions in the pipeline which in turn helps to avoid excessive +parameterization. + +Consider the example of [Landing Zone](https://github.com/GoogleCloudPlatform/blueprints/tree/main/catalog) blueprints. Only the apply-setters function +is being used here. This is an anti-pattern for package-as-interface motivation +and one of the major blockers for not using other functions is the merge behavior +of the pipeline. If this problem is solved, LZ maintainers can rewrite the packages +with best practices aligned to the bigger goal of treating the package as an interface, +discourage excessive use of setters and only use them as parameterization techniques of last resort. + +## Design + +In order to solve this problem, we should merge the pipeline section of the Kptfile +in a custom manner based on the most common use-cases and the expected outputs in +such scenarios. There are no user interface changes. Users will invoke the `kpt pkg +update` command in the same way they do currently. This effort will improve the merged +output of the pipeline section. + +**Is this change backwards compatible**: We are not making any changes to the api, +we are only improving the merged output of the pipeline section. This change will +produce a different output of Kptfile when compared to the current version but this +is not a breaking change. + +## User guide + +Here is what users can expect when they invoke the command, `kpt pkg update` on a package. + +#### Terminology + +**Original**: Source of the last fetched version of the package, represented by the upstreamLock section in Kptfile. + +**Updated upstream**: Declared source of the updated package, represented by the upstream section in Kptfile. + +**Local**: Local fork of the package on disk. + +Firstly, we need to define the identity of the function in order to uniquely identify +them across three sources and perform a 3-way merge. We can identify the function +using the image field value and the input type(configMap or configPath). +Here is an example of the merging apply-setters function when new setters are added +upstream and existing setter values are updated locally. This is the most common +use case where kpt fails to merge them. + +```yaml +Original: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configMap: + image: nginx + tag: 1.0.1 + +Updated upstream: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configMap: + image: nginx + tag: 1.0.1 + new-setter: new-setter-value // new setter is added + +Local: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configMap: + image: nginx + tag: 1.2.0 // value of tag is updated + +Current Output: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configMap: + image: nginx + tag: 1.0.1 // entire pipeline is overridden by upstream + new-setter: new-setter-value + +Expected Output: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configMap: + image: nginx + tag: 1.2.0 // updated tag is preserved + new-setter: new-setter-value // new setter is pulled +``` + +In the above scenario, the value of the setter tag is not updated upstream, so +the modified local value will be preserved. But in the following example, both +upstream and local values change. So, similar to merging resources, upstream +value wins if the same fields in both upstream and local are updated. + +```yaml +Original: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configPath: setters.yaml + +Updated upstream: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configPath: setters-updated.yaml // upstream value changed + +Local: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configPath: setters-local.yaml // local value changed + +Expected Output: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configPath: setters-updated.yaml // upstream overrides local +``` + +Similarly, the upstream version wins if both upstream and local are updated, else local is preserved. + +```yaml +Original: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configPath: setters.yaml + +Updated upstream: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1.2 + configPath: setters.yaml + +Local: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1.1 + configPath: setters.yaml + +Expected Output: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1.2 + configPath: setters-updated.yaml +``` + +This might not be what all users expect. But this is the default behavior in case +of conflict while merging normal resources as well. So we can inherit the behavior +and be consistent. In order to provide more visibility to the users, we can add +log messages in cases of such conflicts and intimate users about the updated value. +In the future, we can add support to a different conflict strategy of `--local-wins` +as an option to the kpt pkg update command. + +#### More examples with expected output + +Newly added upstream functions are appended at the end. + +```yaml +Original: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configPath: setters.yaml + +Updated upstream: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configPath: setters.yaml + - image: gcr.io/kpt-fn/generate-folders:v0.1 + +Local: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configPath: setters.yaml + - image: gcr.io/kpt-fn/set-namespace:v0.1 + configMap: + namespace: foo + +Expected output: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configPath: setters.yaml + - image: gcr.io/kpt-fn/set-namespace:v0.1 + configMap: + namespace: foo + - image: gcr.io/kpt-fn/generate-folders:v0.1 +``` + +If a function is deleted upstream and not changed on the local, it will be deleted on local. + +```yaml +Original: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configPath: setters.yaml + - image: gcr.io/kpt-fn/generate-folders:v0.1 + + +Updated upstream: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configPath: setters.yaml + +Local: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configPath: setters.yaml + - image: gcr.io/kpt-fn/generate-folders:v0.1 + - image: gcr.io/kpt-fn/set-namespace:v0.1 + configMap: + namespace: foo + +Expected output: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configPath: setters.yaml + - image: gcr.io/kpt-fn/set-namespace:v0.1 + configMap: + namespace: foo +``` + +Same function declared multiple times: If the same function is declared multiple +times with the same input type(configMap/configPath), order is used as a tie-breaker +to identify the function, which means the functions are merged based on their order + +```yaml +Original: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: foo + put-value: bar + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: abc + put-comment: ${some-setter-name} + +Updated upstream: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: foo + put-value: bar-new + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: abc + put-comment: ${updated-setter-name} + +Local: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/generate-folders:v0.1 + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: foo + put-value: bar + - image: gcr.io/kpt-fn/set-labels:v0.1 + configMap: + app: db + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: abc + put-comment: ${some-setter-name} + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: YOUR_TEAM + put-value: my-team + +Expected output: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/generate-folders:v0.1 + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: foo + put-value: bar-new + - image: gcr.io/kpt-fn/set-labels:v0.1 + configMap: + app: db + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: abc + put-comment: ${updated-setter-name} + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: YOUR_TEAM + put-value: my-team +``` + +Merging selectors is difficult as there is no identity. If both upstream and +local selectors for a given function diverge, the entire section of selectors +from upstream will override the selectors on local for that function. + +```yaml +Origin: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/ensure-name-substring:v0.1 + selectors: + - kind: Deployment + name: wordpress + - kind: Service + name: wordpress + +Updated upstream: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/ensure-name-substring:v0.1 + selectors: + - kind: Deployment + name: wordpress + - kind: Service + name: wordpress + - kind: Foo + name: wordpress + +Local: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/ensure-name-substring:v0.1 + selectors: + - kind: Deployment + name: my-wordpress + - kind: Service + name: my-wordpress + - namespace: my-space + +Expected output: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/apply-setters:v0.1 + configPath: setters.yaml + - image: gcr.io/kpt-fn/set-namespace:v0.1 + configMap: + namespace: foo + - image: gcr.io/kpt-fn/generate-folders:v0.1 +``` + +## Open issues + + https://github.com/GoogleContainerTools/kpt/issues/2529 + +## Alternatives Considered + +For identifying the function, we can add the function version to the primary key +(in addition to the function name+input config type). But it is highly likely that +changing the function version means updating the function as opposed to adding a new function. + +Why should upstream win in case of conflicts ? Is this what the user always expects? +- Not necessarily. User expectations might be different in different scenarios for resolving conflicts. +But since we already went down the path of upstream-wins strategy in case of conflicts for merging resources, +we are going down that route to maintain consistency. +- There is an open issue to support multiple conflict resolution strategies and provide interactive update behavior to resolve +conflicts which is out of scope for this doc and will be addressed soon. From 8a56a9477d76fd86005db9a3c40d87bf9d25333c Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Wed, 15 Dec 2021 22:15:28 -0800 Subject: [PATCH 2/7] Introduce name field --- docs/design-docs/03-pipeline-merge.md | 95 +++++++++++++++++++++++++-- 1 file changed, 90 insertions(+), 5 deletions(-) diff --git a/docs/design-docs/03-pipeline-merge.md b/docs/design-docs/03-pipeline-merge.md index c3f8545119..7348677694 100644 --- a/docs/design-docs/03-pipeline-merge.md +++ b/docs/design-docs/03-pipeline-merge.md @@ -47,11 +47,20 @@ Here is what users can expect when they invoke the command, `kpt pkg update` on **Local**: Local fork of the package on disk. Firstly, we need to define the identity of the function in order to uniquely identify -them across three sources and perform a 3-way merge. We can identify the function -using the image field value and the input type(configMap or configPath). +them across three sources and perform a 3-way merge. In order to identify the instance of a +function, we should add a new optional field `name` to function definition. + +Here is the merge keys used by update logic to identify function in the order of precedence: + +1. name +2. image(ignoring the version value) +3. function config type(`configMap` or `configPath`) +4. relative order of the function + Here is an example of the merging apply-setters function when new setters are added upstream and existing setter values are updated locally. This is the most common -use case where kpt fails to merge them. +use case where kpt fails to merge them. Since `name` field is not specified, `image` +value is used to identify and merge the function ```yaml Original: @@ -171,8 +180,7 @@ pipeline: ``` This might not be what all users expect. But this is the default behavior in case -of conflict while merging normal resources as well. So we can inherit the behavior -and be consistent. In order to provide more visibility to the users, we can add +of conflict while merging normal resources as well. In order to provide more visibility to the users, we can add log messages in cases of such conflicts and intimate users about the updated value. In the future, we can add support to a different conflict strategy of `--local-wins` as an option to the kpt pkg update command. @@ -334,6 +342,83 @@ pipeline: put-value: my-team ``` +Depending on order of the functions doesn't always yield expected behavior. Users +might reorder the functions or insert a function at random location in the local pipeline. +In this case, we recommend users to leverage name field +in order to merge the functions in deterministic fashion. + +```yaml +Original: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: foo + put-value: bar + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: abc + put-comment: ${some-setter-name} + +Updated upstream: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: foo + put-value: bar-new + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: abc + put-comment: ${updated-setter-name} + +Local: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/search-replace:v0.1 + name: my-new-function + configMap: + by-value: YOUR_TEAM + put-value: my-team + - image: gcr.io/kpt-fn/generate-folders:v0.1 + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: foo + put-value: bar + - image: gcr.io/kpt-fn/set-labels:v0.1 + configMap: + app: db + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: abc + put-comment: ${some-setter-name} + +Expected output: + +pipeline: + mutators: + - image: gcr.io/kpt-fn/search-replace:v0.1 + name: my-new-function + configMap: + by-value: YOUR_TEAM + put-value: my-team + - image: gcr.io/kpt-fn/generate-folders:v0.1 + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: foo + put-value: bar-new + - image: gcr.io/kpt-fn/set-labels:v0.1 + configMap: + app: db + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: abc + put-comment: ${updated-setter-name} +``` + Merging selectors is difficult as there is no identity. If both upstream and local selectors for a given function diverge, the entire section of selectors from upstream will override the selectors on local for that function. From d8374e4d7ed15c292c2dc5f4ef30cc30093d0715 Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Mon, 20 Dec 2021 11:43:22 -0800 Subject: [PATCH 3/7] Minor changes --- docs/design-docs/03-pipeline-merge.md | 61 ++++++++++++++------------- 1 file changed, 31 insertions(+), 30 deletions(-) diff --git a/docs/design-docs/03-pipeline-merge.md b/docs/design-docs/03-pipeline-merge.md index 7348677694..ed37e59aae 100644 --- a/docs/design-docs/03-pipeline-merge.md +++ b/docs/design-docs/03-pipeline-merge.md @@ -6,19 +6,19 @@ ## Why Currently, `kpt pkg update` doesn't merge the `pipeline` section in the Kptfile as expected. -The fact that the `pipeline` section is a non-associative list with no defined function identity and ordering makes -it very difficult to merge with upstream counterparts. This is forcing users to use -setters and discouraging them from declaring other functions in the `pipeline` as -they will be deleted during the `kpt pkg update`. Merging the pipeline correctly -will reduce huge amounts of friction in declaring new functions. This will encourage -users to declare more functions in the pipeline which in turn helps to avoid excessive -parameterization. - -Consider the example of [Landing Zone](https://github.com/GoogleCloudPlatform/blueprints/tree/main/catalog) blueprints. Only the apply-setters function -is being used here. This is an anti-pattern for package-as-interface motivation +The fact that the `pipeline` section is a non-associative list with no defined function identity makes +it very difficult to merge with upstream counterparts. Ordering of the functions is also important. +This friction is forcing users to use `setters` and discouraging them from declaring other functions in the `pipeline` as +they will be deleted during the `kpt pkg update`. [Here](https://github.com/GoogleContainerTools/kpt/issues/2529) +is the example issue. Merging the pipeline correctly will reduce huge amounts of +friction in declaring new functions. This will encourage users to declare more functions +in the pipeline which in turn helps to **avoid excessive parameterization**. + +Consider the example of [Landing Zone](https://github.com/GoogleCloudPlatform/blueprints/tree/main/catalog) blueprints. +Parameters(setters) are the primary interface for the package. This is an anti-pattern for package-as-interface motivation and one of the major blockers for not using other functions is the merge behavior of the pipeline. If this problem is solved, LZ maintainers can rewrite the packages -with best practices aligned to the bigger goal of treating the package as an interface, +with best practices aligned to the bigger goal of treating the package-as-interface, discourage excessive use of setters and only use them as parameterization techniques of last resort. ## Design @@ -30,7 +30,7 @@ update` command in the same way they do currently. This effort will improve the output of the pipeline section. **Is this change backwards compatible**: We are not making any changes to the api, -we are only improving the merged output of the pipeline section. This change will +we are only improving the merged output of the `pipeline` section. This change will produce a different output of Kptfile when compared to the current version but this is not a breaking change. @@ -47,7 +47,7 @@ Here is what users can expect when they invoke the command, `kpt pkg update` on **Local**: Local fork of the package on disk. Firstly, we need to define the identity of the function in order to uniquely identify -them across three sources and perform a 3-way merge. In order to identify the instance of a +a function across three sources to perform a 3-way merge. In order to reliably identify the instance of a function, we should add a new optional field `name` to function definition. Here is the merge keys used by update logic to identify function in the order of precedence: @@ -122,29 +122,29 @@ Original: pipeline: mutators: - - image: gcr.io/kpt-fn/apply-setters:v0.1 - configPath: setters.yaml + - image: gcr.io/kpt-fn/set-labels:v0.1 + configPath: labels.yaml Updated upstream: pipeline: mutators: - - image: gcr.io/kpt-fn/apply-setters:v0.1 - configPath: setters-updated.yaml // upstream value changed + - image: gcr.io/kpt-fn/set-labels:v0.1 + configPath: labels-updated.yaml // upstream value changed Local: pipeline: mutators: - - image: gcr.io/kpt-fn/apply-setters:v0.1 - configPath: setters-local.yaml // local value changed + - image: gcr.io/kpt-fn/set-labels:v0.1 + configPath: labels-local.yaml // local value changed Expected Output: pipeline: mutators: - - image: gcr.io/kpt-fn/apply-setters:v0.1 - configPath: setters-updated.yaml // upstream overrides local + - image: gcr.io/kpt-fn/set-labels:v0.1 + configPath: labels-updated.yaml // upstream overrides local ``` Similarly, the upstream version wins if both upstream and local are updated, else local is preserved. @@ -154,29 +154,29 @@ Original: pipeline: mutators: - - image: gcr.io/kpt-fn/apply-setters:v0.1 - configPath: setters.yaml + - image: gcr.io/kpt-fn/set-annotations:v0.1 + configPath: annotations.yaml Updated upstream: pipeline: mutators: - - image: gcr.io/kpt-fn/apply-setters:v0.1.2 - configPath: setters.yaml + - image: gcr.io/kpt-fn/set-annotations:v0.1.2 + configPath: annotations.yaml Local: pipeline: mutators: - - image: gcr.io/kpt-fn/apply-setters:v0.1.1 - configPath: setters.yaml + - image: gcr.io/kpt-fn/set-annotations:v0.1.1 + configPath: annotations.yaml Expected Output: pipeline: mutators: - - image: gcr.io/kpt-fn/apply-setters:v0.1.2 - configPath: setters-updated.yaml + - image: gcr.io/kpt-fn/set-annotations:v0.1.2 + configPath: annotations.yaml ``` This might not be what all users expect. But this is the default behavior in case @@ -486,5 +486,6 @@ Why should upstream win in case of conflicts ? Is this what the user always expe - Not necessarily. User expectations might be different in different scenarios for resolving conflicts. But since we already went down the path of upstream-wins strategy in case of conflicts for merging resources, we are going down that route to maintain consistency. -- There is an open issue to support multiple conflict resolution strategies and provide interactive update behavior to resolve +- There is an open [issue](https://github.com/GoogleContainerTools/kpt/issues/1437) to support +multiple conflict resolution strategies and provide interactive update behavior to resolve conflicts which is out of scope for this doc and will be addressed soon. From 8ec3092914723bf266c046e8cd4ba2f564ebe5e1 Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Wed, 5 Jan 2022 10:58:58 -0800 Subject: [PATCH 4/7] Adjust yaml blobs --- docs/design-docs/03-pipeline-merge.md | 240 ++++++++++++++++---------- 1 file changed, 149 insertions(+), 91 deletions(-) diff --git a/docs/design-docs/03-pipeline-merge.md b/docs/design-docs/03-pipeline-merge.md index ed37e59aae..190840b457 100644 --- a/docs/design-docs/03-pipeline-merge.md +++ b/docs/design-docs/03-pipeline-merge.md @@ -62,18 +62,21 @@ upstream and existing setter values are updated locally. This is the most common use case where kpt fails to merge them. Since `name` field is not specified, `image` value is used to identify and merge the function +``` +Original +``` ```yaml -Original: - pipeline: mutators: - image: gcr.io/kpt-fn/apply-setters:v0.1 configMap: image: nginx tag: 1.0.1 - -Updated upstream: - +``` +``` +Updated upstream +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/apply-setters:v0.1 @@ -81,18 +84,23 @@ pipeline: image: nginx tag: 1.0.1 new-setter: new-setter-value // new setter is added +``` +``` +Local +``` -Local: - +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/apply-setters:v0.1 configMap: image: nginx tag: 1.2.0 // value of tag is updated - -Current Output: - +``` +``` +Current Output +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/apply-setters:v0.1 @@ -100,9 +108,11 @@ pipeline: image: nginx tag: 1.0.1 // entire pipeline is overridden by upstream new-setter: new-setter-value - -Expected Output: - +``` +``` +Expected Output +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/apply-setters:v0.1 @@ -117,30 +127,37 @@ the modified local value will be preserved. But in the following example, both upstream and local values change. So, similar to merging resources, upstream value wins if the same fields in both upstream and local are updated. +``` +Original +``` ```yaml -Original: - pipeline: mutators: - image: gcr.io/kpt-fn/set-labels:v0.1 configPath: labels.yaml - -Updated upstream: - +``` +``` +Updated upstream +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/set-labels:v0.1 configPath: labels-updated.yaml // upstream value changed - -Local: - +``` +``` +Local +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/set-labels:v0.1 configPath: labels-local.yaml // local value changed - -Expected Output: - +``` +``` +Expected Output +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/set-labels:v0.1 @@ -149,30 +166,37 @@ pipeline: Similarly, the upstream version wins if both upstream and local are updated, else local is preserved. +``` +Original +``` ```yaml -Original: - pipeline: mutators: - image: gcr.io/kpt-fn/set-annotations:v0.1 configPath: annotations.yaml - -Updated upstream: - +``` +``` +Updated upstream +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/set-annotations:v0.1.2 configPath: annotations.yaml - -Local: - +``` +``` +Local +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/set-annotations:v0.1.1 configPath: annotations.yaml - -Expected Output: - +``` +``` +Expected Output +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/set-annotations:v0.1.2 @@ -189,24 +213,29 @@ as an option to the kpt pkg update command. Newly added upstream functions are appended at the end. +``` +Original +``` ```yaml -Original: - pipeline: mutators: - image: gcr.io/kpt-fn/apply-setters:v0.1 configPath: setters.yaml - -Updated upstream: - +``` +``` +Updated upstream +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/apply-setters:v0.1 configPath: setters.yaml - image: gcr.io/kpt-fn/generate-folders:v0.1 - -Local: - +``` +``` +Local +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/apply-setters:v0.1 @@ -214,9 +243,11 @@ pipeline: - image: gcr.io/kpt-fn/set-namespace:v0.1 configMap: namespace: foo - -Expected output: - +``` +``` +Expected output +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/apply-setters:v0.1 @@ -229,25 +260,29 @@ pipeline: If a function is deleted upstream and not changed on the local, it will be deleted on local. +``` +Original +``` ```yaml -Original: - pipeline: mutators: - image: gcr.io/kpt-fn/apply-setters:v0.1 configPath: setters.yaml - image: gcr.io/kpt-fn/generate-folders:v0.1 - - -Updated upstream: - +``` +``` +Updated upstream +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/apply-setters:v0.1 configPath: setters.yaml - -Local: - +``` +``` +Local +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/apply-setters:v0.1 @@ -256,9 +291,11 @@ pipeline: - image: gcr.io/kpt-fn/set-namespace:v0.1 configMap: namespace: foo - -Expected output: - +``` +``` +Expected output +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/apply-setters:v0.1 @@ -272,9 +309,10 @@ Same function declared multiple times: If the same function is declared multiple times with the same input type(configMap/configPath), order is used as a tie-breaker to identify the function, which means the functions are merged based on their order +``` +Original +``` ```yaml -Original: - pipeline: mutators: - image: gcr.io/kpt-fn/search-replace:v0.1 @@ -285,9 +323,11 @@ pipeline: configMap: by-value: abc put-comment: ${some-setter-name} - -Updated upstream: - +``` +``` +Updated upstream +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/search-replace:v0.1 @@ -298,9 +338,11 @@ pipeline: configMap: by-value: abc put-comment: ${updated-setter-name} - -Local: - +``` +``` +Local +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/generate-folders:v0.1 @@ -319,9 +361,11 @@ pipeline: configMap: by-value: YOUR_TEAM put-value: my-team - -Expected output: - +``` +``` +Expected output +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/generate-folders:v0.1 @@ -347,9 +391,10 @@ might reorder the functions or insert a function at random location in the local In this case, we recommend users to leverage name field in order to merge the functions in deterministic fashion. +``` +Original +``` ```yaml -Original: - pipeline: mutators: - image: gcr.io/kpt-fn/search-replace:v0.1 @@ -360,9 +405,11 @@ pipeline: configMap: by-value: abc put-comment: ${some-setter-name} - -Updated upstream: - +``` +``` +Updated upstream +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/search-replace:v0.1 @@ -373,9 +420,11 @@ pipeline: configMap: by-value: abc put-comment: ${updated-setter-name} - -Local: - +``` +``` +Local +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/search-replace:v0.1 @@ -395,9 +444,11 @@ pipeline: configMap: by-value: abc put-comment: ${some-setter-name} - -Expected output: - +``` +``` +Expected output +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/search-replace:v0.1 @@ -423,9 +474,10 @@ Merging selectors is difficult as there is no identity. If both upstream and local selectors for a given function diverge, the entire section of selectors from upstream will override the selectors on local for that function. +``` +Origin +``` ```yaml -Origin: - pipeline: mutators: - image: gcr.io/kpt-fn/ensure-name-substring:v0.1 @@ -434,9 +486,11 @@ pipeline: name: wordpress - kind: Service name: wordpress - -Updated upstream: - +``` +``` +Updated upstream +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/ensure-name-substring:v0.1 @@ -447,9 +501,11 @@ pipeline: name: wordpress - kind: Foo name: wordpress - -Local: - +``` +``` +Local +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/ensure-name-substring:v0.1 @@ -459,9 +515,11 @@ pipeline: - kind: Service name: my-wordpress - namespace: my-space - -Expected output: - +``` +``` +Expected output +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/apply-setters:v0.1 From f7a6625b980a3bd61f3e0fc35748cfad1bcf875b Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Wed, 5 Jan 2022 11:08:04 -0800 Subject: [PATCH 5/7] Address comments --- docs/design-docs/03-pipeline-merge.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/design-docs/03-pipeline-merge.md b/docs/design-docs/03-pipeline-merge.md index 190840b457..8b6f47697a 100644 --- a/docs/design-docs/03-pipeline-merge.md +++ b/docs/design-docs/03-pipeline-merge.md @@ -181,7 +181,7 @@ Updated upstream ```yaml pipeline: mutators: - - image: gcr.io/kpt-fn/set-annotations:v0.1.2 + - image: gcr.io/kpt-fn/set-annotations:v0.2.0 configPath: annotations.yaml ``` ``` @@ -199,7 +199,7 @@ Expected Output ```yaml pipeline: mutators: - - image: gcr.io/kpt-fn/set-annotations:v0.1.2 + - image: gcr.io/kpt-fn/set-annotations:v0.2.0 configPath: annotations.yaml ``` @@ -211,7 +211,7 @@ as an option to the kpt pkg update command. #### More examples with expected output -Newly added upstream functions are appended at the end. +Newly added upstream function. ``` Original From 3552b7cd9586d5ca6bb1f5d02bdee6339e71c422 Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Fri, 7 Jan 2022 14:53:36 -0800 Subject: [PATCH 6/7] Update merge key logic --- docs/design-docs/03-pipeline-merge.md | 159 +++++++++++++++++++------- 1 file changed, 119 insertions(+), 40 deletions(-) diff --git a/docs/design-docs/03-pipeline-merge.md b/docs/design-docs/03-pipeline-merge.md index 8b6f47697a..adb9d4f55b 100644 --- a/docs/design-docs/03-pipeline-merge.md +++ b/docs/design-docs/03-pipeline-merge.md @@ -50,12 +50,23 @@ Firstly, we need to define the identity of the function in order to uniquely ide a function across three sources to perform a 3-way merge. In order to reliably identify the instance of a function, we should add a new optional field `name` to function definition. -Here is the merge keys used by update logic to identify function in the order of precedence: - -1. name -2. image(ignoring the version value) -3. function config type(`configMap` or `configPath`) -4. relative order of the function +We will introduce a new field `name` for identifying functions and merge pipeline as associative list. +The aim is to encourage users eventually to have `name` field specified for all functions +similar to containers in deployment. But in the meanwhile, we will be using image name in +order to identify the function and make it an associative list for merging. The limitation +of this approach is the multiple functions with same image can be declared in the pipeline. +In that case we will fall back to current merge logic of merging pipeline as non-associative list. +In order to have deterministic behavior, we will use image as merge key iff none of the +functions have `name` field specified and iff there are no duplicate image names declare in functions. + +So the algorithm to merge: + +1. If `name` field is specified in all the functions across all sources, merge +it as associative list using `name` field as merge key. +2. If `name` field is not specified in all the functions across all sources, +and if there is no duplicate declaration of image names for functions, use `image` value +(excluding the version) as merge key and merge it as associative list. +3. In all other cases fall back to the current merge behavior. Here is an example of the merging apply-setters function when new setters are added upstream and existing setter values are updated locally. This is the most common @@ -203,12 +214,6 @@ pipeline: configPath: annotations.yaml ``` -This might not be what all users expect. But this is the default behavior in case -of conflict while merging normal resources as well. In order to provide more visibility to the users, we can add -log messages in cases of such conflicts and intimate users about the updated value. -In the future, we can add support to a different conflict strategy of `--local-wins` -as an option to the kpt pkg update command. - #### More examples with expected output Newly added upstream function. @@ -306,8 +311,7 @@ pipeline: ``` Same function declared multiple times: If the same function is declared multiple -times with the same input type(configMap/configPath), order is used as a tie-breaker -to identify the function, which means the functions are merged based on their order +times and `name` field is not specified, fall back to the current merge behavior ``` Original @@ -368,28 +372,18 @@ Expected output ```yaml pipeline: mutators: - - image: gcr.io/kpt-fn/generate-folders:v0.1 - - image: gcr.io/kpt-fn/search-replace:v0.1 - configMap: - by-value: foo - put-value: bar-new - - image: gcr.io/kpt-fn/set-labels:v0.1 - configMap: - app: db - - image: gcr.io/kpt-fn/search-replace:v0.1 - configMap: - by-value: abc - put-comment: ${updated-setter-name} - - image: gcr.io/kpt-fn/search-replace:v0.1 - configMap: - by-value: YOUR_TEAM - put-value: my-team + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: foo + put-value: bar-new + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: abc + put-comment: ${updated-setter-name} ``` -Depending on order of the functions doesn't always yield expected behavior. Users -might reorder the functions or insert a function at random location in the local pipeline. -In this case, we recommend users to leverage name field -in order to merge the functions in deterministic fashion. +In the following scenario, only one function has the `name` field specified. All the +other functions doesn't have name specified, hence we fall back to current merge logic. ``` Original @@ -449,6 +443,59 @@ pipeline: Expected output ``` ```yaml +pipeline: + mutators: + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: foo + put-value: bar-new + - image: gcr.io/kpt-fn/search-replace:v0.1 + configMap: + by-value: abc + put-comment: ${updated-setter-name} +``` + +Here is the ideal scenario which leads to deterministic merge behavior using `name` +field across all sources + +``` +Original +``` +```yaml +pipeline: + mutators: + - image: gcr.io/kpt-fn/search-replace:v0.1 + name: sr1 + configMap: + by-value: foo + put-value: bar + - image: gcr.io/kpt-fn/search-replace:v0.1 + name: sr2 + configMap: + by-value: abc + put-comment: ${some-setter-name} +``` +``` +Updated upstream +``` +```yaml +pipeline: + mutators: + - image: gcr.io/kpt-fn/search-replace:v0.1 + name: sr1 + configMap: + by-value: foo + put-value: bar-new + - image: gcr.io/kpt-fn/search-replace:v0.1 + name: sr2 + configMap: + by-value: abc + put-comment: ${updated-setter-name} +``` +``` +Local +``` +```yaml pipeline: mutators: - image: gcr.io/kpt-fn/search-replace:v0.1 @@ -457,21 +504,53 @@ pipeline: by-value: YOUR_TEAM put-value: my-team - image: gcr.io/kpt-fn/generate-folders:v0.1 + name: gf1 - image: gcr.io/kpt-fn/search-replace:v0.1 + name: sr1 configMap: by-value: foo - put-value: bar-new + put-value: bar - image: gcr.io/kpt-fn/set-labels:v0.1 + name: sl1 configMap: app: db - image: gcr.io/kpt-fn/search-replace:v0.1 + name: sr2 configMap: by-value: abc - put-comment: ${updated-setter-name} + put-comment: ${some-setter-name} ``` - -Merging selectors is difficult as there is no identity. If both upstream and -local selectors for a given function diverge, the entire section of selectors +``` +Expected output +``` +```yaml +pipeline: + mutators: + - image: gcr.io/kpt-fn/search-replace:v0.1 + name: my-new-function + configMap: + by-value: YOUR_TEAM + put-value: my-team + - image: gcr.io/kpt-fn/generate-folders:v0.1 + name: gf1 + - image: gcr.io/kpt-fn/search-replace:v0.1 + name: sr1 + configMap: + by-value: foo + put-value: bar-new + - image: gcr.io/kpt-fn/set-labels:v0.1 + name: sl1 + configMap: + app: db + - image: gcr.io/kpt-fn/search-replace:v0.1 + name: sr2 + configMap: + by-value: abc + put-comment: ${updated-setter-name} +``` + +Merging selectors is difficult as there is no identity. If both upstream and +local selectors for a given function diverge, the entire section of selectors from upstream will override the selectors on local for that function. ``` @@ -537,7 +616,7 @@ pipeline: ## Alternatives Considered For identifying the function, we can add the function version to the primary key -(in addition to the function name+input config type). But it is highly likely that +(in addition to the function name). But it is highly likely that changing the function version means updating the function as opposed to adding a new function. Why should upstream win in case of conflicts ? Is this what the user always expects? From 3dc77a48c5cecde86062dac9dc8061120666721e Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Mon, 10 Jan 2022 16:02:08 -0800 Subject: [PATCH 7/7] nit --- docs/design-docs/03-pipeline-merge.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/docs/design-docs/03-pipeline-merge.md b/docs/design-docs/03-pipeline-merge.md index adb9d4f55b..65365885e6 100644 --- a/docs/design-docs/03-pipeline-merge.md +++ b/docs/design-docs/03-pipeline-merge.md @@ -47,10 +47,8 @@ Here is what users can expect when they invoke the command, `kpt pkg update` on **Local**: Local fork of the package on disk. Firstly, we need to define the identity of the function in order to uniquely identify -a function across three sources to perform a 3-way merge. In order to reliably identify the instance of a -function, we should add a new optional field `name` to function definition. - -We will introduce a new field `name` for identifying functions and merge pipeline as associative list. +a function across three sources to perform a 3-way merge. We will introduce a +new field `name` for identifying functions and merge pipeline as associative list. The aim is to encourage users eventually to have `name` field specified for all functions similar to containers in deployment. But in the meanwhile, we will be using image name in order to identify the function and make it an associative list for merging. The limitation