Skip to content
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

Retro Changelog is not trimming changes #4869

Closed
mreynolds389 opened this issue Aug 11, 2021 · 8 comments
Closed

Retro Changelog is not trimming changes #4869

mreynolds389 opened this issue Aug 11, 2021 · 8 comments
Assignees
Labels
Need BZ The ticket needs to be cloned to a BZ priority_high need urgent fix / highly valuable / easy to fix

Comments

@mreynolds389
Copy link
Contributor

Issue Description

Retro changelog is not trimming changes under some circumstances because we are mixing monotonic and utc times

[11/Aug/2021:11:01:49.792450548 +0200] - DEBUG - DSRetroclPlugin - cltrim: ldrc=0, first_time=1622032141, cur_time=3702600
[11/Aug/2021:11:01:49.842339467 +0200] - DEBUG - DSRetroclPlugin - retrocl_housekeeping - changelog does not need to be trimmed
[11/Aug/2021:11:06:49.953401407 +0200] - DEBUG - DSRetroclPlugin - cltrim: ldrc=0, first_time=1622032141, cur_time=3702900
[11/Aug/2021:11:06:50.012249703 +0200] - DEBUG - DSRetroclPlugin - retrocl_housekeeping - changelog does not need to be trimmed

Look at cur_time. That is the "uptime" of the system, not UTC.

[root@linge ~]# uptime
11:21:42 up 42 days, 20:49, 2 users, load average: 0.15, 0.14, 0.16

In the retro changelog code (trim_changelog) we are using the same monotonic time for the interval and checking for the age of an entry. We need to use the current time when checking the age and not the monotonic time.

@mreynolds389 mreynolds389 added the needs triage The issue will be triaged during scrum label Aug 11, 2021
@keestux
Copy link

keestux commented Aug 11, 2021

Maybe we need to look at the tests too. Is there a test that should have caught this? Whatever the answer (yes or no), we need to investigate.

Is it nsslapd-changelog-trim-interval or nsslapd-changelogtrim-interval?

$ git grep changelog-trim
dirsrvtests/tests/suites/password/regression_test.py:                                         (ldap.MOD_REPLACE, 'nsslapd-changelog-trim-interval', b"5s"),
dirsrvtests/tests/tickets/ticket47871_test.py:           (ldap.MOD_REPLACE, 'nsslapd-changelog-trim-interval', b"5s")]
ldap/servers/plugins/retrocl/retrocl.h:#define CONFIG_CHANGELOG_TRIM_INTERVAL "nsslapd-changelog-trim-interval"

@mreynolds389
Copy link
Contributor Author

@keestux - agreed our tests are not robust enough to pick up on this failure

@keestux
Copy link

keestux commented Aug 11, 2021

@mreynolds389 What's your comment on the typo? Should we make a separate issue for it?

@vashirov
Copy link
Member

Is it nsslapd-changelog-trim-interval or nsslapd-changelogtrim-interval?

nsslapd-changelog-trim-interval is for retro changelog and nsslapd-changelogtrim-interval is for changelog5:

$ grep -inr  "define CONFIG_CHANGELOG_TRIM"
ldap/servers/plugins/retrocl/retrocl.h:56:#define CONFIG_CHANGELOG_TRIM_INTERVAL "nsslapd-changelog-trim-interval"
ldap/servers/plugins/replication/repl_shared.h:34:#define CONFIG_CHANGELOG_TRIM_ATTRIBUTE "nsslapd-changelogtrim-interval"

@keestux
Copy link

keestux commented Aug 11, 2021

@vashirov What? Now that is a good way to confuse users.

@Firstyear
Copy link
Contributor

@vashirov What? Now that is a good way to confuse users.

Yes, we know. But sadly some of these decisions were made many years ago and it can be hard to change in existing releases. We do want to improve these.

IMO I'd love to rename these - are we still able to make breaking changes for 2.x @mreynolds389 ?

A part of the issue here is our tests can't easily manipulate "time" to check this, but I'd be open to some other ideas.

@mreynolds389 mreynolds389 added Need BZ The ticket needs to be cloned to a BZ priority_high need urgent fix / highly valuable / easy to fix and removed needs triage The issue will be triaged during scrum labels Aug 27, 2021
@mreynolds389
Copy link
Contributor Author

@vashirov What? Now that is a good way to confuse users.

Yes, we know. But sadly some of these decisions were made many years ago and it can be hard to change in existing releases. We do want to improve these.

IMO I'd love to rename these - are we still able to make breaking changes for 2.x @mreynolds389 ?

No can't make this change now, but it's not needed IMHO. People "should" be using the python CLI & UI to make these config changes - so the attribute names really shouldn't matter. It's only a issue for the rare cases where the dse.ldif needs to be manually edited.

Anyway time for me to start looking at the real bug here...

@mreynolds389 mreynolds389 self-assigned this Aug 27, 2021
mreynolds389 added a commit to mreynolds389/389-ds-base that referenced this issue Sep 3, 2021
Bug Description:  Monotonic clocks were used to check if an entry was old
                  enough to be trimmed, but the real system time should be
                  used.  So entries were never trimmed from the changelog.

Fix Description:  Make sure monotonic clocks are only used for the
                  trimming interval, and real time clocks are used
                  for entry age.

relates: 389ds#4869

Reviewed by: firstyear(Thanks!)
mreynolds389 added a commit that referenced this issue Sep 3, 2021
Bug Description:  Monotonic clocks were used to check if an entry was old
                  enough to be trimmed, but the real system time should be
                  used.  So entries were never trimmed from the changelog.

Fix Description:  Make sure monotonic clocks are only used for the
                  trimming interval, and real time clocks are used
                  for entry age.

relates: #4869

Reviewed by: firstyear(Thanks!)
mreynolds389 added a commit that referenced this issue Sep 3, 2021
Bug Description:  Monotonic clocks were used to check if an entry was old
                  enough to be trimmed, but the real system time should be
                  used.  So entries were never trimmed from the changelog.

Fix Description:  Make sure monotonic clocks are only used for the
                  trimming interval, and real time clocks are used
                  for entry age.

relates: #4869

Reviewed by: firstyear(Thanks!)
mreynolds389 added a commit that referenced this issue Sep 3, 2021
Bug Description:  Monotonic clocks were used to check if an entry was old
                  enough to be trimmed, but the real system time should be
                  used.  So entries were never trimmed from the changelog.

Fix Description:  Make sure monotonic clocks are only used for the
                  trimming interval, and real time clocks are used
                  for entry age.

relates: #4869

Reviewed by: firstyear(Thanks!)
mreynolds389 added a commit that referenced this issue Sep 3, 2021
Bug Description:  Monotonic clocks were used to check if an entry was old
                  enough to be trimmed, but the real system time should be
                  used.  So entries were never trimmed from the changelog.

Fix Description:  Make sure monotonic clocks are only used for the
                  trimming interval, and real time clocks are used
                  for entry age.

relates: #4869

Reviewed by: firstyear(Thanks!)
@mreynolds389
Copy link
Contributor Author

549d9c6..3f7a2fa 389-ds-base-2.0 -> 389-ds-base-2.0
0491c21..03a6752 389-ds-base-1.4.4 -> 389-ds-base-1.4.4
790096a..5321911 389-ds-base-1.4.3 -> 389-ds-base-1.4.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Need BZ The ticket needs to be cloned to a BZ priority_high need urgent fix / highly valuable / easy to fix
Projects
None yet
Development

No branches or pull requests

4 participants