Skip to content

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
Collaborator

@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
Collaborator

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
Collaborator

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
Collaborator

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