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

Lukas Slebodnik lslebodn at redhat.com
Thu Dec 4 17:44:33 UTC 2014


On (04/12/14 17:57), Jakub Hrozek wrote:
>On Thu, Dec 04, 2014 at 12:04:24PM +0100, Jakub Hrozek wrote:
>> 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.
>
>I don't this there is as much of a technical issue with CI as process
>issue.
If there aren't technical issues with CI could you explain why nobody use it?

LS



More information about the sssd-devel mailing list