Opened 3 years ago

Closed 3 years ago

#5137 closed task (complete)

Remote-management client: base implementation

Reported by: tomek Owned by: tomek
Priority: high Milestone: Kea1.2
Component: remote-management Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets: #5138, #5139
Sensitive: no Defect Severity: N/A
Sub-Project: Mozilla Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 2
Total Hours: 34 Internal?: no

Description

After #5136 is completed, we will need to implement the framework or basic architecture - connect over http, get input, send data, receive responses, print out result. We will probably support on of the basic commands available a as a proof of concept. It seems that list-commands is a good initial command.

Majority of the commands already supported by DHCPv4 and DHCPv6 servers is not expected to be covered in this ticket.

Subtickets

#5136: Remote-management client: designclosedtomek

Change History (16)

comment:1 Changed 3 years ago by tomek

  • Summary changed from remove-management client: base implementation to Remote-management client: base implementation

comment:2 Changed 3 years ago by tomek

Add a subticket #5138.

comment:3 Changed 3 years ago by tomek

Add a subticket #5139.

comment:4 Changed 3 years ago by tomek

Remove a subticket #5138.

comment:5 Changed 3 years ago by tomek

Remove a subticket #5139.

comment:6 Changed 3 years ago by tomek

  • Parent Tickets set to 5138, 5139

comment:7 Changed 3 years ago by tomek

Add a subticket #5136.

comment:8 Changed 3 years ago by tomek

  • Sub-Project changed from DHCP to Mozilla

comment:9 Changed 3 years ago by tomek

  • Owner set to tomek
  • Status changed from new to assigned

comment:10 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 0 to 28
  • Owner changed from tomek to Unassigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 28

The code is now ready for review.

Proposed ChangeLog:

12XX.	[func]		tomek
	Kea-shell, a management client able to connect to REST interface
	provided by Control Agent, has been added.
	(Trac #5137, git tbd)

Couple comments:

  • make install installs the files, but kea-shell does not work without proper PYTHONPATH tweak. The proper solution is to install all *.py files into dedicated directory and modify kea-shell to use that directory. #5170
  • man page is provided, but user's guide section is not. There's #5171 for that.
  • we decided (see ControlAPICliDesign) that the client should have minimal requirements. It works with any python I was able to test it with. The oldest I have is 2.7, the newest is 3.5. Configure will go with whatever is the default in the system, but specific version can be enforced:
    PYTHON=python3.3 ./configure --enable-shell
    
  • Since kea-shell requires python that may not always be available, its compilation is disabled by default. To enable it, --enable-shell is needed.
  • kea-shell is mostly intended for scripting, but can be used manually. When run manually, hit ctrl-d to end inserting parameters. In upcoming tickets (#5138, #5139) this will be mare more convenient (e.g. the input parameters will be skipped for parameter-less commands).

comment:11 Changed 3 years ago by fdupont

  • Owner changed from Unassigned to fdupont

comment:12 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tomek

configure.ac:

  • (needs a second opinion) a text management client (current) or a management text client (I prefer)?
  • it is far more portable to do the substitution in configure than in make, I believe your choice was because it is nearly impossible to have a variable AC_CONFIG_FILES argument. But I have to insist: it does not care if the shell setup is not complete when it is not enabled, please avoid substitution, chmod & co in Makefiles.

src/bin/shell/Makefile.am:

  • see previous comment
  • for CLEANFILES remove all .pyc files (i.e. kea-shell.pyc -> *.pyc)

kea-shell.xml:

  • copyright year 2016 -> 2017
  • synopsis doesn't match ARGUMENTS (different order, no timeout)
  • stdin/stdout -> standard input / standard output and (suggestion for this last one) printed -> displayed
  • there should be some words about optional command parameters (at least they exist)

kea-shell.py:

  • else: if -> elif
  • please avoid long (>80) lines in python: it makes the code too hard to read (so a priori wrong)
  • there are some standards for python code (e.g. the previous comment) with associated tools. I used pychecker and pylint, I don't know (yet) the new code checker.
  • the function timeout_handler should be after the last from and before the code body and must be documented (at least one of the two tools of the previous comment should insist about this too).
  • kea-shell invoked without arguments just stalls (reading the code I found I have to type D because the alarm is set after...

kea_conn.py:

  • defug -> debug

`kea_commection2.py:

  • Estalbish -> Establish (in the 3.py file too)

I have to eat so I stop here. I strongly recommend to run pylint (pip install pylint to get it) on the python code in order to get a decent rating (kea-shell.py is 4.65/10, the worse is 1.33 for kea_connection2.py (default python is 2.7 on my box)).

About python version IMHO we should recommend python3 because it has a correct handling of unicode so of JSON. Now the idea to support both 2 and 3 is right even I believe it is possible with one file (but it is not really important so I shan't insist).

Not very related but still a bug: I configure without gtest (not needed for shell and python) and it fails to build:

  CXXLD    libagent.la
copying selected object files to avoid basename conflicts...
  CXXLD    kea-ctrl-agent
Making all in tests
  CXXLD    libbasic.la
ar: no archive members specified
usage:  ar -d [-TLsv] archive file ...
	ar -m [-TLsv] archive file ...
	ar -m [-abiTLsv] position archive file ...
	ar -p [-TLsv] archive [file ...]
	ar -q [-cTLsv] archive file ...
	ar -r [-cuTLsv] archive file ...
	ar -r [-abciuTLsv] position archive file ...
	ar -t [-TLsv] archive [file ...]
	ar -x [-ouTLsv] archive [file ...]
make[6]: *** [libbasic.la] Error 1
make[5]: *** [all-recursive] Error 1
make[4]: *** [all] Error 2
make[3]: *** [all-recursive] Error 1
make[2]: *** [all-recursive] Error 1
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2

comment:13 Changed 3 years ago by fdupont

  • Owner changed from tomek to fdupont
  • Status changed from reviewing to accepted

I have some free time: I'll try to fix/improve it

comment:14 follow-up: Changed 3 years ago by fdupont

  • Add Hours to Ticket changed from 28 to 32
  • Owner changed from fdupont to tomek
  • Status changed from accepted to reviewing
  • Total Hours changed from 28 to 32

Moved config from Makefile.in to configure.
Try first python3.
Changed the input file from kea-shell.py to kea-shell.in
Fixed doc.
Fixed spelling.
Use more python style, e.g. comments
Use static (aka class vs instance) method sendCA.
Still work, pylint evaluation raised (> 9, some 10).
In tests, removed most long lines
Moved shell_unittest.py to shell_unittest.py.in (to fix the python path).
Fixed the `src/bin/agent/tests/Makefile.am' bug.

Note I don't change any semantic, i.e., the code does exactly the same thing than before.

Ready for a cross review.

comment:15 Changed 3 years ago by fdupont

About the code itself, I have 2 comments for kea_connection3.py:

  • resp.status can be used instead of resp.getcode() but even the documentation says status I prefer the current geocode().
  • the decode("utf-8") is the right thing to do (and it proves that python3 is better).

So I do not propose any change in the code itself.

comment:16 in reply to: ↑ 14 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 32 to 2
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 32 to 34

Replying to fdupont:

Moved config from Makefile.in to configure.
Try first python3.
Changed the input file from kea-shell.py to kea-shell.in
Fixed doc.
Fixed spelling.
Use more python style, e.g. comments
Use static (aka class vs instance) method sendCA.
Still work, pylint evaluation raised (> 9, some 10).
In tests, removed most long lines
Moved shell_unittest.py to shell_unittest.py.in (to fix the python path).
Fixed the `src/bin/agent/tests/Makefile.am' bug.

Note I don't change any semantic, i.e., the code does exactly the same thing than before.

Ready for a cross review.

Thanks a lot for your fixes. I reviewed them and they look fine.

Ran unit-tests on Ubuntu 16.04.1 x64 and they passed.

Code merged. Closing ticket.

Note: See TracTickets for help on using tickets.