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

Better, more configurable Docker support #25

Closed
wants to merge 8 commits into from

Conversation

mattnappo
Copy link
Collaborator

This draft PR contributes to #10 and #19 by:

  • Adding scripts to launch a docker registry with authentication (credentials)
  • A rFaas base image Dockerfile (and scripts) to enrich a docker container with the rFaas executor binary
  • Automatic image polling from a private docker registry instead of DockerHub. The new config/executor_manager.json configuration file serves as an example of how to configure the docker registry and container selection.
  • Configurable, non-hard-coded Docker/Docker registry config for the Executor Manager.

Work that still needs to be done to wrap up Docker support:

  • Docker network configuration scripts
  • More testing of the scripts
  • HTTPS registry support

Right now, the configuration system I added is capable of connecting to any registry. Of course, running a local registry on the same server as the executor manager is possible under this config.

Ubuntu and others added 8 commits March 21, 2023 18:23
@mattnappo mattnappo mentioned this pull request Mar 25, 2023
4 tasks
@@ -22,7 +23,8 @@ set(CMAKE_CXX_STANDARD_REQUIRED TRUE)
string(REPLACE "-DNDEBUG" "" CMAKE_CXX_FLAGS_RELWITHDEBINFO "${CMAKE_CXX_FLAGS_RELWITHDEBINFO}")
string(APPEND CMAKE_CXX_FLAGS_RELWITHDEBINFO " -g -DSPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_DEBUG ")
string(APPEND CMAKE_CXX_FLAGS_DEBUG " -DSPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_DEBUG ")
string(APPEND CMAKE_CXX_FLAGS " -Wall -Wextra ")
string(APPEND CMAKE_CXX_FLAGS " -Wall -Wextra")
#string(APPEND CMAKE_CXX_FLAGS " -v -lstdc++")
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a leftover :-) Did you have trouble with compiling rFaaS on machines with libc++?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps my system was weirdly configured, but compilation with clang only worked with explicit linking of stdc++. However, compilation with gcc worked just fine, so I went with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the binaries were added by accident.

We can change the CMake scripts to name our executables "cold_benchmarker.x" to help gitignore catch these.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apologies. I of course would make sure that the repository is clean before a merge.

REG_IMG=$REG_IP:$REG_PORT/$IMG_NAME

# Build the docker container, login and push to the registry
sudo docker build -t $IMG_NAME - < containers/rfaas-base.Dockerfile
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be simpler not to use sudo here and instead explain in the documentation that users to make sure that they have necessary permissions to run Docker. Furthermore, on clusters the user might be in the docker group but it it is very unlikely that they will have sudo access.

Also, it's safer if never request to execute anything with superuser permissions - less likely that any mistake will have bad consequences :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay. That is a great point. Principle of least privilege!

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as for executables :-)

@@ -0,0 +1,3 @@
#!/bin/bash

sudo docker network create -d sriov --subnet=172.31.80.0/20 -o netdevice=eth0 mynet
Copy link
Contributor

Choose a reason for hiding this comment

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

The subnet should depend on your local network - maybe we can make it a parameter? Wrapping network initialization is definitely a good idea!

Copy link
Contributor

Choose a reason for hiding this comment

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

Another idea: add a second parameter for two types of networking - sriov and host. This way we can easily support a simpler networking mode, at least for testing.

Copy link
Collaborator Author

@mattnappo mattnappo Mar 31, 2023

Choose a reason for hiding this comment

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

I will definitely parameterize this.

@mcopik
Copy link
Contributor

mcopik commented Mar 29, 2023

@mattnappo Thank you! This looks like a good start :-)

I have a similar but extended solution for container selection on a different branch (libfabric-sarus branch); maybe I can try to cherry-pick these changes to the development branch and merge them with your effort to get a nice interface for container type selection.

@mattnappo mattnappo changed the title Better, more configuratble Docker support Better, more configurable Docker support Apr 4, 2023
mattnappo added a commit to mattnappo/rFaaS that referenced this pull request Apr 4, 2023
mattnappo added a commit to mattnappo/rFaaS that referenced this pull request Apr 4, 2023
@mattnappo
Copy link
Collaborator Author

This has been moved to #29

@mattnappo mattnappo closed this Apr 5, 2023
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