sdnfv / openNetVM

A high performance container-based NFV platform from GW and UCR.
http://sdnfv.github.io/onvm/
Other
263 stars 135 forks source link

Update Docker to Ubuntu 18 #202

Closed kevindweb closed 4 years ago

kevindweb commented 4 years ago

Docker was previously at Ubuntu 14, and had not been tested since the last time we moved ONVM to Ubuntu 18.04. Now, running the scripts/docker.sh works correctly.

Summary:

Important Note. Before merging into develop, we need to merge the script cleanup #183 . The docker changes and some onvm go script changes from there would have caused conflicts. Thus, I pre-merged those changes into this PR. So if we merge that one first, then this one will have no issue going into develop.

Usage: Follow the Docker README about installing Docker and everything to test this. Docker containers can be made quite easily, but here is an example command to get an ethernet device into the container for an NF run.

sudo ./docker.sh -h /dev/hugepages -o ~/openNetVM -n onvm-docker -D /dev/uio0 -c "./onvm/go.sh 0,1,2,3 3 0xF0 -a 0x7f000000000 -s stdout"

This command will start up the onvm manager. It detaches the container so we don't see the output. We can run an NF similar to this in another container, or outside the container.

This PR includes
Resolves issues
Breaking API changes
Internal API changes
Usability improvements
Bug fixes 👍
New functionality
New NF/onvm_mgr args
Changes to starting NFs
Dependency updates 👍
Web stats updates

Merging notes:

TODO before merging :

Test Plan:

I would really like one of you guys @sdnfv/gw-undergrads to look into Docker. It's a great tool that we could expand on in the future. As a starting point, review this PR and get an NF spun up in a container for testing.

Notes:

Please ignore any commits before April 5. They will be unimportant as soon as the bash scripts PR is merged.

Review:

@dennisafa @bdevierno1 (because it has to do with bash scripts)

onvm commented 4 years ago

In response to PR creation

CI Message

Your results will arrive shortly

kevindweb commented 4 years ago

@WilliamMaa thanks for leaving for review. Could you comment on what you did to test this to get it working? It's helpful so I know that everything was tested how I imagined it and that the documentation makes sense. Even if you think a PR works, it's always a good idea to leave a quick comment to the team.

kevindweb commented 4 years ago

You're definitely right, backward compatibility is important. I don't know the best way to leave a reference to older Ubuntu versions though. I might add a section to the README for users who want to create containers with older versions of ONVM

twood02 commented 4 years ago

Todo:

onvmstats commented 4 years ago

@onvm test time elapsed

onvm commented 4 years ago

@onvm test time elapsed

CI Message

Your results will arrive shortly

onvm commented 4 years ago

@onvm test time elapsed

CI Message

Error: ERROR: Failed to post results to GitHub

onvm commented 4 years ago

Message

CI Message

Your results will arrive shortly

onvm commented 4 years ago

Time elapsed test

CI Message

Your results will arrive shortly

twood02 commented 4 years ago

@sreya519 - test manager running in Docker with pktgen sending to a docker NF running simple_forward (set it to send back out the same port packets come in)

kevindweb commented 4 years ago

@onvm test CI updates

onvm commented 4 years ago

@onvm test CI updates

CI Message

Your results will arrive shortly

onvmstats commented 4 years ago

@onvm unauthorized?

onvmstats commented 4 years ago

@onvm try

onvm commented 4 years ago

@onvm try

CI Message

Aborting, need an authorized user to run CI

kevindweb commented 4 years ago

For those approving the PR, I just made a change because of a fundamental part of Docker containers. In the example and manager go script, we previously did checks on the running processes to make sure the manager wasn't duplicated, and the NFs wouldn't start if the manager wasn't running. This doesn't work since Docker containers don't have access to the outside processes on the system (outside container or in a different container). I just added a condition that ignores that check if we are in a docker container.

bdevierno1 commented 4 years ago

For those approving the PR, I just made a change because of a fundamental part of Docker containers. In the example and manager go script, we previously did checks on the running processes to make sure the manager wasn't duplicated, and the NFs wouldn't start if the manager wasn't running. This doesn't work since Docker containers don't have access to the outside processes on the system (outside container or in a different container). I just added a condition that ignores that check if we are in a docker container.

Maybe we should leave a message when running in docker? Maybe along the lines of 'please ensure manager is running'.

WilliamMaa commented 4 years ago

For those approving the PR, I just made a change because of a fundamental part of Docker containers. In the example and manager go script, we previously did checks on the running processes to make sure the manager wasn't duplicated, and the NFs wouldn't start if the manager wasn't running. This doesn't work since Docker containers don't have access to the outside processes on the system (outside container or in a different container). I just added a condition that ignores that check if we are in a docker container.

Maybe we should leave a message when running in docker? Maybe along the lines of 'please ensure manager is running'.

I agree with that, we can add a comment in there.

kevindweb commented 4 years ago

Good thoughts guys! I'll add a small comment in docker.sh

dennisafa commented 4 years ago

@twood02 I think i can only merge if I have two approves from users with write-access. Could you approve this? (it's good to go, as discussed in meeting)