curtin

Merge lp:~raharper/curtin/trunk.vmtest-sync-only-once into lp:~curtin-dev/curtin/trunk

trunk.vmtest-sync-only-once
Merge into trunk

Proposed by Ryan Harper on 2017-01-23

Status:	Merged
Merged at revision:	443
Proposed branch:	lp:~raharper/curtin/trunk.vmtest-sync-only-once
Merge into:	lp:~curtin-dev/curtin/trunk
Diff against target:	240 lines (+72/-27) 5 files modified tests/vmtests/__init__.py (+28/-14) tests/vmtests/helpers.py (+27/-6) tests/vmtests/image_sync.py (+2/-1) tools/jenkins-runner (+3/-1) tools/vmtest-sync-images (+12/-5)
To merge this branch:	bzr merge lp:~raharper/curtin/trunk.vmtest-sync-only-once
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Server Team CI bot	continuous-integration		Approve on 2017-01-27
Scott Moser (community)		2017-01-23	Approve on 2017-01-27
Review via email: mp+315387@code.launchpad.net

Commit message

vmtest: overhaul image sync

Changes to support HWE and Centos images have exposed issues in the image
sync process. Our 'make sync-images' was using a simplestreams filter
that did not pickup any of the required HWE kernels which meant that
during vmtest runs we would trigger new image syncs. Compounding this
issue was the fact that the filter used was too-wide which picked up
things like di-initrd,di-kernel files.

Additionally for an empty IMAGE_DIR repository, there were two bugs; one
it assumed that it could load the vmtest.json/vmtest-centos.json files.
Second when the repo was empty, we did not return an iterable type.

The following changes have been made to achieve the goals:
  - make sync-images should download everything we need to run a complete
    vmtest run without acquiring any new files from our image server.
  - Handle empty repositories properly
  - Do not attempt to sync with the streamds on each test, but only if
    specific needed files are missing

Description of the change

vmtest: overhaul image sync

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-01-23:

PASSED: Continuous integration, rev:440
https://jenkins.ubuntu.com/server/job/curtin-ci/289/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-amd64/289
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/289
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-ppc64el/289
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-s390x/289

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/289/rebuild

review: Approve (continuous-integration)

Revision history for this message

Scott Moser (smoser) wrote on 2017-01-23:

image_sync.py:query
query returns a list of dictionaries that is not guaranteed
to be unique per 'ftype'. We'll take the one that sorts first.

maybe we should raise exception if its not unique ?

other comments inline.

Revision history for this message

Ryan Harper (raharper) wrote on 2017-01-23:

Download full text (8.7 KiB)

On Mon, Jan 23, 2017 at 1:18 PM, Scott Moser <email address hidden> wrote:

> image_sync.py:query
> query returns a list of dictionaries that is not guaranteed
> to be unique per 'ftype'. We'll take the one that sorts first.
>
> maybe we should raise exception if its not unique ?
>

Maybe? I think you may have authored that function; I'll defer to what
changes you think best there.

>
>
> other comments inline.
>
>
> Diff comments:
>
> > === modified file 'tests/vmtests/__init__.py'
> > --- tests/vmtests/__init__.py 2016-12-02 02:01:20 +0000
> > +++ tests/vmtests/__init__.py 2017-01-23 17:05:51 +0000
> > @@ -165,7 +165,7 @@
> > return
> >
> >
> > -def get_images(src_url, local_d, distro, release, arch, krel=None,
> sync=True,
> > +def get_images(src_url, local_d, distro, release, arch, krel=None,
> sync=False,
>
> i'd think we would not want to change the signature if we dont have to.
> can't the caller be changed just as easily?
>

Well, I think it got flipped when I merged the centos support; so I prefer
it to not sync-by default given that
the method is called get_images vs. the method above in the file which is
explictly, sync_images.

The callers of get_images use it more like (get_path_to_ftypes) which
happens to have an sync/fallback
method to go and get said requested items.

>
> Also can we change the comment / function comment here to say that it
> returns a dictionary of full paths to files.
>

ACK

>
> > ftypes=None):
> > # ensure that the image items (roottar, kernel, initrd)
> > # we need for release and arch are available in base_dir.
> > @@ -186,10 +186,12 @@
> > common_filters.append('krel=%s' % krel)
> > filters = ['ftype~(%s)' % ("|".join(ftypes.keys()))] +
> common_filters
> >
> > + # only sync if requested, allow env to override
>
> the comment doesnt make sense here, here we are not allowing the
> environment to override (the input parameter is used always)
>

I'll update; I did have the env variable there but changed to having the
callers pass in the environment value.

>
> unrelated to your change, but right around here, it'd be better if we did:
> elif isinstance(ftypes, (list, tuple)):
> - ftypes = dict().fromkeys(ftypes)
> + ftypes = dict().fromkeys(ftypes, '')
>

ACK

>
> > if sync:
> > + logger.info('Syncing images from %s with filters=%s', src_url,
> filters)
> > imagesync_mirror(output_d=local_d, source=src_url,
> > - mirror_filters=common_filters,
> > - max_items=IMAGES_TO_KEEP)
> > + mirror_filters=filters,
> > + max_items=IMAGES_TO_KEEP, verbosity=1)
> >
> > query_str = 'query = %s' % (' '.join(filters))
> > logger.debug('Query %s for image. %s', local_d, query_str)
>
> also unrelated to your change, but confusing...
> down belo here in the code we do:
> if not results and not sync:
> # try to fix this with a sync
>
> we do that even if user passed in sync=False. I think this can result in
> a case where we would sync iteratively.
>

Yes; that's true (and useful) but I think having that controll...

On Mon, Jan 23, 2017 at 1:18 PM, Scott Moser <smoser@ubuntu.com> wrote:

> image_sync.py:query
>   query returns a list of dictionaries that is not guaranteed
>   to be unique per 'ftype'.  We'll take the one that sorts first.
>
>   maybe we should raise exception if its not unique ?
>

Maybe?  I think you may have authored that function; I'll defer to what
changes you think best there.

>
>
> other comments inline.
>
>
> Diff comments:
>
> > === modified file 'tests/vmtests/__init__.py'
> > --- tests/vmtests/__init__.py 2016-12-02 02:01:20 +0000
> > +++ tests/vmtests/__init__.py 2017-01-23 17:05:51 +0000
> > @@ -165,7 +165,7 @@
> >      return
> >
> >
> > -def get_images(src_url, local_d, distro, release, arch, krel=None,
> sync=True,
> > +def get_images(src_url, local_d, distro, release, arch, krel=None,
> sync=False,
>
> i'd think we would not want to change the signature if we dont have to.
> can't the caller be changed just as easily?
>

The callers of get_images use it more like (get_path_to_ftypes) which
happens to have an sync/fallback
method to go and get said requested items.

>
> Also can we change the comment / function comment here to say that it
> returns a dictionary of full paths to files.
>

ACK

>
> >                 ftypes=None):
> >      # ensure that the image items (roottar, kernel, initrd)
> >      # we need for release and arch are available in base_dir.
> > @@ -186,10 +186,12 @@
> >          common_filters.append('krel=%s' % krel)
> >      filters = ['ftype~(%s)' % ("|".join(ftypes.keys()))] +
> common_filters
> >
> > +    # only sync if requested, allow env to override
>
> the comment doesnt make sense here, here we are not allowing the
> environment to  override (the input parameter is used always)
>

I'll update; I did have the env variable there but changed to having the
callers pass in the environment value.

>
> unrelated to your change, but right around here, it'd be better if we did:
>      elif isinstance(ftypes, (list, tuple)):
> -        ftypes = dict().fromkeys(ftypes)
> +        ftypes = dict().fromkeys(ftypes, '')
>

ACK

>
> >      if sync:
> > +        logger.info('Syncing images from %s with filters=%s', src_url,
> filters)
> >          imagesync_mirror(output_d=local_d, source=src_url,
> > -                         mirror_filters=common_filters,
> > -                         max_items=IMAGES_TO_KEEP)
> > +                         mirror_filters=filters,
> > +                         max_items=IMAGES_TO_KEEP, verbosity=1)
> >
> >      query_str = 'query = %s' % (' '.join(filters))
> >      logger.debug('Query %s for image. %s', local_d, query_str)
>
> also unrelated to your change, but confusing...
> down belo here in the code we do:
>     if not results and not sync:
>         # try to fix this with a sync
>
> we do that even if user passed in sync=False.  I think this can result in
> a case where we would sync iteratively.
>

Yes; that's true (and useful) but I think having that controlled via env is
best.

> maybe sync (and the env controlling it) needs to be a 3 way:
>   true: yes, sync always and iteratively
>
  false: never sync, raise exception on missing stuff
>   on_missing: sync if you dont have something
>

Let's iterate on this.  We have two paths that matter.

1) jenkins job which already does a 'make sync-images' which after this
patch gets everything we need
2) developer calling jenkins_runner or just nose.

For 1) I would like sync=never and raise Exception on missing;  we have a
bug in our helper function which was designed to calculate what we need
prior to running
For 2) I would like sync=on_missing

We already raise exception on missing, however it only checks for expected
types passed in; rather than the more exhaustive set of ftypes;  I think
that's fine

For jenkins-runner, I think it makes sense to export the
CURTIN_SYNC_IMAGES=False into the env, to force vmtest to never sync; if
we're missing something
we'll get the exception that's already in place.

For the developer, we're currently not syncing by default (but only on
missing); which is fine; but I think if we invert the default back to True
but modify the results stanza to be a bit more complicated;

if missing and CURTIN_SYNC_IMAGES:
   do_recursive_sync

Right now, the missing check happens too late; if we get *any* results we
skip syncing (in the recursive case where sync=True) which means we
never sync for the right ftypes.

> > @@ -344,19 +346,24 @@
> >
> >      @classmethod
> >      def get_test_files(cls):
> > +        # extract paths to the host environment to be used
>
> # get local absolute filesystem paths for each of the needed file types.
>

ACK

>
> >          img_verstr, ftypes = get_images(
> >              IMAGE_SRC_URL, IMAGE_DIR, cls.distro, cls.release, cls.arch,
> >              krel=cls.krel if cls.krel else cls.release,
> > +            sync=CURTIN_VMTEST_IMAGE_SYNC,
> >              ftypes=('boot-initrd', 'boot-kernel', 'vmtest.root-image'))
> >          logger.debug("Install Image %s\n, ftypes: %s\n", img_verstr,
> ftypes)
> >          logger.info("Install Image: %s", img_verstr)
> >          if not cls.target_krel and cls.krel:
> >              cls.target_krel = cls.krel
> > +
> > +        # extract paths to the target OS tarball to be used
>
> # find local file system path for the OS tarball to be installed.
>

ACK

>
> >          img_verstr, found = get_images(
> >              IMAGE_SRC_URL, IMAGE_DIR,
> >              cls.target_distro if cls.target_distro else cls.distro,
> >              cls.target_release if cls.target_release else cls.release,
> > -            cls.arch, krel=cls.target_krel, ftypes=('vmtest.root-tgz',))
> > +            cls.arch, krel=cls.target_krel,
> sync=CURTIN_VMTEST_IMAGE_SYNC,
> > +            ftypes=('vmtest.root-tgz',))
> >          logger.debug("Target Tarball %s\n, ftypes: %s\n", img_verstr,
> found)
> >          logger.info("Target Tarball: %s", img_verstr)
> >          ftypes.update(found)
> >
> > === modified file 'tools/jenkins-runner'
> > --- tools/jenkins-runner      2016-08-05 18:01:25 +0000
> > +++ tools/jenkins-runner      2017-01-23 17:05:51 +0000
> > @@ -58,7 +58,7 @@
> >  fmt="  %(release)-7s %(arch)s/%(subarch)s %(version_name)-10s"
> >  PYTHONPATH="$PWD" python3 tests/vmtests/image_sync.py query \
> >      --output-format="$fmt" "$IMAGE_DIR" ftype=root-image.gz ||
> > -    { echo "WARNING: error querying images in $IMAGE_DIR" 1>&2; }
> > +    { echo "WARNING: error querying images in $IMAGE_DIR" 1>&2; exit
> $?;}
>
> exit $? would always exit 0 here, unless 'echo' failed, which is fairly
> unlikely.
>
> you probably meant:
>  { ret=$?; echo "WARNING: error querying images in '$IMAGE_DIR'" 1>&2;
> exit $ret; }
>

You're right; in practice it still exited when in the past it would
continue to invoke nose.
I'll fix.

>
> >
> >  echo "$(date -R): vmtest start: nosetests3 ${pargs[*]} ${ntargs[*]}"
> >  nosetests3 "${pargs[@]}" "${ntargs[@]}"
> >
> > === modified file 'tools/vmtest-sync-images'
> > --- tools/vmtest-sync-images  2016-09-21 04:26:33 +0000
> > +++ tools/vmtest-sync-images  2017-01-23 17:05:51 +0000
> > @@ -44,11 +44,17 @@
> >      if len(arg_releases):
> >          filter_sets.append([_fmt_list_filter('release', arg_releases)])
>
> does this path still work?
> it used to allow you to
>   ./tools/vmtest-sync-images zesty
>

I'll check, but it should (but is likely missing the HWE kernels)  The
caller would need to specify
Or we could infer krel=$release.

>
> >      else:
> > -        filter_sets.extend(
> > -            (['os={}'.format(distro), _fmt_list_filter('release', rels)]
> > -             for (distro, rels) in find_releases_by_distro().items()))
> > +        for distname, distro in find_releases_by_distro().items():
> > +            f = ['os={}'.format(distname),
> > +                 _fmt_list_filter('release', distro.get('releases'))]
> > +            # ensure we fetch release=x krel=x items
> > +            krels = distro.get('krels')
> > +            if krels:
> > +                krels = set(krels).union(set(distro.get('releases')))
> > +                f.append(_fmt_list_filter('krel', krels))
> > +            filter_sets.extend([f])
> >
> >      # Sync images.
> >      for filter_set in filter_sets:
> > -        sync_images(IMAGE_SRC_URL, IMAGE_DIR, verbosity=1,
> > +        sync_images(IMAGE_SRC_URL, IMAGE_DIR, verbosity=2,
> >                      filters=filter_set + ITEM_NAME_FILTERS +
> arch_filters)
>
>
> --
> https://code.launchpad.net/~raharper/curtin/trunk.vmtest-
> sync-only-once/+merge/315387
> You are the owner of lp:~raharper/curtin/trunk.vmtest-sync-only-once.
>

lp:~raharper/curtin/trunk.vmtest-sync-only-once updated on 2017-01-23

441. By Ryan Harper on 2017-01-23

Address review feedback

442. By Ryan Harper on 2017-01-23

vmtest: image-sync rework sync logic

Invert some of the sync values to support two main use-cases
- jenkins-runner uses make-sync first, so diable syncing during
unittest execution, raise exception on missing files.
- Developers using nose directly can benefit from on-the-fly image
syncing

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-01-23:

PASSED: Continuous integration, rev:442
https://jenkins.ubuntu.com/server/job/curtin-ci/290/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-amd64/290
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/290
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-ppc64el/290
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-s390x/290

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/290/rebuild

review: Approve (continuous-integration)

lp:~raharper/curtin/trunk.vmtest-sync-only-once updated on 2017-01-25

443. By Ryan Harper on 2017-01-25: merge from trunk

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-01-25:

PASSED: Continuous integration, rev:443
https://jenkins.ubuntu.com/server/job/curtin-ci/294/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-amd64/294
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/294
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-ppc64el/294
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-s390x/294

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/294/rebuild

review: Approve (continuous-integration)

Revision history for this message

Ryan Harper (raharper) wrote on 2017-01-26:

This passes a full-vmtest run locally and on diglett (save for the old-apt test, due to missing apt-proxy on host and is addressed in another branch, https://code.launchpad.net/~raharper/curtin/trunk.skip-apt-proxy-test-if-not-set/+merge/315688)

lp:~raharper/curtin/trunk.vmtest-sync-only-once updated on 2017-01-26

444. By Ryan Harper on 2017-01-26: Drop util.{is_true,is_false}; sync parameter uses '1' for true to match env

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-01-26:

PASSED: Continuous integration, rev:444
https://jenkins.ubuntu.com/server/job/curtin-ci/297/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-amd64/297
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/297
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-ppc64el/297
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-s390x/297

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/297/rebuild

review: Approve (continuous-integration)

Revision history for this message

Scott Moser (smoser) on 2017-01-27:

Revision history for this message

Ryan Harper (raharper) wrote on 2017-01-27:

On Fri, Jan 27, 2017 at 9:05 AM, Scott Moser <email address hidden> wrote:

>
>
> Diff comments:
>
> >
> > === modified file 'tests/vmtests/__init__.py'
> > --- tests/vmtests/__init__.py 2016-12-02 02:01:20 +0000
> > +++ tests/vmtests/__init__.py 2017-01-25 22:27:40 +0000
> > @@ -186,12 +188,22 @@
> > common_filters.append('krel=%s' % krel)
> > filters = ['ftype~(%s)' % ("|".join(ftypes.keys()))] +
> common_filters
> >
> > - if sync:
> > + if util.is_true(sync):
> > + # sync with the default items + common filters to ensure we get
> > + # everything in one go.
> > + sync_filters = common_filters + ITEM_NAME_FILTERS
> > + logger.info('Syncing images from %s with filters=%s', src_url,
> > + sync_filters)
> > imagesync_mirror(output_d=local_d, source=src_url,
> > - mirror_filters=common_filters,
> > - max_items=IMAGES_TO_KEEP)
> > + mirror_filters=sync_filters,
> > + max_items=IMAGES_TO_KEEP, verbosity=1)
> > + else:
> > + logger.info('Image sync disabled, sync=%s', sync)
> > + logger.info('env var CURTIN_VMTEST_IMAGE_SYNC=%s',
> > + CURTIN_VMTEST_IMAGE_SYNC)
>
> this is failure path, right ?
> shouldnt we just raise an exception ? is there some case where this *is
> not* failure?
>

No, this is informative that we're not syncing.
Next we query to see if we have the files we need, and if so we move on.
If we don't have them *and* we've disabled sync; then we rase ValueError on
missing required images

> >
> > - query_str = 'query = %s' % (' '.join(filters))
> > + query_cmd = 'python3 tests/vmtests/image_sync.py'
> > + query_str = '%s query %s %s' % (query_cmd, local_d, '
> '.join(filters))
> > logger.debug('Query %s for image. %s', local_d, query_str)
> > fail_msg = None
> >
>
>
> --
> https://code.launchpad.net/~raharper/curtin/trunk.vmtest-
> sync-only-once/+merge/315387
> You are the owner of lp:~raharper/curtin/trunk.vmtest-sync-only-once.
>

On Fri, Jan 27, 2017 at 9:05 AM, Scott Moser <smoser@ubuntu.com> wrote:

>
>
> Diff comments:
>
> >
> > === modified file 'tests/vmtests/__init__.py'
> > --- tests/vmtests/__init__.py 2016-12-02 02:01:20 +0000
> > +++ tests/vmtests/__init__.py 2017-01-25 22:27:40 +0000
> > @@ -186,12 +188,22 @@
> >          common_filters.append('krel=%s' % krel)
> >      filters = ['ftype~(%s)' % ("|".join(ftypes.keys()))] +
> common_filters
> >
> > -    if sync:
> > +    if util.is_true(sync):
> > +        # sync with the default items + common filters to ensure we get
> > +        # everything in one go.
> > +        sync_filters = common_filters + ITEM_NAME_FILTERS
> > +        logger.info('Syncing images from %s with filters=%s', src_url,
> > +                    sync_filters)
> >          imagesync_mirror(output_d=local_d, source=src_url,
> > -                         mirror_filters=common_filters,
> > -                         max_items=IMAGES_TO_KEEP)
> > +                         mirror_filters=sync_filters,
> > +                         max_items=IMAGES_TO_KEEP, verbosity=1)
> > +    else:
> > +        logger.info('Image sync disabled, sync=%s', sync)
> > +        logger.info('env var CURTIN_VMTEST_IMAGE_SYNC=%s',
> > +                    CURTIN_VMTEST_IMAGE_SYNC)
>
> this is failure path, right ?
> shouldnt we just raise an exception ? is there some case where this *is
> not* failure?
>

> >
> > -    query_str = 'query = %s' % (' '.join(filters))
> > +    query_cmd = 'python3 tests/vmtests/image_sync.py'
> > +    query_str = '%s query %s %s' % (query_cmd, local_d, '
> '.join(filters))
> >      logger.debug('Query %s for image. %s', local_d, query_str)
> >      fail_msg = None
> >
>
>
> --
> https://code.launchpad.net/~raharper/curtin/trunk.vmtest-
> sync-only-once/+merge/315387
> You are the owner of lp:~raharper/curtin/trunk.vmtest-sync-only-once.
>

Revision history for this message

Scott Moser (smoser) wrote on 2017-01-27:

So, i'm almost all +1 on this..
My comments:
* remove the is_true/is_false, and revert to the old
    (I'm not completely opposed to this, but in cloud-init experience i have found
     that it just makes things sloppier. Now, instead of '0', you have to support 'FALSE'
     or 'false', and ultimately that just makes any checker or something more complex (like a jasonschema or something).

Then, 2 inline minor things.

thank you ryan!

Revision history for this message

Ryan Harper (raharper) wrote on 2017-01-27:

On Fri, Jan 27, 2017 at 9:38 AM, Scott Moser <email address hidden> wrote:

> So, i'm almost all +1 on this..
> My comments:
> * remove the is_true/is_false, and revert to the old
> (I'm not completely opposed to this, but in cloud-init experience i
> have found
> that it just makes things sloppier. Now, instead of '0', you have to
> support 'FALSE'
> or 'false', and ultimately that just makes any checker or something
> more complex (like a jasonschema or something).
>

Done

>
> Then, 2 inline minor things.
>

Done

>
> thank you ryan!
>
>
> Diff comments:
>
> >
> > === modified file 'tools/jenkins-runner'
> > --- tools/jenkins-runner 2016-08-05 18:01:25 +0000
> > +++ tools/jenkins-runner 2017-01-25 22:27:40 +0000
> > @@ -58,7 +59,8 @@
> > fmt=" %(release)-7s %(arch)s/%(subarch)s %(version_name)-10s"
> > PYTHONPATH="$PWD" python3 tests/vmtests/image_sync.py query \
> > --output-format="$fmt" "$IMAGE_DIR" ftype=root-image.gz ||
> > - { echo "WARNING: error querying images in $IMAGE_DIR" 1>&2; }
> > + { ret=$?; echo "WARNING: error querying images in $IMAGE_DIR" 1>&2;
> > + exit $ret; }
>
> might as well say FAIL not warn, since you're exiting.
>
> >
> > echo "$(date -R): vmtest start: nosetests3 ${pargs[*]} ${ntargs[*]}"
> > nosetests3 "${pargs[@]}" "${ntargs[@]}"
>
>
> --
> https://code.launchpad.net/~raharper/curtin/trunk.vmtest-
> sync-only-once/+merge/315387
> You are the owner of lp:~raharper/curtin/trunk.vmtest-sync-only-once.
>

lp:~raharper/curtin/trunk.vmtest-sync-only-once updated on 2017-01-27

445. By Ryan Harper on 2017-01-27

vmtests: fix spelling of comment and logging noise

- Drop sync message to debug
- Don't log image sync status on every invocation
- Fix misspelling of absolute
- Change error message in jenkins-runner; it's not a warning if it exits.

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-01-27:

PASSED: Continuous integration, rev:445
https://jenkins.ubuntu.com/server/job/curtin-ci/298/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-amd64/298
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/298
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-ppc64el/298
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-s390x/298

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/298/rebuild

review: Approve (continuous-integration)

Revision history for this message

Scott Moser (smoser) wrote on 2017-01-27:

I am marking approve, but please remove
TRUE_STRINGS and FALSE_STRINGS from util.py.

review: Approve

lp:~raharper/curtin/trunk.vmtest-sync-only-once updated on 2017-01-27

446. By Ryan Harper on 2017-01-27: util: drop TRUE_STRINGS,FALSE_STRINGS; unneeded

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-01-27:

PASSED: Continuous integration, rev:446
https://jenkins.ubuntu.com/server/job/curtin-ci/300/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-amd64/300
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/300
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-ppc64el/300
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-s390x/300

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/300/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

David Britton

Michael Hudson-Doyle

Ryan Harper

curtin developers

 === modified file 'tests/vmtests/__init__.py'
 --- tests/vmtests/__init__.py	2016-12-02 02:01:20 +0000
 +++ tests/vmtests/__init__.py	2017-01-27 19:45:16 +0000
@@ -18,7 +18,7 @@
  from .image_sync import query as imagesync_query
  from .image_sync import mirror as imagesync_mirror
--from .image_sync import (IMAGE_SRC_URL, IMAGE_DIR)
++from .image_sync import (IMAGE_SRC_URL, IMAGE_DIR, ITEM_NAME_FILTERS)
  from .helpers import check_call, TimeoutExpired
  from unittest import TestCase, SkipTest
@@ -32,7 +32,7 @@
  DEVNULL = open(os.devnull, 'w')
  KEEP_DATA = {"pass": "none", "fail": "all"}
--CURTIN_VMTEST_IMAGE_SYNC = os.environ.get("CURTIN_VMTEST_IMAGE_SYNC", False)
++CURTIN_VMTEST_IMAGE_SYNC = os.environ.get("CURTIN_VMTEST_IMAGE_SYNC", "1")
  IMAGE_SYNCS = []
  TARGET_IMAGE_FORMAT = "raw"
@@ -165,11 +165,13 @@
      return
--def get_images(src_url, local_d, distro, release, arch, krel=None, sync=True,
++def get_images(src_url, local_d, distro, release, arch, krel=None, sync="1",
                 ftypes=None):
      # ensure that the image items (roottar, kernel, initrd)
      # we need for release and arch are available in base_dir.
--    # returns updated ftypes dictionary {ftype: item_url}
++    #
++    # returns ftype dictionary with path to each ftype as values
++    # {ftype: item_url}
      if not ftypes:
          ftypes = {
              'vmtest.root-image': '',
@@ -178,7 +180,7 @@
              'boot-initrd': ''
+         }
      elif isinstance(ftypes, (list, tuple)):
--        ftypes = dict().fromkeys(ftypes)
++        ftypes = dict().fromkeys(ftypes, '')
      common_filters = ['release=%s' % release,
                        'arch=%s' % arch, 'os=%s' % distro]
@@ -186,12 +188,17 @@
          common_filters.append('krel=%s' % krel)
      filters = ['ftype~(%s)' % ("|".join(ftypes.keys()))] + common_filters
--    if sync:
++    if sync == "1":
++        # sync with the default items + common filters to ensure we get
++        # everything in one go.
++        sync_filters = common_filters + ITEM_NAME_FILTERS
++        logger.debug('Syncing images from %s with filters=%s', src_url,
++                     sync_filters)
          imagesync_mirror(output_d=local_d, source=src_url,
--                         mirror_filters=common_filters,
--                         max_items=IMAGES_TO_KEEP)
--
--    query_str = 'query = %s' % (' '.join(filters))
++                         mirror_filters=sync_filters,
++                         max_items=IMAGES_TO_KEEP, verbosity=1)
++    query_cmd = 'python3 tests/vmtests/image_sync.py'
++    query_str = '%s query %s %s' % (query_cmd, local_d, ' '.join(filters))
      logger.debug('Query %s for image. %s', local_d, query_str)
      fail_msg = None
@@ -205,14 +212,15 @@
          results = None
          fail_msg = str(e)
--    if not results and not sync:
++    if not results and sync == "1":
          # try to fix this with a sync
          logger.info(fail_msg + "  Attempting to fix with an image sync. (%s)",
                      query_str)
          return get_images(src_url, local_d, distro, release, arch,
--                          krel=krel, sync=True, ftypes=ftypes)
++                          krel=krel, sync="1", ftypes=ftypes)
      elif not results:
--        raise ValueError("Nothing found in query: %s" % query_str)
++        raise ValueError("Required images not found and "
++                         "syncing disabled:\n%s" % query_str)
      missing = []
      found = sorted(f.get('ftype') for f in results)
@@ -344,19 +352,25 @@
      @classmethod
      def get_test_files(cls):
++        # get local absolute filesystem paths for each of the needed file types
          img_verstr, ftypes = get_images(
              IMAGE_SRC_URL, IMAGE_DIR, cls.distro, cls.release, cls.arch,
              krel=cls.krel if cls.krel else cls.release,
++            sync=CURTIN_VMTEST_IMAGE_SYNC,
              ftypes=('boot-initrd', 'boot-kernel', 'vmtest.root-image'))
          logger.debug("Install Image %s\n, ftypes: %s\n", img_verstr, ftypes)
          logger.info("Install Image: %s", img_verstr)
          if not cls.target_krel and cls.krel:
              cls.target_krel = cls.krel
++
++        # get local absolute filesystem paths for the OS tarball to be
++        # installed
          img_verstr, found = get_images(
              IMAGE_SRC_URL, IMAGE_DIR,
              cls.target_distro if cls.target_distro else cls.distro,
              cls.target_release if cls.target_release else cls.release,
--            cls.arch, krel=cls.target_krel, ftypes=('vmtest.root-tgz',))
++            cls.arch, krel=cls.target_krel, sync=CURTIN_VMTEST_IMAGE_SYNC,
++            ftypes=('vmtest.root-tgz',))
          logger.debug("Target Tarball %s\n, ftypes: %s\n", img_verstr, found)
          logger.info("Target Tarball: %s", img_verstr)
          ftypes.update(found)
 === modified file 'tests/vmtests/helpers.py'
 --- tests/vmtests/helpers.py	2016-11-14 22:55:12 +0000
 +++ tests/vmtests/helpers.py	2017-01-27 19:45:16 +0000
@@ -103,6 +103,14 @@
  def find_releases_by_distro():
      """
      Returns a dictionary of distros and the distro releases that will be tested
++
++    distros:
++        ubuntu:
++            releases: []
++            krels: []
++        centos:
++            releases: []
++            krels: []
      """
      # Use the TestLoder to load all test cases defined within tests/vmtests/
      # and figure out what distros and releases they are testing. Any tests
@@ -115,20 +123,33 @@
      # Find all test modules defined in curtin/tests/vmtests/
      module_test_suites = loader.discover(tests_dir, top_level_dir=root_dir)
      # find all distros and releases tested for each distro
--    distros = {}
++    releases = []
++    krels = []
++    rel_by_dist = {}
      for mts in module_test_suites:
          for class_test_suite in mts:
              for test_case in class_test_suite:
                  # skip disabled tests
                  if not getattr(test_case, '__test__', False):
                      continue
--                for (dist, rel) in (
++                for (dist, rel, krel) in (
                          (getattr(test_case, a, None) for a in attrs)
--                        for attrs in (('distro', 'release'),
--                                      ('target_distro', 'target_release'))):
++                        for attrs in (('distro', 'release', 'krel'),
++                                      ('target_distro', 'target_release',
++                                       'krel'))):
++
                      if dist and rel:
--                        distros[dist] = distros.get(dist, set()).union((rel,))
--    return {k: sorted(v) for (k, v) in distros.items()}
++                        distro = rel_by_dist.get(dist, {'releases': [],
++                                                        'krels': []})
++                        releases = distro.get('releases')
++                        krels = distro.get('krels')
++                        if rel not in releases:
++                            releases.append(rel)
++                        if krel and krel not in krels:
++                            krels.append(krel)
++                        rel_by_dist.update({dist: distro})
++
++    return rel_by_dist
  def _parse_ip_a(ip_a):
 === modified file 'tests/vmtests/image_sync.py'
 --- tests/vmtests/image_sync.py	2016-11-14 22:55:12 +0000
 +++ tests/vmtests/image_sync.py	2017-01-27 19:45:16 +0000
@@ -404,7 +404,8 @@
      return next((q for q in (
          query_ptree(sutil.load_content(util.load_file(fpath(path))),
                      max_num=max_items, ifilters=ifilters, path2url=fpath)
--        for path in VMTEST_CONTENT_ID_PATH_MAP.values()) if q), None)
++        for path in VMTEST_CONTENT_ID_PATH_MAP.values() if os.path.exists(
++            fpath(path))) if q), [])
  def main_query(args):
 === modified file 'tools/jenkins-runner'
 --- tools/jenkins-runner	2016-08-05 18:01:25 +0000
 +++ tools/jenkins-runner	2017-01-27 19:45:16 +0000
@@ -3,6 +3,7 @@
  topdir="${CURTIN_VMTEST_TOPDIR:-${WORKSPACE:-$PWD}/output}"
  pkeep=${CURTIN_VMTEST_KEEP_DATA_PASS:-logs,collect}
  fkeep=${CURTIN_VMTEST_KEEP_DATA_FAIL:-logs,collect}
++export CURTIN_VMTEST_IMAGE_SYNC=${CURTIN_VMTEST_IMAGE_SYNC:-0}
  export CURTIN_VMTEST_KEEP_DATA_PASS=$pkeep
  export CURTIN_VMTEST_KEEP_DATA_FAIL=$fkeep
  export CURTIN_VMTEST_TOPDIR="$topdir"
@@ -58,7 +59,8 @@
  fmt="  %(release)-7s %(arch)s/%(subarch)s %(version_name)-10s"
  PYTHONPATH="$PWD" python3 tests/vmtests/image_sync.py query \
      --output-format="$fmt" "$IMAGE_DIR" ftype=root-image.gz ||
--    { echo "WARNING: error querying images in $IMAGE_DIR" 1>&2; }
++    { ret=$?; echo "FATAL: error querying images in $IMAGE_DIR" 1>&2;
++      exit $ret; }
  echo "$(date -R): vmtest start: nosetests3 ${pargs[*]} ${ntargs[*]}"
  nosetests3 "${pargs[@]}" "${ntargs[@]}"
 === modified file 'tools/vmtest-sync-images'
 --- tools/vmtest-sync-images	2016-09-21 04:26:33 +0000
 +++ tools/vmtest-sync-images	2017-01-27 19:45:16 +0000
@@ -42,13 +42,20 @@
      arch_filters = ['arch={}'.format(DEFAULT_ARCH)]
      filter_sets = []
      if len(arg_releases):
--        filter_sets.append([_fmt_list_filter('release', arg_releases)])
++        filter_sets.append([_fmt_list_filter('release', arg_releases),
++                            _fmt_list_filter('krel', arg_releases)])
      else:
--        filter_sets.extend(
--            (['os={}'.format(distro), _fmt_list_filter('release', rels)]
--             for (distro, rels) in find_releases_by_distro().items()))
++        for distname, distro in find_releases_by_distro().items():
++            f = ['os={}'.format(distname),
++                 _fmt_list_filter('release', distro.get('releases'))]
++            # ensure we fetch release=x krel=x items
++            krels = distro.get('krels')
++            if krels:
++                krels = set(krels).union(set(distro.get('releases')))
++                f.append(_fmt_list_filter('krel', krels))
++            filter_sets.extend([f])
      # Sync images.
      for filter_set in filter_sets:
--        sync_images(IMAGE_SRC_URL, IMAGE_DIR, verbosity=1,
++        sync_images(IMAGE_SRC_URL, IMAGE_DIR, verbosity=2,
                      filters=filter_set + ITEM_NAME_FILTERS + arch_filters)