Open Stack

Excerpts from James Slagle's message of 2014-11-28 11:27:20 -0800:
> On Thu, Nov 27, 2014 at 1:29 PM, Sullivan, Jon Paul
> <JonPaul.Sullivan at hp.com> wrote:
> >> -----Original Message-----
> >> From: Ben Nemec [mailto:openstack at nemebean.com]
> >> Sent: 26 November 2014 17:03
> >> To: OpenStack Development Mailing List (not for usage questions)
> >> Subject: Re: [openstack-dev] [diskimage-builder] Tracing levels for
> >> scripts (119023)
> >>
> >> On 11/25/2014 10:58 PM, Ian Wienand wrote:
> >> > Hi,
> >> >
> >> > My change [1] to enable a consistent tracing mechanism for the many
> >> > scripts diskimage-builder runs during its build seems to have hit a
> >> > stalemate.
> >> >
> >> > I hope we can agree that the current situation is not good.  When
> >> > trying to develop with diskimage-builder, I find myself constantly
> >> > going and fiddling with "set -x" in various scripts, requiring me
> >> > re-running things needlessly as I try and trace what's happening.
> >> > Conversley some scripts set -x all the time and give output when you
> >> > don't want it.
> >> >
> >> > Now nodepool is using d-i-b more, it would be even nicer to have
> >> > consistency in the tracing so relevant info is captured in the image
> >> > build logs.
> >> >
> >> > The crux of the issue seems to be some disagreement between reviewers
> >> > over having a single "trace everything" flag or a more fine-grained
> >> > approach, as currently implemented after it was asked for in reviews.
> >> >
> >> > I must be honest, I feel a bit silly calling out essentially a
> >> > four-line patch here.
> >>
> >> My objections are documented in the review, but basically boil down to
> >> the fact that it's not a four line patch, it's a 500+ line patch that
> >> does essentially the same thing as:
> >>
> >> set +e
> >> set -x
> >> export SHELLOPTS
> >
> > I don't think this is true, as there are many more things in SHELLOPTS than just xtrace.  I think it is wrong to call the two approaches equivalent.
> >
> >>
> >> in disk-image-create.  You do lose set -e in disk-image-create itself on
> >> debug runs because that's not something we can safely propagate,
> >> although we could work around that by unsetting it before calling hooks.
> >>  FWIW I've used this method locally and it worked fine.
> >
> > So this does say that your alternative implementation has a difference from the proposed one.  And that the difference has a negative impact.
> >
> >>
> >> The only drawback is it doesn't allow the granularity of an if block in
> >> every script, but I don't personally see that as a particularly useful
> >> feature anyway.  I would like to hear from someone who requested that
> >> functionality as to what their use case is and how they would define the
> >> different debug levels before we merge an intrusive patch that would
> >> need to be added to every single new script in dib or tripleo going
> >> forward.
> >
> > So currently we have boilerplate to be added to all new elements, and that boilerplate is:
> >
> >     set -eux
> >     set -o pipefail
> >
> > This patch would change that boilerplate to:
> >
> >     if [ ${DIB_DEBUG_TRACE:-0} -gt 0 ]; then
> >         set -x
> >     fi
> >     set -eu
> >     set -o pipefail
> >
> > So it's adding 3 lines.  It doesn't seem onerous, especially as most people creating a new element will either copy an existing one or copy/paste the header anyway.
> >
> > I think that giving control over what is effectively debug or non-debug output is a desirable feature.
> 
> I don't think it's debug vs non-debug. I think script writers that
> have explicitly used set -x previously have then operated under the
> assumption that they don't need to add any useful logging since it's
> running -x. In that case, this patch is actually harmful.
> 

I believe James has hit the nail squarely on the head with the paragraph
above.

I propose a way forward for this:

1) Conform all o-r-c scripts to the logging standards we have in
OpenStack, or write new standards for diskimage-builder and conform
them to those standards. Abolish non-conditional xtrace in any script
conforming to the standards.

2) Once that is done, implement optional -x. I rather prefer the explicit
conditional set -x implementation over SHELLOPTS. As somebody else
pointed out, it feels like asking for unintended side-effects. But the
"how" is far less important than the "what" in this case, which step 1
will better define.

Anyone else have a better plan?