davmac314 / dinit

Service monitoring / "init" system
Apache License 2.0
615 stars 49 forks source link

add eventrequest message (wip) #256

Closed james-knippes closed 1 year ago

james-knippes commented 1 year ago

So I had the problem, that when i tried using dinit-monitor to execute some command on service state change, there was no way of executing a command for the initial state of the service when dinit-monitor is started. This resulted in me scripting around it with first calling dinitctl for the initial status and then dinit-monitor to recieve any further changes. Pretty unsatisfying solution.

So I ceated the EVENTREQUEST request type. It should result in dinit outputting the current state of the service as event if requested.

I also added the --request flag to dinit-monitor to make it request the initial status of the monitored services upon startup.

Works fine for my usecase, but I'm not sure if/how this fits into the overall design. What are your thoughts about this @davmac314 ?

james-knippes commented 1 year ago

Also there is currently a problem when monitoring more than one service. dinit-monitor does only recieve initial service events for one of them. (the last one in the list I think). This seems to be related to dinit(control.cc) only responding to the first EVENTREQUEST message it gets? I'm not sure why this is, as I'm not familiar with the dasynq callback stuff.

I've monitored the socket control traffic with the help of socat. And there is definetly only one response despite there being two requests. The requests seem to be in one message tho, this might be a problem?

$ socat -t100 -x -v UNIX-LISTEN:./dinit-testsocket-monitor,mode=777,reus
eaddr,fork UNIX-CONNECT:./dinit-testsocket-orig

> 2023/11/06 00:46:13.000223661  length=1 from=0 to=0
 00                                               .
--
< 2023/11/06 00:46:13.000223938  length=5 from=0 to=4
 3a 01 00 03 00                                   :....
--
> 2023/11/06 00:46:13.000224043  length=8 from=1 to=8
 02 05 00 77 76 6b 62 64                          ...wvkbd
--
< 2023/11/06 00:46:13.000224269  length=7 from=5 to=11
 3b 00 00 00 00 00 00                             ;......
--
> 2023/11/06 00:46:13.000224382  length=7 from=9 to=15
 02 04 00 62 6f 6f 74                             ...boot
--
< 2023/11/06 00:46:13.000224481  length=7 from=12 to=18
 3b 02 01 00 00 00 02                             ;......
--
> 2023/11/06 00:46:13.000224581  length=11 from=16 to=26
 17 01 00 00 00 17 00 00 00 00                    ..........
--
< 2023/11/06 00:46:13.000224686  length=17 from=19 to=35
 64 11 01 00 00 00 00 02 02 08 00 00 00 00 00 00  d...............
 00                                               .
--

Then calling dinit-monitor with --request:

./src/dinit-monitor -r --socket-path ./dinit-testsocket-monitor -c "echo %n is %s" wvkbd boot
boot is started

results in only one initial status.

Any ideas how to fix this?

davmac314 commented 1 year ago

I think that the existing ways of getting a service status should really be enough:

  1. there's already a SERVICESTATUS command specifically so a client can check the current state of a service
  2. the current service status is returned as part of a LOADSERVICE command response, which dinit-monitor already uses to obtain the current status

It really feels to me like it shouldn't be necessary to prompt dinit to re-send status, given that clients are supposed to be able to keep track of the status themselves. dinit-monitor already receives the initial service state when it starts, it shouldn't need to request it. Also, broadcasting a notification without any state change could confuse other clients that weren't expecting it.

If I understand correctly, what you want is to have get dinit-monitor to run the notification command immediately when it starts, with the current service state (whatever it happens to be). I think it should be easy enough to add an option to it to do that, without having to extend the command protocol at all. It might require a little refactoring of the current code but shouldn't be too difficult. I think you should look at doing that instead.

Edit: to be clear, you just need to keep the service state that is returned from the LOADSERVICE query, instead of discarding it. Put the states in a vector, and iterate over the services and execute the notification command for each service/state pair.

Also there is currently a problem when monitoring more than one service.

It looks like there may actually be a bug in the command processing logic so that it doesn't currently process more than one command if multiple commands happen to get buffered together. I'll fix that soon. It's only come up in this case because the usual protocol is for the client to wait for a reply (i.e. command acknowledgement) after issuing each command, which wasn't happening with your implementation, so I don't think it affects any existing dinitctl behaviour (but it should still be fixed, so I'll do that).

james-knippes commented 1 year ago

Sure, got your points. Also implementing it that way makes way more sense and is also simpler. Should note code while sleepy. But I'm glad this excursion may have catched a bug.

made a new pr > #257