Enhanced weave status #1248
Enhanced weave status #1248
Conversation
|
Woot looks nice. IPAM used to have stats about number of free and allocated addresses - can we have them back please? |
|
For DNS, can you shorten the container id to 12 char (I think thats what docker does) and include the peer id / nickname? Don't shorten in the json. Also a bunch of tests depend on the format of status (150_connect_forget_2_test.sh, 500_weave_multi_cidr_test.sh) as did the weave launch-proxy script. |
Personally I'd prefer this to say "on/off", or "enabled/disabled". The "true" and "2s" are really rather obscure. I had to look at the JSON to figure out what these mean. Also what happened to the failure reason in the above? Comes from
how? One thing I worry about is that now users need to run a whole bunch of commands to debug issues. What are we going ask users to send us? The JSON? urgh. I wonder whether should have a I also wonder whether we should ditch the 'macs' and 'routes' sub commands. I cannot think of any situation where their output has been helpful in debugging a user problem.
That's not a good command name. Finally, I'd be tempted to ditch the command category, ie. router, ipam,dns,proxy. |
|
Shouldn't some code from ipam get removed? After all, it outputs some status info right now, the code of which presumably is now dormant. IPAM also has a whole bunch of String methods with zero coverage that I suspect actually have zero call sites, though it's hard to be sure due to format string magic. |
Thanks 😄
Good catch - will do. The intent of this PR is to achieve rough parity with what we have now, whilst leaving in place an extensible framework for other things to hook into - for example we have #1185 coming up and @bboreham wanted to add more detailed output from IPAM (e.g. an exhaustive list of allocations) to name a few things.
Will do.
As with the documentation, I'll update the tests once the output templates have stabilised. If I recall correctly, you favour making the tests examine the JSON - I could look into doing that, however I think there is an argument to be made that this reduces the coverage of the tests. Thoughts @rade?
That is easily arranged.
I'll rework that to be a bit more descriptive.
This addresses the specific case you cited, but if you intended for us to do more - a historic profile of heartbeat failures perhaps - I'll remove the reference to #908 here and the work can be done in a separate PR.
These are two separate issues - IMO it's not unreasonable to ask for a JSON dump as part of an issue submission, but violently agree we don't want users to have to resort to looking at it for interactive debugging - that just means we didn't get the plain text templates right.
The template approach makes this trivial to iterate on - I'll bash something up for review.
Unless you're advocating removing it from the JSON too (which I would disagree with - as I mention above, and contrary to your 'urgh', I think the JSON dump will become lingua franca for reporting issues) we save only a few lines of template by removing these.
We can make a decision on this once we've settled on the final set of outputs - the weave script simply synthesises a URL so it's just a matter of updating the path -> template bindings in
It's a placeholder calculated to engender discussion 😄 I had called it
Yes! Will address comments by pushing additional commits, then do a squash at the end when we're all happy. |
I have a vague memory of seeing such stats, but they're not in current master: Any clue where they went? In the meantime I'll add the owned ranges back in... |
I think they disappeared when we introduced multi-subnet support in IPAM. There simply isn't enough info held in IPAM to produce sensible figures. Notably, IPAM itself does not know about subnets; it's all done in the UI/API layer. |
|
|
re macs & routes...
The JSON should contain everything. I don't think the text version needs to. I would go so far as saying that the text version should only present information that we have a very strong reason to believe is useful to a good chunk of users. |
|
|
isn't that just a subset of the info one gets from
Nice. And nearly there. I don't think we need
We should generally avoid checking status output in tests. In the few places where that is the best option, I suspect we'd ultimately want to extract the info we need via a Re command names... |
That's actually peer name/owned range/version from the IPAM ring. I'll adjust the template to use the more verbose labelled style, as there shouldn't be that many entries...
Brilliant - the invariant you mentioned sounds correct from memory, but I will check. ACK on your remaining points. More commits incoming. |
|
awh
force-pushed
the
issues/1025-enhanced-weave-status
branch
from
c213671 to
af6f664
Compare
|
re |
awh
force-pushed
the
issues/1025-enhanced-weave-status
branch
from
1f6864c to
def1e5e
Compare
|
Current state of play: |
awh
force-pushed
the
issues/1025-enhanced-weave-status
branch
from
def1e5e to
f1cf50a
Compare
Can we have a header please? |
|
Should we include tombstones in the DNS json? |
|
Under IPAM, Entries: 3 is probably not particularly useful to users. |
|
I'd love to make (I've dropped the retry interval) In |
|
in have |
I'd ditch those. |
|
I'd also ditch |
Agreed. And I'd drop the |
|
It would be nice if peers were consistently referenced by their name and nickname. We can do that in a follow-on issue. |
|
New tabular connections: |
|
Regarding |
|
I'm not convinced listing the target peers on |
|
|
|
Using the same column for remote peer name/failure reason 👍 |
|
How about using the same interval notation that we do for IPAM range? |
I considered that but |
|
@awh ready for another review; I'll do some squashing once you are happy. |
|
Am ready to merge once you've done your squashing. |
rade
force-pushed
the
issues/1025-enhanced-weave-status
branch
from
ea0db16 to
1a4d0c5
Compare
rade
force-pushed
the
issues/1025-enhanced-weave-status
branch
from
1a4d0c5 to
fc9a1c8
Compare
Ready for an initial review pass; I will add further commits to update the documentation only when the output format haggling has subsided 😄
Fixes #1025, fixes #1141, fixes #1027, fixes #908.