Implement context cancellation for command based gatherers

[balanza]

Description

Implement context cancellation for all gatherers that execute commands on the host machine:

• ascsers
• cibadmin
• corosync
• dispwork
• mountinfo
• packageversion
• saphostctrl
• sbddump
• sysctl
• verifypassword

[arbulu89]

Hey @balanza
Good job. Just some minor changes to do.

Besides that:
A question about the new test cases.
Why do you mock the execution here? you could test a real cancellation as well, not using a mock. At the end, it is cancelled and it should return the same error.
(for example, this has hidden the fact that you didn't use the ctx coming from Gather in the ascsers_cluster.go

If you think that is already well tested and you prefer to mock is OK.

nitpick:I think we could improve a bit them changing suite.Error(err) to see that this actually has a cancellation error rather than any possible error.

[arbulu89]

Strange that we still need to import os/exec here

[arbulu89]

Did you try putting ctx instead of mock.Anything?
Maybe it validates that the function is called with the expected argument.
I don't know if it works though.

[balanza]

It works and it's actually brilliant! I refactored all tests like that in 5961524

It would have spotted the bug in #379 (comment)
You made my day

[arbulu89]

Great! 💚
You can merge once the Ersk typo is fixed!

[balanza]

A question about the new test cases.
Why do you mock the execution here? you could test a real cancellation as well, not using a mock. At the end, it is cancelled and it should return the same error.

You're right. In the end, command cancellation (and cleanup) is covered by tests on the command executor itself.

I refactor all tests in b064d32 and they work smoothly using the real executor. The less we mock the better, here it comes with a big BUT:

tests will succeed even if ExecCommand isn't hit. For example, imagine the case in which the Gather implementation has an early return on ctx.Err().
By doing this, we are theoretically missing the scenario in which the command has been started and then cancelled.

Anyway, it's nitpick and it's not our case so far. I like what we have now, I just wanted to highlight a possible drawback.

If you are fine with that, we can merge.

looking for further review

[arbulu89]

@balanza That's ok.
I don't see why we should be checking ctx.Err before doing anything. It looks like something we shouldn't do.
At the end, the context we pass is to be used by system calls (commands, api calls, etc), so we can cancel them (even what we return is not that critical).

I think we cover all of our real cases

PD: if we have some early ctx.Err usage in the code, we should aim to a different test format as well