Open Stack

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.

>
> We have a patch that implements that desirable feature.
>
> I don't see a compelling technical reason to reject that patch.

I'm not specifically -2 on this patch based on the implementation.
It's more of the fact that I don't think this patch addresses the
problem in a meaningful way. The problem seems to be that dib either
logs too much or not enough information.

Also, it's a change to the current behavior that could be unexpected.
diskimage-builder has rather poor logging as-is. We don't use echo's
enough to actually say what's going on. Most script writers have just
relied on set -x to log everything explicitly, so there's no need to
echo or log any useful info. This patch turns off all tracing unless
specifically requested via $DIB_DEBUG_TRACE. Also, not all hook
scripts *have* to be bash. Do we have some that are python (i don't
honestly recall)? If so, do those honor $DIB_DEBUG_TRACE in a way that
makes sense? Do we need policy to enforce that?

The first thing we're going to do in our *own* tripleo CI if this
patch lands is set DIB_DEBUG_TRACE=1. Why? Because otherwise the
logging from dib is useless. Likewise on most dib bug reports I see
our first response being "please rerun with DIB_DEBUG_TRACE=1". We
discuss a lot about OpenStack service logs not being useful when
debug=0, yet this patch is about to apply the same problem to dib
unfortunately.

 James Slagle
--