cloud-init-dev team mailing list archive

[Wesley Wiedenmeier <wesley.wiedenmeier@xxxxxxxxx>]

> >>  * where is the upstream bug that prevents 2.2 from working
> >>    correctly?
> 
> > It was at https://github.com/lxc/pylxd/issues/209, but the fix has
> 
> Thanks, please mention it in the code.

Sure, I'll add a comment in there.

> >>  * i think we can live without 'alias: t'.
> 
> > The 'alias' key is used to reference the image, on the ubuntu daily
> > sstream server the aliases I found were p, t, x, y, z. It's now been
> switched
> > over to ubuntu/trusty/default with the switch to images from
> > images.linuxcontainers.org.
> 
> The ubuntu/daily streams also have other more explicit alias:
>   lxc launch ubuntu-daily:xenial
>   lxc launch ubuntu-daily:16.04

Oh, nice. I'll switch to using those.

> >>  * apparently we should be using dnf rather than yum in any recent
> >>    RH distro
> 
> > Oh, I didn't know that dnf was officially supported outside Fedora.
> > It shouldn't take much to switch that back over, just gotta update
> > the setup image functions.
> 
> I only know because cloud-init had a MP from a RH developer asking
> for it (see commit log)
>  https://bugs.launchpad.net/cloud-init/+bug/1647118

It may be good to keep both the yum path in place and add a config
directive to tell the platform which to use, just in case things
switch around more.

> >>  * system_ready_script can be simplified by being a command,
> >>    passed directly to 'execute'.
> >>      system_ready_script: ['test', '-e',
> >>                            '/run/cloud-init/result.json']
> >>    that saves writing the file and executing it on the other
> >>    side, and also then assuming 'bash'.
> 
> >That makes sense, although it does limit how complex the scripts can
> >get. Its probably best to keep them simple though.
> 
> If we needed, we could allow the config to provide stdin also, but with
> the above, the only limit on complexity that i can see is the command
> line length limit, which shouldn't be too big of a problem
>   http://stackoverflow.com/questions/6846263/maximum-length-of-command-line-
> argument-that-can-be-passed-to-sqlplus-from-lin
> 
> xargs --show-limits:
>   POSIX upper limit on argument length (this system): 2091520
> 
> :)

Haha yeah, we're definitely not in danger of hitting that limit.

> >>  * "Pass $HOME through in tox citest envs for lxc client use"
> >>    message doesn't make sense. where are we using the lxc client? just
> >>    for developer debug ?
> 
> > It is used for
> 
> Sentance fragment ^ ?

Whoops, I must have forgotten to finish that. It is used for exporting 
lxd images that need to have their metadata modified to write the
cloud seed into the target on. Pylxd does have an export function for
images, but it does the export as a tmpfile with handle, not as a split
tarball in the filesystem like we need. The lxc client handles that
nicely, but it needs $HOME set so it can locate its config file.

> >>  * "Disable centos66 test"
> >>    why ?
> 
> > Its such an old release that pretty much nothing is going to be
> > backported there, testing on it doesn't really make sense IMO. I
> > think the one time I tried running centos 6 tests they failed, but I
> > don't recall why. I can get tests going there in the future if
> > they're needed.
> 
> Well, if they were working, then it makes sense to run it, as a stable
> platform likely means regression/change-of-behavior are the fault of
> cloud-init, not the OS.

I'll go ahead and re-enable that and see if it can work smoothly, cause
it could be useful to catch distro issues that break cloud-init early
to save time with bug triage.

> >> Easily droppable commits / things that can be done separartely:
> >> - "Use LOG.warning instead of deprectated LOG.warn"
> >>    why? This is just not necessary in this merge proposal.
> >> - "Change behavior of upgrade in setup_image"
> 
> >The old behavior of update was a bug, it was fixed before the
> >original merge, but didn't make it in. Tests that pull from repos
> >are very slow without the new behavior, and are unreliable
> >if -proposed is enabled.
> 
> Thats fine... but its very easily stand alone, lets just get the rest
> in and then taht easy change makes some sense.

It would be possible to split this whole merge into 4 pieces:
 - pylxd compatibility (independant)
 - bddeb and tree_run functionality (independant)

 - setup_image/collect refactor and bug fixes
 - platform cleanup and additional distro support

I can put together merge proposals for all of that separately. The
pylxd compat and bddeb parts are independant of everything else, but
the setup_image/collect refactor and platform cleanup do depend on each
other, so it is probably best to keep them together.

There's already a merge proposal that I can update for the bddeb part of
this. I can then do the improvements we discussed to the bddeb process
in that merge proposal, since none of the other stuff is blocked on that.

The pylxd compatibility fix can be pulled into a separate mp as well,
leaving this mp for just the setup_image and platform cleanup. It
would require quite a bit of work to decouple the setup_image/collect
cleanup from the platform cleanup, since some functions changed
signature and some of the platform improvements use setup_image.upgrade.

> >The LOG.warning/warn switch was just to silence a deprecation
> >warning. It isn't really essential. I could rebase and pull some of
> >the little commits together into one general purpose cleanup commit
> >to clean up the log. Some of this definitely could have been done
> >in other merges, I'll do it like that in the future.
> 
> Same as above, i was just listing things that very easily make this
> merge proposal smaller.  We can do the LOG.warning thing for sure,
> and probably should do it throughout cloud-init.

I can pull that out from this merge and make a new branch to do that
across all of cloud-init. Just grepping through I can see a few
places that are still using LOG.warning in cloud-init.

> >>  * creating the tarball..
> >>    I think what we want to do here is:
> >>      a.) make-tarball on "this side"
> >>      b.) modify bddeb to be able to just create a debian/ dir
> >>          either
> >>          i.) have it create the orig tarball and a debian.tar.gz
> >>              into a provided (empty) output dir.
> >>          ii.) have it just create the debian.tar.gz and we create
> >>              the tarball example of doing that
> >>              http://paste.ubuntu.com/23870964/
> >>      c.) move the tarball and debian dir over, extract tarball,
> >>         extract debian/
> >>      d.) mk-build-deps --install .... debian/control
> >>      e.) dpkg-buildpackage
> >>
> >>    doing this should mean we do  not have to 'install additional
> >>    build deps' but rather just have mk-build-deps do the right
> >>    thing.
> 
> >I had been trying to avoid modifying anything outside cloud_tests,
> >but adding the --debian-tar-only option in bddeb definitely does
> >simplify this. This may be a useful feature in general for debugging
> >cloud-init builds that do not work or doing custom deb builds, as
> >going through the template system can be inconvenient.
> 
> >This also avoids having to go through a source repo in case the
> >current version of cloud-init in tree has different deps from the
> >one in the source repo.
> 
> > Using dpkg-buildpackage instead of debuild may be quicker too.
> 
> Yeah, it makes the build much simpler in the container.  I'm tempted
> to further improve bddeb to list what packages need installing even,
> then we could drop the mk-build-deps and equivs ... but that is not
> necessary now.  The complexity in that comes really from the obnoxious
>  'python3-pyflakes | pyflakes (<< 1.1.0-2)'
> 
> in package/debian/control.in

Yeah, I kinda like the idea of bddeb to handle build deps entirely. That may
make things simpler for users who want to build as well since there have been
lots of questions on irc about how to build packages. We should probably add
more documentation on cloud-inits build system at some point as well.

> >>  * when building with dpkg-buildpackage or debuild, good to
> >>    allow setting DEB_BUILD_OPTIONS=nocheck (which will not run pep8 and
> >>    nosetests)
> 
> >Nice, I didn't know that was an option. Lets us avoid the additional
> >build deps step easily, since that was mostly to avoid failing because
> >testing stuff wasn't there.
> 
> There is info at https://bugs.launchpad.net/cloud-init/+bug/1652329
> on what the plan here is... ultimately, package build wont run pep8, but
> would still run nosetests and useful to be able to not run them.

I do like the idea of dropping pep8 in the build, since the actual source used
for the builds whill have already been checked.

> >>  * i think the above path means we do not depend on deb-src lines.
> 
> >The switch back to ubuntu daily will handle that, but this also
> >avoids going through source repo at all which can be nice if a local
> >change adds a new build dep.
> 
> And simply use of mk-build-deps would too..
> I know i'm kind of over-doing this, but i really like a "build a deb"
> thing that just can be easily used, and very ideally could even
> re-use an existing container, so i dont have to go through the apt-get update
> stuff there...  so ultimately, i'd like something that just checks deps
> and builds and installs packages (and apt-get update) if needed.

I definitely like 'build a deb' as well, just to simplify the current build
system. I think it makes sense to pull the bddeb part of this merge into a
separate branch and continue to develop it further as well. It may be good once
kvm platform is supported to add an option to build there as well for users
building not on a system with lxd.
-- 
https://code.launchpad.net/~wesley-wiedenmeier/cloud-init/+git/cloud-init/+merge/308218
Your team cloud init development team is requested to review the proposed merge of ~wesley-wiedenmeier/cloud-init:integration-testing into cloud-init:master.