<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Wed, Oct 25, 2017 at 12:23 AM James E. Blair <<a href="mailto:corvus@inaugust.com" target="_blank">corvus@inaugust.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
A number of issues related to how jobs are defined and run on projects<br>
with stable branches have come up recently. I believe they are all<br>
related, and they, as well as the solutions to them, must be considered<br>
together.<br>
<br>
<br>
The Problems<br>
============<br>
<br>
I've identified the following five problems:<br>
<br>
1) Parents should have variants applied<br>
<br>
Currently inheritance and variance are distinct. Inheritance modifies a<br>
job's configuration statically at the time the configuration is loaded.<br>
Variants are applied dynamically right before the job runs.<br>
<br>
That means that when a job starts, we pick a single job to start with,<br>
then apply variants of that particular job. But since the inheritance<br>
has already been applied, any variants which a user may expect to apply<br>
to a parent job will not be applied. When the inheritance chain is<br>
created at configuration time, only the "reference" definition of the<br>
job is used -- that is, the first appearance of the job.<br>
<br>
In other words, more than likely, most jobs defined in a stable branch<br>
are going to inherit from a parent job defined on the master branch --<br>
even if that parent job has a stable branch variant.<br>
<br>
2) Variants may cause parents to be ignored<br>
<br>
We currently ignore the parent attribute on a job variant. If we did<br>
not, then when the variant is applied, it would pull in all of the<br>
attributes of its parent, which is likely to be on the master branch<br>
(since its parent will, as in #1, be the reference definition).<br>
<br>
This ignoring of the parent attribute happens at configuration time<br>
(naturally, since that is when inheritance is applied).<br>
<br>
This means that if the first job that matches a change is a variant<br>
(i.e., the reference definition of a job has a non-matching branch<br>
matcher), this top-level variant job will not actually have a parent.<br>
<br>
3) Variants may add duplicate pre/post playbooks<br>
<br>
Currently, the master branch does not have an implied branch matcher, so<br>
jobs that exist on master and stable branches will generally be derived<br>
from both.<br>
<br>
If such a job adds a pre or post playbook, the job that is ultimately<br>
created and run for a change on the stable branch may have those added<br>
by both the variant defined on the master branch as well as that defined<br>
on the stable branch (since pre and post playbooks are cumulative).<br>
<br>
4) Variants on branches without explicit playbooks should use branch<br>
playbooks<br>
<br>
Whenever a job includes a pre-run, run (including the implicit run), or<br>
post-run playbook, Zuul remembers where and uses that branch of that<br>
repo to run that playbook. If a job were constructed from the master<br>
branch, and then had a stable branch variant applied but did not repeat<br>
the pre-run, run, or post-run attributes from the master, then Zuul<br>
would end up attempting to run the playbook from the master branch<br>
rather than the stable.<br>
<br>
5) The master branch should have implied branch matchers<br>
<br>
Currently jobs defined in an untrusted project on any branch other than<br>
'master' have an implicit branch matcher applied to them. This is what<br>
allows the version of a job in a stable branch to only affect the stable<br>
branch. The fact that there is no implicit branch matcher applied to<br>
the master branch is what allows jobs defined in zuul-jobs to run on<br>
changes to any branch.<br>
<br>
However, this also means that jobs on stable branches are frequently<br>
built from variants on both the master and stable branch. This may work<br>
for a short while, but will fail as soon as someone wants to add<br>
something to the master branch which should not exist on the stable<br>
branch (e.g., enabling a new service by default).<br>
<br>
<br>
The Design Considerations<br>
=========================<br>
<br>
In looking at these, I believe they have come about because of two<br>
design goals which we did not appreciate were in moderate tension with<br>
each other:<br>
<br>
A) In-repo configuration for a project should apply to that branch.<br>
Changing the behavior of a job on a stable branch should merely involve<br>
changing the configuration or playbook in that stable branch. When a<br>
project branches master to create a new stable branch, both branches<br>
should initially have the same content and behavior, but then evolve<br>
independently.<br>
<br>
B) We should be able to define jobs not only in a central repository<br>
such as project-config, but a central *untrusted* repository such as<br>
zuul-jobs or openstack-zuul-jobs.<br>
<br>
I think these are valid and important to keep in mind as we consider<br>
solutions.<br>
<br>
<br>
The Changes<br>
===========<br>
<br>
I believe the following changes will address all five problems and<br>
achieve both design goals:<br>
<br>
a) Apply inheritance at the same time as variance<br>
<br>
Rather than applying inheritance at configuration time, apply it at the<br>
time the job is frozen before being run. We can perform a depth-first<br>
traversal up the hierarchy of parents, applying all of the matching<br>
variants at each level as we return down. With the following graph<br>
(where lines indicate parent relationships):<br>
<br>
0 base<br>
/ \<br>
1 devstack 2 devstack 4 altbase<br>
\ / |<br>
3 tempest 5 tempest<br>
/ \<br>
6 foo 7 foo<br>
<br>
Would have the following jobs applied in order:<br>
<br>
0 base, 1 devstack, 2 devstack, 3 tempest, 4 altbase,<br>
5 tempest, 6 foo, 7 foo<br></blockquote><div><br></div></div><div dir="ltr"><div class="gmail_quote"><div>We will need somewhere in the logs a description of the traversal that was</div><div>done to build the final job. I believe that would help debugging issues that</div><div>may arise from unexpected inheritance behaviour.</div></div></div><div dir="ltr"><div class="gmail_quote"><div><br></div><div>Andrea Frittoli (andreaf)</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
b) Add an implicit branch matcher to the master branch<br>
<br>
Generally this will add clarity to projects with multiple branches,<br>
however, if we always add the an implicit branch matcher, then it makes<br>
it difficult to use repos like zuul-jobs to define jobs that run<br>
everywhere. So do this only if the project has multiple branches. If<br>
the project only has a single branch, omit the implicit branch matcher.<br>
<br>
c) Add a config option to disable the implicit branch matcher<br>
<br>
There are some times when an implicit branch matcher on master may be<br>
undesirable. For example when tempest was becoming branchless, it had<br>
multiple branches, but we would have wanted the jobs defined on master<br>
to be applicable everywhere. Or if, for some reason, we wanted to<br>
create a feature branch on zuul-jobs. For these cases, it's necessary<br>
to have an option to disable the implicit branch matcher. We can add a<br>
new kind of configuration object to zuul.yaml, for example:<br>
<br>
- meta:<br>
implicit-branch-matcher: False<br>
<br>
Which would be intended only to apply to the current branch. Or we<br>
could add an option to the tenant config file, so the admin can indicate<br>
that a certain project should not have an implicit branch matcher<br>
applied to certain branches.<br>
<br>
<br>
The Solutions<br>
=============<br>
<br>
Reconsidering the problems above with those three changes:<br>
<br>
1) Parents should have variants applied<br>
<br>
Change (a) is a straightforward solution to this problem.<br>
<br>
Note that simply applying change (a) to our current configuration is<br>
likely to, in some cases, bring in new parents on the master branch<br>
which are not intended to apply to stable branches, and so change (a) is<br>
best made with change (b) at the same time to reconcile this.<br>
<br>
2) Variants may cause parents to be ignored<br>
<br>
Change (a) means that it is safe to honor the parent attribute in all<br>
variants. Typically, all variants will have the same parent, and the<br>
resulting job application will be straightforward. In the example in<br>
change (a), that would mean base, devstack, tempest, foo. However, if a<br>
variant did have a different parent, (e.g., tempest -> altbase in the<br>
example), that case is handled as well.<br>
<br>
In all cases, we will either be honoring the parent specified by the<br>
user, or defaulting to the tenant-configured default base job.<br>
<br>
3) Variants may add duplicate pre/post playbooks<br>
<br>
Change (b) means that variants on the master branch will no longer (by<br>
default) apply to variants on stable branches. This is the most likely<br>
route for duplicate pre/post playbooks to occur. They can still occur<br>
if a user explicitly adds them in two matching variants, however, this<br>
will no longer automatically happen when a project creates a new branch,<br>
which is the main concern.<br>
<br>
4) Variants on branches without explicit playbooks should use branch<br>
playbooks<br>
<br>
The main reason for a variant not to repeat the playbook is in an<br>
attempt to solve problem (3). With that solved by (b), there should be<br>
little reason not to have explicit playbook attributes set on the job in<br>
all branches. By doing so, Zuul will use the appropriate<br>
branch-specific playbook.<br>
<br>
5) The master branch should have implied branch matchers<br>
<br>
Changes (b) and (c) are straightforward solutions to this problem.<br>
<br>
<br>
The Bonus Change<br>
================<br>
<br>
(d) Remove the implicit run playbook<br>
<br>
This is not required to solve any of the problems stated, however, it<br>
does make the solution to problem (4) even more explicit.<br>
<br>
Moreover, despite being an initial proponent of the implicit playbook, I<br>
have found that in practice, we have so many jobs that do not have<br>
playbooks at all (i.e., we're making heavy use of inheritance and<br>
variance) that it is becoming difficult to determine where to look for a<br>
job's run playbook. Declaring the run playbook explicitly will help<br>
with discoverability.<br>
<br>
<br>
The Proposal<br>
============<br>
<br>
I have been experimenting with late-binding inheritance (i.e., the<br>
change described as (a)) in [1]. Change (b) was also required[2] in<br>
order to reconcile all of the tests (as described in the solution to<br>
problem (1)).<br>
<br>
I have worked through these changes enough to feel comfortable that it<br>
is a workable solution that addresses the issues without bringing new<br>
problems to the table.<br>
<br>
I think we should implement changes a-d as soon as possible -- ideally<br>
before additional use of in-repo stable branch jobs cause more issues in<br>
production. At this stage, I don't think we need to make any allowances<br>
for backwards compatibility -- the way jobs are constructed and being<br>
ported into projects and across branches is largely compatible with<br>
these changes already. We would just be catching the implementation up<br>
to the implicit understanding.<br>
<br>
We do need a change to the infra-manual Zuul v3 migration page to<br>
indicate that not only job definitions, but playbooks and roles should<br>
appear in all branches of in-repo configuration.<br>
<br>
I hope I have articulated the problems sufficiently to facilitate<br>
discussion. I'm happy to discuss them more in depth, either via email<br>
or IRC. Of course, these may not be the only solutions to these<br>
problems. If you see other possible solutions, I'm happy to discuss<br>
them as well.<br>
<br>
Thanks,<br>
<br>
Jim<br>
<br>
[1] <a href="https://review.openstack.org/511352" rel="noreferrer" target="_blank">https://review.openstack.org/511352</a><br>
[2] <a href="https://review.openstack.org/514459" rel="noreferrer" target="_blank">https://review.openstack.org/514459</a><br>
<br>
_______________________________________________<br>
OpenStack-Infra mailing list<br>
<a href="mailto:OpenStack-Infra@lists.openstack.org" target="_blank">OpenStack-Infra@lists.openstack.org</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-infra" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-infra</a></blockquote></div></div></div>