[PATCH 0/4][layerindex-web] bug fix and performance improve

[PATCH 0/4][layerindex-web] bug fix and performance improve

[Robert Yang]

Hi Paul,

The patch 4 can improve a lot on update.py (up to 98%), here is the testing
data, please feel free to give your comments.

$ time update.py -b master --nofetch [--fullreload]

                   Before    Now       Reduced
No update:         276s      3.6s      98%
Partial update:    312s      87s       72%
Full repload:      1016s     980s      3%

Note:
* All of the testing are based on --nofetch

* "No update" means all layers on the branch is up-to-date, for
  example, when we run it twice, there is no update in the second run, so we
  only need about 3s now, which is the most common case when we use cron to run
  it per half an hour.

* "Partly update" means part of the layers have been updated.

* "Fullreload" means all of the layers have been updated.

// Robert

The following changes since commit 611c96883c35240d3c291951146154d828745774:

  requirements.txt: use the most recent Django 1.8 version (2018-03-26 08:29:27 +1300)

are available in the git repository at:

  git://git.pokylinux.org/poky-contrib rbt/layerindex
  http://git.pokylinux.org/cgit.cgi//log/?h=rbt/layerindex

Robert Yang (4):
  fixup! update: don't stop on unsatisfied layer dependencies
  update.py: add an option --timeout for lockfile
  update.py: print failed layers summary in the end
  update_layer.py: move layer validation to update.py (Performance
    improve)

 layerindex/update.py       | 97 ++++++++++++++++++++++++++++++++++++++++++++--
 layerindex/update_layer.py | 39 ++-----------------
 layerindex/utils.py        | 14 +++++--
 3 files changed, 108 insertions(+), 42 deletions(-)

-- 
2.7.4

[PATCH 1/4] fixup! update: don't stop on unsatisfied layer dependencies

[Robert Yang]

The previous commit broke the layer order, e.g.:
A -> B -> C -> D

The algorithm is checking the dependencies one by one, and until we find D, add
D to layerquery_sorted, and add it "collections", the one in "collections"
means it's dependencies are OK, then C, B and A will check against collections,
so that update_layer.py will update them one by one. The previous commit added
A/B/C/D to collections directly, so that when check against it, all the
dependencies are met, thus broke the layer sorting, and then there would be
failures if we pass layer A to update_layer.py before B, C and D (suppose they
are newly to database). This commit fix the problem.

BTW., why I use collections to record the one whose dependencies are matched,
but not directly use layerquery_sorted, it is because collections contains both
the ones to be added/updated and the ones in database, but layerquery_sorted
only contains the ones to be updated/added.

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 layerindex/update.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/layerindex/update.py b/layerindex/update.py
index f60b943..07240ab 100755
--- a/layerindex/update.py
+++ b/layerindex/update.py
@@ -348,12 +348,11 @@ def main():
                     deps = re.search("^LAYERDEPENDS = \"(.*)\"", output, re.M).group(1) or ''
                     recs = re.search("^LAYERRECOMMENDS = \"(.*)\"", output, re.M).group(1) or ''
 
-                    collections.add((col, ver))
-
                     deps_dict = utils.explode_dep_versions2(bitbakepath, deps + ' ' + recs)
                     if len(deps_dict) == 0:
                         # No depends, add it firstly
                         layerquery_sorted.append(layer)
+                        collections.add((col, ver))
                         continue
                     deps_dict_all[layer] = {'requires': deps_dict, 'collection': col, 'version': ver}
 
@@ -374,6 +373,7 @@ def main():
                             # All the depends are in collections:
                             del(deps_dict_all[layer])
                             layerquery_sorted.append(layer)
+                            collections.add((value['collection'], value['version']))
 
                     if not len(deps_dict_all):
                         break
-- 
2.7.4

[PATCH 2/4] update.py: add an option --timeout for lockfile

[Robert Yang]

We have an update.py running perodically in background, but we also need run it
manually, for example, run it to update actual_branch, the manually run usually
failed because can't get lockfile, we have to run it again and again. A timeout
option helps a lot in such a case. Now the following command can make sure we
can run the command successfully:
$ update.py -b master -a actual_branch -t 2000

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 layerindex/update.py |  5 ++++-
 layerindex/utils.py  | 14 +++++++++++---
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/layerindex/update.py b/layerindex/update.py
index 07240ab..e4ca7b6 100755
--- a/layerindex/update.py
+++ b/layerindex/update.py
@@ -156,6 +156,9 @@ def main():
     parser.add_option("-l", "--layer",
             help = "Specify layers to update (use commas to separate multiple). Default is all published layers.",
             action="store", dest="layers")
+    parser.add_option("-t", "--timeout",
+            help = "Specify timeout in seconds to get layerindex.lock. Default is 30 seconds.",
+            type="int", action="store", dest="timeout", default=30)
     parser.add_option("-r", "--reload",
             help = "Reload recipe data instead of updating since last update",
             action="store_true", dest="reload")
@@ -265,7 +268,7 @@ def main():
         update.save()
     try:
         lockfn = os.path.join(fetchdir, "layerindex.lock")
-        lockfile = utils.lock_file(lockfn)
+        lockfile = utils.lock_file(lockfn, options.timeout, logger)
         if not lockfile:
             logger.error("Layer index lock timeout expired")
             sys.exit(1)
diff --git a/layerindex/utils.py b/layerindex/utils.py
index 08a4001..ebcfcaf 100644
--- a/layerindex/utils.py
+++ b/layerindex/utils.py
@@ -309,8 +309,10 @@ class ListHandler(logging.Handler):
         return log
 
 
-def lock_file(fn):
-    starttime = time.time()
+def lock_file(fn, timeout=30, logger=None):
+    start = time.time()
+    last = start
+    counter = 1
     while True:
         lock = open(fn, 'w')
         try:
@@ -318,8 +320,14 @@ def lock_file(fn):
             return lock
         except IOError:
             lock.close()
-            if time.time() - starttime > 30:
+            current = time.time()
+            if current - start > timeout:
                 return None
+            # Print a message in every 5 seconds
+            if logger and (current - last > 5):
+                last = current
+                logger.info('Trying to get lock on %s (tried %s seconds) ...' % (fn, (5 * counter)))
+                counter += 1
 
 def unlock_file(lock):
     fcntl.flock(lock, fcntl.LOCK_UN)
-- 
2.7.4

[PATCH 3/4] update.py: print failed layers summary in the end

[Robert Yang]

This makes it easy to see which layers are failed. For example:

ERROR: Failed layers on branch master: openembedded-core meta-python

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 layerindex/update.py | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/layerindex/update.py b/layerindex/update.py
index e4ca7b6..44a7b90 100755
--- a/layerindex/update.py
+++ b/layerindex/update.py
@@ -319,6 +319,7 @@ def main():
             # unreliable due to leaking memory (we're using bitbake internals in a manner in which
             # they never get used during normal operation).
             last_rev = {}
+            failed_layers = {}
             for branch in branches:
                 # If layer_A depends(or recommends) on layer_B, add layer_B before layer_A
                 deps_dict_all = {}
@@ -384,9 +385,12 @@ def main():
                     # If nothing changed after a run then some dependencies couldn't be resolved
                     if operator.eq(deps_dict_all_copy, deps_dict_all):
                         logger.warning("Cannot find required collections on branch %s:" % branch)
+                        layer_names = []
                         for layer, value in deps_dict_all.items():
                             logger.error('%s: %s' % (layer.name, value['requires']))
+                            layer_names.append(layer.name)
                         logger.warning("Known collections on branch %s: %s" % (branch, collections))
+                        failed_layers[branch] = layer_names
                         break
 
                 for layer in layerquery_sorted:
@@ -427,6 +431,11 @@ def main():
                         # Interrupted by user, break out of loop
                         logger.info('Update interrupted, exiting')
                         sys.exit(254)
+            if failed_layers:
+                print()
+                for branch, err_msg_list in failed_layers.items():
+                    logger.error("Failed layers on branch %s: %s" % (branch, " ".join(err_msg_list)))
+                print()
         finally:
             utils.unlock_file(lockfile)
 
-- 
2.7.4

[PATCH 4/4] update_layer.py: move layer validation to update.py (Performance improve)

[Robert Yang]

The utils.setup_django() costs a lot of time, but both update.py and
update_layer.py calls it, so move layer validation from update_layer.py to
update.py to avoid calling update_layer.py when possible can save a lot of
time.

Now we don't have to call update_layer.py in the following cases:
* The branch doesn't exist
* The layer is already update to date on specified branch (when no
  reload)
* The layer dir or conf/layer.layer doesn't exist

We can save up to 98% time in my testing:

$ update.py -b master --nofetch [--fullreload]

                   Before    Now       Reduced
No update:         276s      3.6s      98%
Partial update:    312s      87s       72%
Full repload:      1016s     980s      3%

Note:
* All of the testing are based on --nofetch

* "No update" means all layers on the branch is up-to-date, for
  example, when we run it twice, there is no update in the second run, so we
  only need about 3s now, which is the most common case when we use cron to run
  it per half an hour.

* "Partly update" means part of the layers have been updated.

* "Fullreload" means all of the layers have been updated.

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 layerindex/update.py       | 79 +++++++++++++++++++++++++++++++++++++++++++++-
 layerindex/update_layer.py | 39 +++--------------------
 2 files changed, 82 insertions(+), 36 deletions(-)

diff --git a/layerindex/update.py b/layerindex/update.py
index 44a7b90..2bb2df7 100755
--- a/layerindex/update.py
+++ b/layerindex/update.py
@@ -140,6 +140,15 @@ def fetch_repo(vcs_url, repodir, urldir, fetchdir, layer_name):
         logger.error("Fetch of layer %s failed: %s" % (layer_name, e.output))
         return (vcs_url, e.output)
 
+def print_subdir_error(newbranch, layername, vcs_subdir, branchdesc):
+    # This will error out if the directory is completely invalid or had never existed at this point
+    # If it previously existed but has since been deleted, you will get the revision where it was
+    # deleted - so we need to handle that case separately later
+    if newbranch:
+        logger.info("Skipping update of layer %s for branch %s - subdirectory %s does not exist on this branch" % (layername, branchdesc, vcs_subdir))
+    elif vcs_subdir:
+        logger.error("Subdirectory for layer %s does not exist on branch %s - if this is legitimate, the layer branch record should be deleted" % (layername, branchdesc))
+
 def main():
     if LooseVersion(git.__version__) < '0.3.1':
         logger.error("Version of GitPython is too old, please install GitPython (python-git) 0.3.1 or later in order to use this script")
@@ -195,7 +204,7 @@ def main():
 
     utils.setup_django()
     import settings
-    from layerindex.models import Branch, LayerItem, Update, LayerUpdate
+    from layerindex.models import Branch, LayerItem, Update, LayerUpdate, LayerBranch
 
     logger.setLevel(options.loglevel)
 
@@ -337,6 +346,74 @@ def main():
                         collections.add((layerbranch.collection, layerbranch.version))
 
                 for layer in layerquery:
+                    layerbranch = layer.get_layerbranch(branch)
+                    branchname = branch
+                    branchdesc = branch
+                    newbranch = False
+                    branchobj = utils.get_branch(branch)
+                    if layerbranch:
+                        if layerbranch.actual_branch:
+                            branchname = layerbranch.actual_branch
+                            branchdesc = "%s (%s)" % (branch, branchname)
+                    else:
+                        # LayerBranch doesn't exist for this branch, create it
+                        newbranch = True
+                        layerbranch = LayerBranch()
+                        layerbranch.layer = layer
+                        layerbranch.branch = branchobj
+                        layerbranch_source = layer.get_layerbranch(branchobj)
+                        if not layerbranch_source:
+                            layerbranch_source = layer.get_layerbranch(None)
+                        if layerbranch_source:
+                            layerbranch.vcs_subdir = layerbranch_source.vcs_subdir
+
+                    # Collect repo info
+                    urldir = layer.get_fetch_dir()
+                    repodir = os.path.join(fetchdir, urldir)
+                    repo = git.Repo(repodir)
+                    assert repo.bare == False
+                    try:
+                        if options.nocheckout:
+                            topcommit = repo.commit('HEAD')
+                        else:
+                            topcommit = repo.commit('origin/%s' % branchname)
+                    except:
+                        if newbranch:
+                            logger.info("Skipping update of layer %s - branch %s doesn't exist" % (layer.name, branchdesc))
+                        else:
+                            logger.info("layer %s - branch %s no longer exists, removing it from database" % (layer.name, branchdesc))
+                            if not options.dryrun:
+                                layerbranch.delete()
+                        continue
+
+                    if layerbranch.vcs_subdir and not options.nocheckout:
+                        # Find latest commit in subdirectory
+                        # A bit odd to do it this way but apparently there's no other way in the GitPython API
+                        topcommit = next(repo.iter_commits('origin/%s' % branchname, paths=layerbranch.vcs_subdir), None)
+                        if not topcommit:
+                            print_subdir_error(newbranch, layer.name, layerbranch.vcs_subdir, branchdesc)
+                            if not (newbranch and layerbranch.vcs_subdir):
+                                logger.error("Failed to get last revision for layer %s on branch %s" % (layer.name, branchdesc))
+                            continue
+
+                    if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload:
+                        logger.info("Layer %s is already up-to-date for branch %s" % (layer.name, branchdesc))
+                        collections.add((layerbranch.collection, layerbranch.version))
+                        continue
+
+                    if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload:
+                        # Check out appropriate branch
+                        if not options.nocheckout:
+                            utils.checkout_layer_branch(layerbranch, repodir, logger=logger)
+                        layerdir = os.path.join(repodir, layerbranch.vcs_subdir)
+                        if layerbranch.vcs_subdir and not os.path.exists(layerdir):
+                            print_subdir_error(newbranch, layer.name, layerbranch.vcs_subdir, branchdesc)
+                            continue
+
+                        if not os.path.exists(os.path.join(layerdir, 'conf/layer.conf')):
+                            logger.error("conf/layer.conf not found for layer %s - is subdirectory set correctly?" % layer.name)
+                            continue
+
                     cmd = prepare_update_layer_command(options, branchobj, layer, initial=True)
                     logger.debug('Running layer update command: %s' % cmd)
                     ret, output = run_command_interruptible(cmd)
diff --git a/layerindex/update_layer.py b/layerindex/update_layer.py
index 60a1f2e..69ca3c6 100644
--- a/layerindex/update_layer.py
+++ b/layerindex/update_layer.py
@@ -287,19 +287,10 @@ def main():
     # Collect repo info
     repo = git.Repo(repodir)
     assert repo.bare == False
-    try:
-        if options.nocheckout:
-            topcommit = repo.commit('HEAD')
-        else:
-            topcommit = repo.commit('origin/%s' % branchname)
-    except:
-        if layerbranch:
-            logger.info("layer %s - branch %s no longer exists, removing it from database" % (layer.name, branchdesc))
-            if not options.dryrun:
-                layerbranch.delete()
-        else:
-            logger.info("Skipping update of layer %s - branch %s doesn't exist" % (layer.name, branchdesc))
-        sys.exit(1)
+    if options.nocheckout:
+        topcommit = repo.commit('HEAD')
+    else:
+        topcommit = repo.commit('origin/%s' % branchname)
 
     tinfoil = None
     tempdir = None
@@ -329,17 +320,6 @@ def main():
                 # Find latest commit in subdirectory
                 # A bit odd to do it this way but apparently there's no other way in the GitPython API
                 topcommit = next(repo.iter_commits('origin/%s' % branchname, paths=layerbranch.vcs_subdir), None)
-                if not topcommit:
-                    # This will error out if the directory is completely invalid or had never existed at this point
-                    # If it previously existed but has since been deleted, you will get the revision where it was
-                    # deleted - so we need to handle that case separately later
-                    if newbranch:
-                        logger.info("Skipping update of layer %s for branch %s - subdirectory %s does not exist on this branch" % (layer.name, branchdesc, layerbranch.vcs_subdir))
-                    elif layerbranch.vcs_subdir:
-                        logger.error("Subdirectory for layer %s does not exist on branch %s - if this is legitimate, the layer branch record should be deleted" % (layer.name, branchdesc))
-                    else:
-                        logger.error("Failed to get last revision for layer %s on branch %s" % (layer.name, branchdesc))
-                    sys.exit(1)
 
             layerdir = os.path.join(repodir, layerbranch.vcs_subdir)
             layerdir_start = os.path.normpath(layerdir) + os.sep
@@ -354,17 +334,6 @@ def main():
                 if not options.nocheckout:
                     utils.checkout_layer_branch(layerbranch, repodir, logger=logger)
 
-                if layerbranch.vcs_subdir and not os.path.exists(layerdir):
-                    if newbranch:
-                        logger.info("Skipping update of layer %s for branch %s - subdirectory %s does not exist on this branch" % (layer.name, branchdesc, layerbranch.vcs_subdir))
-                    else:
-                        logger.error("Subdirectory for layer %s does not exist on branch %s - if this is legitimate, the layer branch record should be deleted" % (layer.name, branchdesc))
-                    sys.exit(1)
-
-                if not os.path.exists(os.path.join(layerdir, 'conf/layer.conf')):
-                    logger.error("conf/layer.conf not found for layer %s - is subdirectory set correctly?" % layer.name)
-                    sys.exit(1)
-
                 logger.info("Collecting data for layer %s on branch %s" % (layer.name, branchdesc))
                 try:
                     (tinfoil, tempdir) = recipeparse.init_parser(settings, branch, bitbakepath, nocheckout=options.nocheckout, logger=logger)
-- 
2.7.4

Re: [PATCH 0/4][layerindex-web] bug fix and performance improve

[Burton, Ross]

On 18 April 2018 at 12:04, Robert Yang <liezhi.yang@windriver.com> wrote:
>   fixup! update: don't stop on unsatisfied layer dependencies

Pretty sure this isn't what you intended.

Ross

Re: [PATCH 4/4] update_layer.py: move layer validation to update.py (Performance improve)

[Paul Eggleton]

On Wednesday, 18 April 2018 11:04:33 PM NZST Robert Yang wrote:
> The utils.setup_django() costs a lot of time, but both update.py and
> update_layer.py calls it, so move layer validation from update_layer.py to
> update.py to avoid calling update_layer.py when possible can save a lot of
> time.

Unfortunately I still can't merge this, as mentioned earlier it breaks 
handling older python 2-bound releases.

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: [PATCH 4/4] update_layer.py: move layer validation to update.py (Performance improve)

[Robert Yang]

Hi Paul,

On 04/19/2018 04:55 AM, Paul Eggleton wrote:
> On Wednesday, 18 April 2018 11:04:33 PM NZST Robert Yang wrote:
>> The utils.setup_django() costs a lot of time, but both update.py and
>> update_layer.py calls it, so move layer validation from update_layer.py to
>> update.py to avoid calling update_layer.py when possible can save a lot of
>> time.
> 
> Unfortunately I still can't merge this, as mentioned earlier it breaks
> handling older python 2-bound releases.

I think you're talking about the one that I sent a few months ago, but this
one is a completely new patch, that one moved tinfoil into update.py which
broke python2, but this patch doesn't, it doesn't touch anything about tinfo,
so it works when I switch from YP 2.0 -> 2.2 -> 2.4, and vice visa.

// Robert

> 
> Cheers,
> Paul
> 

Re: [PATCH 0/4][layerindex-web] bug fix and performance improve

[Robert Yang]

Hi Ross,

On 04/18/2018 07:12 PM, Burton, Ross wrote:
> On 18 April 2018 at 12:04, Robert Yang <liezhi.yang@windriver.com> wrote:
>>    fixup! update: don't stop on unsatisfied layer dependencies
> 
> Pretty sure this isn't what you intended.

Thanks, but this is intended, I used:

$ git commit --fixup=8dd24abf99578e3d2572062ca04dbeac4321b2ea

While the 8dd24abf99578e3d2572062ca04dbeac4321b2ea is made by Paul,
I use this way to specify what it fixes, I can update the subject line
if this is not acceptable.

// Robert

> 
> Ross
> 

Re: [PATCH 4/4] update_layer.py: move layer validation to update.py (Performance improve)

[Paul Eggleton]

Hi Robert

On Thursday, 19 April 2018 6:45:17 PM NZST Robert Yang wrote:
> On 04/19/2018 04:55 AM, Paul Eggleton wrote:
> > On Wednesday, 18 April 2018 11:04:33 PM NZST Robert Yang wrote:
> >> The utils.setup_django() costs a lot of time, but both update.py and
> >> update_layer.py calls it, so move layer validation from update_layer.py 
to
> >> update.py to avoid calling update_layer.py when possible can save a lot 
of
> >> time.
> > 
> > Unfortunately I still can't merge this, as mentioned earlier it breaks
> > handling older python 2-bound releases.
> 
> I think you're talking about the one that I sent a few months ago, but this
> one is a completely new patch, that one moved tinfoil into update.py which
> broke python2, but this patch doesn't, it doesn't touch anything about 
> tinfo,
> so it works when I switch from YP 2.0 -> 2.2 -> 2.4, and vice visa.

My apologies - I didn't read the patch properly. Let me take a closer look at 
it tomorrow.

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: [PATCH 4/4] update_layer.py: move layer validation to update.py (Performance improve)

[Paul Eggleton]

Hi Robert

I like the performance improvements, thanks. A few comments below though.

On Wednesday, 18 April 2018 11:04:33 PM NZST Robert Yang wrote:
> The utils.setup_django() costs a lot of time, but both update.py and
> update_layer.py calls it, so move layer validation from update_layer.py to
> update.py to avoid calling update_layer.py when possible can save a lot of
> time.
> 
> Now we don't have to call update_layer.py in the following cases:
> * The branch doesn't exist
> * The layer is already update to date on specified branch (when no
>   reload)
> * The layer dir or conf/layer.layer doesn't exist
> 
> We can save up to 98% time in my testing:
> 
> $ update.py -b master --nofetch [--fullreload]
> 
>                    Before    Now       Reduced
> No update:         276s      3.6s      98%
> Partial update:    312s      87s       72%
> Full repload:      1016s     980s      3%
> 
> Note:
> * All of the testing are based on --nofetch
> 
> * "No update" means all layers on the branch is up-to-date, for
>   example, when we run it twice, there is no update in the second run, so we
>   only need about 3s now, which is the most common case when we use cron to run
>   it per half an hour.
> 
> * "Partly update" means part of the layers have been updated.
> 
> * "Fullreload" means all of the layers have been updated.
> 
> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
> ---
>  layerindex/update.py       | 79 +++++++++++++++++++++++++++++++++++++++++++++-
>  layerindex/update_layer.py | 39 +++--------------------
>  2 files changed, 82 insertions(+), 36 deletions(-)
> 
> diff --git a/layerindex/update.py b/layerindex/update.py
> index 44a7b90..2bb2df7 100755
> --- a/layerindex/update.py
> +++ b/layerindex/update.py
> @@ -140,6 +140,15 @@ def fetch_repo(vcs_url, repodir, urldir, fetchdir, layer_name):
>          logger.error("Fetch of layer %s failed: %s" % (layer_name, e.output))
>          return (vcs_url, e.output)
>  
> +def print_subdir_error(newbranch, layername, vcs_subdir, branchdesc):
> +    # This will error out if the directory is completely invalid or had never existed at this point
> +    # If it previously existed but has since been deleted, you will get the revision where it was
> +    # deleted - so we need to handle that case separately later
> +    if newbranch:
> +        logger.info("Skipping update of layer %s for branch %s - subdirectory %s does not exist on this branch" % (layername, branchdesc, vcs_subdir))
> +    elif vcs_subdir:
> +        logger.error("Subdirectory for layer %s does not exist on branch %s - if this is legitimate, the layer branch record should be deleted" % (layername, branchdesc))
> +
>  def main():
>      if LooseVersion(git.__version__) < '0.3.1':
>          logger.error("Version of GitPython is too old, please install GitPython (python-git) 0.3.1 or later in order to use this script")
> @@ -195,7 +204,7 @@ def main():
>  
>      utils.setup_django()
>      import settings
> -    from layerindex.models import Branch, LayerItem, Update, LayerUpdate
> +    from layerindex.models import Branch, LayerItem, Update, LayerUpdate, LayerBranch
>  
>      logger.setLevel(options.loglevel)
>  
> @@ -337,6 +346,74 @@ def main():
>                          collections.add((layerbranch.collection, layerbranch.version))
>  
>                  for layer in layerquery:
> +                    layerbranch = layer.get_layerbranch(branch)
> +                    branchname = branch
> +                    branchdesc = branch
> +                    newbranch = False
> +                    branchobj = utils.get_branch(branch)
> +                    if layerbranch:
> +                        if layerbranch.actual_branch:
> +                            branchname = layerbranch.actual_branch
> +                            branchdesc = "%s (%s)" % (branch, branchname)
> +                    else:
> +                        # LayerBranch doesn't exist for this branch, create it
> +                        newbranch = True
> +                        layerbranch = LayerBranch()
> +                        layerbranch.layer = layer
> +                        layerbranch.branch = branchobj
> +                        layerbranch_source = layer.get_layerbranch(branchobj)
> +                        if not layerbranch_source:
> +                            layerbranch_source = layer.get_layerbranch(None)
> +                        if layerbranch_source:
> +                            layerbranch.vcs_subdir = layerbranch_source.vcs_subdir
> +

It seems that you are missing a call somewhere to layerbranch.save() later
on, guarded with "if newbranch:". This was easy to miss though when copying
over from update_layer.py seeing as we were relying upon saving it later on
there (and that is still needed).

> +                    if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload:
> +                        logger.info("Layer %s is already up-to-date for branch %s" % (layer.name, branchdesc))
> +                        collections.add((layerbranch.collection, layerbranch.version))
> +                        continue
> +
> +                    if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload:
> +                        # Check out appropriate branch
> +                        if not options.nocheckout:
> +                            utils.checkout_layer_branch(layerbranch, repodir, logger=logger)

Isn't "if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload:"
superfluous here? We're skipping to the next layer if that wasn't true via
"continue" above, so it should always evaluate to true.

Additionally this results in the checkout being done twice - once here and
a second time in the update_layer.py script. I guess it's not taking much time
the second time though so we could probably ignore it.

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: [PATCH 4/4] update_layer.py: move layer validation to update.py (Performance improve)

[Robert Yang]

Hi Paul,

Thanks for the reply, please see my comments inline.

On 04/23/2018 09:55 AM, Paul Eggleton wrote:
> Hi Robert
> 
> I like the performance improvements, thanks. A few comments below though.
> 
> On Wednesday, 18 April 2018 11:04:33 PM NZST Robert Yang wrote:
>> The utils.setup_django() costs a lot of time, but both update.py and
>> update_layer.py calls it, so move layer validation from update_layer.py to
>> update.py to avoid calling update_layer.py when possible can save a lot of
>> time.
>>
>> Now we don't have to call update_layer.py in the following cases:
>> * The branch doesn't exist
>> * The layer is already update to date on specified branch (when no
>>    reload)
>> * The layer dir or conf/layer.layer doesn't exist
>>
>> We can save up to 98% time in my testing:
>>
>> $ update.py -b master --nofetch [--fullreload]
>>
>>                     Before    Now       Reduced
>> No update:         276s      3.6s      98%
>> Partial update:    312s      87s       72%
>> Full repload:      1016s     980s      3%
>>
>> Note:
>> * All of the testing are based on --nofetch
>>
>> * "No update" means all layers on the branch is up-to-date, for
>>    example, when we run it twice, there is no update in the second run, so we
>>    only need about 3s now, which is the most common case when we use cron to run
>>    it per half an hour.
>>
>> * "Partly update" means part of the layers have been updated.
>>
>> * "Fullreload" means all of the layers have been updated.
>>
>> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
>> ---
>>   layerindex/update.py       | 79 +++++++++++++++++++++++++++++++++++++++++++++-
>>   layerindex/update_layer.py | 39 +++--------------------
>>   2 files changed, 82 insertions(+), 36 deletions(-)
>>
>> diff --git a/layerindex/update.py b/layerindex/update.py
>> index 44a7b90..2bb2df7 100755
>> --- a/layerindex/update.py
>> +++ b/layerindex/update.py
>> @@ -140,6 +140,15 @@ def fetch_repo(vcs_url, repodir, urldir, fetchdir, layer_name):
>>           logger.error("Fetch of layer %s failed: %s" % (layer_name, e.output))
>>           return (vcs_url, e.output)
>>   
>> +def print_subdir_error(newbranch, layername, vcs_subdir, branchdesc):
>> +    # This will error out if the directory is completely invalid or had never existed at this point
>> +    # If it previously existed but has since been deleted, you will get the revision where it was
>> +    # deleted - so we need to handle that case separately later
>> +    if newbranch:
>> +        logger.info("Skipping update of layer %s for branch %s - subdirectory %s does not exist on this branch" % (layername, branchdesc, vcs_subdir))
>> +    elif vcs_subdir:
>> +        logger.error("Subdirectory for layer %s does not exist on branch %s - if this is legitimate, the layer branch record should be deleted" % (layername, branchdesc))
>> +
>>   def main():
>>       if LooseVersion(git.__version__) < '0.3.1':
>>           logger.error("Version of GitPython is too old, please install GitPython (python-git) 0.3.1 or later in order to use this script")
>> @@ -195,7 +204,7 @@ def main():
>>   
>>       utils.setup_django()
>>       import settings
>> -    from layerindex.models import Branch, LayerItem, Update, LayerUpdate
>> +    from layerindex.models import Branch, LayerItem, Update, LayerUpdate, LayerBranch
>>   
>>       logger.setLevel(options.loglevel)
>>   
>> @@ -337,6 +346,74 @@ def main():
>>                           collections.add((layerbranch.collection, layerbranch.version))
>>   
>>                   for layer in layerquery:
>> +                    layerbranch = layer.get_layerbranch(branch)
>> +                    branchname = branch
>> +                    branchdesc = branch
>> +                    newbranch = False
>> +                    branchobj = utils.get_branch(branch)
>> +                    if layerbranch:
>> +                        if layerbranch.actual_branch:
>> +                            branchname = layerbranch.actual_branch
>> +                            branchdesc = "%s (%s)" % (branch, branchname)
>> +                    else:
>> +                        # LayerBranch doesn't exist for this branch, create it
>> +                        newbranch = True
>> +                        layerbranch = LayerBranch()
>> +                        layerbranch.layer = layer
>> +                        layerbranch.branch = branchobj
>> +                        layerbranch_source = layer.get_layerbranch(branchobj)
>> +                        if not layerbranch_source:
>> +                            layerbranch_source = layer.get_layerbranch(None)
>> +                        if layerbranch_source:
>> +                            layerbranch.vcs_subdir = layerbranch_source.vcs_subdir
>> +
> 
> It seems that you are missing a call somewhere to layerbranch.save() later
> on, guarded with "if newbranch:". This was easy to miss though when copying
> over from update_layer.py seeing as we were relying upon saving it later on
> there (and that is still needed).

We don't need it to save the new branch here, we just want to know whether it is
a new branch or not, the logic is:

if newbranch and "the branch doesn't exist" in the repo:
     continue
else: # "the branch exits in the repo"
     update_layer.py

So we don't have to save the new branch here, the update_layer.py will save the
new branch when needed, this can reducing a lot of calls to update_layer, which
can save a lot of time.

> 
>> +                    if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload:
>> +                        logger.info("Layer %s is already up-to-date for branch %s" % (layer.name, branchdesc))
>> +                        collections.add((layerbranch.collection, layerbranch.version))
>> +                        continue
>> +
>> +                    if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload:
>> +                        # Check out appropriate branch
>> +                        if not options.nocheckout:
>> +                            utils.checkout_layer_branch(layerbranch, repodir, logger=logger)
> 
> Isn't "if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload:"
> superfluous here? We're skipping to the next layer if that wasn't true via
> "continue" above, so it should always evaluate to true.

Let's ignore the update.reload here, then the logic will be clear:

if layerbranch.vcs_last_rev == topcommit.hexsha:
     continue
else:
     checkout_layer_branch()

So we can't ignore the else block here, the "if" block doesn't cover the "else"
block. And If update.reload, then we can't continue, so the first "if" is:
"if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload"

And for the second "if", we need checkout the branch when:
"layerbranch.vcs_last_rev != topcommit.hexsha or update.reload"

And check whether there is a conf/layer.layer in the repo, we have a lot of
layers which are only for specific releases, for example, a layer is for morty
branch only, then we cleanup its master branch, just leave a README
(no conf/layer.conf) file on master branch which says this layer is for morty
only, check conf/layer.conf in update.py rather than in update_layer.py can
save a lot of time, too.

> 
> Additionally this results in the checkout being done twice - once here and
> a second time in the update_layer.py script. I guess it's not taking much time
> the second time though so we could probably ignore it.

Yes, you're right, the second checkout will be much faster, and it's worth to do
it in update.py since calling update_layer.py is very slow.

The update_layer.py is slow is because setup_django() is slow, and we call
setup_django() in both update.py and update_layer.py, so avoid calling
update_layer.py when possible can save a lot of time.

// Robert

> 
> Cheers,
> Paul
> 

Re: [PATCH 4/4] update_layer.py: move layer validation to update.py (Performance improve)

[Paul Eggleton]

On Monday, 23 April 2018 3:06:43 PM NZST Robert Yang wrote:
> On 04/23/2018 09:55 AM, Paul Eggleton wrote:
> > It seems that you are missing a call somewhere to layerbranch.save() later
> > on, guarded with "if newbranch:". This was easy to miss though when copying
> > over from update_layer.py seeing as we were relying upon saving it later on
> > there (and that is still needed).
> 
> We don't need it to save the new branch here, we just want to know whether it is
> a new branch or not, the logic is:
> 
> if newbranch and "the branch doesn't exist" in the repo:
>      continue
> else: # "the branch exits in the repo"
>      update_layer.py
> 
> So we don't have to save the new branch here, the update_layer.py will save the
> new branch when needed, this can reducing a lot of calls to update_layer, which
> can save a lot of time.

OK - I had missed that you hadn't removed the code from
update_layer.py that does the actual creation, so the logic is fine. 
I think however we need to adjust the "LayerBranch doesn't exist
for this branch, create it" comment here though, something like
"LayerBranch doesn't exist for this branch, create it temporarily
(we won't save this - update_layer.py will do the actual creation
if it gets called)."
 
> >> +                    if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload:
> >> +                        logger.info("Layer %s is already up-to-date for branch %s" % (layer.name, branchdesc))
> >> +                        collections.add((layerbranch.collection, layerbranch.version))
> >> +                        continue
> >> +
> >> +                    if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload:
> >> +                        # Check out appropriate branch
> >> +                        if not options.nocheckout:
> >> +                            utils.checkout_layer_branch(layerbranch, repodir, logger=logger)
> > 
> > Isn't "if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload:"
> > superfluous here? We're skipping to the next layer if that wasn't true via
> > "continue" above, so it should always evaluate to true.
> 
> Let's ignore the update.reload here, then the logic will be clear:
> 
> if layerbranch.vcs_last_rev == topcommit.hexsha:
>      continue
> else:
>      checkout_layer_branch()
>
> So we can't ignore the else block here, the "if" block doesn't cover the "else"
> block. And If update.reload, then we can't continue, so the first "if" is:
> "if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload"
> 
> And for the second "if", we need checkout the branch when:
> "layerbranch.vcs_last_rev != topcommit.hexsha or update.reload"
> 
> And check whether there is a conf/layer.layer in the repo, we have a lot of
> layers which are only for specific releases, for example, a layer is for morty
> branch only, then we cleanup its master branch, just leave a README
> (no conf/layer.conf) file on master branch which says this layer is for morty
> only, check conf/layer.conf in update.py rather than in update_layer.py can
> save a lot of time, too.

Right, I understand the logic, what I'm saying is that the two if statements
are opposites, and the first one results in a "continue" if true, so the second one
can logically never evaluate to false, thus there's no point doing the check - just
put the code inside the block immediately afterwards, i.e.:

                    if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload:
                        logger.info("Layer %s is already up-to-date for branch %s" % (layer.name, branchdesc))
                        collections.add((layerbranch.collection, layerbranch.version))
                        continue

                    # Check out appropriate branch
                    if not options.nocheckout:
                        utils.checkout_layer_branch(layerbranch, repodir, logger=logger)
                    ...

If you wanted to make it even more clear you could have it in an "else" block
instead, but that's not strictly necessary.

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: [PATCH 4/4] update_layer.py: move layer validation to update.py (Performance improve)

[Robert Yang]

Yes, you're right, I updated the two comments in the repo:

+                    if layerbranch:
+                        if layerbranch.actual_branch:
+                            branchname = layerbranch.actual_branch
+                            branchdesc = "%s (%s)" % (branch, branchname)
+                    else:
+                        # LayerBranch doesn't exist for this branch, create it 
temporarily
+                        # (we won't save this - update_layer.py will do the 
actual creation
+                        # if it gets called).
+                        newbranch = True
+                        layerbranch = LayerBranch()
+                        layerbranch.layer = layer

...

+                    if layerbranch.vcs_last_rev == topcommit.hexsha and not 
update.reload:
+                        logger.info("Layer %s is already up-to-date for branch 
%s" % (layer.name, branchdesc))
+                        collections.add((layerbranch.collection, 
layerbranch.version))
+                        continue
+                    else:
+                        # Check out appropriate branch
+                        if not options.nocheckout:
+                            utils.checkout_layer_branch(layerbranch, repodir, 
logger=logger)
+                        layerdir = os.path.join(repodir, layerbranch.vcs_subdir)
+                        if layerbranch.vcs_subdir and not os.path.exists(layerdir):
+                            print_subdir_error(newbranch, layer.name, 
layerbranch.vcs_subdir, branchdesc)
+                            continue
+
+                        if not os.path.exists(os.path.join(layerdir, 
'conf/layer.conf')):
+                            logger.error("conf/layer.conf not found for layer 
%s - is subdirectory set correctly?" % layer.name)
+                            continue

// Robert

On 04/23/2018 11:35 AM, Paul Eggleton wrote:
> On Monday, 23 April 2018 3:06:43 PM NZST Robert Yang wrote:
>> On 04/23/2018 09:55 AM, Paul Eggleton wrote:
>>> It seems that you are missing a call somewhere to layerbranch.save() later
>>> on, guarded with "if newbranch:". This was easy to miss though when copying
>>> over from update_layer.py seeing as we were relying upon saving it later on
>>> there (and that is still needed).
>>
>> We don't need it to save the new branch here, we just want to know whether it is
>> a new branch or not, the logic is:
>>
>> if newbranch and "the branch doesn't exist" in the repo:
>>       continue
>> else: # "the branch exits in the repo"
>>       update_layer.py
>>
>> So we don't have to save the new branch here, the update_layer.py will save the
>> new branch when needed, this can reducing a lot of calls to update_layer, which
>> can save a lot of time.
> 
> OK - I had missed that you hadn't removed the code from
> update_layer.py that does the actual creation, so the logic is fine.
> I think however we need to adjust the "LayerBranch doesn't exist
> for this branch, create it" comment here though, something like
> "LayerBranch doesn't exist for this branch, create it temporarily
> (we won't save this - update_layer.py will do the actual creation
> if it gets called)."
>   
>>>> +                    if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload:
>>>> +                        logger.info("Layer %s is already up-to-date for branch %s" % (layer.name, branchdesc))
>>>> +                        collections.add((layerbranch.collection, layerbranch.version))
>>>> +                        continue
>>>> +
>>>> +                    if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload:
>>>> +                        # Check out appropriate branch
>>>> +                        if not options.nocheckout:
>>>> +                            utils.checkout_layer_branch(layerbranch, repodir, logger=logger)
>>>
>>> Isn't "if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload:"
>>> superfluous here? We're skipping to the next layer if that wasn't true via
>>> "continue" above, so it should always evaluate to true.
>>
>> Let's ignore the update.reload here, then the logic will be clear:
>>
>> if layerbranch.vcs_last_rev == topcommit.hexsha:
>>       continue
>> else:
>>       checkout_layer_branch()
>>
>> So we can't ignore the else block here, the "if" block doesn't cover the "else"
>> block. And If update.reload, then we can't continue, so the first "if" is:
>> "if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload"
>>
>> And for the second "if", we need checkout the branch when:
>> "layerbranch.vcs_last_rev != topcommit.hexsha or update.reload"
>>
>> And check whether there is a conf/layer.layer in the repo, we have a lot of
>> layers which are only for specific releases, for example, a layer is for morty
>> branch only, then we cleanup its master branch, just leave a README
>> (no conf/layer.conf) file on master branch which says this layer is for morty
>> only, check conf/layer.conf in update.py rather than in update_layer.py can
>> save a lot of time, too.
> 
> Right, I understand the logic, what I'm saying is that the two if statements
> are opposites, and the first one results in a "continue" if true, so the second one
> can logically never evaluate to false, thus there's no point doing the check - just
> put the code inside the block immediately afterwards, i.e.:
> 
>                      if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload:
>                          logger.info("Layer %s is already up-to-date for branch %s" % (layer.name, branchdesc))
>                          collections.add((layerbranch.collection, layerbranch.version))
>                          continue
> 
>                      # Check out appropriate branch
>                      if not options.nocheckout:
>                          utils.checkout_layer_branch(layerbranch, repodir, logger=logger)
>                      ...
> 
> If you wanted to make it even more clear you could have it in an "else" block
> instead, but that's not strictly necessary.
> 
> Cheers,
> Paul
>