WIP: Add a process manager "class" #130

vpodzime · 2021-04-20T10:10:06Z

In many places there's a need to keep track of sub-processes
spawned by the main process. Such processes need to be looked up
by various parameters, interacted with, and, eventually,
terminated.

Having a common, reliable and fast implementation with high test
coverage that holds the various pieces of information about
sub-processes together is beneficial.

Ticket: CFE-3572
Changelog: None

Steps:

libutils/proc_manager.h

vpodzime · 2021-04-21T09:05:43Z

I replaced the file descriptors by FILE streams. Our code works with them and while it's possible to go from a FILE stream to an FD (fileno()), the other direction is impossible. Lookup functions "by FD" still make sense to me, though.

vpodzime · 2021-04-22T14:39:37Z

Actually, we will need the termination functionality for custom promise modules.

libutils/array_map.c

vpodzime · 2021-04-23T09:03:16Z

@olehermanse please review, if you have a chance. The only missing bit here, AFAICT, are unit tests for the termination functions.

libutils/hash_map.c

vpodzime · 2021-04-26T17:04:20Z

@cf-bottom jenkins, please

cf-bottom · 2021-04-26T17:08:44Z

Sure, I triggered a build:

Jenkins: https://ci.cfengine.com/job/pr-pipeline/6479/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-6479/

olehermanse

Looks good in general, just some smaller comments.

olehermanse · 2021-04-27T03:41:20Z

libutils/array_map.c

+            memmove(map->values + i, map->values + i + 1,
+                    sizeof(MapKeyValue) * (map->size - i - 1));


Especially because of the memmove(), I think this function should have unit tests (which would be running using ASAN).

It's just a copy-pasted code from ArrayMapRemove().

Actually, we don't have any unit tests doing ArrayMapRemove(). I created CFE-3643 instead of trying to save the world in this PR.

libutils/proc_manager.c

libutils/array_map.c

libutils/hash_map.c

libutils/proc_manager.c

Allowing values being removed from the map without being destroyed (keys are always owned by the map and thus destroyed). Ticket: CFE-3572 Changelog: None

In many places there's a need to keep track of sub-processes spawned by the main process. Such processes need to be looked up by various parameters and interacted with. Having a common, reliable and fast implementation with high test coverage that holds the various pieces of information about sub-processes together is beneficial. In the future, we should extend this new API with the process spawning and termination functionality. Ticket: CFE-3572 Changelog: None

vpodzime · 2021-04-27T08:33:11Z

@cf-bottom jenkins with exotics, please

cf-bottom · 2021-04-27T08:33:32Z

Alright, I triggered a build:

(with exotics)

Jenkins: https://ci.cfengine.com/job/pr-pipeline/6482/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-6482/

vpodzime · 2021-04-27T13:15:37Z

There are failures on exotics. And we also need to be able to associate some custom data with the sub-processes managed.

olehermanse

Looks good to me, thanks for working on this! I see there are some remaining TODOs, but feel free to address those in separate PRs.

olehermanse · 2021-05-19T11:58:05Z

libutils/proc_manager.h

+    pid_t pid;            /** PID of the subprocess                          [always >= 0]      */
+    FILE *input;          /** stream of the input to the subprocess          [can be %NULL]     */
+    FILE *output;         /** stream of the output from the subprocess       [can be %NULL]     */
+    char lookup_io;       /** which stream/FD to use for lookup by stream/FD ['i', 'o' or '\0'] */


To me, this seems like a setting that would be in the top level proc manager, not each subprocess entry.

This depends on the direction of the communication with the particular subprocess.

olehermanse · 2021-05-19T12:33:38Z

libutils/proc_manager.h

+ * @return whether the subprocess was successfully added or not (e.g. in case of
+ *         the process with the same id, PID or lookup stream/FD added before)
+ *
+ * @note Ownership of #id, #cmd and #description is transferred to the #manager.


What about the file streams? Are they closed by Subprocess / ProcManager?

Only in case of termination.

olehermanse · 2021-05-19T12:38:05Z

tests/unit/proc_manager_force_terminate_test.c

+                                         pid, input, output, 'o');
+    assert_true(success);
+
+    bool graceful_termination_failed = false;;


Suggested change

bool graceful_termination_failed = false;;

bool graceful_termination_failed = false;

olehermanse · 2021-05-19T12:38:43Z

tests/unit/proc_manager_force_terminate_test.c

+        close(parent_write_child_read[1]);
+        close(parent_read_child_write[0]);
+
+        /* Also close the ends the child should normally use, we don't need them. */


Suggested change

/* Also close the ends the child should normally use, we don't need them. */

/* Also close the ends the child should normally use, we don't need them */

olehermanse · 2021-05-19T12:39:00Z

tests/unit/proc_manager_force_terminate_test.c

+        /* Ignore SIGTERM */
+        signal(SIGTERM, SIG_IGN);
+
+        /* Wait to be killed. */


Suggested change

/* Wait to be killed. */

/* Wait to be killed */

olehermanse · 2021-05-19T12:41:09Z

tests/unit/proc_manager_force_terminate_test.c

+/**
+ * This file contains ProcManager tests that need to fork() a child process
+ * which totally confuses the unit test framework used by the other tests.
+ */


Worth mentioning that this file does not use said test framework.

Suggested change

/**

* This file contains ProcManager tests that need to fork() a child process

* which totally confuses the unit test framework used by the other tests.

*/

/**

* This file contains ProcManager tests that need to fork() a child process

* which totally confuses the unit test framework used by the other tests.

* This test doesn't use the framework, it instead implements it's own

* assert macros with failure printing and a "normal" main function.

*/

olehermanse · 2021-05-19T20:04:15Z

tests/unit/proc_manager_test.c

+    /* Now try to insert the process again (with a different command and
+     * description). This shouldn't be allowed. */
+    ret = ProcManagerAddProcess(manager,
+                                xstrdup("test-process"), xstrdup("/some/command2 arg1 arg2"), xstrdup("better test process"),
+                                (pid_t) 1234, input, output, 'i');


Unclear here whether this should fail because of id or pid or both.

olehermanse · 2021-05-19T20:36:49Z

libutils/proc_manager.c

+    if (id == NULL)
+    {
+        NDEBUG_UNUSED int ret = xasprintf(&id, "%jd", (intmax_t) pid);
+        assert(ret > 0);
+    }


Untested functionality.

olehermanse

There are failures on exotics. And we also need to be able to associate some custom data with the sub-processes managed.

Need to be addressed one way or another, before merging.

vpodzime added the WIP Work in Progress label Apr 20, 2021

KotzaBoss reviewed Apr 20, 2021

View reviewed changes

libutils/proc_manager.h Outdated Show resolved Hide resolved

KotzaBoss reviewed Apr 20, 2021

View reviewed changes

libutils/proc_manager.h Show resolved Hide resolved

KotzaBoss reviewed Apr 20, 2021

View reviewed changes

libutils/proc_manager.h Outdated Show resolved Hide resolved

vpodzime force-pushed the master-proc_manager branch from d298749 to 7397247 Compare April 20, 2021 12:01

larsewi reviewed Apr 20, 2021

View reviewed changes

libutils/proc_manager.h Show resolved Hide resolved

vpodzime force-pushed the master-proc_manager branch from 7397247 to 8d8774b Compare April 21, 2021 09:01

vpodzime force-pushed the master-proc_manager branch from 8d8774b to 839af1f Compare April 21, 2021 09:10

vpodzime mentioned this pull request Apr 22, 2021

CFE-3625: Increase Filedescriptor Limit to a more contemporary Value cfengine/core#4609

Closed

vpodzime force-pushed the master-proc_manager branch 3 times, most recently from 80179dd to c61c251 Compare April 22, 2021 12:44

NorthernTechHQ deleted a comment from lgtm-com bot Apr 22, 2021

vpodzime force-pushed the master-proc_manager branch from c61c251 to 317bc63 Compare April 22, 2021 12:56

vpodzime changed the title ~~WIP: Add a process manager "class"~~ Add a process manager "class" Apr 22, 2021

vpodzime marked this pull request as ready for review April 22, 2021 13:27

vpodzime requested review from KotzaBoss, larsewi and olehermanse April 22, 2021 13:28

vpodzime force-pushed the master-proc_manager branch from 317bc63 to 38ee191 Compare April 22, 2021 20:26

KotzaBoss reviewed Apr 22, 2021

View reviewed changes

libutils/array_map.c Show resolved Hide resolved

vpodzime force-pushed the master-proc_manager branch 2 times, most recently from 60adb29 to af97d5a Compare April 23, 2021 09:02

NorthernTechHQ deleted a comment from lgtm-com bot Apr 23, 2021

larsewi reviewed Apr 26, 2021

View reviewed changes

libutils/hash_map.c Outdated Show resolved Hide resolved

vpodzime force-pushed the master-proc_manager branch from af97d5a to d8a4e45 Compare April 26, 2021 10:44

vpodzime force-pushed the master-proc_manager branch from d8a4e45 to ea96219 Compare April 26, 2021 10:47

NorthernTechHQ deleted a comment from lgtm-com bot Apr 26, 2021

vpodzime force-pushed the master-proc_manager branch from ea96219 to ce3a560 Compare April 26, 2021 16:37

vpodzime removed the WIP Work in Progress label Apr 26, 2021

vpodzime requested review from KotzaBoss and larsewi April 26, 2021 17:04

olehermanse previously approved these changes Apr 27, 2021

View reviewed changes

vpodzime added 2 commits April 27, 2021 09:55

Add MapRemoveSoft() function

59cbd54

Allowing values being removed from the map without being destroyed (keys are always owned by the map and thus destroyed). Ticket: CFE-3572 Changelog: None

vpodzime dismissed olehermanse’s stale review via 042b7b9 April 27, 2021 08:30

vpodzime force-pushed the master-proc_manager branch from ce3a560 to 042b7b9 Compare April 27, 2021 08:30

vpodzime requested a review from olehermanse April 27, 2021 08:59

vpodzime added the WIP Work in Progress label Apr 27, 2021

olehermanse approved these changes May 19, 2021

View reviewed changes

olehermanse requested changes May 19, 2021

View reviewed changes

vpodzime changed the title ~~Add a process manager "class"~~ WIP: Add a process manager "class" May 20, 2021

larsewi removed request for KotzaBoss and larsewi August 11, 2025 11:34

		memmove(map->values + i, map->values + i + 1,
		sizeof(MapKeyValue) * (map->size - i - 1));

	bool graceful_termination_failed = false;;
	bool graceful_termination_failed = false;

	/* Also close the ends the child should normally use, we don't need them. */
	/* Also close the ends the child should normally use, we don't need them */

WIP: Add a process manager "class" #130

Are you sure you want to change the base?

WIP: Add a process manager "class" #130

Uh oh!

Conversation

vpodzime commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vpodzime commented Apr 21, 2021

Uh oh!

vpodzime commented Apr 22, 2021

Uh oh!

Uh oh!

vpodzime commented Apr 23, 2021

Uh oh!

Uh oh!

vpodzime commented Apr 26, 2021

Uh oh!

cf-bottom commented Apr 26, 2021

Uh oh!

olehermanse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpodzime Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vpodzime commented Apr 27, 2021

Uh oh!

cf-bottom commented Apr 27, 2021

Uh oh!

vpodzime commented Apr 27, 2021

Uh oh!

olehermanse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olehermanse left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

vpodzime commented Apr 20, 2021 •

edited

Loading

vpodzime Apr 27, 2021 •

edited

Loading