Abstract exec environment

[pablochacin]

Description

Introduce an abstraction for the runtime environment exposed by golang's os package, to facilitate testing. It is inspired by the GlobalState used in k6, but abstracts it into multiple interfaces.

The focus is on the agent's CLI command to facilitate testing changes such as signal handling and the cancellation of execution contexts.

Therefore it is out of scope to change the access to the os package in all packages. Follow-up PRs will address those.

The current scope is to abstract:

• The execution of processes
• Control the current process (e.g. create execution lock, start/stop profiling)
• Access to environment variables

Todo in follow-up PRs:

• Abstract signal handling
• Provider logging interface
• Abstract the access to file system

Out of scope:

• The kubernetes package still access the OS environment variables directly.

Fixes #118

Checklist:

• My code follows the coding style of this project
• I have performed a self-review of my code
• I have commented on my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works.
• Any dependent changes have been merged and published in downstream modules
  

[roobre]

Left some minor comments regarding implementation.

As for the approach, I like it so far. Perhaps the process Lock would be my only caveat: I think making the directory configurable allows us to write integration tests for it without the need for a mock, which may increase our confidence not only in the fact we are calling it correctly, but in that it works for locking the process.

[roobre]

I'm not 100% sure on the interface here. I get Lock and Unlock, but the Profiler method seems to be just a proxy for the profiler constructor. Also not sure about what Name is used for.

If we make another implementation of this, which for the sake of the example could be a kubernetes-driven lock, why would that implementation need to also proxy the profiler constructor and return a name?

[pablochacin]

the Profiler method seems to be just a proxy for the profiler constructor.

The main reason the profiler is a constructor method is that the configuration would come from CLI arguments passed to the command, we cannot just start/stop it.

If we make another implementation of this ... why would that implementation need to also proxy the profiler constructor and return a name?

This is a good point. Another option would be to move it to the Runtime interface but I'm not sure that solves the issue. Because at some point we need some "anchor" to get all the interfaces for the Runtime. I think as the profiler profiles the process, this was a natural place. In any case, if not relevant, it is relatively easy to mock a dummy profiler.

Also not sure about what Name is used for.

As for returning a Name, it is the process name, it prevents having to have access to os.Args[0] to get the name. It doesn't seem to be something complex to mock.

[roobre]

My main point was not "these things should not exist", but rather "do these things actually belong to the same interface"? My point is that I don't see how the process name and the profiler are related to each other, so my first thought is whether we should split this.

[pablochacin]

As mentioned before, at the end we need to have a single root for all interfaces, the runtime environment, which gives access to all other interfaces. So if we split it, we need to put it in another place. The question remains, where?

As for the actual place, these methods (lock, profile) relates to controlling the process execution. As I expect it to have more functions in the future. For example, for implementing #117 I was expecting an interface or method for handling signals.

I agree that it may be debatable if the name belongs here, but again if not here, where?

[roobre]

The comment was motivated by the principle of keeping interfaces small (I'm refraining from quoting Rob Pike here 😈).

Here we are not using the interface to encapsulate behavior, we're doing so because the Go language enforces us to use interfaces if we want to mock things for testing.

I agree that it may be debatable if the name belongs here, but again if not here, where?

I think whenever we are injecting this interface, we can inject others. Something along the lines of:

rootCmd := commands.BuildRootCmd(Context{
    FSLock: locker.New(), // With default path
    ProcessInfo: processinfo.New(),
    Profiler: profiler.New(),
    Environment: env.New(),
})

That being said, that's my style of doing things, which it's not easy (or perhaps possible) to prove to be better than the current approach. We can keep this one and refactor it if it becomes a problem, or we reach an agreement on a new style.

[pablochacin]

I Don't disagree with any of your points. Keeping interfaces small is a well-known and uncontroversial principle. By the way, predates go for at least one decade. In any case, I have always considered it a guideline and not a mandate.

If I understand correctly, you are suggesting in the code above, FSLock, ProcessInfo, Profiler, and Environment are all interfaces, each with one method?

Having many one-method interfaces that are commonly used together doesn't seem right to me either.
Having many interfaces that are used together and are not reused elsewhere seems to be of limited benefit.

For example, the Lock method in the Process interface has a very concrete meaning: it acquires a lock for the process, it is not a generic lock mechanism. I don't see the use of having it separated from the Process interface.

It would help me a lot to understand your objection to the current design if you could provide your alternative design in terms of which interfaces would you define because having this abstract conversation is not helping me at this point.

[roobre]

If I understand correctly, you are suggesting in the code above, FSLock, ProcessInfo, Profiler, and Environment are all interfaces, each with one method?

That's what I have in mind, yes. Some people prefer to make those function pointers, which is not very different. I think the best known example to this is the classic func() time.Time, which defaults to time.Now.

Having many one-method interfaces that are commonly used together doesn't seem right to me either.

I agree with this, I don't think either approach is obviously better than another. When in doubt, I lean toward what the principles say, but I think your approach to lean towards the simpler is also very valid.

With no clear benefit from one versus the other this becomes another style discussion so I'm fine moving on with the current approach just for the sake of not blocking this further.

[pablochacin]

I think making the directory configurable allows us to write integration tests for it without the need for a mock, which may increase our confidence not only in the fact we are calling it correctly, but in that, it works for locking the process.

This is already tested in the unit tests for the file-based lock. Could you please elaborate on which kind of integration scenarios you think we would need to test?

From my perspective, I don't see the value of creating integration tests outside the defined scope and use cases of the project. but we continue discussing here

[roobre]

This is already tested in the unit tests for the file-based lock. Could you please elaborate on which kind of integration scenarios you think we would need to test?

My previous comment was not particularly useful. It was driven out of my force of habit plus own biases of preferring integration tests whenever possible. The code already has a good, solid framework for unit testing, so after thinking about this it does not make any sense for me to try to steer towards a different direction.

It makes much more sense to keep using the existing approach than trying to change it now, so let's go for that.

[pablochacin]

@roobre I refactored the Profiler interface following your feedback and I think it is now in a much better shape. Thanks!

Regarding splitting the Process interface in sub-interfaces, I'm leaving it for a follow up PR, mostly because very soon more functions will be added and then maybe it will be more evident the best way to do it.