GitLab

Re-think a very old idea in assign - that no_connect violations for
unmapped links should not be assessed against links whose endpoints are
not mapped. The problem with this is that assign is too hesitant to map
nodes - if there is more than one link that can't be mapped or whose
other end isn't mapped yet, then assign thinks it's better to leave the
node unmapped. This leads to confusing solutions (in which nodes remain
unmapped when really it's the links that are the problem), and I beleive
it keeps assign from exploring valuable parts of the solution space.

This patch chanes the behavior, so that no_connect violations get scored
even when the endpoints of the link are not mapped.

Not well enough tested to run in production.

I think this must get the award for oldest (frequently excercised) bug
in the Emulab source. Thanks to the magic of the git pickaxe, I
discovered that code with this problem was first committed Monday, May
22, 2000, when it used to be in assign_hw/assign.cc

assign used to use a priority queue to 'randomly' select unassigned
nodes to try assigning. Turns out this is not very random! A node can
get unlucky, get a very low random priority assigned to it, and get
stuck in the queue for a very, very, very long time. As a result, I've
seen assign try to re-assign the same node several thousand times in a
row, when there are others waiting to be re-assigned. Of course, this is
going to result in very poor exploration of the state space.

I changed the unassigned node selection process to use a regular list,
and select an item from the list at random at the time of item removal.
This gets a much more even distribution of nodes, at the small cost of
linear iteration over the list of unassigned nodes.  This should be a
pretty minor cost, though, as iteration over this list should be cheap
and the list itself should generally be pretty small.

Also added some debugging statements.

Add debugging statements useful in tracking down which vnodes assign attempts
to assign to which pnodes.

Was returning a structure with a pointer to a stack-allocated string in
it. Use knowledge about where that string came from to avoid having to
make a copy of it, instead.

There is still a minor memory leak, but left behind a note rather than
fixing it (which would be quite involved).

Because it's operating on char*s, the STL list remove() function
doesn't work; it needs to do a strcmp, not a simple comparison. So, hand-write
the loop to do this.

Also add a santity check to produce a fatal error rather than allowing this
function to go into an infinite loop.

Think I got it this time (yeah, I've said that four times before).
It requires some extra bookkeeping; we were recording source and
dest interface names in tb_plink, but not source and dest node
names. With this new information, find_best_link() now has sole
responsibility for figuring out if the vlink and plink go in the
same direction, and the callers of find_best_link() are now
responsible for knowing which direction they are traversing the
vlink in (and thus whether they should tell find_best_link() to
check the source or dest of the vlink.)

Okay, I know I said this the first three times, but I think I've got it
this time - the problem had to do with how I was handling the flipped
flag (of course). Many of the calls find_best_link() give the dest_pv
of a link instead of the pv (source pnode). When we do this, we have to
invert the flipped flag, 'cause we're effectively reversing the order
of the link.

Normalized the error message for excessive link properties and corrected the behaviour of the parser when no link properties are specified.

than earlier.

The main change is to add a NEWER_GCC macro, separate from NEW_GCC, to
detect GCC 4.3 and up.

This is necessary because, in GCC 4.3, hash_fun.h moved from ext/ to
backward/.  New comments in port.h give the details.

common.h, fstring.h: use NEWER_GCC to locate hash_fun.h.  port.h:
define NEWER_GCC if GCC >= 4.3.

If a request was made for a node type that was not present in the physical
topology, then the error message was, "Too much bandwidth on emulated links."
This change adds a flag in mapping_precheck() that remembers whether the
requested node type was found.  If not, then a new error message is printed
instead of the bandwidth one.

components out of order. Other minor fixes included to get GCC to
reduce the number of GCC warnings.

pointers not copied from the values returned by XStr. To test if this
was the cause of the memory corruption, the "fix" of performing a deep
copy of the virtual root before annotating. If this doesn't work,
re-enabling the fix should be easy enough.

Fixed minor type mismatches which were giving out warnings during make. I don't think any were particularly harmful.

the problem causing the seg-faults (sadly valgrind wasn't terribly
insightful either), but it is something bad that was fixed.

Fixes the issue of missing interfaces as needed for the post-assign phase. removed dangling XStr usage, although I need to trace down more illegal uses if any exist.

More checks on pointers inside annotate because I still can't reproduce the core dumping that Leigh is facing.

Removed stray array which had been allocated on the stack. I don't know if this is responsible for the crashes that Leigh is reporting, but it wasn't a good thing and it's gone now.

Removing unnecessary debug statements which weren't removed earlier in excessive eagerness to commit and push code.

Fixed a bug dealing with exclusive nodes. Modified the parser to check for both an element exclusive and an attribute exclusive (did I mention that I am starting to detest v1?).

An unrelated bug which would have caused some nasty type-precheck failures also got fixed in the process.

IMP: The change to score.cc undoes the change that Rob made some time ago to correct some fixed interface bug. I am not sure what that bug was but if I leave Rob's changes in, assign fails to honour the fixed interface and maps the link onto some other interface.

Annotations work - well, ok, they haven't been rigorously tested yet, but I think they'll hold up. The only thing that is almost certainly broken is the conversion from links with multiple interfaces into the "node + point-to-point" links that assign knows how to deal with.

Mapper now has a flag to determine whether to output the text format or rspec. rspec outputs are named *.ptop.xml and *.vtop.xml for the advertisement and request respectively.

Add extension and vclass support. It should be working correctly now. There's a lot of ugliness w.r.t. hardware types and sliver types, while dealing with switches, nodes and vclasses. I have tried to hide most of it in rspec_parser_v2.cc and rspec_parser_helper.cc.

Fixed some bugs in the parsers. A full features and desires test now works. There are stil some extension support which needs to be added to ptopgen.

Added support for set-type-limits in the advertisement. It is implemented as an extension. There is no support for this in v1. It will only be in v2 and higher. To this end, ptopgen has been modified. ptopgen also now adds type slot information to nodes as an extension. However, support for other extensions still have to be added. Added another annotater class for rspec v2 but this in a very incomplete state as of now and the relevant code has been commented out of solution.cc temporarily. Assign's arguments have also been modified in mapper, so that it expects rspec inputs.

Cleared up yet another merge conflict in the advertisement parser and updated the makefile for assign

I now think my last bugfix was incorrect - what we need to change when
the link is flipped is which end we compare, rather than which interface
on the vlink we compare with.

Quite a bear to track down!