From 83203623cd03c7247f91cf826086f5bf5a3a156d Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Thu, 23 Jul 2020 20:41:32 +0200 Subject: [PATCH] Optimized get config xpath of large lists, such as `a[x=1000]` in a list of 100000s `a:s`. --- CHANGELOG.md | 1 + lib/src/clixon_datastore_read.c | 148 +++++++++++++++++++++++++++++--- lib/src/clixon_xml_sort.c | 9 +- test/test_perf_cli.sh | 6 +- test/test_perf_netconf.sh | 13 ++- test/test_perf_restconf.sh | 2 - test/test_rpc.sh | 12 +-- test/test_upgrade_module.sh | 6 +- 8 files changed, 163 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b475f5b7..d148b086 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -81,6 +81,7 @@ Expected: July 2020 ### Minor changes +* Optimized get config xpath of large lists, such as `a[x=1000]` in a list of 100000s `a:s`. * Added docker support for three restconf modes: nginx/fcgi(default); evhtp ; and none. * Added [Vagrant tests](test/vagrant/README.md) * Added new function `clicon_xml2str()` to complement xml_print and others that returns a malloced string. diff --git a/lib/src/clixon_datastore_read.c b/lib/src/clixon_datastore_read.c index d52752c0..7d2b93e6 100644 --- a/lib/src/clixon_datastore_read.c +++ b/lib/src/clixon_datastore_read.c @@ -120,6 +120,115 @@ singleconfigroot(cxobj *xt, done: return retval; } + +/*! Recurse up from x0 up to x0t then create objects from x1t down to new object x1 + */ +static int +xml_copy_bottom_recurse(cxobj *x0t, + cxobj *x0, + cxobj *x1t, + cxobj **x1pp) +{ + int retval = -1; + cxobj *x0p = NULL; + cxobj *x1p = NULL; + cxobj *x1 = NULL; + cxobj *x1a = NULL; + cxobj *x0a = NULL; + cxobj *x0k; + cxobj *x1k; + yang_stmt *y = NULL; + cvec *cvk = NULL; /* vector of index keys */ + cg_var *cvi; + char *keyname; + + if (x0 == x0t){ + *x1pp = x1t; + goto ok; + } + if ((x0p = xml_parent(x0)) == NULL){ + clicon_err(OE_XML, EFAULT, "Reached top of tree"); + goto done; + } + if (xml_copy_bottom_recurse(x0t, x0p, x1t, &x1p) < 0) + goto done; + y = xml_spec(x0); + /* Look if it exists */ + if (match_base_child(x1p, x0, y, &x1) < 0) + goto done; + if (x1 == NULL){ /* If not, create it and copy it one level only */ + if ((x1 = xml_new(xml_name(x0), x1p, CX_ELMNT)) == NULL) + goto done; + if (xml_copy_one(x0, x1) < 0) + goto done; + /* Copy all attributes */ + x0a = NULL; + while ((x0a = xml_child_each(x0, x0a, -1)) != NULL) { + /* Assume ordered, skip after attributes */ + if (xml_type(x0a) != CX_ATTR) + break; + if ((x1a = xml_new(xml_name(x0a), x1, CX_ATTR)) == NULL) + goto done; + if (xml_copy_one(x0a, x1a) < 0) + goto done; + } + /* Key nodes in lists are copied */ + if (y && yang_keyword_get(y) == Y_LIST){ + /* Loop over all key variables */ + cvk = yang_cvec_get(y); /* Use Y_LIST cache, see ys_populate_list() */ + cvi = NULL; + /* Iterate over individual keys */ + while ((cvi = cvec_each(cvk, cvi)) != NULL) { + keyname = cv_string_get(cvi); + if ((x0k = xml_find_type(x0, NULL, keyname, CX_ELMNT)) != NULL){ + if ((x1k = xml_new(keyname, x1, CX_ELMNT)) == NULL) + goto done; + if (xml_copy(x0k, x1k) < 0) + goto done; + } + + } + } + } + *x1pp = x1; + ok: + retval = 0; + done: + return retval; +} + +static int +xml_copy_from_bottom(cxobj *x0t, + cxobj *x0, + cxobj *x1t) +{ + int retval = -1; + cxobj *x1p = NULL; + cxobj *x0p = NULL; + cxobj *x1 = NULL; + yang_stmt *y = NULL; + + if (x0 == x0t) + goto ok; + x0p = xml_parent(x0); + if (xml_copy_bottom_recurse(x0t, x0p, x1t, &x1p) < 0) + return -1; + y = xml_spec(x0); + /* Look if it exists */ + if (match_base_child(x1p, x0, y, &x1) < 0) + goto done; + if (x1 == NULL){ /* If not, create it and copy complete tree */ + if ((x1 = xml_new(xml_name(x0), x1p, CX_ELMNT)) == NULL) + goto done; + if (xml_copy(x0, x1) < 0) + goto done; + } + ok: + retval = 0; + done: + return retval; +} + /*! Given XML tree x0 with marked nodes, copy marked nodes to new tree x1 * Two marks are used: XML_FLAG_MARK and XML_FLAG_CHANGE * @@ -563,20 +672,33 @@ xmldb_get_cache(clicon_handle h, goto done; xml_spec_set(x1t, xml_spec(x0t)); - /* Iterate through the match vector - * For every node found in x0, mark the tree up to t1 - */ - for (i=0; i.. but is .. */ /* XXX where should we apply default values once? */ if (xml_default_recurse(x1t) < 0) diff --git a/lib/src/clixon_xml_sort.c b/lib/src/clixon_xml_sort.c index 826d75d6..87289a69 100644 --- a/lib/src/clixon_xml_sort.c +++ b/lib/src/clixon_xml_sort.c @@ -1129,11 +1129,11 @@ xml_sort_verify(cxobj *x0, /*! Given child tree x1c, find (first) matching child in base tree x0 and return as x0cp * @param[in] x0 Base tree node * @param[in] x1c Modification tree child - * @param[in] yc Yang spec of tree child + * @param[in] yc Yang spec of tree child. If null revert to linear search. * @param[out] x0cp Matching base tree child (if any) * @retval 0 OK * @retval -1 Error - * XXX: only handles first match + * @note only handles first match */ int match_base_child(cxobj *x0, @@ -1153,6 +1153,11 @@ match_base_child(cxobj *x0, clixon_xvec *xvec = NULL; *x0cp = NULL; /* init return value */ + /* Revert to simple xml lookup if no yang */ + if (yc == NULL){ + *x0cp = xml_find(x0, xml_name(x1c)); + goto ok; + } /* Special case is if yc parent (yp) is choice/case * then find x0 child with same yc even though it does not match lexically * However this will give another y0c != yc diff --git a/test/test_perf_cli.sh b/test/test_perf_cli.sh index 388f9a70..1eed7dce 100755 --- a/test/test_perf_cli.sh +++ b/test/test_perf_cli.sh @@ -100,15 +100,13 @@ new "netconf write large config" expecteof_file "time -p $clixon_netconf -qf $cfg" 0 "$fconfig" "^]]>]]>$" 2>&1 | awk '/real/ {print $2}' # Here, there are $perfnr entries in candidate -new "netconf write large config again" -expecteof_file "time -p $clixon_netconf -qf $cfg" 0 "$fconfig" "^]]>]]>$" 2>&1 | awk '/real/ {print $2}' # Now commit it from candidate to running new "netconf commit large config" expecteof "time -p $clixon_netconf -qf $cfg" 0 "]]>]]>" "^]]>]]>$" 2>&1 | awk '/real/ {print $2}' -# CLI get (XXX why does this take so much time?) -# See: EXPAND_ONLY_INTERACTIVE in cligen. If set it is acceptable but there are some side-effects +# CLI get. This takes a lot of time because expand_dbvar gets perfnr (eg 100000) entries +# and loops over it. new "cli get $perfreq small config 1 key index" { time -p for (( i=0; i<$perfreq; i++ )); do rnd=$(( ( RANDOM % $perfnr ) )) diff --git a/test/test_perf_netconf.sh b/test/test_perf_netconf.sh index 0cc49414..3f0ce9d1 100755 --- a/test/test_perf_netconf.sh +++ b/test/test_perf_netconf.sh @@ -101,8 +101,6 @@ new "netconf write large config" expecteof_file "time -p $clixon_netconf -qf $cfg" 0 "$fconfig" "^]]>]]>$" 2>&1 | awk '/real/ {print $2}' # Here, there are $perfnr entries in candidate -new "netconf write large config again" -expecteof_file "time -p $clixon_netconf -qf $cfg" 0 "$fconfig" "^]]>]]>$" 2>&1 | awk '/real/ {print $2}' # Now commit it from candidate to running new "netconf commit large config" @@ -110,19 +108,26 @@ expecteof "time -p $clixon_netconf -qf $cfg" 0 "]]>]]>" "^]]>]]>" "^]]>]]>$" 2>&1 | awk '/real/ {print $2}' # Having a large db, get and put single entries many times # Note same entries in the range alreay there, db has same size # NETCONF get 1 key index +# Note this is done by streaming input into one single netconf client. it is much +# slower if it is started and stopped for each request. (next) new "netconf get $perfreq small config 1 key index" { time -p for (( i=0; i<$perfreq; i++ )); do rnd=$(( ( RANDOM % $perfnr ) )) echo "]]>]]>" done | $clixon_netconf -qf $cfg > /dev/null; } 2>&1 | awk '/real/ {print $2}' +new "netconf get $perfreq small config 1 key index start/stop" +{ time -p for (( i=0; i<$perfreq; i++ )); do + rnd=$(( ( RANDOM % $perfnr ) )) + echo "]]>]]>" | $clixon_netconf -qf $cfg > /dev/null; +done +} 2>&1 | awk '/real/ {print $2}' + # NETCONF get 1 key and one non-key index new "netconf get $perfreq small config 1 key + 1 non-key index" { time -p for (( i=0; i<$perfreq; i++ )); do diff --git a/test/test_perf_restconf.sh b/test/test_perf_restconf.sh index 850670c1..b121351a 100755 --- a/test/test_perf_restconf.sh +++ b/test/test_perf_restconf.sh @@ -112,8 +112,6 @@ new "netconf write large config" expecteof_file "time -p $clixon_netconf -qf $cfg" 0 "$fconfig" "^]]>]]>$" 2>&1 | awk '/real/ {print $2}' # Here, there are $perfnr entries in candidate -new "netconf write large config again" -expecteof_file "time -p $clixon_netconf -qf $cfg" 0 "$fconfig" "^]]>]]>$" 2>&1 | awk '/real/ {print $2}' # Now commit it from candidate to running new "netconf commit large config" diff --git a/test/test_rpc.sh b/test/test_rpc.sh index d577f7e6..a9efa306 100755 --- a/test/test_rpc.sh +++ b/test/test_rpc.sh @@ -142,24 +142,24 @@ new "netconf edit-config ok" expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' "^]]>]]>$" if ! $YANG_UNKNOWN_ANYDATA ; then -new "netconf edit-config extra arg should fail" +new "netconf edit-config extra arg: should fail" expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' "^applicationunknown-elementextraerrorFailed to find YANG spec of XML node: extra with parent: edit-config in namespace: urn:ietf:params:xml:ns:netconf:base:1.0]]>]]>$" fi -new "netconf edit-config empty target should fail" +new "netconf edit-config empty target: should fail" expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^applicationdata-missingmissing-choiceconfig-targeterror]]>]]>$' -new "netconf edit-config missing target should fail" +new "netconf edit-config missing target: should fail" expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^applicationmissing-elementtargeterrorMandatory variable]]>]]>$' -new "netconf edit-config missing config should fail" +new "netconf edit-config missing config: should fail" expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^applicationdata-missingmissing-choiceedit-contenterror]]>]]>$' # Negative errors (namespace/module missing) -new "netconf wrong rpc namespace (should fail)" +new "netconf wrong rpc namespace: should fail" expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^applicationunknown-elementgeterror]]>]]>$' -new "restconf wrong rpc (should fail" +new "restconf wrong rpc: should fail" expectpart "$(curl $CURLOPTS -X POST -H "Content-Type: application/yang-data+json" u $RCPROTO://localhost/restconf/operations/clixon-foo:get)" 0 'HTTP/1.1 412 Precondition Failed' '{"ietf-restconf:errors":{"error":{"error-type":"protocol","error-tag":"operation-failed","error-severity":"error","error-message":"yang module not found"}}}' if [ $RC -ne 0 ]; then diff --git a/test/test_upgrade_module.sh b/test/test_upgrade_module.sh index 37be014c..c3cf9d16 100755 --- a/test/test_upgrade_module.sh +++ b/test/test_upgrade_module.sh @@ -299,16 +299,16 @@ EOF # There is some issue with having different payloads in the config file # That is why there are tests with different payloads -new "b payload only---------" +new "b payload only" testall '' '' new "b payload and interfaces payload---------" testall '' '' -new "a payload only---------" +new "a payload only" testall '' '' -new "empty payload---------" +new "empty payload" testall '' '' rm -rf $dir