Opened 12 months ago

Closed 8 months ago

#5078 closed task (complete)

Add control commands forwarding within Control Agent via unix sockets

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Kea1.2
Component: remote-management Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Mozilla Feature Depending on Ticket:
Estimated Difficulty: 8 Add Hours to Ticket: 5
Total Hours: 53 Internal?: no

Description

See http://kea.isc.org/wiki/ControlAPIDesign

The Kea Control Agent wll forward most of the commands to the respective servers. This ticket is about creating connections (unix sockets) with these servers and forwarding control messages to them.

Subtickets

Change History (16)

comment:1 Changed 12 months ago by marcin

  • Milestone changed from Kea-proposed to Kea1.2

comment:2 Changed 12 months ago by fdupont

As usual: if it is new connection code please avoid unix sockets.

comment:3 Changed 12 months ago by marcin

  • Component changed from management API to remote-management

comment:4 Changed 9 months ago by tomek

  • Summary changed from Add control commads forwarding within Control Agent via unix sockets to Add control commands forwarding within Control Agent via unix sockets

comment:5 Changed 9 months ago by tomek

  • Sub-Project changed from DHCP to Mozilla

comment:6 Changed 9 months ago by marcin

  • Owner set to marcin
  • Status changed from new to accepted

comment:7 Changed 9 months ago by hschempf

  • Milestone changed from Kea1.2 to Kea1.2-final

comment:8 Changed 8 months ago by tomek

  • Milestone changed from Kea1.2-final to Kea1.2

comment:9 Changed 8 months ago by fdupont

  • Owner changed from marcin to fdupont

Hum, it seems I implemented this as #5138... I have a control channel client class and a forward control agent command, so it matches the description even my code is synchronous.
I am taking the ticket to avoid silly double effort.

comment:10 Changed 8 months ago by fdupont

  • Owner changed from fdupont to marcin
  • Status changed from accepted to assigned

comment:11 Changed 8 months ago by marcin

  • Add Hours to Ticket changed from 0 to 40
  • Owner changed from marcin to UnAssigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 40

I implemented asiolink::UnixDomainSocket class using boost::asio. I believe there are still some improvements that can be made. For example, we could implement receive() in terms of asynchronous async_receive_some to allow for timeouts if the other server doesn't respond. We could probably also increase the buffer size limits etc. We can consider doing it for the final release. There is no more time now.

The commands are forwarded by the CA to the respective servers if the command is not handled locally by the CA or hooks library. I have added a status value of 2 to indicate that the local command manager doesn't support some command (as opposed to the case when the command was processed and there was an error).

I also implemented "graceful" reconfiguration of the CA (maintaining TCP connections with the external world). I think this still needs some testing and field experience to maybe identify the cases where it still doesn't work correctly. For example, if there are active TCP connections and data are being transferred during reconfiguration.

In any case, I have done some basic system testing and both the reconfiguration and commands forwarding seemed to work.

Proposed ChangeLog entry:

12XX.	[func]		marcin
	Control agent forwards commands to be handled by other Kea
	servers.
	(Trac #5078, git abcd)

comment:12 Changed 8 months ago by tmark

  • Owner changed from UnAssigned to tmark

comment:13 follow-up: Changed 8 months ago by tmark

  • Owner changed from tmark to marcin

Larger issues:

As you pointed out, UnixDomainSocket? doesn't yet support async operations. It will need to if for no other reason than to support a timeout receives. Ultimately you may find that this should use the CtrlAgentProcess?'s IOservice rather than it's own. Please create a ticket for this. We probably want a second one to apply it to CA?


Service "labels" in the CA config file, "dhcp4-server", "dhcp6-server", etc..,
do not match the values a CA client would put in the command's service list.
This goes more to the topic of eventually supporting multiple instances of
services and using some sort of labeling to identify them. The label value
used in the command service list ought to be the same as the value in
CA config file. I think the values should be arbitrary strings rather than
enums. If for some reason it might be useful for CA to know what "type" of
service an entry is we do that as a type attribute:

        "dhcp4-server-marcin":
        {
        "type": "dhcp4"
        "socket-type": "unix",
        "socket-name": "/path/to/the/unix/marcin-v4"
        }

where type might also include "external" or "custom" ... I can envision people
building control-capable services.

I think for 1.2 final we should eliminate mapping services to enums for
locating their control sockets.


ca_command_mgr.h/cc:handleCommand()

per your 5078a rebase, I do not think we want/need the forward flag.
We discussed the basic flow here and I think we agree that:

When the CA receives a command is received:
Pass the command to the hook library
If the hook library does not handle the command and:

a: The command contains a service list:

forward the command to the service(s) listed

b: The command does NOT contain s service list:

CA should attempt to handle the command


ca_command_mgr.h/cc:tryForwardCommand()

I'm not crazy about calling this method "tryForwardCommand()" why not simply
forwardCommand()? In reality nearly every method could have the word "try"
as prefix because they can nearly all fail so we always "trying" to do
something. Plus it looks funny to wrap it in a try/catch block.


ca_command_mgr.h/cc:tryForwardCommand() -

Seems like the logic to fetch the context could be placed in method.
CtrlAgentCommandMgr::getCtrlAgentCfgContext()


ca_command_mgr.h/cc:tryForwardCommand() -

The block of logic which does the connect, write, and receive does not
attempt to emit any error detail as it does a catch(...). If we end up
with a failure here there will be no indication of what it was.

Why aren't you catching UnixDomainSocketError? and issuing something
more helpful?


ca_command_mgr.h/cc:tryForwardCommand() -

I'm not real clear on why you limit the processing to 1 entry?
It's already parsed into a vector of services, you already return
the answer as list of responses. Was this just a time limitation?
We probably need a ticket for this if you don't already have one.


ca_command_mgr.h/cc:tryForwardCommand() -

You a log message at INFO that indicates a message was successfully
forwarded. There's nothing at debug level before it forwards.
If it were to get stuck we wouldn't see that. It might also be helfpul
in looking at timing, we could see in debug when the command sent and
then when it comes back.


ca_controller.h:

Why do you need this?

+ using DControllerBase::getIOService;


ca_process.cc:CtrlAgentProcess::run()

I'm not clear on why you garbageCollectListeners() is called in
the run() loop? Does something besides a reconfigure create new
listeners or is it because the reconfigure is being done inside
a callback? If it the latter, that would be worth a comment.


Don't forget to update the example simple.json to not use "localhost"
or at least add a comment explaining you can only use IP address right now.

Unit tests pass on OS-X.

comment:14 in reply to: ↑ 13 Changed 8 months ago by marcin

  • Add Hours to Ticket changed from 40 to 48
  • Estimated Difficulty changed from 0 to 8
  • Owner changed from marcin to tmark
  • Total Hours changed from 40 to 48

Replying to tmark:

Larger issues:

As you pointed out, UnixDomainSocket? doesn't yet support async operations. It will need to if for no other reason than to support a timeout receives. Ultimately you may find that this should use the CtrlAgentProcess?'s IOservice rather than it's own. Please create a ticket for this. We probably want a second one to apply it to CA?

http://kea.isc.org/ticket/5189


Service "labels" in the CA config file, "dhcp4-server", "dhcp6-server", etc..,
do not match the values a CA client would put in the command's service list.
This goes more to the topic of eventually supporting multiple instances of
services and using some sort of labeling to identify them. The label value
used in the command service list ought to be the same as the value in
CA config file. I think the values should be arbitrary strings rather than
enums. If for some reason it might be useful for CA to know what "type" of
service an entry is we do that as a type attribute:

        "dhcp4-server-marcin":
        {
        "type": "dhcp4"
        "socket-type": "unix",
        "socket-name": "/path/to/the/unix/marcin-v4"
        }

where type might also include "external" or "custom" ... I can envision people
building control-capable services.

I think for 1.2 final we should eliminate mapping services to enums for
locating their control sockets.

http://kea.isc.org/ticket/5190


ca_command_mgr.h/cc:handleCommand()

per your 5078a rebase, I do not think we want/need the forward flag.
We discussed the basic flow here and I think we agree that:

When the CA receives a command is received:
Pass the command to the hook library
If the hook library does not handle the command and:

a: The command contains a service list:

forward the command to the service(s) listed

b: The command does NOT contain s service list:

CA should attempt to handle the command

Removed forward flag and the code follows this logic.


ca_command_mgr.h/cc:tryForwardCommand()

I'm not crazy about calling this method "tryForwardCommand()" why not simply
forwardCommand()? In reality nearly every method could have the word "try"
as prefix because they can nearly all fail so we always "trying" to do
something. Plus it looks funny to wrap it in a try/catch block.

Renamed.


ca_command_mgr.h/cc:tryForwardCommand() -

Seems like the logic to fetch the context could be placed in method.
CtrlAgentCommandMgr::getCtrlAgentCfgContext()

But doesn't have to be as this is the only occurrence of this code within this class.


ca_command_mgr.h/cc:tryForwardCommand() -

The block of logic which does the connect, write, and receive does not
attempt to emit any error detail as it does a catch(...). If we end up
with a failure here there will be no indication of what it was.

Why aren't you catching UnixDomainSocketError? and issuing something
more helpful?

Catching and printing the details of the exception.


ca_command_mgr.h/cc:tryForwardCommand() -

I'm not real clear on why you limit the processing to 1 entry?
It's already parsed into a vector of services, you already return
the answer as list of responses. Was this just a time limitation?
We probably need a ticket for this if you don't already have one.

I updated the code to forward to all servers. This was due to time limitations, but as we already extended the time for the 1.2-beta release, I decided to put this in.


ca_command_mgr.h/cc:tryForwardCommand() -

You a log message at INFO that indicates a message was successfully
forwarded. There's nothing at debug level before it forwards.
If it were to get stuck we wouldn't see that. It might also be helfpul
in looking at timing, we could see in debug when the command sent and
then when it comes back.

Added a couple of log messages.


ca_controller.h:

Why do you need this?

+ using DControllerBase::getIOService;

Don't need this anymore. I did need at some point and the code was later changed.


ca_process.cc:CtrlAgentProcess::run()

I'm not clear on why you garbageCollectListeners() is called in
the run() loop? Does something besides a reconfigure create new
listeners or is it because the reconfigure is being done inside
a callback? If it the latter, that would be worth a comment.

Added a comment.


Don't forget to update the example simple.json to not use "localhost"
or at least add a comment explaining you can only use IP address right now.

This is the part of the doc ticket you reviewed for me.

Unit tests pass on OS-X.

comment:15 Changed 8 months ago by tmark

  • Add Hours to Ticket changed from 48 to 5
  • Owner changed from tmark to marcin
  • Total Hours changed from 48 to 53

New tickets and changes look good. Some commentary recommendations:

ca_command_mgr.h:

In the handleCommand() commentary, you refer to both "hooks libraries"
and "callouts" interchangably. I'd suggest replacing the latter with the former.

+ / If the received command doesn't include 'service' parameter or this
+
/ parameter is blank, the command is handled by the Control Agent or the
+ / attached hooks libraries.
+
/
+ / If the non-blank 'service' parameter has been specified the callouts
+
/ are executed. If the callouts process the command the result is returned
+ / to the controlling client. Otherwise, the command is forwarded to each
+
/ Kea server listed in the 'service' parameter.

Also, in the first paragraph you state "Control Agent or the attached" rather
than the other way around:

"...first handled by the attached hooks libraries and if still unhandled, the

Control agent itself"

Something to that affect.


In the commentary for forwardCommand():

The method description implies more than one server:

"Tries to forward received control command to Kea servers."

I'd suggest:

"Tries to forward received control command to a specified server"

The following statement:

+ / When the Control Agent was unable to process the control command
+
/ because it doesn't recognize it, the command should be forwarded to
+ / the specific Kea services listed within a 'service' parameter. This
+
/ method forwards the command to the specified Kea service.

is somewhat inaccurate in that the method is called not when the command has
not been recognized the CA but rather because forwarding was specified via the
service parameter. In any event I don't think the statement is neededn in
describing forwardCommand(). I think it is sufficient to simply describe it
as attempting to forward a command to a service.

and I think also, unnecessary in describing the method.

Units pass under OS-X.

Address these as you see fit then merge the ticket. Good work, BTW!

(I forgot to add my previous review hours so I added 5 to this go round)

Thomas

comment:16 Changed 8 months ago by marcin

  • Resolution set to complete
  • Status changed from reviewing to closed

Fixed the remaining issues and merged with commit 19a50ed1ccafae19ef10d84cba73992cadf49753

Note: See TracTickets for help on using tickets.