Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add feature to use podman container when determining version for vcloud-benchmark #1132

Merged
merged 30 commits into from
Dec 12, 2024

Conversation

ricffb
Copy link
Contributor

@ricffb ricffb commented Dec 3, 2024

This MR adds extra functionality for the vcloud-benchmark.py.

Some tools like PeSCo crash under newer linux versions even while obtaining the version.
And in this case the crash even causes the entire benchmark execution to fail (since it causes a index out of bounds directly in the tool info module). These tools are run inside of a podman container in the benchcloud, denoted with the --vcloudContainerImage option.

I propose to go ahead and also use this image while obtaining the version of the tool.

I manually tested this code with cpachecker, PeSCo, VeriAbs and nacpa. I also tested the fallback by renaming "podman" to "podmans".

I don't know, if we want to make this way of obtaining a Version optional via yet another flag (or if the --no-container should serve double duty here).

@dbeyer is strongly interested in some form of containering the version retrieval for the Hors Concours runs of SV-COMP25.

I am grateful for feedback how to improve, so that we can swiftly incorporate this feature.

PeSCo also needs the full tool_directory mounted to the container. I expect this from other python based tools aswell.
This is why I added the find_tool_base_dir function that mimics how the find_executable works but tries to determine the candidate that actually won.

@ricffb ricffb requested a review from PhilippWendler December 3, 2024 22:33
@dbeyer
Copy link
Member

dbeyer commented Dec 4, 2024

@ricffb @PhilippWendler I fully support this and confirm that this feature is required for competition execution. This solution looks elegant.

@PhilippWendler
Copy link
Member

PhilippWendler commented Dec 4, 2024

_version_from_tool is not public API of tool-info modules (as indicated by the dash prefix). It is a utility method that is used by some tool-info modules, but not necessarily by all of them. And of course the tool-info module might overwrite _version_from_tool with a method that does something else (such as accepting more parameters). In summary, this currently only works for some tool-info modules and depends on their internal implementation details. Only the version method must be used from the outside.

I am also surprised that this currently works at all (and do not understand why). The _version_from_tool method should never actually be called on the benchmark.tool object, because the latter is a wrapper (ContainerizedTool) around the actual tool-info module. The call to version happens on the wrapper and is then forward to the actual tool-info module, and all internal calls should happen strictly inside its class and not involve the wrapper at all. What is the call chain here if you say it succeeds?

If a container image is given as parameter, I think we should never execute the tool outside of this image. So there should be no fallback. And I would actually say that the whole tool-info module should live inside the Podman container (just like it currently lives inside a BenchExec container). The tool-info module's methods are free to do what they want, and for example can call version from inside other methods (several tool-info modules actually require that). And this would fail because Podman cannot be executed from within the containerized tool-info module. Furthermore, the tool-info module can even execute the tool from other methods, such as for example from within executable or cmdline.

@ricffb
Copy link
Contributor Author

ricffb commented Dec 4, 2024

@PhilippWendler You are right. Both setups had a --no-container lingering around in the command line.
Removing it will not work (gives an error from multiprocessing).

@ricffb ricffb force-pushed the use-podman-container-when-getting-version branch from 9a476c6 to 329913c Compare December 4, 2024 22:12
@ricffb ricffb force-pushed the use-podman-container-when-getting-version branch from 329913c to ed499c4 Compare December 4, 2024 22:16
Copy link
Member

@PhilippWendler PhilippWendler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the new solution is much better than the previous one. I think it now works in all cases if --tool-directory is passed and the Python installations inside and outside of the container do not clash.

I have some recommendations on how the code and the podman container can be improved further, but I do not see any crucial problems. So if it is required it could be merged and used, and improvements be added later.

contrib/vcloud/podman_containerized_tool.py Outdated Show resolved Hide resolved
contrib/vcloud/benchmarkclient_executor.py Outdated Show resolved Hide resolved
contrib/vcloud-benchmark.py Outdated Show resolved Hide resolved
contrib/vcloud-benchmark.py Outdated Show resolved Hide resolved
contrib/vcloud/benchmarkclient_executor.py Outdated Show resolved Hide resolved
contrib/vcloud/podman_containerized_tool.py Outdated Show resolved Hide resolved
contrib/vcloud/podman_containerized_tool.py Outdated Show resolved Hide resolved
contrib/vcloud/podman_containerized_tool.py Outdated Show resolved Hide resolved
contrib/vcloud/podman_containerized_tool.py Outdated Show resolved Hide resolved
contrib/vcloud/podman_containerized_tool.py Show resolved Hide resolved
@PhilippWendler
Copy link
Member

I don't know, if we want to make this way of obtaining a Version optional via yet another flag (or if the --no-container should serve double duty here).

Passing --no-container and --vcloudContainerImage should probably produce an error, because it is just confusing. And if --vcloudContainerImage is passed, we always want to use it for the tool-info module.

benchexec/containerized_tool.py Outdated Show resolved Hide resolved
benchexec/containerized_tool.py Outdated Show resolved Hide resolved
test/tasks/benchmark.xml Outdated Show resolved Hide resolved
@ricffb ricffb force-pushed the use-podman-container-when-getting-version branch from de905ee to 5bf07ad Compare December 9, 2024 12:51
@ricffb ricffb force-pushed the use-podman-container-when-getting-version branch from 2aac8ce to dde1397 Compare December 9, 2024 15:22
@PhilippWendler
Copy link
Member

Please do not mark comment threads that were created by others as "resolved". The respective reviewer should close it once they agree that their comments are sufficiently addressed. Any other process has a high risk of missing open problems and creates a high mental load for reviewers for keeping track of what they still need to check.

Copy link
Member

@PhilippWendler PhilippWendler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be merged as soon as CI is green.

Would still better to get a more strictly isolated container, though, for example without network access (currently tools do not have network access, but this container allows it).

@ricffb ricffb force-pushed the use-podman-container-when-getting-version branch from e49b2fe to a9aa5b2 Compare December 11, 2024 20:32
@ricffb ricffb force-pushed the use-podman-container-when-getting-version branch from 37b3140 to 195dada Compare December 11, 2024 20:58
@dbeyer dbeyer merged commit f28d99c into main Dec 12, 2024
15 checks passed
@dbeyer dbeyer deleted the use-podman-container-when-getting-version branch December 12, 2024 01:30
@dbeyer
Copy link
Member

dbeyer commented Dec 12, 2024

Many thanks @ricffb and @PhilippWendler ! I merged it now because I needed it already now.
I would like to ask you to make further improvements in a new MR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants