Divingbell overrides functionality regression - Assistance needed
Hello team, I'm seeking for advices and assistance in resolving broken overrides in Divingbell. They do not work since Helm version update. More specifically, secrets with scripts for configuration changes get generated identical (as `default`). Details I have are collected in this Story: https://storyboard.openstack.org/#!/story/2005095#comment-117207, including some help on how to reproduce the issue and test different versions. Thank you. Best regards, -- Roman Gorshunov
My best guess would be a change in the Sprig library between the Helm versions. In particular, the deep merge responsible for recursively merging the dictionaries together. Merge is called here in the gotpl: https://github.com/openstack/openstack-helm-infra/blob/master/helm-toolkit/t... Which calls this function defined here in Sprig library: https://github.com/Masterminds/sprig/blob/master/dict.go#L80 Which in turn leverages another third party library: https://github.com/imdario/mergo I see this commit where version of mergo was updated from 0.2.2 to 0.3.7, and added a new mergeOverwrite function to Sprig: https://github.com/Masterminds/sprig/commit/4c59c1250a68e7f7a7e48c54f68c38b5... My guess is that mergo library may have decided to move this override behavior into a new function (mergeOverwrite), and in the process broke backwards compatibility with the existing merge function. I would suggest trying to replace all `merge` with `mergeOverwrite` in `_daemonset_overrides.tpl` and see if this resolves the issue. Regards, -----Original Message----- From: Roman Gorshunov [mailto:paye600@gmail.com] Sent: Wednesday, February 27, 2019 8:07 AM To: airship-discuss@lists.airshipit.org Cc: ANDERSON, CRAIG <ca846m@att.com> Subject: Divingbell overrides functionality regression - Assistance needed Hello team, I'm seeking for advices and assistance in resolving broken overrides in Divingbell. They do not work since Helm version update. More specifically, secrets with scripts for configuration changes get generated identical (as `default`). Details I have are collected in this Story: https://urldefense.proofpoint.com/v2/url?u=https-3A__storyboard.openstack.org_-23-21_story_2005095-23comment-2D117207&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_aS85wtQ&m=uvBY6fCNMxPzMQ76iaiDzyLXGWttwkHsjDJ5e8PrsgA&s=89z0icRegvgXUdqpYCs8sOXElI7dpnLzn6cxu3sCNfg&e=, including some help on how to reproduce the issue and test different versions. Thank you. Best regards, -- Roman Gorshunov
Hello Craig, Thank you for your reply and support. I have tried to do a quick replace of `merge` to `mergeOverwrite`, and it didn't work. Linting fails on the following error:
[ERROR] templates/: parse error in "helm-toolkit/templates/utils/_daemonset_overrides.tpl": template: helm-toolkit/templates/utils/_daemonset_overrides.tpl:52: function "mergeOvewrite" not defined
Best regards, -- Roman Gorshunov
Ok, our version of Helm must not be new enough to contain the version of Sprig library which has that patchset, so I guess it must be a different issue. But I would still suspect something with `merge` or one of the other changes to dict.go between helm versions. -----Original Message----- From: Roman Gorshunov [mailto:paye600@gmail.com] Sent: Thursday, February 28, 2019 2:52 AM To: airship-discuss@lists.airshipit.org Cc: TANG, ROY <rt7380@att.com>; ANDERSON, CRAIG <ca846m@att.com> Subject: Re: Divingbell overrides functionality regression - Assistance needed Hello Craig, Thank you for your reply and support. I have tried to do a quick replace of `merge` to `mergeOverwrite`, and it didn't work. Linting fails on the following error:
[ERROR] templates/: parse error in "helm-toolkit/templates/utils/_daemonset_overrides.tpl": template: helm-toolkit/templates/utils/_daemonset_overrides.tpl:52: function "mergeOvewrite" not defined
Best regards, -- Roman Gorshunov
Hello Craig, team, With some help from Scott I've found exact commit in dependency of helm - mergo, which breaks functionality: https://github.com/imdario/mergo/commit/df0700a1e899196d8020767981b82623c873... If line 52 (new one) returns `false`, then everything seems to work again. Would appreciate any help. Thank you. Details I have collected are in this Story: https://storyboard.openstack.org/#!/story/2005095#comment-117207, including some help on how to reproduce the issue and test different versions. Best regards, -- Roman Gorshunov On Thu, Feb 28, 2019 at 5:32 PM ANDERSON, CRAIG <ca846m@att.com> wrote:
Ok, our version of Helm must not be new enough to contain the version of Sprig library which has that patchset, so I guess it must be a different issue. But I would still suspect something with `merge` or one of the other changes to dict.go between helm versions.
-----Original Message----- From: Roman Gorshunov [mailto:paye600@gmail.com] Sent: Thursday, February 28, 2019 2:52 AM To: airship-discuss@lists.airshipit.org Cc: TANG, ROY <rt7380@att.com>; ANDERSON, CRAIG <ca846m@att.com> Subject: Re: Divingbell overrides functionality regression - Assistance needed
Hello Craig, Thank you for your reply and support. I have tried to do a quick replace of `merge` to `mergeOverwrite`, and it didn't work. Linting fails on the following error:
[ERROR] templates/: parse error in "helm-toolkit/templates/utils/_daemonset_overrides.tpl": template: helm-toolkit/templates/utils/_daemonset_overrides.tpl:52: function "mergeOvewrite" not defined
Best regards, -- Roman Gorshunov
Oops, update: .. with the help of Pete, not Scott. Sorry. On Wed, Mar 6, 2019 at 5:49 PM Roman Gorshunov <paye600@gmail.com> wrote:
Hello Craig, team,
With some help from Scott I've found exact commit in dependency of helm - mergo, which breaks functionality: https://github.com/imdario/mergo/commit/df0700a1e899196d8020767981b82623c873... If line 52 (new one) returns `false`, then everything seems to work again.
Would appreciate any help. Thank you.
Details I have collected are in this Story: https://storyboard.openstack.org/#!/story/2005095#comment-117207, including some help on how to reproduce the issue and test different versions.
Best regards, -- Roman Gorshunov
On Thu, Feb 28, 2019 at 5:32 PM ANDERSON, CRAIG <ca846m@att.com> wrote:
Ok, our version of Helm must not be new enough to contain the version of Sprig library which has that patchset, so I guess it must be a different issue. But I would still suspect something with `merge` or one of the other changes to dict.go between helm versions.
-----Original Message----- From: Roman Gorshunov [mailto:paye600@gmail.com] Sent: Thursday, February 28, 2019 2:52 AM To: airship-discuss@lists.airshipit.org Cc: TANG, ROY <rt7380@att.com>; ANDERSON, CRAIG <ca846m@att.com> Subject: Re: Divingbell overrides functionality regression - Assistance needed
Hello Craig, Thank you for your reply and support. I have tried to do a quick replace of `merge` to `mergeOverwrite`, and it didn't work. Linting fails on the following error:
[ERROR] templates/: parse error in "helm-toolkit/templates/utils/_daemonset_overrides.tpl": template: helm-toolkit/templates/utils/_daemonset_overrides.tpl:52: function "mergeOvewrite" not defined
Best regards, -- Roman Gorshunov
According to https://github.com/imdario/mergo/blob/master/merge.go#L212-L214 : // MergeWithOverwrite will do the same as Merge except that non-empty dst attributes will be overriden by // non-empty src attribute values. // Deprecated: use Merge(…) with WithOverride So it would appear that this library made a change that broke backwards compatibility with their interface added in version 0.3.7, and that MergeWithOverwrite should be used with this verison of the library. Sprig exposes this with its `mergeOverwrite` function. Note that Sprig made the mergo version update (from 0.2.2 to 0.3.7) in the same commit that mergeOverwrite was added: https://github.com/Masterminds/sprig/commit/4c59c1250a68e7f7a7e48c54f68c38b5... Given this as the case, I don't know how it would be possible that we could have a version of helm which both fails (because of new mergo version) and at the same time doesn't have mergeOverwrite (which was missing according to your earlier test). Just to confirm, were you using Helm 1.12.x when you tried replacing `merge` with `mergOverwrite`? -----Original Message----- From: Roman Gorshunov [mailto:paye600@gmail.com] Sent: Wednesday, March 06, 2019 9:28 AM To: ANDERSON, CRAIG <ca846m@att.com>; airship-discuss@lists.airshipit.org Cc: TANG, ROY <rt7380@att.com> Subject: Re: Divingbell overrides functionality regression - Assistance needed Oops, update: .. with the help of Pete, not Scott. Sorry. On Wed, Mar 6, 2019 at 5:49 PM Roman Gorshunov <paye600@gmail.com> wrote:
Hello Craig, team,
With some help from Scott I've found exact commit in dependency of helm - mergo, which breaks functionality: https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_imdari o_mergo_commit_df0700a1e899196d8020767981b82623c8738932&d=DwIFaQ&c=LFY Z-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_aS85wtQ&m=1MOIIBAVEcoin5q75GkbgRj DRRkoS7kHgavWlrLPTj0&s=jwfTMA8dPqMjzUGEXnUJ9nSrsD7jI8B_LVuBu0HhnJQ&e= If line 52 (new one) returns `false`, then everything seems to work again.
Would appreciate any help. Thank you.
Details I have collected are in this Story: https://urldefense.proofpoint.com/v2/url?u=https-3A__storyboard.openst ack.org_-23-21_story_2005095-23comment-2D117207&d=DwIFaQ&c=LFYZ-o9_HUM eMTSQicvjIg&r=aimn2OylFgog_5_aS85wtQ&m=1MOIIBAVEcoin5q75GkbgRjDRRkoS7k HgavWlrLPTj0&s=4wYIA7lsyCya6IttBmz8A2mwUDQg-HEAYt3AbYfjuQ4&e=, including some help on how to reproduce the issue and test different versions.
Best regards, -- Roman Gorshunov
On Thu, Feb 28, 2019 at 5:32 PM ANDERSON, CRAIG <ca846m@att.com> wrote:
Ok, our version of Helm must not be new enough to contain the version of Sprig library which has that patchset, so I guess it must be a different issue. But I would still suspect something with `merge` or one of the other changes to dict.go between helm versions.
-----Original Message----- From: Roman Gorshunov [mailto:paye600@gmail.com] Sent: Thursday, February 28, 2019 2:52 AM To: airship-discuss@lists.airshipit.org Cc: TANG, ROY <rt7380@att.com>; ANDERSON, CRAIG <ca846m@att.com> Subject: Re: Divingbell overrides functionality regression - Assistance needed
Hello Craig, Thank you for your reply and support. I have tried to do a quick replace of `merge` to `mergeOverwrite`, and it didn't work. Linting fails on the following error:
[ERROR] templates/: parse error in "helm-toolkit/templates/utils/_daemonset_overrides.tpl": template: helm-toolkit/templates/utils/_daemonset_overrides.tpl:52: function "mergeOvewrite" not defined
Best regards, -- Roman Gorshunov
Just to confirm, were you using Helm 1.12.x when you tried replacing `merge` with `mergOverwrite`? I used helm 2.12.3.I confirm that mergeOverwrite does not work.
Link with test: http://paste.openstack.org/show/747400/ Details: - line 4: helm client version v2.12.3 (this version is used in openstack-helm-infra by default) - line 16: latest openstack-helm-infra (commit id e836707ad070e910d6130eced066bc0e50481d2b) - line 30: `helm lint` of helm-toolkit - line 33: ./templates/utils/_daemonset_overrides.tpl uses `merge` - line 40: replace `merge` with `mergeOverwrite` - line 41: confirm replacement - line 50: linting fails with error `function "mergeOverwrite" not defined` Thank you for your help, Craig. -- Roman Gorshunov
Playing a little bit, I have just found out that using helm version v2.13.0-rc.1 and higher, AND `mergeOverwrite` allows to pass linting. Log: http://paste.openstack.org/show/747402/ I've submitted a test PS to see if it would pass standard voting gates: https://review.openstack.org/#/c/641656/ The non-voting `openstack-helm-infra-airship-divingbell` gate should still fail, as it would use old `merge`. Waiting for gates to complete the work. -- Roman Gorshunov On Thu, Mar 7, 2019 at 1:55 PM Roman Gorshunov <paye600@gmail.com> wrote:
Just to confirm, were you using Helm 1.12.x when you tried replacing `merge` with `mergOverwrite`? I used helm 2.12.3.I confirm that mergeOverwrite does not work.
Link with test: http://paste.openstack.org/show/747400/ Details: - line 4: helm client version v2.12.3 (this version is used in openstack-helm-infra by default) - line 16: latest openstack-helm-infra (commit id e836707ad070e910d6130eced066bc0e50481d2b) - line 30: `helm lint` of helm-toolkit - line 33: ./templates/utils/_daemonset_overrides.tpl uses `merge` - line 40: replace `merge` with `mergeOverwrite` - line 41: confirm replacement - line 50: linting fails with error `function "mergeOverwrite" not defined`
Thank you for your help, Craig.
-- Roman Gorshunov
Good news: Tests on gates have succeeded. I have formatted a proper patch set which is now ready for review [0]. If applied, this patch will update Helm from v2.12.3 to v2.13.0, and replace `merge` with `mergeOverwrite` where appropriate in *daemonset_overrides.tpl files from helm-toolkit and ceph-osd. I'm actually not sure how to test if changes in *daemonset_overrides.tpl are correct. Not so good news: This didn't help to the divingbell overrides, and they are still not working. Logs: http://paste.openstack.org/show/747433/ I'm stuck again. [0] https://review.openstack.org/#/c/641656/5 -- Roman Gorshunov On Thu, Mar 7, 2019 at 2:17 PM Roman Gorshunov <paye600@gmail.com> wrote:
Playing a little bit, I have just found out that using helm version v2.13.0-rc.1 and higher, AND `mergeOverwrite` allows to pass linting. Log: http://paste.openstack.org/show/747402/
I've submitted a test PS to see if it would pass standard voting gates: https://review.openstack.org/#/c/641656/ The non-voting `openstack-helm-infra-airship-divingbell` gate should still fail, as it would use old `merge`.
Waiting for gates to complete the work.
-- Roman Gorshunov
On Thu, Mar 7, 2019 at 1:55 PM Roman Gorshunov <paye600@gmail.com> wrote:
Just to confirm, were you using Helm 1.12.x when you tried replacing `merge` with `mergOverwrite`? I used helm 2.12.3.I confirm that mergeOverwrite does not work.
Link with test: http://paste.openstack.org/show/747400/ Details: - line 4: helm client version v2.12.3 (this version is used in openstack-helm-infra by default) - line 16: latest openstack-helm-infra (commit id e836707ad070e910d6130eced066bc0e50481d2b) - line 30: `helm lint` of helm-toolkit - line 33: ./templates/utils/_daemonset_overrides.tpl uses `merge` - line 40: replace `merge` with `mergeOverwrite` - line 41: confirm replacement - line 50: linting fails with error `function "mergeOverwrite" not defined`
Thank you for your help, Craig.
-- Roman Gorshunov
I was able to get it working, so the PS is green now. Swapping the arguments order for the calls made to `mergeOverwrite` fixed it to give the correct (overrides) dict precedence. The reason the arguments were "backwards" before was because mergo was very fussy about ordering and would core dump when we tried using merge with the "intuitive" argument order. Given that the test passed without encountering that error either, it seems we can conclude this is resolved in the current version of mergo. The original problem must have been due to a change in the original `merge` function, which actually appears to be behaving the same as the `mergeOverwrite` function added in Helm after the mergo update. Therefore I believe the test would likely also pass on Helm v2.12.3 using the `merge` function as before but with swapped argument positions. However I think we should just do the Helm v2.13.0 upgrade anyway unless someone has an objection, or before someone proves my Helm v2.12.3 theory wrong :) Roy, it may also be interesting to see if we have issues with `null` parameters in this version of Helm. If this works now without throwing nil pointer exceptions, we could just add this test into the gate script to make sure it doesn't show up again (I'm crossing my fingers). Thanks, -----Original Message----- From: Roman Gorshunov [mailto:paye600@gmail.com] Sent: Thursday, March 07, 2019 4:04 PM To: ANDERSON, CRAIG <ca846m@att.com>; airship-discuss@lists.airshipit.org Cc: TANG, ROY <rt7380@att.com> Subject: Re: Divingbell overrides functionality regression - Assistance needed Good news: Tests on gates have succeeded. I have formatted a proper patch set which is now ready for review [0]. If applied, this patch will update Helm from v2.12.3 to v2.13.0, and replace `merge` with `mergeOverwrite` where appropriate in *daemonset_overrides.tpl files from helm-toolkit and ceph-osd. I'm actually not sure how to test if changes in *daemonset_overrides.tpl are correct. Not so good news: This didn't help to the divingbell overrides, and they are still not working. Logs: https://urldefense.proofpoint.com/v2/url?u=http-3A__paste.openstack.org_show_747433_&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_aS85wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=AzlCjMLPTU89x1SC_X7C2jE78TYtRST2DUGPMmM0Jpc&e= I'm stuck again. [0] https://urldefense.proofpoint.com/v2/url?u=https-3A__review.openstack.org_-23_c_641656_5&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_aS85wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=_tTepiy_shl58qYy5kHFP0r3wx6-JHpiCYDEq5pxJh8&e= -- Roman Gorshunov On Thu, Mar 7, 2019 at 2:17 PM Roman Gorshunov <paye600@gmail.com> wrote:
Playing a little bit, I have just found out that using helm version v2.13.0-rc.1 and higher, AND `mergeOverwrite` allows to pass linting. Log: https://urldefense.proofpoint.com/v2/url?u=http-3A__paste.openstack.or g_show_747402_&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_aS85 wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=d-lYsZ5PXoRxgXFJCW dj84tcen9JqEuTvztcEEujT3I&e=
I've submitted a test PS to see if it would pass standard voting gates: https://urldefense.proofpoint.com/v2/url?u=https-3A__review.openstack. org_-23_c_641656_&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_a S85wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=hSiLdJ1M9Hv0zlQ pbKg2gogxOuiMZ6jVdmRQaTdp1D0&e= The non-voting `openstack-helm-infra-airship-divingbell` gate should still fail, as it would use old `merge`.
Waiting for gates to complete the work.
-- Roman Gorshunov
On Thu, Mar 7, 2019 at 1:55 PM Roman Gorshunov <paye600@gmail.com> wrote:
Just to confirm, were you using Helm 1.12.x when you tried replacing `merge` with `mergOverwrite`? I used helm 2.12.3.I confirm that mergeOverwrite does not work.
Link with test: https://urldefense.proofpoint.com/v2/url?u=http-3A__paste.openstack. org_show_747400_&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_ aS85wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=AwTIrAw_OBoL 4c98ESbnyVIQzkeFPy5YFi9CFL8Ewak&e= Details: - line 4: helm client version v2.12.3 (this version is used in openstack-helm-infra by default) - line 16: latest openstack-helm-infra (commit id e836707ad070e910d6130eced066bc0e50481d2b) - line 30: `helm lint` of helm-toolkit - line 33: ./templates/utils/_daemonset_overrides.tpl uses `merge` - line 40: replace `merge` with `mergeOverwrite` - line 41: confirm replacement - line 50: linting fails with error `function "mergeOverwrite" not defined`
Thank you for your help, Craig.
-- Roman Gorshunov
Craig, sure I can give it a spin, it does look more promising. Do you have your working version somewhere I can pull? I lost track of the ps and don't see it in this email thread. -----Original Message----- From: ANDERSON, CRAIG Sent: Thursday, March 7, 2019 11:23 PM To: Roman Gorshunov <paye600@gmail.com>; airship-discuss@lists.airshipit.org Cc: TANG, ROY <rt7380@att.com> Subject: RE: Divingbell overrides functionality regression - Assistance needed I was able to get it working, so the PS is green now. Swapping the arguments order for the calls made to `mergeOverwrite` fixed it to give the correct (overrides) dict precedence. The reason the arguments were "backwards" before was because mergo was very fussy about ordering and would core dump when we tried using merge with the "intuitive" argument order. Given that the test passed without encountering that error either, it seems we can conclude this is resolved in the current version of mergo. The original problem must have been due to a change in the original `merge` function, which actually appears to be behaving the same as the `mergeOverwrite` function added in Helm after the mergo update. Therefore I believe the test would likely also pass on Helm v2.12.3 using the `merge` function as before but with swapped argument positions. However I think we should just do the Helm v2.13.0 upgrade anyway unless someone has an objection, or before someone proves my Helm v2.12.3 theory wrong :) Roy, it may also be interesting to see if we have issues with `null` parameters in this version of Helm. If this works now without throwing nil pointer exceptions, we could just add this test into the gate script to make sure it doesn't show up again (I'm crossing my fingers). Thanks, -----Original Message----- From: Roman Gorshunov [mailto:paye600@gmail.com] Sent: Thursday, March 07, 2019 4:04 PM To: ANDERSON, CRAIG <ca846m@att.com>; airship-discuss@lists.airshipit.org Cc: TANG, ROY <rt7380@att.com> Subject: Re: Divingbell overrides functionality regression - Assistance needed Good news: Tests on gates have succeeded. I have formatted a proper patch set which is now ready for review [0]. If applied, this patch will update Helm from v2.12.3 to v2.13.0, and replace `merge` with `mergeOverwrite` where appropriate in *daemonset_overrides.tpl files from helm-toolkit and ceph-osd. I'm actually not sure how to test if changes in *daemonset_overrides.tpl are correct. Not so good news: This didn't help to the divingbell overrides, and they are still not working. Logs: https://urldefense.proofpoint.com/v2/url?u=http-3A__paste.openstack.org_show_747433_&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_aS85wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=AzlCjMLPTU89x1SC_X7C2jE78TYtRST2DUGPMmM0Jpc&e= I'm stuck again. [0] https://urldefense.proofpoint.com/v2/url?u=https-3A__review.openstack.org_-23_c_641656_5&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_aS85wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=_tTepiy_shl58qYy5kHFP0r3wx6-JHpiCYDEq5pxJh8&e= -- Roman Gorshunov On Thu, Mar 7, 2019 at 2:17 PM Roman Gorshunov <paye600@gmail.com> wrote:
Playing a little bit, I have just found out that using helm version v2.13.0-rc.1 and higher, AND `mergeOverwrite` allows to pass linting. Log: https://urldefense.proofpoint.com/v2/url?u=http-3A__paste.openstack.or g_show_747402_&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_aS85 wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=d-lYsZ5PXoRxgXFJCW dj84tcen9JqEuTvztcEEujT3I&e=
I've submitted a test PS to see if it would pass standard voting gates: https://urldefense.proofpoint.com/v2/url?u=https-3A__review.openstack. org_-23_c_641656_&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_a S85wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=hSiLdJ1M9Hv0zlQ pbKg2gogxOuiMZ6jVdmRQaTdp1D0&e= The non-voting `openstack-helm-infra-airship-divingbell` gate should still fail, as it would use old `merge`.
Waiting for gates to complete the work.
-- Roman Gorshunov
On Thu, Mar 7, 2019 at 1:55 PM Roman Gorshunov <paye600@gmail.com> wrote:
Just to confirm, were you using Helm 1.12.x when you tried replacing `merge` with `mergOverwrite`? I used helm 2.12.3.I confirm that mergeOverwrite does not work.
Link with test: https://urldefense.proofpoint.com/v2/url?u=http-3A__paste.openstack. org_show_747400_&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_ aS85wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=AwTIrAw_OBoL 4c98ESbnyVIQzkeFPy5YFi9CFL8Ewak&e= Details: - line 4: helm client version v2.12.3 (this version is used in openstack-helm-infra by default) - line 16: latest openstack-helm-infra (commit id e836707ad070e910d6130eced066bc0e50481d2b) - line 30: `helm lint` of helm-toolkit - line 33: ./templates/utils/_daemonset_overrides.tpl uses `merge` - line 40: replace `merge` with `mergeOverwrite` - line 41: confirm replacement - line 50: linting fails with error `function "mergeOverwrite" not defined`
Thank you for your help, Craig.
-- Roman Gorshunov
https://review.openstack.org/#/c/632200 Thanks -----Original Message----- From: TANG, ROY Sent: Friday, March 08, 2019 6:46 AM To: ANDERSON, CRAIG <ca846m@att.com>; Roman Gorshunov <paye600@gmail.com>; airship-discuss@lists.airshipit.org Subject: RE: Divingbell overrides functionality regression - Assistance needed Craig, sure I can give it a spin, it does look more promising. Do you have your working version somewhere I can pull? I lost track of the ps and don't see it in this email thread. -----Original Message----- From: ANDERSON, CRAIG Sent: Thursday, March 7, 2019 11:23 PM To: Roman Gorshunov <paye600@gmail.com>; airship-discuss@lists.airshipit.org Cc: TANG, ROY <rt7380@att.com> Subject: RE: Divingbell overrides functionality regression - Assistance needed I was able to get it working, so the PS is green now. Swapping the arguments order for the calls made to `mergeOverwrite` fixed it to give the correct (overrides) dict precedence. The reason the arguments were "backwards" before was because mergo was very fussy about ordering and would core dump when we tried using merge with the "intuitive" argument order. Given that the test passed without encountering that error either, it seems we can conclude this is resolved in the current version of mergo. The original problem must have been due to a change in the original `merge` function, which actually appears to be behaving the same as the `mergeOverwrite` function added in Helm after the mergo update. Therefore I believe the test would likely also pass on Helm v2.12.3 using the `merge` function as before but with swapped argument positions. However I think we should just do the Helm v2.13.0 upgrade anyway unless someone has an objection, or before someone proves my Helm v2.12.3 theory wrong :) Roy, it may also be interesting to see if we have issues with `null` parameters in this version of Helm. If this works now without throwing nil pointer exceptions, we could just add this test into the gate script to make sure it doesn't show up again (I'm crossing my fingers). Thanks, -----Original Message----- From: Roman Gorshunov [mailto:paye600@gmail.com] Sent: Thursday, March 07, 2019 4:04 PM To: ANDERSON, CRAIG <ca846m@att.com>; airship-discuss@lists.airshipit.org Cc: TANG, ROY <rt7380@att.com> Subject: Re: Divingbell overrides functionality regression - Assistance needed Good news: Tests on gates have succeeded. I have formatted a proper patch set which is now ready for review [0]. If applied, this patch will update Helm from v2.12.3 to v2.13.0, and replace `merge` with `mergeOverwrite` where appropriate in *daemonset_overrides.tpl files from helm-toolkit and ceph-osd. I'm actually not sure how to test if changes in *daemonset_overrides.tpl are correct. Not so good news: This didn't help to the divingbell overrides, and they are still not working. Logs: https://urldefense.proofpoint.com/v2/url?u=http-3A__paste.openstack.org_show_747433_&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_aS85wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=AzlCjMLPTU89x1SC_X7C2jE78TYtRST2DUGPMmM0Jpc&e= I'm stuck again. [0] https://urldefense.proofpoint.com/v2/url?u=https-3A__review.openstack.org_-23_c_641656_5&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_aS85wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=_tTepiy_shl58qYy5kHFP0r3wx6-JHpiCYDEq5pxJh8&e= -- Roman Gorshunov On Thu, Mar 7, 2019 at 2:17 PM Roman Gorshunov <paye600@gmail.com> wrote:
Playing a little bit, I have just found out that using helm version v2.13.0-rc.1 and higher, AND `mergeOverwrite` allows to pass linting. Log: https://urldefense.proofpoint.com/v2/url?u=http-3A__paste.openstack.or g_show_747402_&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_aS85 wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=d-lYsZ5PXoRxgXFJCW dj84tcen9JqEuTvztcEEujT3I&e=
I've submitted a test PS to see if it would pass standard voting gates: https://urldefense.proofpoint.com/v2/url?u=https-3A__review.openstack. org_-23_c_641656_&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_a S85wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=hSiLdJ1M9Hv0zlQ pbKg2gogxOuiMZ6jVdmRQaTdp1D0&e= The non-voting `openstack-helm-infra-airship-divingbell` gate should still fail, as it would use old `merge`.
Waiting for gates to complete the work.
-- Roman Gorshunov
On Thu, Mar 7, 2019 at 1:55 PM Roman Gorshunov <paye600@gmail.com> wrote:
Just to confirm, were you using Helm 1.12.x when you tried replacing `merge` with `mergOverwrite`? I used helm 2.12.3.I confirm that mergeOverwrite does not work.
Link with test: https://urldefense.proofpoint.com/v2/url?u=http-3A__paste.openstack. org_show_747400_&d=DwIBaQ&c=LFYZ-o9_HUMeMTSQicvjIg&r=aimn2OylFgog_5_ aS85wtQ&m=C3ro_V1fT7fdpF_d9veBkPBnYfsWSeCOKV8ajDB3C-8&s=AwTIrAw_OBoL 4c98ESbnyVIQzkeFPy5YFi9CFL8Ewak&e= Details: - line 4: helm client version v2.12.3 (this version is used in openstack-helm-infra by default) - line 16: latest openstack-helm-infra (commit id e836707ad070e910d6130eced066bc0e50481d2b) - line 30: `helm lint` of helm-toolkit - line 33: ./templates/utils/_daemonset_overrides.tpl uses `merge` - line 40: replace `merge` with `mergeOverwrite` - line 41: confirm replacement - line 50: linting fails with error `function "mergeOverwrite" not defined`
Thank you for your help, Craig.
-- Roman Gorshunov
participants (3)
-
ANDERSON, CRAIG
-
Roman Gorshunov
-
TANG, ROY