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