[SSSD] [PATCH v3] CI: Allow disabling distro-(in)dependent tests

Lukas Slebodnik lslebodn at redhat.com
Thu Sep 18 18:44:01 UTC 2014


On (18/09/14 18:16), Jakub Hrozek wrote:
>On Thu, Sep 18, 2014 at 03:55:47PM +0200, Lukas Slebodnik wrote:
>> >From: Nikolai Kondrashov <Nikolai.Kondrashov at redhat.com>
>> >Date: Tue, 2 Sep 2014 17:45:22 +0300
>> >Subject: [PATCH 1/1] CI: Allow disabling distro-(in)dependent tests
>> >
>> >Add support for --no-common and --no-distro options to contrib/ci/run.
>> >
>> >These options disable running distribution-independent and
>> >distribution-dependent tests respectively, allowing running common
>> >distro-independent tests separately.
>> >
>> >The mock builds and Clang analyzer run are considered
>> >distro-independent. The test building, running them under Valgrind,
>> >distcheck target and collecting and verifying coverage data are
>> >considered distro-dependent.
>> >
>> >This allows CI service to run them on separate dedicated hosts, instead
>> >of running on all hosts, speeding up overall execution.
>> >---
>> I really tried to avoid review this patches. I was expecting Michal will come
>> to the meeting and explain his concerns, but someone had to ACK this patch.
>> 
>> This is the first time I read the patch. I'm so Sorry I have to reject it.
>> I know that there was an agreement on meeting that we will push the patch.
>> But on the meeting, there wasn't anyone who read/test the patch.
>> 
>> 1) I disagree that mock is distro independent patch.
>>    The purpose of mock is to build sssd in clean environment. I gave you two
>>    options: to use mock or to prepare clean Vm for each test.
>>    Currently we do not buid sssd in clean enviroment for rhel7,
>>    fedora-rawhdide and debian. The debian cannot be solved easily, but other
>>    two options can be solved with mock. I mentioned it few times that we have
>>    hardcoded mock builds for rhel6 and fedora-20. It means than on rhel7 we run
>>    mockbuild for rhel6 and fedora-20.
>>    Why? "The purpose of mock is to build sssd in clean environment", therefore
>>    on rhel7 we should build with mock just for rhel7.
>
>This sounds a fair point, but -- Nick mentioned inclusion of this patch is
>blocking his rollout. If that's the case, I don't see a reason to hold
>nack the patch over this issue. Let's file a ticket and move on to
>something more pressing for us.
>
>> 
>> 2) clang analysis is ignored anyway. It will be easier to remove it, if we
>>    want to speed up building.
>
>Sounds like another ticket, not something that needs to be done now.
>
>> 
>> 3) I don't like solution how you solved the problem.
>>    You added 6 if-else statements(some of them even nested) for 2 new command
>>    line options. Sorry, It does not scale and script is much more complicated.
>
>And? This is a test script. In the deamon we should totally build
>readable, debuggable code because chances are you will be reading the
>code 5 minutes before a deadline while an unhappy user is waiting for a
>fix. Not here.
>
>About scalability -- do we care too much? The way I see the future
>development of this script, we'll likely add a couple of features like
>the ones you listed here, but the tests themselves (like: Check if a
>user from AD can log in) will be developed in something else, maybe a
>variant of the framework IPA uses.
>
>> 
>> You wanted to speed up process.
>> So, here is my proposal:
>>     * run mock build just one. (the same as testing platform)
>
>After a quick thought I agree, let's file a ticket.
>
>>     * enable tmpfs mock plugin on all platforms.
>
>Sounds like a good idea, let's file a ticket.
>
>>     * remove clang analysis, because it is ignored anyway.
>
>I don't know the process into too much detail, but sounds like we should
>file a ticket.
>
>Overall, I don't see a reason to hold off this patch if it's blocking
>Nikolai. Again, this is a test. If the pach /improves/ the situation,
>let's push it and carry on. It doesn't have to be perfect.
>
Sorry, I disagree.
The most problematic part CI was very long time spent in mock builds.

Nikolai's patch is just workaround real problem. It complicates CI script
with if/else statements. It becames muchm more tangled.
Have you seen the script after change?

The first point from my proposal is very small change.
    * remove execution of 2nd mock test (-3 lines)
    * use default config instaed of hardcoded "epel-6-$ARCH"

The second point is server side configuration.

Result of this small change:
    * reduced time spent with mock build
    * script will not be complicated with additiona if/else statemens
    * very small change istead of Nikolai's version
      1 file changed, 107 insertions(+), 83 deletions(-)

If Nikolai does not have a time I can prepare patch myself.

LS



More information about the sssd-devel mailing list