Issue 5843 - dsconf / dscreate should be able to handle lmdb parameters #5943

progier389 · 2023-10-04T12:03:20Z

Description:

dscreate changes:
- make db_lib a standard option instead of an advanced one
- add mdb_max_size as standard option
dsconf instance backend config set changes:
- add --mdb_max_size option associated with nsslapd-mdb-max-size
- add --mdb_max_readers option associated with nsslapd-mdb_max_readers
- add --mdb_max_dbs option associated with nsslapd-mdb_max_dbs

Issue: #5843

Reviewed by: @Firstyear and @droideck ( Thanks ! )

dscreate changes: - make db_lib a standard option instead of an advanced one - add lmdb_size as standard option dsconf instance backend config set changes: - add --mdb_max_size option associated with nsslapd-mdb-max-size - add --mdb_max_readers option associated with nsslapd-mdb_max_readers - add --mdb_max_dbs option associated with nsslapd-mdb_max_dbs

Firstyear · 2023-10-05T01:45:54Z

src/lib389/lib389/cli_conf/backend.py

@@ -65,6 +65,10 @@
        'deadlock_policy': 'nsslapd-db-deadlock-policy',
        'db_home_directory': 'nsslapd-db-home-directory',
        'db_lib': 'nsslapd-backend-implement',
+        'lmdb_size': 'nsslapd-mdb-max-size',    # For dscreate (unit is Gb)


Why have this as two separate parameters? Just because of the different units? Why not do something else to parse the units or similar?

I hesitated about that one, but IMHO you are right: parsing human friendly size would be more consistent and that is not hard do. So I will change that.

Firstyear · 2023-10-05T01:46:50Z

src/lib389/lib389/cli_ctl/dblib.py

@@ -236,7 +236,7 @@ def dblib_bdb2mdb(inst, log, args):
        total_dbi += be['dbi']

    # Round up dbmap size
-    dbmap_size = DEFAULT_DBMAP_SIZE
+    dbmap_size = DEFAULT_LMDB_SIZE * GIGABYTE
    while (total_dbsize * DBSIZE_MARGIN > dbmap_size):
        dbmap_size *= 1.25


I'm not sure what this is trying to achieve? maybe some better comments about this algo and why.

When looking at the whole function, it is clear that this code computes a size for lmdb database that is large enough to store the existing data.
IMHO the puzzling question is not what it tries to achieve but why it is done this way
(starting from the default size and increasing the size by steps until it is large enough)
rather than using directly the computed size plus a margin ?
There was probably a reason but I just do not remember it and whatever it was it is likely arguable
But that piece of code was written 2 years ago and the only thing that changed is the way to get the default value. So my feeling is that we are getting out of the scope of this change and if you think we should really change things in this area, we should better open a new issue.

Anyway I have to review that code to fix a regression due to the change of default db map size from 2Gb to 20Gb
So I spend the time to understand what I have done 2 years ago ! -;)
Then I added a few comments, especially on the tests done after the size computation.

Firstyear · 2023-10-05T01:47:22Z

src/lib389/lib389/instance/options.py

@@ -114,6 +115,8 @@ def parse_inf_config(self, config):
                    v = config.getint(self._section, k)
                elif self._type[k] == bool:
                    v = config.getboolean(self._section, k)
+                elif self._type[k] == float:


This should be an int not a float. But better would be a way to parse the units.

Fixed (it is now a string)

Firstyear · 2023-10-05T01:47:31Z

src/lib389/lib389/instance/setup.py

@@ -446,6 +449,36 @@ def create_from_cli(self):
            slapd['root_password'] = rootpw1
            break

+        # Database implementation (db_lib)
+        while 1:


while True.

I looked at other occurrences as I cut and pasted that piece of code from previous option handling
and indeed the file is plagued by that one.
grep -c 'while 1' instance/setup.py
11
I will global substitute them ...

droideck

Beside that, I see that the dblib test fails (I'm not sure if it's related):

    def test_dblib_migration(topo_m2, init_user):
        """
        Verify dsctl dblib xxxxxxx sub commands (migration between bdb and lmdb)
    
        :id: 5d327c34-e77a-46e5-a8aa-0a552f9bbdef
        :setup: Two suppliers Instance
        :steps:
            1. Determine current database
            2. Switch to the other database
            3. Check that replication works
        :expectedresults:
            1. Success
            2. Success
            3. Success
        """
        s1 = topo_m2.ms["supplier1"]
        s2 = topo_m2.ms["supplier2"]
        db_lib = s1.get_db_lib()
        repl = ReplicationManager(DEFAULT_SUFFIX)
        users = UserAccounts(s1, DEFAULT_SUFFIX)
        assert users.get('test entry')
        args = FakeArgs({'tmpdir': None})
        if db_lib == 'bdb':
            dblib_bdb2mdb(s1, log, args)
            dblib_cleanup(s1, log, args)
            _check_db(s1, log, 'mdb')
            repl.test_replication_topology([s1, s2])
            dblib_mdb2bdb(s1, log, args)
            dblib_cleanup(s1, log, args)
            _check_db(s1, log, 'bdb')
            repl.test_replication_topology([s1, s2])
        else:
            dblib_mdb2bdb(s1, log, args)
            dblib_cleanup(s1, log, args)
            _check_db(s1, log, 'bdb')
            repl.test_replication_topology([s1, s2])
            dblib_bdb2mdb(s1, log, args)
            dblib_cleanup(s1, log, args)
>           _check_db(s1, log, 'mdb')

dirsrvtests/tests/suites/clu/dsctl_dblib_test.py:118: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
dirsrvtests/tests/suites/clu/dsctl_dblib_test.py:56: in _check_db
    assert DatabaseConfig(inst).get_db_lib() == impl
/usr/lib/python3.11/site-packages/lib389/backend.py:1107: in __init__
    self._db_lib = self.get_attr_val_utf8_l('nsslapd-backend-implement')
/usr/lib/python3.11/site-packages/lib389/_mapped_object.py:800: in get_attr_val_utf8_l
    x = self.get_attr_val(key)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <lib389.backend.DatabaseConfig object at 0x7f566e5fd710>
key = 'nsslapd-backend-implement', use_json = False

    def get_attr_val(self, key, use_json=False):
        self._log.debug("%s getVal(%r)" % (self._dn, key))
        # We might need to add a state check for NONE dn.
        if self._instance.state != DIRSRV_STATE_ONLINE:
>           raise ValueError("Invalid state. Cannot get properties on instance that is not ONLINE")
E           ValueError: Invalid state. Cannot get properties on instance that is not ONLINE

src/lib389/lib389/utils.py

progier389 · 2023-10-10T09:57:25Z

I fixed the review comments and the failing clu test case (migration tools tests)
Tests are now OK.
There is still a failure in vlv test in a very recently added test case (about testing VLV reindexing)
And I am pretty sure it is a genuine bug (vlv index is a real nightmare with lmdb) but unrelated to the current PR
I will open an issue about that one.

droideck

Besides the minor issue, looks good to me!

src/lib389/lib389/utils.py

@droideck

Regression with dscreate from-instance before creating first instance: the target directory does not exists and a warning is inserted in the template file which is then broken. Fix is pretty trivial: Select an existing directory to compute the file system free space. Issue #5943 Reviewed by: @droideck (Thanks !)

progier389 force-pushed the i5843 branch from ec8509c to 29a8d39 Compare October 4, 2023 12:36

Firstyear requested changes Oct 5, 2023

View reviewed changes

droideck requested changes Oct 5, 2023

View reviewed changes

src/lib389/lib389/utils.py Show resolved Hide resolved

Issue 5843 - fix review comments

064851d

progier389 force-pushed the i5843 branch 3 times, most recently from f617a30 to 0c0cce7 Compare October 9, 2023 16:06

Issue 5843 - Fix CI regression due to default db map size increase

31899fd

progier389 force-pushed the i5843 branch from 0c0cce7 to 31899fd Compare October 9, 2023 17:07

progier389 linked an issue Oct 10, 2023 that may be closed by this pull request

dsconf / dscreate should be able to handle lmdb parameters #5843

Closed

droideck approved these changes Oct 10, 2023

View reviewed changes

src/lib389/lib389/utils.py Outdated Show resolved Hide resolved

progier389 force-pushed the i5843 branch 2 times, most recently from 2227d72 to f905952 Compare October 11, 2023 10:57

Issue 5843 - fix review comments 2

85c85f2

progier389 force-pushed the i5843 branch from f905952 to 85c85f2 Compare October 11, 2023 14:27

Firstyear approved these changes Oct 12, 2023

View reviewed changes

progier389 merged commit 415eead into 389ds:main Oct 13, 2023
190 of 195 checks passed

progier389 mentioned this pull request Oct 13, 2023

Issue 5843 - Fix dscreate create-template issue #5950

Merged

progier389 deleted the i5843 branch March 28, 2024 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 5843 - dsconf / dscreate should be able to handle lmdb parameters #5943

Issue 5843 - dsconf / dscreate should be able to handle lmdb parameters #5943

progier389 commented Oct 4, 2023 •

edited

Loading

Firstyear Oct 5, 2023

progier389 Oct 5, 2023

Firstyear Oct 5, 2023

progier389 Oct 5, 2023

progier389 Oct 10, 2023

Firstyear Oct 5, 2023

progier389 Oct 10, 2023

Firstyear Oct 5, 2023

progier389 Oct 5, 2023

droideck left a comment

progier389 commented Oct 10, 2023

droideck left a comment

Issue 5843 - dsconf / dscreate should be able to handle lmdb parameters #5943

Issue 5843 - dsconf / dscreate should be able to handle lmdb parameters #5943

Conversation

progier389 commented Oct 4, 2023 • edited Loading

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

droideck left a comment

Choose a reason for hiding this comment

progier389 commented Oct 10, 2023

droideck left a comment

Choose a reason for hiding this comment

progier389 commented Oct 4, 2023 •

edited

Loading