-
Notifications
You must be signed in to change notification settings - Fork 15
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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!
scripts/docker/registry.yaml
Outdated
#REGISTRY_HTTP_TLS_KEY: /certs/domain.unencrypted.key | ||
#REGISTRY_HTTP_SECRET: supersecrettext | ||
volumes: | ||
- /home/ubuntu/rfaas/containers/registry:/var/lib/registry |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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/..
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to merge!
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.
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 thelibfabric-sarus
branch, and #29.Opening as draft since I just need to finish testing.