Session Recording #4892

atmaxinger · 2023-03-28T16:12:13Z

Basics

Short descriptions of your changes are in the release notes
(added as entry in doc/news/_preparation_next_release.md which contains _(my name)_)
Please always add them to the release notes.
Details of what you changed are in commit messages
(first line should have module: short statement syntax)
References to issues, e.g. close #X, are in the commit messages.
The buildservers are happy. If not, fix in this order:
- add a line in doc/news/_preparation_next_release.md
- reformat the code with scripts/dev/reformat-all
- make all unit tests pass
- fix all memleaks
- fix the CI itself (or rebase if already fixed)
The PR is rebased with current master.

Checklist

I added unit tests for my code
I fully described what my PR does in the documentation
(not in the PR description)
I fixed all affected documentation (see Documentation Guidelines)
I fixed all affected decisions (see Decision Process)
I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
I updated all meta data (e.g. README.md of plugins and METADATA.ini)
I mentioned every code not directly written by me in reuse syntax

Review

Documentation is introductory, concise, good to read and describes everything what the PR does
Examples are well chosen and understandable
Code is conforming to our Coding Guidelines
APIs are conforming to our Design Guidelines
Code is consistent to our Design Decisions

Labels

Add the "work in progress" label if you do not want the PR to be reviewed yet.
Add the "ready to merge" label if everything is done and no further pushes are planned by you.

src/libs/record/record.c

flo91

Again an impressive amount of work and progress! 😲
The code is well structured and not too hard to get into.
Good documentation and lots of tests are present.

Keep up that great work! 👍

My findings are available in the comments below.
I'll make a 2nd review when this PR is ready to merge.

doc/contrib/recording.md

doc/help/kdb-record.md

src/libs/elektra/contracts.c

src/libs/elektra/diff.c

src/libs/elektra/kdb.c

src/libs/record/record.c

src/plugins/recorder/recorder.c

src/tools/kdb/recordundo.cpp

tests/cframework/tests.h

atmaxinger · 2023-04-05T11:08:10Z

Does anyone have an idea how to debug the test issues on Cirrus CI?

There are a ton of shell tests that fail with the following error:

198/286 Test #198: testscr_check_kdb_internal_suite ..............***Failed    0.01 sec
ELEKTRA KDB INTERNAL TEST SUITE
Check if script tests the correct version
fatal (no cleanup necessary): Script was not compiled (0.9.14) with this Elektra version (): KDB_VERSION mismatch for kdb-tool (kdb), use CHECK_VERSION=NO to disable

It looks fishy that the second parenthesis is empty. The elektra version number should be present there. It is read from system:/elektra/version/constants/KDB_VERSION. Kind of seems like it can't find the kdb executable there.

atmaxinger · 2023-04-05T11:09:05Z

jenkins build libelektra please

atmaxinger · 2023-04-05T13:33:53Z

Ah okay, for future reference: The install script didn't complete because it couldn't find the man page for elektra-record.

atmaxinger · 2023-04-12T08:40:34Z

jenkins build libelektra please

atmaxinger · 2023-04-12T11:13:35Z

jenkins build libelektra please

atmaxinger · 2023-04-12T11:15:55Z

jenkins build libelektra please

atmaxinger · 2023-04-12T13:59:24Z

jenkins build libelektra please

atmaxinger · 2023-04-12T15:05:13Z

jenkins build libelektra please

atmaxinger · 2023-04-12T16:03:36Z

jenkins build libelektra please

atmaxinger · 2023-04-12T16:58:22Z

jenkins build libelektra please

atmaxinger · 2023-04-12T19:52:59Z

jenkins build libelektra please

atmaxinger · 2023-04-12T20:13:10Z

jenkins build libelektra please

atmaxinger · 2023-04-12T21:17:09Z

@flo91 @markus2330
I think I'm at a bit of a loss here.

The test program test_record segfaults in Jenkins on Fedora 37, and only in Jenkins. No other CI builds fail, and I can't reproduce it even with the same Docker images locally on multiple computers. Even all the ASAN builds are successful, and locally valgrind did not find any erroneous reads or writes.

What's even weirder is I've enabled --verbose for ctest to see the output. All other tests output something, but the test_record doesn't. It seems to crash before main.

See for example the output for test_sha-256:

test 273
        Start 273: test_sha-256

273: Test command: /home/jenkins/workspace/libelektra_PR-4892/build\ directory/bin/test_sha-256 "/home/jenkins/workspace/libelektra_PR-4892/build directory/tests/ctest"
273: Working Directory: /home/jenkins/workspace/libelektra_PR-4892/build directory/tests/ctest
273: Environment variables: 
273:  LD_LIBRARY_PATH=/home/jenkins/workspace/libelektra_PR-4892/build directory/lib
273: Test timeout computed to be: 1500
273: SHA-256    TESTS
273: ==================
273: 
273: 
273: test_sha-256 RESULTS: 13 test(s) done. 0 error(s).
165/170 Test #273: test_sha-256 .......................   Passed    0.00 sec

And now the output of test_record:

test 272
        Start 272: test_record

272: Test command: /home/jenkins/workspace/libelektra_PR-4892/build\ directory/bin/test_record "/home/jenkins/workspace/libelektra_PR-4892/build directory/tests/ctest"
272: Working Directory: /home/jenkins/workspace/libelektra_PR-4892/build directory/tests/ctest
272: Environment variables: 
272:  LD_LIBRARY_PATH=/home/jenkins/workspace/libelektra_PR-4892/build directory/lib
272: Test timeout computed to be: 1500
164/170 Test #272: test_record ........................***Exception: SegFault  0.00 sec

atmaxinger · 2023-04-13T18:25:07Z

Oh wow, that was a truly multi-level bug cascade. But everything seems to work now 😁.

atmaxinger · 2023-04-19T22:21:24Z

jenkins build libelektra please

atmaxinger · 2023-04-20T07:53:00Z

jenkins build libelektra please

atmaxinger · 2023-04-20T11:35:43Z

jenkins build libelektra please

Co-authored-by: Markus Raab <[email protected]>

…currency

…oncurrency

atmaxinger · 2023-05-29T19:06:17Z

jenkins build libelektra please

kodebach

LGTM, just a few suggestions for the docs

doc/help/elektra-glossary.md

kodebach · 2023-05-30T23:24:34Z

doc/help/kdb-record-start.md

+
+Note: when you activate session recording, concurrency of Elektra will be somewhat limited.
+As long as it is active, a global lock will be created to ensure no two processes will write data simultaneously.
+This behavior is similar as to when multiple processes will write to the same configuration file.


Suggested change

This behavior is similar as to when multiple processes will write to the same configuration file.

The errors reports to applications in such cases are similar to when multiple processes write to the same configuration file.

doc/help/kdb-record-start.md

doc/help/kdb-record-state.md

src/libs/ease/array.c

src/plugins/ansible/README.md

src/libs/elektra/errors.c

kodebach · 2023-05-31T00:01:48Z

One more thing, you should add meta:/elektra/deleted to doc/METADATA.ini (and probably meta:/elektra/* as reserved).

…ill be removed

…ent with terminology

flo91

Outstanding and comprehensive work done here! 🎉
The contribution is well structured, well documented and includes lots of test cases. 👍🏻

I've just found a few small mistakes and have a few questions.
This PR got already merged during my review, but it shouldn't be a problem to create a follow-up PR where you incorporate these changes to improve the already high quality of your contribution even a bit more.

Additionally, it would be great if you find time to also review my PR for the ODBC backend: #4915

Thank you!

flo91 · 2023-06-04T19:52:57Z

doc/RECORD.ini

+	this key denotes the parent key of the session. Only changes same of
+	below this key will be recorded.


Suggested change

this key denotes the parent key of the session. Only changes same of

below this key will be recorded.

this key denotes the parent key of the session.

Only changes of this key and below this key will be recorded.

flo91 · 2023-06-04T19:56:18Z

doc/contrib/recording.md

+A recording session is a period of time during which changes to the Key Database (KDB) are tracked and accumulated.
+It begins when recording starts and ends when recording stops.
+Throughout the recording session, all changes made to the KDB are recorded, including additions, modifications, and deletions of keys and their associated values.
+The recording session provides a complete audit trail of all changes made to the KDB during the specified period of time.


Suggested change

The recording session provides a complete audit trail of all changes made to the KDB during the specified period of time.

The same sentence that you already removed from doc/help/elektra-glossary.md after the review from @kodebach.

flo91 · 2023-06-04T20:14:50Z

doc/dev/hooks.md

+Used for the session recording plugin.
+Hard coded to search for a plugin named `recorder`.
+
+The following function must be exported:


Suggested change

The following function must be exported:

The following functions must be exported:

flo91 · 2023-06-04T20:16:54Z

doc/dev/hooks.md

+  - Signature: `int (Plugin * handle, Key * parentKey)`
+  - Called in `kdbSet` before the storage phase.
+  - The `parentKey` must not be modified, except for adding errors and warnings.
+  - Must ensure that this is only process that can record changes until `unlock` is called.


Suggested change

- Must ensure that this is only process that can record changes until `unlock` is called.

- Must ensure that this is the only process that can record changes until `unlock` is called.

flo91 · 2023-06-04T20:19:32Z

doc/help/elektra-glossary.md

+- **recording session**:
+  A recording session is a period of time during which changes to the Key Database (KDB) are tracked and accumulated.
+  It begins when recording starts and ends when recording stops.
+  Throughout the recording session, all changes made to the KDB are recorded, including additions, modifications, and deletions of keys and their associated values.


Suggested change

Throughout the recording session, all changes made to the KDB are recorded, including additions, modifications, and deletions of keys and their associated values.

Throughout the recording session, all changes made to the KDB are recorded, including additions, modifications, and deletions of keys and their associated values and metadata.

flo91 · 2023-06-08T01:45:13Z

src/bindings/cpp/tests/testcpp_diff.cpp

+	EXPECT_EQ (diffWithDifferences.isEmpty (), false);
+	EXPECT_EQ (diffWithoutDifferences.isEmpty (), true);


Could be replaced with EXPECT_FALSE resp. EXPECT_TRUE.

flo91 · 2023-06-08T11:32:08Z

src/libs/record/record.c

+		ksDel (ksCut (toRecord, ksAtCursor (blacklistedKeys, it)));
+	}
+
+	if (ksGetSize (toRecord) == 0 && ksGetSize (newKeys) != 0)


Is the 2nd condition (ksGetSize (newKeys) != 0) really necessary here?
Maybe you could also add the check for an empty KeySet newKeys at the beginning of this function (after the NULL-pointer checks).

flo91 · 2023-06-08T11:40:26Z

src/libs/record/record.c

+	return successful;
+}
+
+bool elektraRecordGetDiff (KDB * handle, ElektraDiff ** diff, Key * errorKey)


Documentation is missing for this function.

flo91 · 2023-06-08T11:41:13Z

src/libs/record/record.c

+	return ks;
+}
+
+bool elektraRecordExportSession (KDB * handle, Plugin * plugin, Key * parentKey, Key * errorKey)


Documentation is also missing for this function.

flo91 · 2023-06-08T11:42:20Z

src/libs/record/record.c

+		// We need to duplicate the ks, as `getDiffFromSessionStorage` will remove it from sessionStorage
+		sessionStorageBackup = ksDup (ksBelow (sessionStorage, sessionKey));
+
+		// Remove all keys from session storage that are not initiall under parent key


"initiall" -> "initially"

atmaxinger force-pushed the recording branch from 3842b1d to 3552ce8 Compare March 29, 2023 19:03

atmaxinger commented Mar 30, 2023

View reviewed changes

src/libs/record/record.c Outdated Show resolved Hide resolved

markus2330 mentioned this pull request Mar 31, 2023

/elektra hierarchy #4894

Closed

atmaxinger force-pushed the recording branch from 218dc1a to 98f6199 Compare April 1, 2023 15:05

flo91 self-requested a review April 4, 2023 19:54

flo91 suggested changes Apr 4, 2023

View reviewed changes

atmaxinger force-pushed the recording branch 3 times, most recently from 98cdae1 to ad68ed9 Compare April 11, 2023 22:21

atmaxinger force-pushed the recording branch 3 times, most recently from 0d50bb2 to 91f27e1 Compare April 13, 2023 17:00

atmaxinger force-pushed the recording branch from 7f93498 to d072027 Compare April 20, 2023 10:50

atmaxinger and others added 8 commits May 29, 2023 16:10

plugins/ansible: remove unnecessary check for meta value

fcaa180

doc: update recording tutorial

194918c

doc: apply suggestions by @markus2330

3b74e18

Co-authored-by: Markus Raab <[email protected]>

doc: add session recording concepts to glossary

b3bd8bf

doc: update recording contributer documentation

5d07e77

doc: remove unnecessary break in kdb record-export manpage

8ad7983

doc: update glossary manpage

cf1ac0c

plugins/ansible: add tests

f7aab7e

atmaxinger force-pushed the recording branch from 2996cc2 to f7aab7e Compare May 29, 2023 14:16

restyled-commits and others added 6 commits May 29, 2023 16:18

Restyled by prettier-markdown

b260e76

doc: fix spelling

64d69da

doc: specify in the man pages that starting recording will affect con…

bd73f4a

…currency

doc: specify in the release notes that session recording will limit c…

762e016

…oncurrency

doc: update spelling

6aab52e

doc: add specification for recording

5746f8b

atmaxinger requested review from kodebach and markus2330 May 30, 2023 07:44

kodebach approved these changes May 31, 2023

View reviewed changes

atmaxinger added 7 commits May 31, 2023 10:00

doc: apply suggestions from @kodebach

bf87cc6

plugins/ansible: clarify that key marked with meta:/elektra/deleted w…

294dd42

…ill be removed

errors: fix CodeQL warning

57aec63

ease: update Doxygen comment

f10b216

rename meta:/elektra/deleted to meta:/elektra/removed to stay consist…

ef7f01d

…ent with terminology

doc: add elektra/removed to METADATA spec

dc7f1fa

plugins/ansible: fix spelling

44d2a2e

flo91 approved these changes Jun 4, 2023

View reviewed changes

markus2330 merged commit de26195 into ElektraInitiative:master Jun 7, 2023

flo91 reviewed Jun 8, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Session Recording #4892

Session Recording #4892

atmaxinger commented Mar 28, 2023 •

edited

Loading

flo91 left a comment

atmaxinger commented Apr 5, 2023

atmaxinger commented Apr 5, 2023

atmaxinger commented Apr 5, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 13, 2023

atmaxinger commented Apr 19, 2023

atmaxinger commented Apr 20, 2023

atmaxinger commented Apr 20, 2023

atmaxinger commented May 29, 2023

kodebach left a comment

kodebach May 30, 2023

kodebach commented May 31, 2023 •

edited

Loading

flo91 left a comment

flo91 Jun 4, 2023

flo91 Jun 4, 2023

flo91 Jun 4, 2023

flo91 Jun 4, 2023

flo91 Jun 4, 2023

flo91 Jun 8, 2023

flo91 Jun 8, 2023

flo91 Jun 8, 2023

flo91 Jun 8, 2023

flo91 Jun 8, 2023

	This behavior is similar as to when multiple processes will write to the same configuration file.
	The errors reports to applications in such cases are similar to when multiple processes write to the same configuration file.

		this key denotes the parent key of the session. Only changes same of
		below this key will be recorded.

	The following function must be exported:
	The following functions must be exported:

	- Must ensure that this is only process that can record changes until `unlock` is called.
	- Must ensure that this is the only process that can record changes until `unlock` is called.

	Throughout the recording session, all changes made to the KDB are recorded, including additions, modifications, and deletions of keys and their associated values.
	Throughout the recording session, all changes made to the KDB are recorded, including additions, modifications, and deletions of keys and their associated values and metadata.

		EXPECT_EQ (diffWithDifferences.isEmpty (), false);
		EXPECT_EQ (diffWithoutDifferences.isEmpty (), true);

Session Recording #4892

Session Recording #4892

Conversation

atmaxinger commented Mar 28, 2023 • edited Loading

Basics

Checklist

Review

Labels

flo91 left a comment

Choose a reason for hiding this comment

atmaxinger commented Apr 5, 2023

atmaxinger commented Apr 5, 2023

atmaxinger commented Apr 5, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 12, 2023

atmaxinger commented Apr 13, 2023

atmaxinger commented Apr 19, 2023

atmaxinger commented Apr 20, 2023

atmaxinger commented Apr 20, 2023

atmaxinger commented May 29, 2023

kodebach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kodebach commented May 31, 2023 • edited Loading

flo91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atmaxinger commented Mar 28, 2023 •

edited

Loading

kodebach commented May 31, 2023 •

edited

Loading