Tagged with WIP since because this not a straightforward approach, so better be discussed first.

The lazy-object-proxy happens to be packaged in Fedora but not in CentOS unfortunately. However, the library itself is tiny, hence could be bundled in the lib389 and be benefitial elsewhere, too.

Finally, a note to self: the rules in lint.py lack actual descriptions (only error message and remedy are included), thus add those.

Why are you removing logging here? I think it needs to be here so that the verbose sets up prpoerly for some reason?

Aren't some of the healthchecks determined by your server configuration too? Or am I imagining this ...

Why are you removing logging here? I think it needs to be here so that the verbose sets up prpoerly for some reason?

Beacuse it is an unused import. Please, don't tell me there is some obscure magic that would need this unused import. :)

Aren't some of the healthchecks determined by your server configuration too? Or am I imagining this ...

I don't think it is selective based on server configuration (not accounting e.g. Backends where it is done dynamically). But you made me recheck the approach, which turns out to be quite good I guess. So...

I realized that using the error messages from the lint.py is rather very poor solution. I've got a better idea (at least I hope so) - list checks per actual _lint_* functions by sanely taking e.g. their __doc__ methods. @mreynolds Mark, you wrote the healthcheck, thoughts? :)

Thanks

Beacuse it is an unused import. Please, don't tell me there is some obscure magic that would need this unused import. :)

It's python, of course there is magic. Maybe I'm confusing this with something else though, so for now lets remove it.

I realized that using the error messages from the lint.py is rather very poor solution. I've got a better idea (at least I hope so) - list checks per actual lint* functions by sanely taking e.g. their doc methods. @mreynolds Mark, you wrote the healthcheck, thoughts? :)

Okay so a question is, with listing the possible checks, can we actually use that output? IE if I list all the healtchecks, can I run a single check specifically? What's the end goal of being able to list the checks?

Reading the code in lib389/cli_ctl/health.py, it looks like when we do a healthcheck, we check everything anyway, so really, a better approach could actually be rather than most of the lazy proxy and listing and such, why not add a "description" to the dicts in lib389/lint.py, then move "CHECK_OBJECTS" from lib389/cli_ctl/health.py to lib389/lint.py, have a top level lint executor in DirSrv, and perhaps even an "get_healthchecks" or something?

That way the cli tool becomes something like:

for item in inst.get_healthchecks(): 
    item.lint()

And we can have the list as:

for (code, desc) in inst.get_healthcheck_meta:
    desc ...

Where the get_healthcheck_meta, does a list comp on get_healthchecks, and get's all the needed metadata from lint.py?

That way also we have the capability that get_healthchecks can actually make decisiuons based on your server config, IE you don't check TLS on an instance where nsslapd-security: off etc.

Aren't some of the healthchecks determined by your server configuration too? Or am I imagining this ...

I don't think it is selective based on server configuration (not accounting e.g. Backends where it is done dynamically). But you made me recheck the approach, which turns out to be quite good I guess. So...
I realized that using the error messages from the lint.py is rather very poor solution. I've got a better idea (at least I hope so) - list checks per actual lint* functions by sanely taking e.g. their doc methods. @mreynolds Mark, you wrote the healthcheck, thoughts? :)

Actually @firstyear wrote healthcheck in lib389 (I think, it wasn't me, maybe if not firstyear then @tbordaz?), I just added a lot more checks and made some minor fixes. :-)

@firstyear, yes, the ultimate goal is to be able to run specific checks. So if one is failing, you can just retest that single check without having to run through ALL the checks over and over.

In freeipa's healthcheck this is how I implemented the DS checks. I just imported lib389 and called each class's lint functions. So each class from lib389 (Backends, Config, replication, etc) are the available "checks" in IDM's healthcheck tool. For example, you could rerun all the Backend checks without calling all the other classes/checks/lints. So in "dsctl healthcheck" we should really do the same thing, only makes sense.

As for where the lint data is, I could care less where it goes, but keeping them in one file enforces the proper format of the objects. What we can not do is change how classes' lint functions report their results. I need those JSON reports to remain in the same format since this tool is now being consumed by other products.

Aren't some of the healthchecks determined by your server configuration too? Or am I imagining this ...
I don't think it is selective based on server configuration (not accounting e.g. Backends where it is done dynamically). But you made me recheck the approach, which turns out to be quite good I guess. So...
I realized that using the error messages from the lint.py is rather very poor solution. I've got a better idea (at least I hope so) - list checks per actual lint* functions by sanely taking e.g. their doc methods. @mreynolds Mark, you wrote the healthcheck, thoughts? :)

Actually @firstyear wrote healthcheck in lib389 (I think, it wasn't me, maybe if not firstyear then @tbordaz?), I just added a lot more checks and made some minor fixes. :-)

It was me :) I noticed this in @mhonek's comment, but wasn't going to point it out since you've done so much work to get it over the line in the last few years.

Saying this you could probably point to anything in lib389 and blame me for it at this point >.<

@firstyear, yes, the ultimate goal is to be able to run specific checks. So if one is failing, you can just retest that single check without having to run through ALL the checks over and over.
In freeipa's healthcheck this is how I implemented the DS checks. I just imported lib389 and called each class's lint functions. So each class from lib389 (Backends, Config, replication, etc) are the available "checks" in IDM's healthcheck tool. For example, you could rerun all the Backend checks without calling all the other classes/checks/lints. So in "dsctl healthcheck" we should really do the same thing, only makes sense.
As for where the lint data is, I could care less where it goes, but keeping them in one file enforces the proper format of the objects. What we can not do is change how classes' lint functions report their results. I need those JSON reports to remain in the same format since this tool is now being consumed by other products.

Right, if the goal is to run specific checks, then listing them is not the best place to start. We need to look at how we plan to request and execute those checks, and once we understand that, then we can think about how to list them in a manner that works. So I'd actually say we need to do something like updating def lint(): to take a selector of some kind, refactoring the checks to probably be whole classes rather than dictionaries in lint.py so that they have the associated execution functions and logic, and then writing some py.tests for just running individual checks.

After that, then enough would be in place to glue on a healthcheck "select" layer into the cli, and a way to list the checks from the instance.

some key things to consider is a healtcheck could be instance specific, and the list should reflect that it's not relevant to this instance, and that some checks can be run multiple times, IE backend checks. So "perfect world" the check list would actually look something like:

healthcheck list
tls_version
backend_indexed:userRoot
backend_mappingtree:userRoot
backend_indexed:otherRoot
backend_mappingtree:otherRoot

So a user could do:

healthcheck tls_version backend_indexed
OR
healthcheck backend_mappingtree:otherRoot

The first should check the tls_version AND that all backends are indexed, the second should only check that otherRoot has a mapping tree.

But to do this, you need a proper selection framework in place, and probably more extensibility to the checks, so I think healthcheck "under the hood" may need a bit of re-architecting. :) Honestly when I wrote it I always imagined it as "just run them all yolo", but in hindsight that was not a good decision. (lib389 is basically a series of william's unfortunate mistakes that happens to know how to test and setup an ldap server and everyone else on the team tolerates ;) )

Does that help?

Seems my memory fails me repeatedly. Sorry for the false attribution, and sorry for not following the actual ticket description.

I've now rebased onto latest master, and add a commit on top that implements feature as per original description of the ticket.

Ad the lazy-object-proxy certainly may be made gone. The major advantage I've seen in it was the ability to explore the utility without providing an existing instance wherever possible. But I can remove it.

Ad William's suggestions: looks good. I can go and implement this (I think I can do it without actually changing the current behaviour of the lint functions). But I'd like to have @mreynolds thoughts on this approach, too.

BTW, @mreynolds where does the IPA's usage of the healthcheck live, so that I can check what not to break? Thanks :)

Seems my memory fails me repeatedly. Sorry for the false attribution, and sorry for not following the actual ticket description.

I think it's not your memory as much as the fact I think I wrote the healthcheck about ... I dunno 2016 or 2017 I think? And then it got left for a while, and Mark took over when IPA wanted to use it .... Still a few people have had a hand in it :)

I've now rebased onto latest master, and add a commit on top that implements feature as per original description of the ticket.
Ad the lazy-object-proxy certainly may be made gone. The major advantage I've seen in it was the ability to explore the utility without providing an existing instance wherever possible. But I can remove it.

Well, I think we'll need the dnstance to exist based on my suggestion .... and the healthcheck doesn't make sense without an instance?

Ad William's suggestions: looks good. I can go and implement this (I think I can do it without actually changing the current behaviour of the lint functions). But I'd like to have @mreynolds thoughts on this approach, too.
BTW, @mreynolds where does the IPA's usage of the healthcheck live, so that I can check what not to break? Thanks :)

IIRC they use the cli/json format, so I think we can change our internals as we like here.

Okay, so I'm a bit confused on how to use the new options. First there is no "list checks", but there is a "list errors". Then we can run individual CHECKS, but what are the checks? There is no way to list the available checks. I think we should have a "list checks" and in that output we describe/list the errors that can occur with that CHECK.

Okay, so I'm a bit confused on how to use the new options. First there is no "list checks", but there is a "list errors". Then we can run individual CHECKS, but what are the checks? There is no way to list the available checks.

Sorry for the confusion. The --list-errors refers to the actual error codes/message as in lint.py. And there is --check, and if you do --help you can see the checks you can use (this is generated from the CHECK_OBJECTS). I surely can do --list-checks and point the --help for --check to it.

I think we should have a "list checks" and in that output we describe/list the errors that can occur with that CHECK.

IIUC, that would mean that for each check we should keep a metadata of what errors a check may throw which is redundant to the actual usage in the code (thus error-prone); or parse the Python's AST on the fly (probably not).

Anyway, please, what do you think of William's proposal? FWIW, that would definitely require --list-checks anyway.

Just to be clear, my current nomenclature understanding is this: check == class; error == <object from the lint.py>. With William's proposal: check == (<class._lint_* method>, <some parameter>).

Yes I like William's proposal, but it will require a lot more work. I feel it is worth the effort and it will make the healthcheck tool very flexible. As long as we don't change how the "lint" functions work then I'm OK with it - meaning as long as it doesn't break IDM healthcheck tool I'm OK with it :-)

hey there,

I feel like the worst person having to say this because I can see you put in a lot of effort to this, but I think it's not quite the right direction.

Perhaps it was a communication thing, but I think you have selected a level of granularity that is too coarse. You have DSLint at the level of the DSLdapObject, when its not quite that. DSLdapObject has many DSLints, for example, in really terrible pseudo code:

Backend:
    lints = [
        BackendFreeSpace
        BackendIndexed
        BackendMappingTree
    ]
BackendFreeSpace(DSLint): 
     ...
BackendIndexed(DSLint): 
     ....
BackendMappingTree(DSLint): 
    ....

This is the level of granularty we want. That way, you can actually then do something like:

BackendIndexed(DSLint): 
     def __init__(self, be: Backend)
         self.be = ...
    def selector(self)
        f(backend_indexed:%{self.be.name})

This way you could have each item yield a unique selector id, in this case say backend_indexed:userRoot etc.

So to do a list you would have:

DSLints(object)
     [
            // list of object type that can provide a lint.
     ]
    def __init__(self, instance): 
          self.lints = [for lint in types, lint(instance).lints]
    def selectors(self): 
         [for lint in self.lints, lint.selector()]
    def get_by_selector(self, selector): 
        [ for lint in self.lists, lint.selector() if selector startswith ....]
lints = DSLints(instance)
lists.list_selectors()

Now, this lets you trivially wire in a cli that can:

• List every lint
• List every lint and what backend or associated instance data it will act upon
• filter and reduce the lints based on an admin wanting to only execute individual lints

So sorry, but I think I need to ask you to rework this :( :(

@firstyear Maybe I just misunderstood your latest comment, however I think the granularity is essentially the same.

In your variant, the product type of possibilities is (<lint class>, <selector>). It is lint-centric, more linear.

In my implementation it is (<any class deriving from DSLint>, <_lint_ function name>) or (<any class deriving from DSLints>, <selector1>, <_lint_ function name>, <selector2>). It is any-class-centric, more hierarchical. In both cases, you can parse the selector however you want, hence e.g. implement sub-selectors. So, with the current implementation the --list-checks may look like (the red icons are colon, character, colon; pardon the stupid pagure):

config:hr_timestamp
config:passwordscheme
backends:a:mappingtree
backends:a:search
backends:a:virt_attrs
backends:b:mappingtree
backends:b:search
backends:b:virt_attrs
encryption:check_tls_version
fschecks:file_perms
refint:attr_indexes
refint:update_delay
monitor-disk-space:disk_space
replication:agmts_status
replication:conflicts
changelog:cl_trimming
dseldif:nsstate
ssl:certificate_expiration

And you can do e.g.: --check replication dseldif:nsstate, or --check backends:b, or --check backends:b:search, or --check backends:*:search fschecks:file_perms, etc. Sure, it's not crazy clever; if you specify something twice it will get executed twice but fixing that is probably non-trivial and unworthy.

One premise I followed was not to break the behaviour of the <class>.lint() method of the existing functions since IPA's healthcheck tool uses them directly.

My perception is that there is a difference in design in our approaches, however granularity and capabilities are essentially the same.

Anyhow, I hope I cleared the thing a bit for you (and probably others as well) and didn't misunderstand your point either. Please, let me know what you think.

@firstyear Maybe I just misunderstood your latest comment, however I think the granularity is essentially the same.
In your variant, the product type of possibilities is (, ). It is lint-centric, more linear.
In my implementation it is (, <lint function name>) or (, , <lint function name>, ). It is any-class-centric, more hierarchical. In both cases, you can parse the selector however you want, hence e.g. implement sub-selectors. So, with the current implementation the --list-checks may look like (the red icons are colon, character, colon; pardon the stupid pagure):

Honestly, reading the code I didn't connect that. That's probably why I didn't understand it.

config:hr_timestamp
config:passwordscheme
backends🅰mappingtree
backends🅰search
backends🅰virt_attrs
backends🅱mappingtree
backends🅱search
backends🅱virt_attrs
encryption:check_tls_version
fschecks:file_perms
refint:attr_indexes
refint:update_delay
monitor-disk-space:disk_space
replication:agmts_status
replication:conflicts
changelog:cl_trimming
dseldif:nsstate
ssl:certificate_expiration

And you can do e.g.: --check replication dseldif:nsstate, or --check backends:b, or --check backends🅱search, or --check backends:*:search fschecks:file_perms, etc. Sure, it's not crazy clever; if you specify something twice it will get executed twice but fixing that is probably non-trivial and unworthy.
One premise I followed was not to break the behaviour of the .lint() method of the existing functions since IPA's healthcheck tool uses them directly.
My perception is that there is a difference in design in our approaches, however granularity and capabilities are essentially the same.
Anyhow, I hope I cleared the thing a bit for you (and probably others as well) and didn't misunderstand your point either. Please, let me know what you think.

Yeah, I think you didn't understand. Perhaps what I'm worried about is that your approach is "too clever", I didn't even spot the connection between the callable _lint filter for discovery of the types.

How does this handle attempting to lint on a single backend instance? IE the backend_index:userRoot I mentioned as an example?

I'm going to read the code again to be sure of what I'm thinking though. Thanks for explaining :)

These should still be calls to super, what's the reason to change it?

Same here

Couldn't this be a list comprehension?

Okay, so rereading it a few more times, I think my concern is this is toooooo much magic. I wonder if there is a way to make dslint simpler or communicate better what it's doing?

How does this handle attempting to lint on a single backend instance? IE the backend_index:userRoot I mentioned as an example?

So, the DSLints is basically just a flat-map (map, as in that it passes rest of the spec into the _lint_* methods, if necessary). And DSLint just yields from whatever _lint_* methods get matched by this rest of the spec. In the exact backend matching case the DSLints will hit/iterate exactly once. The backend_index:userRoot in your model is the same as backends:userRoot:index in mine. If that is what was asked.

These should still be calls to super, what's the reason to change it?

Since now there is multiple inheritance in place... super only gives you a "proxy" where there is only a single __init__ off the parent classes (in this case it would be the one of the DSLogging, because __mro__...). Now, it seems I don't need to call __init__ of DSLint(s) (I'll probably drop it in the next rebase, need to test properly first) but for making things obvious I would rather keep the explicit naming in the call as is since mere reordering in the class' header would change behaviour which is unwanted). Either that, or I miss some functionality of super I could leverage instead.

Couldn't this be a list comprehension?

It sure could but what would be the reason? The current way gives you the same results except it may need fewer iterations in some runtime cases.

... magic ...

The only real magic there is the way of getting the _lint_* methods of the respective classes, and maybe the unconventional special value spec=List for them (which I found to be the only sane way to get the methods be able to have multiple targets without a need for a mechanism external to the method itself which would lead to separation of functionality and duplication; additionally, this keeps the original behaviour being transparently a subset of the new behaviour). The rest is really just a bunch of yield from with complexity added only due to the need of handling the special cases of optional values and any-matchers (asterisks).

However, I agree the clarity of the axpected usage should be improved and I'll give it a thought.

How does this handle attempting to lint on a single backend instance? IE the backend_index:userRoot I mentioned as an example?

So, the DSLints is basically just a flat-map (map, as in that it passes rest of the spec into the lint methods, if necessary). And DSLint just yields from whatever lint methods get matched by this rest of the spec. In the exact backend matching case the DSLints will hit/iterate exactly once. The backend_index:userRoot in your model is the same as backends:userRoot:index in mine. If that is what was asked.

These should still be calls to super, what's the reason to change it?

Since now there is multiple inheritance in place... super only gives you a "proxy" where there is only a single init off the parent classes (in this case it would be the one of the DSLogging, because mro...). Now, it seems I don't need to call init of DSLint(s) (I'll probably drop it in the next rebase, need to test properly first) but for making things obvious I would rather keep the explicit naming in the call as is since mere reordering in the class' header would change behaviour which is unwanted). Either that, or I miss some functionality of super I could leverage instead.

If you don't need the init on dslint, then just remove it and use super on dslogging I think.

Couldn't this be a list comprehension?

It sure could but what would be the reason? The current way gives you the same results except it may need fewer iterations in some runtime cases.

How does it give fewer iterationsL

... magic ...

The only real magic there is the way of getting the lint* methods of the respective classes, and maybe the unconventional special value spec=List for them (which I found to be the only sane way to get the methods be able to have multiple targets without a need for a mechanism external to the method itself which would lead to separation of functionality and duplication; additionally, this keeps the original behaviour being transparently a subset of the new behaviour). The rest is really just a bunch of yield from with complexity added only due to the need of handling the special cases of optional values and any-matchers (asterisks).
However, I agree the clarity of the axpected usage should be improved and I'll give it a thought.

It took me 4 goes to read this, and I still couldn't work it out. But I guess that's what people feel about my python too, I think python as a language is an unreadable nightmare.

So yeah, I'm happier with the design as you have explained it, but perhaps some extra comments in your code are required to aid readability for the future, and some thought to simplication would be good too. But otherwise, it's getting closer to "accept" I think :)

If you don't need the init on dslint, then just remove it and use super on dslogging I think.

Some extensive googling and Simon helped with understanding why. So it is back. The original idea of why not to use super() appeared probably when I was trying abstract classes approach. Lovely Python...

How does it give fewer iterationsL

Looking at the code again it does not. But theoretically it does it with smaller memory footprint. :) Anyway, I still don't see what benefit would it bring changing from generator to list comprehension.

It took me 4 goes to read this, and I still couldn't work it out. But I guess that's what people feel about my python too, I think python as a language is an unreadable nightmare.
So yeah, I'm happier with the design as you have explained it, but perhaps some extra comments in your code are required to aid readability for the future, and some thought to simplication would be good too. But otherwise, it's getting closer to "accept" I think :)

I've added more commentary in the code (the latest commit). It should improve understanding of the code itself and also how to use the functionality. As for simplification, I am not sure where with the current design. The couple of IFs is necessary for the special cases' handling, and the rest is just yielding from something built up; but suggestions are welcome. Also, there is a test case included that might help with understanding how this works.

If you don't need the init on dslint, then just remove it and use super on dslogging I think.

Some extensive googling and Simon helped with understanding why. So it is back. The original idea of why not to use super() appeared probably when I was trying abstract classes approach. Lovely Python...

Sometimes python's magic is "too much" magic :)

How does it give fewer iterationsL

Looking at the code again it does not. But theoretically it does it with smaller memory footprint. :) Anyway, I still don't see what benefit would it bring changing from generator to list comprehension.

My argument to it, would be that it's a bit easier for a reader to understand. Generators are confusing as heck, and really I would only reach for them if there was no other option (IE the filter gen/map code). It's more than just about performance, but about our ability to maintain this as a team (which you know, I have a history of being bad at, writing code that only I understand, so I'm hoping I have learnt from my mistakes ....).

It took me 4 goes to read this, and I still couldn't work it out. But I guess that's what people feel about my python too, I think python as a language is an unreadable nightmare.
So yeah, I'm happier with the design as you have explained it, but perhaps some extra comments in your code are required to aid readability for the future, and some thought to simplication would be good too. But otherwise, it's getting closer to "accept" I think :)

I've added more commentary in the code (the latest commit). It should improve understanding of the code itself and also how to use the functionality. As for simplification, I am not sure where with the current design. The couple of IFs is necessary for the special cases' handling, and the rest is just yielding from something built up; but suggestions are welcome. Also, there is a test case included that might help with understanding how this works.

Great, yes, I'm much much happier with this now. Sorry to have been so difficult in the end :)

I'd say ping @spichugi or @mreynolds to have a look at it as well, but otherwise if they don't object than you can consider than a "eventually consistent Ack" for merging this at the end of the week then :)

Looks good to me!

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4040

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.