WIP - nso-nipap rewrite #1049
WIP - nso-nipap rewrite #1049
Conversation
| private int th = -1; | ||
| private NavuContainer ncsRoot; | ||
| private NavuContainer operRoot; | ||
| private int th = -1; |
| * | ||
| * @param path Path to the prefix request | ||
| * @param parentPrefix From prefix | ||
| * @throws Exception |
There was a problem hiding this comment.
Can we be a bit more specific about the exceptions?
| try { | ||
|
|
||
| AddPrefixOptions child_opts = new AddPrefixOptions(); | ||
| child_opts.put("prefix_length", "32"); |
There was a problem hiding this comment.
32 sounds like IPv4... shouldn't this at least be based on AFI?
| { | ||
| Prefix p = oldPrefix; | ||
|
|
||
| if (maapi.exists(th, attributePath + "/" + nipap._customer_id_)) { |
There was a problem hiding this comment.
indent here seems real messed up
| /** | ||
| * Fetch attributes and returns the populated prefix object | ||
| * | ||
| * @param oldPrefix Used if you already have a prefix object |
There was a problem hiding this comment.
What's the deal with oldPrefix ? Why is this used? Why do we ever want reuse of these objects?
| ConfUInt32 prefixIdValue = new ConfUInt32(prefix.id); | ||
| wsess.setElem(prefixIdValue, responsePath + "/" + nipap._prefix_id_); | ||
|
|
||
| if(prefix.customer_id != null){ |
There was a problem hiding this comment.
space between if and (
fix similar below
| * @throws Exception | ||
| */ | ||
| protected int getPrefixId(String path) throws Exception { | ||
| return (int) ((ConfUInt32)wsess.getElem(path + "/" + |
There was a problem hiding this comment.
loss of precision here, right? Why not use a long throughout?
| poolSpec.put("name", poolName); | ||
| List poolRes = Pool.list(nipapCon, poolSpec); | ||
|
|
||
| if(poolRes.size() < 1){ |
There was a problem hiding this comment.
Check for != 1. No risk of getting 2 or more now but if someone changes the poolSpec code above there is a risk. No reason not to check for exactly one.
| try { | ||
|
|
||
| AddPrefixOptions child_opts = new AddPrefixOptions(); | ||
| child_opts.put("prefix_length", "32"); |
There was a problem hiding this comment.
We should probably not hard-code prefix lengths to 32.
Various style & indent fixes.
Renamed some functions to make it clear whether they operate on CDB or NIPAP.
To make it clear what types of errors which can occurr in the different helper functions, all exceptions thrown are now listed instead of a generic "Exception". Also make some minor style fixes.
Updated the addHostPrefixFromPrefix function to set the host prefix prefix length based on IP version.
fredsod
force-pushed
the
nso-nipap-rewrite
branch
from
b21ceae
to
29d60b5
Compare
| tailf:persistent true; | ||
| } | ||
|
|
||
| choice response-choice { |
There was a problem hiding this comment.
This looks like somewhat of an anti-pattern to me. Better have a leaf for status, it can be an enum or something with "ok", "error" or whatever. Then you have a separate leaf that contains the error message. Right now you don't even have a leaf under the "ok" case so there's nothing there to read which is über weird ;) because you can't go to one single path and get the status, you have to probe around
Added templates for creating NIPAP allocation requests.
Added python helper functions for creating NIPAP requests.
Python helpers for creating NIPAP requests
| attributes['node'] if 'node' in attributes else '-1') | ||
|
|
||
| template_vars.add('ORDER_ID', | ||
| attributes['order_id'] if 'order_id' in attributes else '-1') |
There was a problem hiding this comment.
continuation line under-indented for visual indent
| attributes['description'] if 'description' in attributes else '-1') | ||
|
|
||
| template_vars.add('NODE', | ||
| attributes['node'] if 'node' in attributes else '-1') |
There was a problem hiding this comment.
continuation line under-indented for visual indent
| attributes['customer_id'] if 'customer_id' in attributes else '-1') | ||
|
|
||
| template_vars.add('DESCRIPTION', | ||
| attributes['description'] if 'description' in attributes else '-1') |
There was a problem hiding this comment.
continuation line under-indented for visual indent
| """ | ||
|
|
||
| template_vars.add('CUSTOMER_ID', | ||
| attributes['customer_id'] if 'customer_id' in attributes else '-1') |
There was a problem hiding this comment.
continuation line under-indented for visual indent
| if from_prefix_allocation_name not in alloc.from_prefix_request: | ||
| return None | ||
|
|
||
| from_pref_alloc = alloc.from_prefix_request[from_prefix_allocation_name] |
There was a problem hiding this comment.
line too long (80 > 79 characters)
| Arguments: | ||
| root -- a maagic root for the current transaction | ||
| pool_name -- name of pool to request from | ||
| main_allocation_name -- name of allocation which the from-prefix allocation belongs to |
There was a problem hiding this comment.
line too long (94 > 79 characters)
| return None | ||
|
|
||
|
|
||
| def from_prefix_read(root, pool_name, main_allocation_name, from_prefix_allocation_name): |
There was a problem hiding this comment.
line too long (89 > 79 characters)
| Arguments: | ||
| service -- the requesting service node | ||
| pool_name -- name of pool to request from | ||
| main_allocation_name -- name of main allocation which the from-prefix is appended to |
There was a problem hiding this comment.
line too long (92 > 79 characters)
|
|
||
|
|
||
| def from_prefix_request(service, pool_name, main_allocation_name, | ||
| from_pref_allocation_name, prefix_attributes=None): |
There was a problem hiding this comment.
continuation line under-indented for visual indent
|
|
||
|
|
||
| def prefix_request(service, svc_xpath, pool_name, allocation_name, | ||
| prefix_length, family=4, prefix_attributes=None): |
There was a problem hiding this comment.
continuation line under-indented for visual indent
Improve error handling in nso-nipap to avoid locking CDB on certain failures, for example when unable to communicate with the NIPAP backend. Certain errors caused exceptions to be thrown which were not handled within the request loop which caused sub.sync() to never be run.
Improve error handling to avoid CDB lockup
Pass the path to re-deploy as a ConfPath-object rather than as a String. From some reason the string, which is extracted directly from CDB, can't be parsed by the ConfPath-constructor.
nso-nipap: Pass re-deploy path as ConfPath
This is almost a complete rewrite of the nso-nipap cdb subscriber.
Todo: