On Thu, Nov 27, 2014 at 1:29 PM, Sullivan, Jon Paul <[email protected]> wrote: >> -----Original Message----- >> From: Ben Nemec [mailto:[email protected]] >> 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 -- _______________________________________________ OpenStack-dev mailing list [email protected] http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
