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

Added container configuration system from libfabric-sarus into mater #31

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mattnappo
Copy link
Collaborator

@mattnappo mattnappo commented Jun 3, 2023

This PR adds support for Docker, Singularity, and Sarus containers. It also allows for configuration of these environments in config/executor_manager.json. Most all of the code in this PR is from the libfabric-sarus branch, and #29.

Opening as draft since I just need to finish testing.

@mattnappo mattnappo marked this pull request as ready for review June 5, 2023 13:25
Copy link
Contributor

@mcopik mcopik left a comment

Choose a reason for hiding this comment

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

Looks very good to me except a minor fix - I will try it locally and try to merge ASAP!

#REGISTRY_HTTP_TLS_KEY: /certs/domain.unencrypted.key
#REGISTRY_HTTP_SECRET: supersecrettext
volumes:
- /home/ubuntu/rfaas/containers/registry:/var/lib/registry
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that necessary? It seems that it might prevent user from starting a registry.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you referring to the volume mount?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the mounting of /home/ubuntu/rfaas/...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, starting a registry does require a volume. The user can mount to wherever they please on their local machine. We should either (a) make it clear that this is a point of configuration, or (b) mount to a set place, like /opt/rfaas/registry or something like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mattnappo Then I think we need to make it clear that it needs to be adapted by the user. Can we maybe put some hardcoded value like REGISTRY_LOCATION_REPLACE_ME? Otherwise people might miss it easily.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mcopik Latest commit should resolve this. The documentation is still a WIP: I am going to add more to it.

I am also currently performing a final test of running the benchmarks with Docker. I will notify you when these tests pass, and then we can merge.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good to merge!

@mattnappo mattnappo self-assigned this Jun 27, 2023
mattnappo and others added 4 commits June 30, 2023 18:34
execvp expects a nullptr at the end as a char*. The previous method of doing this resulted in a 'std::logic_error' what(): basic_string::_M_construct null not valid error, since we were trying to construct a std::string with a nullptr. This commit fixes this bug.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants