IP Allocation Management

Implements the scheme described at https://github.com/weaveworks/weave/wiki/IP-allocation-design, with address ranges controlled by a CRDT gossiped between peers.
The design doc on the Wiki will be deleted when this is merged.
weave launch takes a new -iprange argument specifying the subnet to work within.
Commands updated to take advantage of IPAM: weave run, weave start, weave attach, weave expose, weave hide
Future work
Currently there is no fall-back range if you don't specify one on the command-line; it would be better to pick from a set of possibilities (checking the range is currently unused before we start using it).
How to use IPAM for WeaveDNS?  It needs its own special subnet.
How should we add support for subnets in general?
We get a bit of noise in the weave logs from containers going down, now that the weave script is calling ethtool and curl via containers. It looks like "Container 1c5a03204b5eea3aeb6f43ece41c3f260a87329bfd89ded34ea3056826b59814 died"
If we hear that a container has died we should knock it out of pending?
Interrogate Docker to check container exists and is alive at the point we assign an IP
To lessen the chance of simultaneous election of two leaders at start-up, peers could wait for a bit to see if more peers arrive before running an election.  Also, we could look at the command-line parameters supplied to Weave, and wait longer in the case that we have been asked to connect to someone specific.
[It would be good to move PeerName out of package router into a more
central package so both ipam and router can depend on it.]
There is no specific logic to time-out requests such as "give me some space": the logic will be re-run and the request re-sent next time something else happens (e.g. it receives another request, or some
periodic gossip). This means we may send out requests more frequently than required, but this is innocuous and it keeps things simple.  It is possible for nothing to happen, e.g. if everyone else has
disconnected.  We could have a timer to re-consider things.
The operation to Split() one space into two, to give space to another Peer, is conceptually simple but the implementation is fiddly to maintain the various lists and limits within MutableSpace. Perhaps a different free-list implementation would make this easier.

@dpw this is the new PR, squashed and ready for review.

Thanks.  But we seem to have some build failures?

There also seems to be a whole bunch of stuff missing - I don't see any changes to existing weave - the script, router main, etc.

18 files changed here vs 29 in #527.

Sorry about missing files.  Should be all there now.

Noted; I will refraim from rewriting previous commits for now because that often loses the context of comments here on GitHub

I'll take another look at this once the first commit is hooked into the makefile 'test' target so that I know that it is self-contained, passes its own tests, etc.
I haven't actually looked at the code in the first and second commit, so my eyes remain mostly fresh.

Commits re-organised and test-passing.  Also extracted -pktdebug change out to a separate PR #567

I can't see what I said about -pktdebug now, but what I meant was split it out into its own commit, not into its own PR (where I see you have incurred the wrath of Matthias due to leaving -debug doing nothing).

I've seen the following bug several times.  I guess I can reproduce it about one time in three, though I have better and worse runs of luck.  Just reproduced it with a build from this branch checked out sometime this morning.
I'm running weave on my laptop host OS (dwragg-laptop) and in a VM (ubuntu1404):
Host:
# ~dwragg/work/weaveworks/go/src/github.com/weaveworks/weave/weave launch -alloc <IP_ADDRESS>/24

VM:
# ~dwragg/work/weaveworks/go/src/github.com/weaveworks/weave/weave launch -alloc <IP_ADDRESS>/24 <IP_ADDRESS>

Host:
# ~dwragg/work/weaveworks/go/src/github.com/weaveworks/weave/weave expose

VM:
# ~dwragg/work/weaveworks/go/src/github.com/weaveworks/weave/weave expose
# ~dwragg/work/weaveworks/go/src/github.com/weaveworks/weave/weave status
weave router git-1f40f87d34f0
Encryption off
Our name is f6:43:c5:fe:24:e9(ubuntu1404)
Sniffing traffic on &{34 65535 ethwe 2a:37:24:d7:30:a7 up|broadcast|multicast}
MACs:
02:e0:38:b0:f1:4d -> 02:e0:38:b0:f1:4d(dwragg-laptop) (2015-04-23 12:37:12.861200086 +0000 UTC)
2a:37:24:d7:30:a7 -> f6:43:c5:fe:24:e9(ubuntu1404) (2015-04-23 12:37:10.136586417 +0000 UTC)
92:18:02:b2:25:77 -> f6:43:c5:fe:24:e9(ubuntu1404) (2015-04-23 12:37:10.660167879 +0000 UTC)
f6:43:c5:fe:24:e9 -> f6:43:c5:fe:24:e9(ubuntu1404) (2015-04-23 12:37:11.448019404 +0000 UTC)
0e:59:47:2c:9c:46 -> 02:e0:38:b0:f1:4d(dwragg-laptop) (2015-04-23 12:37:11.843317058 +0000 UTC)
Peers:
f6:43:c5:fe:24:e9(ubuntu1404) (v2) (UID<PHONE_NUMBER>246475338)
   -> 02:e0:38:b0:f1:4d(dwragg-laptop) [<IP_ADDRESS>:6783]
02:e0:38:b0:f1:4d(dwragg-laptop) (v2) (UID<PHONE_NUMBER>550528852)
   -> f6:43:c5:fe:24:e9(ubuntu1404) [<IP_ADDRESS>:46939]
Routes:
unicast:
f6:43:c5:fe:24:e9 -> 00:00:00:00:00:00
02:e0:38:b0:f1:4d -> 02:e0:38:b0:f1:4d
broadcast:
f6:43:c5:fe:24:e9 -> [02:e0:38:b0:f1:4d]
02:e0:38:b0:f1:4d -> []
Reconnects:

Allocator subnet <IP_ADDRESS>+256
<IP_ADDRESS> -> f6:43:c5:fe:24:e9(ubuntu1404) (0, 2, 126)
<IP_ADDRESS> -> 02:e0:38:b0:f1:4d(dwragg-laptop) (0, 0, 127)
Address ranges we own:
  <IP_ADDRESS>+127 (1/0)
# /home/dwragg/work/weaveworks/go/src/github.com/weaveworks/weave/weave reset

Host:
# docker logs weave
[...]
panic: Trying to report free on space I don't own
[...]

Yes, by resetting weave on one machine, I killed the weave router on another.  See https://gist.github.com/dpw/3766c53f9ae700f8b26e for full panic.

The panic after tombstone should be addressed by https://github.com/bboreham/weave/commit/5e10c237c01b0b40c2850e11b0b3f1fb4c137920

Kicking off round 2 on the implementation: Although GetFor is gone, there is still a file called getfor.go.

The code under ipam/ring and ipam/space pass addresses around as net.IP, but usually need to store and operate on them as integers.  So there is lots of clutter converting them back and forth, and odd things like utils.Add and utils.Subtract.  And the code appears to assume 32-bit IPv4 addresses throughout, so it is not like the abstraction provided by net.IP is buying you anything.
So simply do type address uint32 in one place, and then use that type throughout the rest of the ipam code.  Only use net.IP where it is really relevant (e.g. when parsing and formatting IP addresses in the HTTP API to the allocator).

It seems that space.Space and its associated functions are not actually public - allocator.go works purely with space.Set. It would be nice to make that explicit by lower casing, so that the true API to the Space code is clearer.
Actually, I'd go a bit further.  space.Set should probably be called Space (as it is the abstraction exposed by the space module). What is currently called Space should be called something like range, except that is a reserved word in go, so spaceRange or something.

I think  a rebase is in order so I can do another pass on some of the parts already commented on without it all becoming terribly confusing.

The separation of SpaceSet and Space seems to make things rather verbose, as we often end up having similar code in two forms (Allocate, NumFreeAddresses, etc.).  It looks like fundamentally we are managing two sets of addresses: those this peer owns, and those this peer has allocated.  I'm not sure why we need the separation to implement that, and by avoiding it, I think we can reduce the line count considerably.
(GiveUpSpace does use a heuristic which is based on knowledge of Spaces, but as noted in my line comment, I'm not sure what the rationale for that is.)
I can't see a strong need for the use of bitsets to represent allocated addresses.  A peer will manage addresses for thousands rather than millions of containers, so we can afford to use a few bytes to track each one.
On the other hand, we do need to represent the ranges of addresses owned by a peer compactly, because a peer could own a whole /8 or bigger.
Given that there are no stringent performance requirements, we can use a common representation for both sets.  The "owned" set suggests a representation based on ranges.  I've sketched this idea out in https://gist.github.com/dpw/dd984ed746e80350e485 . I've tried to implement the main operations from SpaceSet; the few omissions should be easy to add.  It's 212 lines, including some tests (although no doubt it could use a few more).

I have nothing against asserts.  I like asserts.   Some of my best friends are asserts.  But...
Asserts are a bit like executable comments.  And similarly to comments, the best assert is one that is not there because it would be obvious from the code.  So an assertion should only be added when it cannot reasonably be made superfluous by improvements to the code around it.
That leaves plenty of cases where asserts are appropriate.  And obviousness is subjective.  But I feel that some parts of the IPAM code lean a bit too heavily on asserts.
And because I assume we'll see Assert spread to other parts of the codebase, I'd prefer it if the main variant of Assert did not take a message.  The condition in an assert should generally be self-explanatory (if it's not, that is a hint that the code around it needs further consideration).  Although it is unfortunate that golang does not have a built-in assert that would include the condition expression as the panic message, I doubt the message is vital: the function name and line number should be sufficient to track down the failing assert.  My worry is that the message (combined with golang's cumbersome error handling) will create a temptation to use Assert for conditions that would be better regarded as errors.  If Assert just takes a condition, it will be clear that it is only for sanity checks.

The asserts in ring were added to prevent future, potentially innocuous
looking changes from breaching invariants.  There are subleties to the
algorithm that need to be expressed. For sure I've learnt on the side of
including more, but I don't think that causes any harm. None of them should
be triggerable by any combination of user actions.
I am concerned that a bug in ipam will take down the router; I'm thinking
perhaps catching these panics and closing the ipam actor thread (with a big
error in the logs) may be preferable?  If we can make future requests error
gracefully.
I don't mind re:including a message in the assert. I don't see the harm in
being verbose, but I will go ahead and remove them.
On Wednesday, 29 April 2015, David Wragg<EMAIL_ADDRESS>wrote:

I have nothing against asserts. I like asserts. Some of my best friends
are asserts. But...
Asserts are a bit like executable comments. And similarly to comments, the
best assert is one that is not there because it would be obvious from the
code. So an assertion should only be added when it cannot reasonably be
made superfluous by improvements to the code around it.
That leaves plenty of cases where asserts are appropriate. And obviousness
is subjective. But I feel that some parts of the IPAM code lean a bit too
heavily on asserts.
And because I assume we'll see Assert spread to other parts of the
codebase, I'd prefer it if the main variant of Assert did not take a
message. The condition in an assert should generally be self-explanatory
(if it's not, that is a hint that the code around it needs further
consideration). Although it is unfortunate that golang does not have a
built-in assert that would include the condition expression as the panic
message, I doubt the message is vital: the function name and line number
should be sufficient to track down the failing assert. My worry is that the
message (combined with golang's cumbersome error handling) will create a
temptation to use Assert for conditions that would be better regarded as
errors. If Assert just takes a condition, it will be clear that it is
only for sanity checks.
—
Reply to this email directly or view it on GitHub
https://github.com/weaveworks/weave/pull/563#issuecomment-97429116.

Discussed this with David and he'd prefer to not special case assertions inside the IPAM code.  I've written it anyway, but I'm not suggesting we include this code in this PR: https://github.com/tomwilkie/weave/commit/46570f17e0c8dc9eafd2e083e6761315243d7957

I've put the "allow Gossiper.Gossip to return nil" and "introduce PeerUID type" changes on master in https://github.com/weaveworks/weave/commit/ace56da7b3639d7a466b24ccd96e54791a11600f and https://github.com/weaveworks/weave/commit/c7dd48d7699ed5e62955f58b14e2405f8a108cae, respectively.


Update JumboCommand.java

Added backtick to Jumbo command per request of night. Hope I did it correctly, I'm bad. 😴

Why did you submit a one line pr.......

Also 10/10 job fallowing the issue template

Make a proper pr and come back. Also double check that request because well you didn't meat it

Oof, sorry for not following the format. Just did a quick edit that walshy said zNiGHTIYIaRe should make. But he couldn't /shrug


Exporter Modifies Date/Time Stamps of Existing Image Files

It appears that (at least on Windows) the glTF exporter is always stomping on the date/time stamp of image files when exporting as "glTF Separate (.glTF + .bin + textures)". This is causing issues with other software that uses the timestamp as an indication that the files have been modified (causing them all to reload, etc.).
See comments below: It actually appears that the exporter is reading and then immediately writing over the existing files.
To Reproduce
Steps to reproduce the behavior:

Apply a simple diffuse texture to the default Cube.
Note the "Date modified" time of the texture file.
Export as "glTF Separate (.glTF + .bin + textures)"
Note the "Date modified" time of the texture file has been modified to match the .glTF and .bin files.

Expected behavior
If texture files already exist on disk and have not been modified in Blender, the exporter should not modify them in any way during export.
Version

OS: [Windows]
Blender Version: 2.81 Alpha (2019-10-09 07:42)

Exporting to the same directory the image file is located in?
I think the exporter is completely overwriting the file. Apparently when it decides to write an image to the export directory, it will completely clobber any existing file with the same file. That could cause some pretty bad data loss :/

Exporting to the same directory (so all paths are relative) causes it to stomp on the modified time. It does not appear to actually stomp on the contents of the file if it exists. If I delete the file, the exporter appears to re-save it which is probably acceptable.
So it's a good thing that it doesn't stomp on existing files, but it really should not change the modified times as that causes other applications to think it has been modified on disk...

@scurest So I looked at this briefly and io_scene_gltf2/blender/exp/gltf2_blender_gltf2_exporter.py in finalize_images(...) appears to always write out any image files that it gathered. So it appears that it is (?) actually loading the images during export only to save them back out to the same file. (My confusion was thinking that it was trying to save the already in-Blender-memory images, but I believe now that it's simply read and immediately writing the same file.)
In order to replace our FBX workflow, we need the exporter to simply ignore any external image file processing. It doesn't need to read or try to convert any data from them. It simply needs to fill in the glTF JSON with their URI.
Ideally if the export mode is set to "SEPARATE" it would simply skip the redundant reading and writing of image files as I think most (?) people would prefer not to have their files potentially stomped on/reloaded underneath them. This should be a fairly common workflow for most game development.

Agreed this exporter should never stomp on pre-existing files.  We do need it to convert files under various circumstances, for example when a user includes a texture that's not JPG or PNG, when a user includes separate metallic and roughness images, separate BaseColor and Alpha images, that sort of thing.  Also, if you're overwriting a glTF with a newer copy of the same glTF, we want it to overwrite the old glTF's converted images with the newly converted images.  It's a challenge to figure out the difference between artist-supplied images and output images from previous runs, but one clue could be to check if the image is used in the blend project itself.
Until this is fixed though, I would strongly recommend that people export to a dedicated export folder, when using the "separate" mode export.

So I edited this bug's name and description now that I know what's happening. What I think should happen is that there should be a check made in gltf2_blender_gather_image.py / gather_image() to see if destFilename == srcFilename and skip calling __get_image_data(). __gether_uri() should simply set data=None when it creates the ImageData instance (as image_data would be None).
Then in gltf2_blender_gltf2_exporter.py the finalize_images() method should only attempt to write the image if image.data != None.
The filename comparison may need to be normalized/etc. to ensure a correct comparison, but this seems like a fairly easy fix that shouldn't require adding another option to the exporter and it should speed up export times when exporting models with several large PBR textures.
Since I'm not very Python-savvy, the best I was able to do for now was to simply skip reading and writing all images if the mode is set to glTF Separate, but some may want to export to a new directory and have the existing functionality still work.
Exporting to a different folder isn't an option for us right now as we need to maintain a single copy of our textures.

I'm not sure if anyone had additional opinions on how else to fix this issue, but it looks like the os.path.samefile() could be used to verify the source and destination files aren't the same and prevent the exporter from reading and then re-writing textures. We'd like to get rid of our current hack/patch at some point though.


[BUG] Broken Links in Docs 📃

🐛🐛 Bug Report
In the Notes Subsection of the How to Upload a Dataset page in docs, the links to all the examples (Uploading MNIST, Uploading CIFAR, and Uploading COCO) are broken.
⚗ Current Behavior
The links are broken and point to older versions of the examples directory
🧰 Possible Solution  (optional)
Fix links to redirect to examples/old/ directory

I checked out the examples you mentioned and I believe they're pretty old, to the point of being outdated compared to the latest API. I think the best course of action would be to create new examples using the latest API and then link those to the docs.

@DebadityaPal I believe you are write. These examples are deprecated as they use deprecated code (like hub.tensor, hub.load). I think the dataset page you mentioned should not be actually visible in the latest version of the docs.
Use getting started or dataset concepts to learn how to upload a dataset.
It might be useful to remove such old pages from being searchable in the last docs version @AbhinavTuli

@DebadityaPal @haiyangdeperci @mynameisvinn
If you are planning to rewrite, respectively, fix some of the examples, you might think about doing it in the KERAS-IO approach??
They are using the script tutobooks to generate from the versioned and tested python script the docs in markdown and a living hands-on example for colab.

Markdown for Docs: https://autokeras.com/tutorial/image_classification/
Colab for interactive usage: https://colab.research.google.com/github/keras-team/autokeras/blob/master/docs/ipynb/image_classification.ipynb
Python: https://github.com/keras-team/autokeras/blob/master/docs/py/image_classification.py

I think this will bring three major advantages (See also: #335):

We can check with a CI/CD pipeline if our examples are up to date because we see immediately if the python alias now our markdown docs are not running anymore.
New starters can straight test hub on colab in the cloud.
The example folder and the doc folder become closer or will be merged in the longterm, which simplified the maintainability.

There's this Github Action linkinator-action that checks your README and other markdowns for 404s. Maybe worth checking out, used by node.js itself.

I will like to work on this issue @SauravMaheshkar

Will be fixed by #618.

I’m closing this issue because it has been inactive for a few months. Please reopen if you still encounter this issue in the upcoming versions of docs.


cry about it week 7

cry about it

wait. i thought this was gonna add the mod files to my rep

didn't cry about it

laughed about it in fact