[SSSD] [PATCH] CI: Remove Valgrind leak checking

Jakub Hrozek jhrozek at redhat.com
Thu Dec 4 11:04:24 UTC 2014


On Thu, Dec 04, 2014 at 11:00:05AM +0100, Lukas Slebodnik wrote:
> On (04/12/14 10:53), Nikolai Kondrashov wrote:
> >On 12/04/2014 10:20 AM, Nikolai Kondrashov wrote:
> >>On 12/03/2014 11:18 PM, Lukas Slebodnik wrote:
> >>>On (03/12/14 21:58), Jakub Hrozek wrote:
> >>>>On Wed, Dec 03, 2014 at 09:47:08PM +0100, Lukas Slebodnik wrote:
> >>>>>On (03/12/14 18:27), Nikolai Kondrashov wrote:
> >>>>>>On 12/03/2014 06:19 PM, Nikolai Kondrashov wrote:
> >>>>>>>Hi everyone,
> >>>>>>>
> >>>>>>>The attached patch removes Valgrind leak checking as suggested by Lukas on
> >>>>>>>private sssd-ci list:
> >>>>>>>
> >>>>>>>     Valgrind will ***NEVER*** find memory leaks in sssd. We use talloc for memory
> >>>>>>>     allocations and maintaining suppresion database is nightmare from my POV.
> >>>>>>>
> >>>>>>>     I vote for removing testing memory leaks with valgrind
> >>>>>>>
> >>>>>>>Personally, I'm not sure if leak checking is completely useless, but perhaps
> >>>>>>>it is. Has anyone caught any leaks with this so far?
> >>>>>>>
> >>>>>>>Regarding maintenance, we haven't had to add any new leak suppressions in more
> >>>>>>>than two months, so it doesn't look so bad so far.
> >>>>>>>
> >>>>>>>However, if we feel this is irritating and/or useless, we should remove it.
> >>>>>>>
> >>>>>>>Does anyone have an opinion on this?
> >>>>>>
> >>>>>>Oh, and Jakub has said this in response to Lukas:
> >>>>>>
> >>>>>>    We should keep the valgrind tests for checking for unallocated access or
> >>>>>>    use-after free.
> >>>>>>
> >>>>>>    Checking for leaks would only make sense if we compiled our external
> >>>>>>    libraries (like the idmap library) with malloc instead of talloc.
> >>>>>>
> >>>>>My opinion is clean. Jakub would like to have checking memory leaks
> >>>>>with valgrind.
> >>>>
> >>>>I thought about the leaks some more. There /are/ cases where leak checks
> >>>>are valuable even though we use talloc -- one example is the tests Sumit
> >>>>pushed the other day. Some of the Kerberos functions internally allocate
> >>>>data and must be freed with other corresponding libkrb5 functions.
> >>>>
> >>>>Given that Sumit also sent a patch that fixed a leak in leak checking
> >>>>code (sic!) I do think developers care about valgrind
> >>>>
> >>>>Therefore, I think it still makes sense to keep the leak checks around.
> >>>>
> >>>Who will be responsible for maintaining suppression database?
> >>>It's easy to add a suppresion and ignore real memory leaks.
> >>>Currently, neither author nor reviewer found valgrind problems.
> >>>
> >>>When should be the database updated? With the problematic patch (after?)
> >>>
> >>>>>We need to find a compromise.
> >>>>>Therefore NACK.
> >>>>>
> >>>>>BTW.
> >>>>>a) This is a reason why maintaining suppression database in git tree is the
> >>>>>worst solution. (it would be even worse with clang static analyser)
> >>>>>b) Our CI should be more flexible. Currently we need to wait until solution
> >>>>>will be pushed to the upstream. Meanwhile, CI is unusable.
> >>>>
> >>>>I think this is a valid point.
> >>>>
> >>>>Well, could we keep the suppression databases on the CI server itself? Maybe
> >>>>in some 'staging' branch that would eventually be merged upstream if we
> >>>>absolutely need to keep the suppression upstream?
> >>>>
> >>>>Or could we have some directory on the CI where we would just drop a
> >>>>file while a patch waits for a review?
> >>>>
> >>>I had proposal for more declarative CI [1]. (It was different use case)
> >>>The result of discussion was that we can implement in future if there is demand
> >>>
> >>>It would be easy to use (temporary) different YAML configuration on problematic
> >>>machine(platform) and the referential configuration in git tree would be
> >>>unchanged.
> >>
> >>Guys, I don't want to start arguing on all of these points. I think it's not
> >>worth it.
> There's no reason for arguing but there's time for discussion.
> It's fact that current version of CI is very inflexible and current situation
> is yet another proof of it.
> 
> >>I'd rather just remove Valgrind checks altogether.
> Jakub is against removing valgrind check.
> BTW it is the same sitauation as with clang.

except valgrind is much more useful than clang.

> Your solution is to completly remove test from upstream gt repo.
> So others cannot use it (even with some limitation)

Removing the valgrind checks completely is the worst of all solutions. A
leak in the back end code is a bug, period. Even though talloc helps a
lot, we still have leaks from time to time.

> 
> >>I don't want to implement a complicated solution for this,
> >>where a simple one is good enough.
> >>I'll be fine with someone else doing it.
> >>
> There's big difference between complicated solution and good enough solution.
> The current version somehow works but it is far far away from good solution.
> The primary idea was to use the same script on developer machine and jenkins
> instance. It seems that this assumption wasn't right and is root of all
> problems
> 
> >>Just to clarify my position, this is how I think my solution is supposed to be
> >>used:
> >>
> >>1. Run CI on the server *before* pushing the changes to the public repo. E.g.
> >>    prepare a branch to be pushed then push it to CI with "each" tag to have
> >>    all the commits tested. If Valgrind check fails, ask patch submitter to
> >>    either fix the issue or add suppression in the same commit. Disable
> >>    Valgrind check in the same commit and submit a CI bug, if that is not
> >>    possible.
> If there isn't problem in sssd and only on old platforms there isn't reason
> to block the patches. Tests for old platforms are just to check whether sssd
> can be compiled (there isn't problem with packages(rpms) and there aren't
> missing dependencies). Therefore we should have flexible way how to disable
> some kind of test just per specific(old) plaform. (Adding if conditions to CI
> script isn't flexible way)
> 
> >>2. If Valgrind check starts failing *after* changes are pushed (e.g. an
> >>    updated dependency breaks something), stop everything and push a commit either
> >>    fixing it or adding a suppression. Disable Valgrind check and submit a CI
> >>    bug, if that is not possible.
> >>
> Agree, it is ideal situation. As you can see we are not in this situation.
> Neither author nor reviewer tested patches.

I could swear I did, but apparently not..I know I did run the patches
through Coverity as it uncovered an error which was fixed during review.

> 
> >I would like to add that these are basic rules of running CI (when you drop
> >"Valgrind" from them). If we're to use CI, we should follow them. And Valgrind
> >is just perhaps a bit more finicky that the regular build system and its
> >dependencies.
> >
> My point is different.
> If tests passed on latest platform (including valgrind) they can be pushed
> and problematic parts on older platforms should be fixed ASAP
>     (suppress valgrind warnings, add backward compatible definition of
>      functions, fix ...)
> CI should simplify developers work not to complicate it.

...or, could we run valgrind on more stable platforms only? The most
false positives (leaks from other libraries) would be caused by rebases,
and since RHEL doesn't rebase so often, would it make sense to only
enable the leak checks there?

That way, we would see leaks introduced to the sssd master but would
only be forced to check the suppressions if either a new dependency is
added or a dependency is rebased.



More information about the sssd-devel mailing list