<div dir="ltr"><div><div><div><div>Bogdan,<br><br></div>I don't want to maintain catalog resources 10 (or 20) times. It's an incredible amount of work to upkeep. There has to be a better way to ensure we don't lose some things. The approach I had in mind would have covered these cases:<br></div>1 - track all hiera lookups and make sure we catch new/lost lookups<br></div>2 - ensure all classes called from top level tasks are passed these values from hiera<br><br></div>We lost in this case a hiera lookup. Such a test would cover this, rather than comparing resulting puppet resources.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 29, 2015 at 5:39 PM, Bogdan Dobrelya <span dir="ltr"><<a href="mailto:bdobrelia@mirantis.com" target="_blank">bdobrelia@mirantis.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 29.10.2015 15:24, Bogdan Dobrelya wrote:<br>
> Hello.<br>
> There are few types of a deployment regressions possible. When changing<br>
> a module version to be used from upstream (or internal module repo), for<br>
> example from Liberty to Mitaka. Or when changing the composition layer<br>
> (modular tasks in Fuel). Specifically, adding/removing/changing classes<br>
> and a class parameters.<br>
><br>
> An example regression for swift deployment data [0]. Something was<br>
> changed unnoticed by existing noop tests and as a result<br>
> the swift data became being stored in root partition.<br>
><br>
> Suggested per-commit based regressions detection [1] for deployment data<br>
> assumes to automatically detect if a class in a noop catalog run has<br>
> gained or lost a parameter or if it has been updated to another value by<br>
> a patch under test. Later, this check could even replace existing noop<br>
> tests, everything will be checked automatically, unless every deployment<br>
> scenario is covered by a corresponding template, which are represented<br>
> as YAML files [2] in Fuel.<br>
> Note: The tool [3] can help to get all deployment cases (-Y) and all<br>
> deployment tasks (-S) as well.<br>
><br>
> I propose to review the patch [1], understand how it works (see tl;dr<br>
> section below) and to start using it ASAP. The earlier we commit the<br>
> "initial" data layer state, less regressions would pop up.<br>
><br>
> (tl;dr)<br>
> The check should be done for every modular component (aka deployment<br>
> task). Data generated in the noop catalog run for all classes and<br>
> defines of a given deployment task should be verified against its<br>
> "acknowledged" (committed) state.<br>
> And fail the test gate, if changes has been found, like new parameter<br>
> with a defined value, removed a parameter, changed a parameter's value.<br>
><br>
> In order to remove a regression, a patch author will have to add (and<br>
> reviewers should acknowledge) detected changes in the committed state of<br>
> the deployment data. This may be done manually, with a tool like [3] or<br>
> by a pre-commit hook, or even at the CI side!<br>
> The regression check should show the diff between committed state and a<br>
> new state proposed in a patch. Changed state should be *reviewed* and<br>
> accepted with a patch, to became a committed one. So the deployment data<br>
> will evolve with *only* approved changes. And those changes would be<br>
> very easy to be discovered for each patch under review process!<br>
> No more regressions, everyone happy.<br>
><br>
> Examples:<br>
><br>
> - A. A patch author removed the mpm_module parameter from the<br>
> composition layer (apache modular task). The test should fail with a<br>
><br>
> Diff:<br>
>       @@ -90,7 +90,7 @@<br>
>          manage_user            => 'true',<br>
>          max_keepalive_requests => '100',<br>
>          mod_dir                => '/etc/httpd/conf.d',<br>
>       -  mpm_module             => 'false',<br>
>       +  mpm_module             => 'prefork',<br>
>          name                   => 'Apache',<br>
>          package_ensure         => 'installed',<br>
>          ports_file             => '/etc/httpd/conf/ports.conf',<br>
><br>
> It illustrates that the mpm_module's committed value was "false".<br>
> But the new one came as the 'prefork', likely from the apache class<br>
> defaults.<br>
> The solution:<br>
> Follow the failed build link and see for detected changes (a diff).<br>
> Acknowledge the changes and include rebuilt templates in the patch as a<br>
> new revision. The tool [3] (use -h for help) example command:<br>
> ./utils/jenkins/fuel_noop_tests.rb -q -b -s api-proxy/api-proxy_spec.rb<br>
><br>
> Or edit the committed templates manually and include data changes in the<br>
> patch as well.<br>
><br>
> -B. An upstream module author added the new parameter mpm_mode with a<br>
> default '123'. The test should fail with a<br>
><br>
> Diff:<br>
>        @@ -90,6 +90,7 @@<br>
>           manage_user            => 'true',<br>
>           max_keepalive_requests => '100',<br>
>           mod_dir                => '/etc/httpd/conf.d',<br>
>        +  mpm_mode               => '123',<br>
>           mpm_module             => 'false',<br>
>           name                   => 'Apache',<br>
>           package_ensure         => 'installed',<br>
><br>
> It illustrates that the composition layer is not consistent with the<br>
> upstream module data schema, and that could be a potential regression in<br>
> deployment (new parameter added upstream and goes with defaults, being<br>
> ignored by the composition manifest).<br>
> The solution is the same as for the case A.<br>
><br>
> [0] <a href="https://bugs.launchpad.net/fuel/+bug/1508482" rel="noreferrer" target="_blank">https://bugs.launchpad.net/fuel/+bug/1508482</a><br>
> [1] <a href="https://review.openstack.org/240015" rel="noreferrer" target="_blank">https://review.openstack.org/240015</a><br>
<br>
</div></div>Please use the 6th revision to catch the idea. Next revisions are filled<br>
with tons of auto-generated templates representing committed state of<br>
the deployment data plane. This is still WIP...<br>
<br>
We are thinking on should we check the data state for *all* classes and<br>
defines of each deployment task, or should we do this only for some of<br>
the classes. The latter one would drastically decrease the amount of<br>
auto-generated data templates. But the former one would allow to catch<br>
more regressions.<br>
<div class="HOEnZb"><div class="h5"><br>
> [2]<br>
> <a href="https://github.com/openstack/fuel-library/tree/master/tests/noop/astute.yaml" rel="noreferrer" target="_blank">https://github.com/openstack/fuel-library/tree/master/tests/noop/astute.yaml</a><br>
> [3]<br>
> <a href="https://review.openstack.org/#/c/240015/7/utils/jenkins/fuel_noop_tests.rb" rel="noreferrer" target="_blank">https://review.openstack.org/#/c/240015/7/utils/jenkins/fuel_noop_tests.rb</a><br>
><br>
<br>
<br>
--<br>
Best regards,<br>
Bogdan Dobrelya,<br>
Irc #bogdando<br>
<br>
__________________________________________________________________________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</div></div></blockquote></div><br></div>