-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 6056 - WebUI supports only instances with BDB #6299
base: main
Are you sure you want to change the base?
Conversation
<div className="ds-left-indent-md"> | ||
<Grid | ||
title={_("Database maximum size in bytes. The practical maximum size of an LMDB database is limited by the system’s addressable memory (nsslapd-mdb-max-size).")} | ||
className="ds-margin-top" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed several fields are very close to bottom of the tabs, I think it would look a little nicer if you used this class instead: ds-margin-top-xlg
Do this for all the grids below so they are consistent, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, it looks way better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few small issues. For the rest I need to try the PR locally.
className="ds-margin-top" | ||
> | ||
<GridItem className="ds-label" span={4}> | ||
{_("Database Max DBS")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{_("Database Max DBS")} | |
{_("Database Max DBs")} |
<Tab eventKey={0} title={<TabTitleText>{_("Database Size")}</TabTitleText>}> | ||
<div className="ds-left-indent-md"> | ||
<Grid | ||
title={_("Database maximum size in bytes. The practical maximum size of an LMDB database is limited by the system’s addressable memory (nsslapd-mdb-max-size).")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maximum size should be a multiple of the page size, which can vary (4K, 16K, 64K on different architectures). And as a user, I'd prefer to enter the size in megabytes, than calculate the right size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that idea, let me look at how to determine the system page size and make the max size a multiple of it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1MB contains 256 4K pages or 64 16K pages or 16 64K pages. So if we change the maximum size to use megabytes, it'd cover all cases.
<Tab eventKey={5} title={<TabTitleText>{_("Advanced Settings")}</TabTitleText>}> | ||
<div className="ds-left-indent-md"> | ||
<Grid | ||
title={_("Location for database memory mapped files. You must specify a subdirectory of a tempfs type filesystem (nsslapd-db-home-directory).")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
title={_("Location for database memory mapped files. You must specify a subdirectory of a tempfs type filesystem (nsslapd-db-home-directory).")} | |
title={_("Location for database memory mapped files. You must specify a subdirectory of a tempfs type filesystem (nsslapd-db-home-directory).")} |
Not sure we want to configure nsslapd-import-cachesize in lmdb case (Although it is still used in some computation |
I think the import cache is used for other things that are not DB related (regardless of BDB/MDB) - so it still might be needed. Just proceed with caution :-) |
@@ -88,6 +90,7 @@ export class Database extends React.Component { | |||
chainingConfig: {}, | |||
chainingUpdated: 0, | |||
chainingActiveKey: 0, | |||
backendImplement: "mdb", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not to use BE_IMPL_MDB
const here?
value={this.state.dbhomedir} | ||
type="text" | ||
id="dbhomedir" | ||
aria-describedby="dbhomedir" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, when I change dbhome directory, the button still stays grey. And if I change something else and then save, dbhome dir still resets to the default - /var/lib/dirsrv/slapd-inst/db
And it happens only for LMDB. BDB is fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, the button change is a copy and paste error.
Regarding the dbhome setting, when I try changing the db home directory with bdb, an unwilling to perform message is returned, in the error logs it mentions this config cannot be changed when the server is running. IIUC then do we need a config option for changing the db home in the UI at all?
WDYT
handleTimeChange(value) { | ||
this.setState({ | ||
compacttime: value, | ||
}, () => { this.validateSaveBtn() }); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need this in LMDB part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct, removing this
Let me look into this, cautiously... |
Apparently no, according to 'grep -ri import.cache' it is only used by bdb: And I do not think that removing a now obscure parameter from cockpit is really dangerous (people can still view/change it through dsconf anyway), I am afraid that if we left it, some people may ask questions about it and we will be in trouble to answer ... |
Thanks for the clarification |
Right, the indexing buffer. I knew it wasn't an internal DB cache, but forgot exactly how it was used. Ok if it's not used by MBM then it's safe to drop. Thanks!! |
Description: BDB specific attributes are not supported by the web console, resulting in an "Ooops" when the Database tab is selected. Fix Description: Add a new global Database configuration element for MDB layout. Add support to detect what DB engine is configured, and display the correct layout on rendering. Relates: 389ds#6065 Reviewed by:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
I think, @vashirov's ack is also needed, though. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Database Maximum Size hint says the size is in bytes, but the form accepts megabytes now.
If Database Max Readers set to one of the numbers within 1-25 range, it gets set to 26. Not sure if it's possible to exclude this range from setting in the first place, @droideck do you know?
Database Max DBs has a minimum value of 36 that can be set, but if set it to anything within 36-68 range, it gets reset to 69. Should 69 be the minimum then? I don't know where it comes from though.
I will correct the first issue. I see the same behaviour when I run the associated commands at the CLI level:
And the same for max dbs:
I suspect the issue is not at the UI level as it gets/sets values via dsconf over ldapi. Let me have a look into it. |
Technically, yes, |
@droideck, does it allow to have a sparse range? I.e. allow to set 0 and then 26-512? If not, I'm fine with the current behaviour. |
For that, I see two options:
|
I didn't realise there was a sparse range of values allowed for these mdb configs, I guess I could have fuzzed the CLI instead of pointing the finger at the layer underneath the UI, sorry @progier389 I would prefer @droideck option 2, where we handle the logic in changeConfig() , onMinus() and onPlus(), it feels cleaner to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
Description:
BDB specific attributes are not supported by the web console, resulting in an "Ooops" when the Database tab is selected.
Fix Description:
Add a new global Database configuration element for MDB layout. Add support to detect what DB engine is configured, and display the correct layout on rendering.
Relates: #6065
Reviewed by: