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