Merge lp://qastaging/~doanac/uci-engine/1-run-worker-no-jenkins into lp://qastaging/uci-engine

FAILED: Continuous integration, rev:451
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/536/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/536/rebuild

PASSED: Continuous integration, rev:452
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/540/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/540/rebuild

8 + user = config.get('uid', 'nobody')
9 + group = config.get('gid', 'nogroup')

There's no uid or gid parameter in config.yaml, so this will always return nobody/nogroup. Could I suggest that you set the nobody/nogroup defaults in config.yaml instead of on the get() method? This way when someone is looking at config.yaml or `juju get` they'll know the default behaviour of the charm.

10 + core.host.mkdir('/var/log/ci-tickets', user, group, 0755)

Can we put this under /srv/${charm_name_without_unit}/logs/ instead? I had a pair of admittedly-unfinished MPs¹ a while back that tried to bring consistency to where we place code, config, logs, and variable data.

This follows IS policy for prodstack deployments²:

"Your application should create a /srv/${external-service-name}/${instance-type}/${service-name} directory for the code itself, and /srv/${external-service-name}/{${instance-type}-logs,scripts,etc,var} as needed."³

It also gives us quicker disaster operations, as everything our code relies on and produces lives in a consistent, centralised location.

I'm not suggesting we hijack your MP to get us 100% there, but I think slowly driving to it might be better than the touch-the-world approach I tried and failed at before. What do you think?

Thanks Andy

¹ lp:~ev/ubuntu-ci-services-itself/better-structure-and-logging and lp:~ev/ubuntu-ci-services-itself/better-structure-and-logging-part2
² https://wiki.canonical.com/InformationInfrastructure/WebOps/Juju/Prodstack#Writing_Charms_for_Prodstack_Deployment
³ instance-type would be staging or production

Yeah for finally having a brain that doesn't involve jenkins \o/

191 + while True:
192 + buff = proc.stdout.read(1)
193 + if buff:
194 + sys.stdout.write(buff)
195 + else:
196 + break

I have mixed feelings there, this could use readline() instead of read(1) for better performance and should catch EPIPE no ?

23 + ds_name = str(params['ticket_id']) + '-image'
639 + self._ticket_id = str(ticket_id)

Shouldn't we start to use only unicode internally in preparation for python 3 and why is this needed here ? What happens if the string can't be converted ?

I've made a few changes while reviewing trying to explain the rationale in each commit at lp:~vila/uci-engine/1-run-worker-no-jenkins , happy to discuss them further if needed.

On 04/25/2014 03:25 AM, Evan Dandrea wrote:
> Review: Needs Fixing
>
> 8 + user = config.get('uid', 'nobody')
> 9 + group = config.get('gid', 'nogroup')
>
> There's no uid or gid parameter in config.yaml, so this will always return nobody/nogroup. Could I suggest that you set the nobody/nogroup defaults in config.yaml instead of on the get() method? This way when someone is looking at config.yaml or `juju get` they'll know the default behaviour of the charm.

good catch

> 10 + core.host.mkdir('/var/log/ci-tickets', user, group, 0755)
>
> Can we put this under /srv/${charm_name_without_unit}/logs/ instead? I had a pair of admittedly-unfinished MPs¹ a while back that tried to bring consistency to where we place code, config, logs, and variable data.

That's perfectly sensible.

On 04/25/2014 04:57 AM, Vincent Ladeuil wrote:
> Review: Needs Fixing
>
> Yeah for finally having a brain that doesn't involve jenkins \o/
>
> 191 + while True:
> 192 + buff = proc.stdout.read(1)
> 193 + if buff:
> 194 + sys.stdout.write(buff)
> 195 + else:
> 196 + break
>
> I have mixed feelings there, this could use readline() instead of read(1) for better performance

The real solution is probably to have a select loop, but I didn't want
to go overboard. The problem with readline (in general) is that if you
have a program that does something like:

  sys.stdout.write('doing something long:')
  /lots of time
  sys.stdout.write(' PASS\n')

you block for a long time with not sense of progress. The publish only
writes one line of output, so this won't matter much.

 > and should catch EPIPE no ?

I think subprocess.py handles EPIPE for read operations - its just write
operations to the pid's STDIN you have to be careful on.

> 23 + ds_name = str(params['ticket_id']) + '-image'
> 639 + self._ticket_id = str(ticket_id)
>
> Shouldn't we start to use only unicode internally in preparation for python 3 and why is this needed here ? What happens if the string can't be converted ?

ticket_id is an integer there's no unicode conversion issues. Originally
this value came from jenkins as an environment variable (ie string). I
wanted to make sure we deal with ticket-id in code as its real data type
(an integer). So I think this is safe?

> I've made a few changes while reviewing trying to explain the rationale in each commit at lp:~vila/uci-engine/1-run-worker-no-jenkins , happy to discuss them further if needed.

=
http://bazaar.launchpad.net/~vila/uci-engine/1-run-worker-no-jenkins/revision/453

Now that I look at this - I don't think we need to adjust the sys-path.
We are launched from run_worker.py which already has this sorted out for
us. I think I'll remove this ugliness all together.

The other stuff looks fine. I'll rebase/patch them into my tree. I wish
BZR had a "git format-patch" function for things like this :/

ev: revno's 453 and 454 fix your issues

vila: revno's 455 and 456 should have the stuff you wanted

PASSED: Continuous integration, rev:456
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/544/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/544/rebuild

> On 04/25/2014 04:57 AM, Vincent Ladeuil wrote:
> > Review: Needs Fixing
> >
> > Yeah for finally having a brain that doesn't involve jenkins \o/
> >
> > 191 + while True:
> > 192 + buff = proc.stdout.read(1)
> > 193 + if buff:
> > 194 + sys.stdout.write(buff)
> > 195 + else:
> > 196 + break
> >
> > I have mixed feelings there, this could use readline() instead of read(1)
> for better performance
>
> The real solution is probably to have a select loop, but I didn't want
> to go overboard.

Urgh, yeah, select...

> The problem with readline (in general) is that if you
> have a program that does something like:
>
> sys.stdout.write('doing something long:')
> /lots of time
> sys.stdout.write(' PASS\n')
>
> you block for a long time with not sense of progress.

Well, if the sending process doesn't use flush() you won't get anything on
the receiving end either, so may be read() instead of read(1) ? I don't know
ppa_sync well enough to be sure, but it looks like it's using logging so the
case above won't happen no ?

> The publish only
> writes one line of output, so this won't matter much.
>
> > and should catch EPIPE no ?
>
> I think subprocess.py handles EPIPE for read operations - its just write
> operations to the pid's STDIN you have to be careful on.

Hmm, that doesn't match my memory, EINTR is filtered but if you don't
catch/report/reraise EPIPE, your function will just be killed by EPIPE IIRC.

May be you're handling that at a higher level (or lower in ppa_sync.py) but
I'm a bit worried that we miss some failures in the called process if the
EPIPE is just swallowed (if subprocess really does that).

>
> > 23 + ds_name = str(params['ticket_id']) + '-image'
> > 639 + self._ticket_id = str(ticket_id)
> >
> > Shouldn't we start to use only unicode internally in preparation for python
> 3 and why is this needed here ? What happens if the string can't be converted
> ?
>
> ticket_id is an integer there's no unicode conversion issues. Originally
> this value came from jenkins as an environment variable (ie string). I
> wanted to make sure we deal with ticket-id in code as its real data type
> (an integer). So I think this is safe?
>
> > I've made a few changes while reviewing trying to explain the rationale in
> each commit at lp:~vila/uci-engine/1-run-worker-no-jenkins , happy to discuss
> them further if needed.
>
>
> =
> http://bazaar.launchpad.net/~vila/uci-engine/1-run-worker-no-
> jenkins/revision/453
>
> Now that I look at this - I don't think we need to adjust the sys-path.
> We are launched from run_worker.py which already has this sorted out for
> us. I think I'll remove this ugliness all together.

Great.

>
> The other stuff looks fine. I'll rebase/patch them into my tree. I wish
> BZR had a "git format-patch" function for things like this :/

I don't get why you don't just use pull/merge here... it makes it harder to track which points you disagree about and don't discuss further.

Digging further about the use of __file__, there are still other sites that don't use abspath:

59 os.path.dirname(__file__), '../../unit_config')
107 + cmd = os.path.join(os.path.dirname(__file__), 'lander_service_wrapp...

On 04/28/2014 03:20 AM, Vincent Ladeuil wrote:
>> On 04/25/2014 04:57 AM, Vincent Ladeuil wrote:
>>> Review: Needs Fixing
>>>
>>> Yeah for finally having a brain that doesn't involve jenkins \o/
>>>
>>> 191 + while True:
>>> 192 + buff = proc.stdout.read(1)
>>> 193 + if buff:
>>> 194 + sys.stdout.write(buff)
>>> 195 + else:
>>> 196 + break
>>>
>>> I have mixed feelings there, this could use readline() instead of read(1)
>> for better performance
>>
>> The real solution is probably to have a select loop, but I didn't want
>> to go overboard.
>
> Urgh, yeah, select...
>
>> The problem with readline (in general) is that if you
>> have a program that does something like:
>>
>> sys.stdout.write('doing something long:')
>> /lots of time
>> sys.stdout.write(' PASS\n')
>>
>> you block for a long time with not sense of progress.
>
> Well, if the sending process doesn't use flush() you won't get anything on
> the receiving end either, so may be read() instead of read(1) ? I don't know
> ppa_sync well enough to be sure, but it looks like it's using logging so the
> case above won't happen no ?

seems like we've come full circle. I did this hack so we could stream
the output as it happened. however, in practice the only output is:

   "RC=0"

so how about we solve this by just going back to subprocess.check_output
and not have to sort this out.

> Digging further about the use of __file__, there are still other sites that don't use abspath:
>
> 59 os.path.dirname(__file__), '../../unit_config')
> 107 + cmd = os.path.join(os.path.dirname(__file__), 'lander_service_wrapper.py')
>
> 512 +sys.path.append(os.path.join(os.path.dirname(__file__), '../../bin'))
>
> Oh well, there are also other places in other components, may be that needs
> to be addressed once and for all in a different MP...

yeah - i don't think we should fix this here.

> On the other hand, lander/bin/lander_merge_parameters.py doesn't seem to be used anymore, time to remove it ?

also lander_archive.py. I thought I could delete these (along with their
test files) as a separate MP.

vila - revno 457 removes the subprocess crap and makes this easier to deal with

PASSED: Continuous integration, rev:457
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/551/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/551/rebuild

> seems like we've come full circle. I did this hack so we could stream
> the output as it happened. however, in practice the only output is:
>
> "RC=0"
>
> so how about we solve this by just going back to subprocess.check_output
> and not have to sort this out.

Hmpf, indeed ;)

> yeah - i don't think we should fix this here.

Ack

> also lander_archive.py. I thought I could delete these (along with their
> test files) as a separate MP.

Fair enough.

Attempt to merge into lp:uci-engine failed due to conflicts:

text conflict in lander/lander/run_worker.py

PASSED: Continuous integration, rev:458
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/565/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/565/rebuild