feat: Add more events for starting and stopping of worker #187

jirihnidek · 2023-11-13T08:28:21Z

When worker is started, then it sends event "started"
It also sends event "stopped", when it is terminated and process cannot handle message anymore
This functionality is IMHO important for developers of yggdrasil worker. When you develop new worker it is important to see that your worker "communication" with yggd during start. It is also decency to say something like "good bye" at the end.
Small refactoring of code (do not use numeric constant)
Updated doc strings
Extended documentation in com.redhat.Yggdrasil1.Worker1.xml
according changes in code

jirihnidek · 2023-11-13T08:35:48Z

Output on terminal with echo worker:

jhnidek@thinkpad-p1:~/github/redhat-insights/yggdrasil$ go run ./worker/echo -log-level trace
2023/11/13 09:18:02 connecting to session bus: unix:path=/run/user/1000/bus
2023/11/13 09:18:02 emitting event STARTED
^C2023/11/13 09:18:08 emitting event STOPPED

Output on terminal with yggd:

[yggd] 2023/11/13 09:18:02 /home/jhnidek/github/redhat-insights/yggdrasil/internal/work/dispatcher.go:113: received signal: &dbus.Signal{Sender:":1.203", Path:"/com/redhat/Yggdrasil1/Worker1/echo", Name:"com.redhat.Yggdrasil1.Worker1.Event", Body:[]interface {}{0x4, "", "", map[string]string{}}, Sequence:0x7}
[yggd] 2023/11/13 09:18:02 /home/jhnidek/github/redhat-insights/yggdrasil/cmd/yggd/client.go:191: emitted event: {Worker:echo Name:STARTED MessageID: ResponseTo: Data:map[]}
[yggd] 2023/11/13 09:18:08 /home/jhnidek/github/redhat-insights/yggdrasil/internal/work/dispatcher.go:113: received signal: &dbus.Signal{Sender:":1.203", Path:"/com/redhat/Yggdrasil1/Worker1/echo", Name:"com.redhat.Yggdrasil1.Worker1.Event", Body:[]interface {}{0x5, "", "", map[string]string{}}, Sequence:0xa}
[yggd] 2023/11/13 09:18:08 /home/jhnidek/github/redhat-insights/yggdrasil/internal/work/dispatcher.go:117: cannot find sender: cannot get name for sender: :1.203

subpop

Works great! Can we follow this up with a PR that re-emits these events on the public com.redhat.Yggdrasil1 D-Bus interface?

subpop · 2023-11-14T15:56:32Z

ipc/interfaces.go

 	WorkerEventNameWorking WorkerEventName = 3
+
+	// WorkerEventNameStarted is emitted when finished starting
+	// process and it can


Looks like you left off the remainder of the documentation sentence.

Good catch. I was probably distracted by somebody, when I was writing this.

subpop · 2023-11-14T15:56:44Z

ipc/interfaces.go

+	WorkerEventNameStarted WorkerEventName = 4
+
+	// WorkerEventNameStopped is emitted when worker is stopped,
+	// and it cannot process any message


Suggested change

// and it cannot process any message

// and it cannot process any message.

Good point. Is part of PR now.

subpop · 2023-11-14T15:57:18Z

ipc/interfaces.go

 )

 func (e WorkerEventName) String() string {
 	switch e {
-	case 1:
+	case WorkerEventNameBegin:


Wow. We were not doing this before? Good find and refactor!

subpop · 2023-11-14T16:10:42Z

If I'm not mistaken, this PR fixes #135, correct?

jirihnidek · 2023-11-15T07:38:08Z

If I'm not mistaken, this PR fixes #135, correct?

No, it does not. It could be used as a first step of getting correct list of workers, but you proposed better approach (easier to implement and more reliable).

Your idea was something like this: "It will be easier to list all workers using interface com.redhat.Yggdrasil1.Worker1.*"

You are welcome,

Your External Memory ;-)

* When worker is started, then it sends event "started" * It also sends event "stopped", when it is terminated and proces cannot handle message anymore * This functionality is IMHO important for developers of yggdrasil worker. When you develop new worker it is important to see that your worker "communication" with yggd during start. It is also decency to say something like "good bye" at the end. * Small refactoring of code (do not use numeric constant) * Updated doc strings * Extended documentation in com.redhat.Yggdrasil1.Worker1.xml according changes in code

jirihnidek · 2023-11-15T08:02:09Z

Works great! Can we follow this up with a PR that re-emits these events on the public com.redhat.Yggdrasil1 D-Bus interface?

Do you mean that other workers could have some benefits from the fact that there is another buddy processing dispatched messages? Interesting idea, but I would implement that, when it will be really needed. We have many more urgent card in backlog.

subpop · 2023-11-15T15:43:02Z

If I'm not mistaken, this PR fixes #135, correct?

No, it does not. It could be used as a first step of getting correct list of workers, but you proposed better approach (easier to implement and more reliable).

Your idea was something like this: "It will be easier to list all workers using interface com.redhat.Yggdrasil1.Worker1.*"

You are welcome,

Your External Memory ;-)

Oh yes, I remember that now. I just commented on #135 with that suggestion so that Future Link can be reminded as well.

subpop · 2023-11-15T15:48:46Z

Works great! Can we follow this up with a PR that re-emits these events on the public com.redhat.Yggdrasil1 D-Bus interface?

Do you mean that other workers could have some benefits from the fact that there is another buddy processing dispatched messages? Interesting idea, but I would implement that, when it will be really needed. We have many more urgent card in backlog.

No, yggd implements the com.redhat.Yggdrasil1 interface and exports itself on the main system bus. This is the API through which yggctl communicates with yggd. Part of this interface is a signal, WorkerEvent. That signal is emitted when workers emit events. Looking at the code that emits the signals in yggd's client.go, the STARTED and STOPPED signals will be emitted, but STARTED and STOPPED will be undocumented signals until the com.redhat.Yggdrasil1 D-Bus interface specification is updated to mention them.

subpop

LGTM

jirihnidek · 2023-11-15T16:14:43Z

Works great! Can we follow this up with a PR that re-emits these events on the public com.redhat.Yggdrasil1 D-Bus interface?

Do you mean that other workers could have some benefits from the fact that there is another buddy processing dispatched messages? Interesting idea, but I would implement that, when it will be really needed. We have many more urgent card in backlog.

No, yggd implements the com.redhat.Yggdrasil1 interface and exports itself on the main system bus. This is the API through which yggctl communicates with yggd. Part of this interface is a signal, WorkerEvent. That signal is emitted when workers emit events. Looking at the code that emits the signals in yggd's client.go, the STARTED and STOPPED signals will be emitted, but STARTED and STOPPED will be undocumented signals until the com.redhat.Yggdrasil1 D-Bus interface specification is updated to mention them.

New signals are documented here: https://github.com/RedHatInsights/yggdrasil/pull/187/files#diff-ec9037acd507f9680945dd78261e1be61e857c68fa392d3bcba5e10332e0e2a9 Should these signals be documented elsewhere?

So, do I understand you correctly that you want to transmit STARTED and STOPPED signals in the same ways as WORKING signal is transmitted here:

yggdrasil/cmd/yggd/client.go

Lines 180 to 193 in cc61d41

    
           go func() { 
        
           	for e := range c.dispatcher.WorkerEvents { 
        
           		args := []interface{}{e.Worker, e.Name, e.MessageID, e.ResponseTo} 
        
           		switch e.Name { 
        
           		case ipc.WorkerEventNameWorking: 
        
           			args = append(args, e.Data) 
        
           		} 
        
           		if err := c.conn.Emit("/com/redhat/Yggdrasil1", "com.redhat.Yggdrasil1.WorkerEvent", args...); err != nil { 
        
           			log.Errorf("cannot emit event: %v", err) 
        
           			continue 
        
           		} 
        
           		log.Debugf("emitted event: %+v", e) 
        
           	} 
        
           }()

subpop · 2023-11-15T16:27:59Z

New signals are documented here: https://github.com/RedHatInsights/yggdrasil/pull/187/files#diff-ec9037acd507f9680945dd78261e1be61e857c68fa392d3bcba5e10332e0e2a9 Should these signals be documented elsewhere?

Yes. That is the D-Bus API used by workers to communicate with their "dispatcher". That API could run on any bus: the system bus or a private session bus. The D-Bus interface specification I linked to is the "public" D-Bus API that is exported by yggd in order to enable yggctl communication.

So, do I understand you correctly that you want to transmit STARTED and STOPPED signals in the same ways as WORKING signal is transmitted here:

yggdrasil/cmd/yggd/client.go

Lines 180 to 193 in cc61d41

go func() {

for e := range c.dispatcher.WorkerEvents {

args := []interface{}{e.Worker, e.Name, e.MessageID, e.ResponseTo}

switch e.Name {

case ipc.WorkerEventNameWorking:

args = append(args, e.Data)

}

if err := c.conn.Emit("/com/redhat/Yggdrasil1", "com.redhat.Yggdrasil1.WorkerEvent", args...); err != nil {

log.Errorf("cannot emit event: %v", err)

continue

}

log.Debugf("emitted event: %+v", e)

}

}()

Given the way that code is written, they will be emitted, but their "Name" value might be unknown. We should also ensure that they're documented in the externally facing D-Bus API so that clients and code generation utilities can expect additional values beyond 1, 2 and 3.

jirihnidek force-pushed the more_worker_events branch from 8b6314f to e4e2a48 Compare November 13, 2023 08:56

subpop requested changes Nov 14, 2023

View reviewed changes

jirihnidek force-pushed the more_worker_events branch from e4e2a48 to 238e672 Compare November 15, 2023 07:54

subpop approved these changes Nov 15, 2023

View reviewed changes

subpop merged commit 818d654 into RedHatInsights:main Nov 15, 2023
10 checks passed

jirihnidek deleted the more_worker_events branch November 15, 2023 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add more events for starting and stopping of worker #187

feat: Add more events for starting and stopping of worker #187

jirihnidek commented Nov 13, 2023 •

edited

Loading

jirihnidek commented Nov 13, 2023 •

edited

Loading

subpop left a comment

subpop Nov 14, 2023

jirihnidek Nov 15, 2023

subpop Nov 14, 2023

jirihnidek Nov 15, 2023

subpop Nov 14, 2023

subpop commented Nov 14, 2023

jirihnidek commented Nov 15, 2023

jirihnidek commented Nov 15, 2023

subpop commented Nov 15, 2023

subpop commented Nov 15, 2023

subpop left a comment

jirihnidek commented Nov 15, 2023

subpop commented Nov 15, 2023

	// and it cannot process any message
	// and it cannot process any message.

feat: Add more events for starting and stopping of worker #187

feat: Add more events for starting and stopping of worker #187

Conversation

jirihnidek commented Nov 13, 2023 • edited Loading

jirihnidek commented Nov 13, 2023 • edited Loading

subpop left a comment

Choose a reason for hiding this comment

subpop Nov 14, 2023

Choose a reason for hiding this comment

jirihnidek Nov 15, 2023

Choose a reason for hiding this comment

subpop Nov 14, 2023

Choose a reason for hiding this comment

jirihnidek Nov 15, 2023

Choose a reason for hiding this comment

subpop Nov 14, 2023

Choose a reason for hiding this comment

subpop commented Nov 14, 2023

jirihnidek commented Nov 15, 2023

jirihnidek commented Nov 15, 2023

subpop commented Nov 15, 2023

subpop commented Nov 15, 2023

subpop left a comment

Choose a reason for hiding this comment

jirihnidek commented Nov 15, 2023

subpop commented Nov 15, 2023

jirihnidek commented Nov 13, 2023 •

edited

Loading

jirihnidek commented Nov 13, 2023 •

edited

Loading