-
Notifications
You must be signed in to change notification settings - Fork 99
CLI netlab status allow --json for API handling
#3004
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
83a14ba to
d3e9a4e
Compare
d3e9a4e to
9ac3cdd
Compare
ipspace
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few initial comments. I think it would make sense if you do another pass and simplify things, and then I'll look at them some more.
Most important: there should be a single data-gathering function, and the resulting data structure should be used directly for the JSON dump (potentially with an extra wrapper around a list), or to generate the text printout.
| p_status[provider] = p_module.call('get_lab_status') or get_empty_box() | ||
|
|
||
| def show_lab_nodes(topology: Box) -> None: | ||
| def normalize_providers(providers: typing.Any) -> typing.List[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be necessary. Hopefully we can agree (between ourselves) what data format to use ;)
|
|
||
| if args.all: | ||
| display_active_labs(topology,args,lab_states) | ||
| if args.json: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to keep this decision tree clean - one parameter, one action, and then handle the specifics in the action
| show_lab_log(args,lab_states) | ||
| else: | ||
| show_lab(args,lab_states) | ||
| if args.json: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every action should follow the same blueprint -- get data, create a data structure out of that, and print the data structure in table, YAML, or JSON format (see "show" commands).
Having a totally different approach to collect data for the JSON format makes little sense.
| return [p.strip() for p in providers.split(',') if p.strip()] | ||
| return [str(providers)] | ||
|
|
||
| def build_active_labs_payload(lab_states: Box) -> dict: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you're taking a data structure that we built somewhere and create a different data structure for the JSON printout? That makes no sense. We should get it right in the first place.
Also, please use Box, not dict. The code will become much easier to read (that's why I started using Box in the first place). There are two utility functions in netsim.data: get_box to convert a dict into a Box and get_empty_box that returns an empty Box. Do not create boxes by calling Box directly, the two utility functions set just the right combination of flags.
Thanks (and sorry) for taking the time to review this. I’ve got a few other things on the back burner right now, so I’d prefer to park/close this PR for the moment and come back in ~a month to do a clean refactor if you think the idea of adding a Lesson learned: I’ll be more careful about raising this kind of AI experiments on something that seems cool, without correctly understanding the changes. |
Been there, done that ;) No worries, I love the idea. I hope you don't mind if I just go ahead and implement it? |
|
One other thing (I thought I wrote that down, but evidently not) -- these changes would break the exiting API introduced in #2969. @captainpacket -- Are you OK with that, or should we return existing text in "status" field and a copy of the data in JSON-encoded format? |
I'm glad you like the idea as well, and yes, please feel free to implement it! I'll be looking at how you solve this, it should help me better understand that part of the codebase.
@captainpacket could you please share more information on how you are using this API? Before seeing that PR, I was thinking it would be nice to have something that lets other team members view lab status and potentially take actions without needing CLI access. Our main lab is currently poorly managed by an unreadably long bash script that gets the job done but isn't ideal. This HTTP server could be a great foundation for building a more user-friendly wrapper around netlab. That said, this could grow in scope pretty quickly depending on where we want to take it. It comes down to how much we want to solve this problem — is it worth diving deeper into, or are there bigger fish to fry? |
|
@sdargoeuves I wrote a small FastAPI wrapper to solve the same problem you did - a lot of bash trickery. The PR I submitted is a simplified version of that; it implements some extra things like multilab, graphs, etc. I'm using a job execution engine (Semaphore UI) with a small client runner script to access the different functions, but it could easily have an actual frontend. Feel free to use - https://github.com/forwardnetworks/netlab/blob/dev/netlab_api.py |
I have no idea if this is something we want to consider. After discovering the new HTTP server, I wanted to test and thought it was a shame to not a have readable output in the browser.
If we are happy to go down this rabbit hole, I don't mind checking what other commands we could json-ify, if not feel free to close this, and leave that for a distant future!
Overview
Added
--jsonflag tonetlab statusfor programmatic access, improving API usability and automation capabilities.Changes
status.py
--jsonargument to CLI parserbuild_*_payload()functions that construct structured data oncebuild_active_labs_payload()- all labs overviewbuild_lab_detail_payload()- single lab with nodes/toolsbuild_lab_log_payload()- lab event logapi.py
netlab statuscommand/status/<id/all>Example Usage
CLI
HTTP