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

Update Docker to Ubuntu 18 #202

Merged
merged 8 commits into from
May 23, 2020
Merged

Conversation

kevindweb
Copy link
Contributor

@kevindweb kevindweb commented Apr 7, 2020

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:

  • Dependencies: None

TODO before merging :

  • PR is ready for review
  • Sanity check Docker container

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
Copy link

onvm commented Apr 7, 2020

In response to PR creation

CI Message

Your results will arrive shortly

onvm
onvm previously approved these changes Apr 7, 2020
Copy link

@onvm onvm left a comment

Choose a reason for hiding this comment

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

In response to PR creation

CI Message

Run successful see results:
✔️ PR submitted to develop branch
✔️ Pktgen performance check passed
✔️ Speed Test performance check passed
✔️ Linter passed

[Results from nimbnode23]

  • Median TX pps for Pktgen: 7723246
    Performance rating - 100.30% (compared to 7700000 average)

  • Median TX pps for Speed Tester: 42153877
    Performance rating - 100.37% (compared to 42000000 average)

@twood02 twood02 added this to the ONVM 20.05 milestone Apr 9, 2020
@dennisafa dennisafa mentioned this pull request Apr 10, 2020
3 tasks
@kevindweb
Copy link
Contributor Author

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

WilliamMaa
WilliamMaa previously approved these changes Apr 13, 2020
Copy link
Contributor

@WilliamMaa WilliamMaa left a comment

Choose a reason for hiding this comment

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

There does not seems to be many changes to the actual functionalities. The readme file is easy to follow. Most changes seems just relates to new syntax or keep updating the docker, I just copy pasted the changes, it seems to be working fine on Ubuntu 18. Although I think it would be nicer if we keep a record on how to run dockers on older versions of Ubuntu considering there might be some people still using Ubuntu 16 or 14. Ubuntu 16 and 14 still have LTS versions. And I am not sure if this will work on those versions.

@kevindweb
Copy link
Contributor Author

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

@kevindweb kevindweb dismissed stale reviews from WilliamMaa and onvm via b1c44c9 April 14, 2020 20:12
@twood02
Copy link
Member

twood02 commented Apr 15, 2020

Todo:

  • Add a line referencing the docker hub that says to check there for available tags
  • @sreya519 will test that both manager and NFs can run in docker

@onvmstats
Copy link
Collaborator

@onvm test time elapsed

@onvm
Copy link

onvm commented Apr 17, 2020

@onvm test time elapsed

CI Message

Your results will arrive shortly

@onvm
Copy link

onvm commented Apr 17, 2020

@onvm test time elapsed

CI Message

Error: ERROR: Failed to post results to GitHub

@onvm
Copy link

onvm commented Apr 17, 2020

Message

CI Message

Your results will arrive shortly

@onvm
Copy link

onvm commented Apr 17, 2020

Time elapsed test

CI Message

Your results will arrive shortly

Copy link

@onvm onvm left a comment

Choose a reason for hiding this comment

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

Time elapsed test

CI Message

Run successful see results:
Test took: 6 seconds
✔️ PR submitted to develop branch
✔️ Linter passed

sreyanalla
sreyanalla previously approved these changes Apr 29, 2020
Copy link
Contributor

@sreyanalla sreyanalla left a comment

Choose a reason for hiding this comment

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

Set up a Cloudlab environment to run and test the PR. Read through summary/ReadME and made sure to follow all set up instructions to run NF (everything is relatively easy to follow). Created docker containers as discussed in the PR. Looks like both manager and NFs are able to run in the docker successfully.

twood02
twood02 previously approved these changes Apr 29, 2020
@twood02
Copy link
Member

twood02 commented Apr 29, 2020

@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
Copy link
Contributor Author

@onvm test CI updates

@onvm
Copy link

onvm commented May 14, 2020

@onvm test CI updates

CI Message

Your results will arrive shortly

onvm
onvm previously approved these changes May 14, 2020
Copy link

@onvm onvm left a comment

Choose a reason for hiding this comment

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

@onvm test CI updates

CI Message

Run successful see results:
Test took: 5 minutes, 43 seconds
✔️ Pktgen performance check passed

[Results from nimbnode23]

  • Median TX pps for Pktgen: 7721721
    Performance rating - 100.28% (compared to 7700000 average) ✔️ Speed Test performance check passed

  • Median TX pps for Speed Tester: 42230530
    Performance rating - 100.55% (compared to 42000000 average)

@onvmstats
Copy link
Collaborator

@onvm unauthorized?

@onvmstats
Copy link
Collaborator

@onvm try

@onvm
Copy link

onvm commented May 14, 2020

@onvm try

CI Message

Aborting, need an authorized user to run CI

@kevindweb kevindweb dismissed stale reviews from onvm, twood02, and sreyanalla via 70d0a51 May 21, 2020 14:55
@kevindweb
Copy link
Contributor Author

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.

sreyanalla
sreyanalla previously approved these changes May 21, 2020
Copy link
Contributor

@sreyanalla sreyanalla left a comment

Choose a reason for hiding this comment

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

Given changes made to the PR, I tested by running NFs (simple_forward) inside and outside a container. When the NF runs inside the container, it runs regardless of whether or not the manager is running. When the NF is outside the container, it's only able to run if there's a running manager. Output seems to be as expected given the new changes.

@bdevierno1
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@twood02 twood02 added the NeedsReleaseNote Needs updated release note info label May 22, 2020
@dennisafa
Copy link
Member

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

@dennisafa dennisafa merged commit b37dd52 into sdnfv:develop May 23, 2020
@twood02 twood02 removed the NeedsReleaseNote Needs updated release note info label May 27, 2020
@kevindweb kevindweb deleted the docker_ubuntu18_update branch June 1, 2020 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants