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 to ONVM 19.07 #8

Merged
merged 14 commits into from
Feb 1, 2020
Merged

Update to ONVM 19.07 #8

merged 14 commits into from
Feb 1, 2020

Conversation

archit-p
Copy link
Contributor

This pull request updates the onvm-snort repository to run with onvm 19.07.

Changes were made in libdaq code and configure scripts. A gitignore file was added to ensure clean commits.

daq-2.0.6/Makefile Show resolved Hide resolved
daq-2.0.6/os-daq-modules/daq_netvm.c Outdated Show resolved Hide resolved
daq-2.0.6/os-daq-modules/daq_netvm.c Outdated Show resolved Hide resolved
@archit-p
Copy link
Contributor Author

Oops looks like I forgot to update the openNetVM version in the repository. Would it be better to use a openNetVM as a submodule here?

@twood02
Copy link
Member

twood02 commented Oct 13, 2019

Yes, using a submodule would be ideal. Let us know when it is ready for us to take a look again. Thanks @archit-p !

@archit-p
Copy link
Contributor Author

Hi,

I've updated the repository to include openNetVM 19.07 as a submodule. Have also updated the installation instructions in install.md, and installation script in install.sh. I'd really appreciate it if someone could follow these instructions and try a fresh installation!

@archit-p
Copy link
Contributor Author

Hello team,

Just a gentle reminder on the open pull request. Please provide suggestions on how to take this forward.

@twood02
Copy link
Member

twood02 commented Oct 17, 2019

Thanks @archit-p . @dennisafa is going to take a look but he may not get to it for another week because of class work.

Install.md Outdated Show resolved Hide resolved
@archit-p
Copy link
Contributor Author

@dennisafa is the current code building alright?

@dennisafa
Copy link
Member

dennisafa commented Oct 22, 2019 via email

@archit-p
Copy link
Contributor Author

Hi dennis,

No problems, you could share the issues here

Install.md Show resolved Hide resolved
@archit-p
Copy link
Contributor Author

archit-p commented Oct 25, 2019 via email

@dennisafa
Copy link
Member

That's strange, I didn't need to update my $PATH. Could you share the snort config.log file? It might contain what the error is exactly.

On Fri, 25 Oct 2019, 8:37 am Dennis Afanasev, @.> wrote: @.* commented on this pull request. ------------------------------ In Install.md <#8 (comment)>: > sh - ./configure --enable-sourcefire + aclocal + autoconf + autoheader + automake -a + +3. Run the configuration script. I think the issue is that the $PATH variable needs to be initialized to point to the daq-modules-config file. This file is found in /daq-2.0.6/os-daq-modules — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8?email_source=notifications&email_token=AHMM4LEO5PPRTH5LVSR32NLQQJPINA5CNFSM4I76KWHKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJF6B4I#discussion_r338869726>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHMM4LE5ACXAWGBTY3XAC3LQQJPINANCNFSM4I76KWHA .

Screen Shot 2019-10-24 at 11 21 13 PM

Here is the relevant config.log section. Looks like there are issue with -lonvm and -lonvmhelper

@archit-p
Copy link
Contributor Author

archit-p commented Oct 25, 2019 via email

@dennisafa
Copy link
Member

dennisafa commented Oct 25, 2019

Looks like $DPDK_TARGET is not being interpreted correctly. Could you try echo $DPDK_TARGET and if nothing pops up, export $DPDK_TARGET=x86_64-native-linuxapp-gcc or your corresponding architecture. On a side note, I believe I should remove the use of $DPDK_TARGET and stick to $RTE_TARGET in daq-2.0.6/configure.ac. WDYT? On Fri, 25 Oct 2019, 8:52 am Dennis Afanasev, notifications@github.com wrote:

That's strange, I didn't need to update my $PATH. Could you share the snort config.log file? It might contain what the error is exactly. … <#m_306845524257962216_> On Fri, 25 Oct 2019, 8:37 am Dennis Afanasev, @.*> wrote: @.** commented on this pull request. ------------------------------ In Install.md <#8 (comment) <#8 (comment)>>: > sh - ./configure --enable-sourcefire + aclocal + autoconf + autoheader + automake -a + +3. Run the configuration script. I think the issue is that the $PATH variable needs to be initialized to point to the daq-modules-config file. This file is found in /daq-2.0.6/os-daq-modules — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8 <#8>?email_source=notifications&email_token=AHMM4LEO5PPRTH5LVSR32NLQQJPINA5CNFSM4I76KWHKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJF6B4I#discussion_r338869726>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHMM4LE5ACXAWGBTY3XAC3LQQJPINANCNFSM4I76KWHA . [image: Screen Shot 2019-10-24 at 11 21 13 PM] https://user-images.githubusercontent.com/32916582/67540859-01ba2280-f6b5-11e9-992d-2af22fd63aba.png Here is the relevant config.log section. Looks like there are issue with -lonvm and -lonvmhelper — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8?email_source=notifications&email_token=AHMM4LDHD6AWV3HYZOG34JLQQJQ53A5CNFSM4I76KWHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECHB2HQ#issuecomment-546184478>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHMM4LDBWT62T5AI2VI5BKDQQJQ53ANCNFSM4I76KWHA .

Yes, let's stick with using $RTE_TARGET as that is part of the openNetVM install guide. Thank you.

@archit-p
Copy link
Contributor Author

Sure, will do that!

I remember the old guide using two variables RTE_TARGET and DPDK_TARGET with the same value.

onvm_nflib_start_signal_handler(nf_local_ctx, sig_handler);
/*function table not needed */

/*
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Remove unnecessary commented code

printf("netvm init done\n");
/* Complete onvm handshake */
onvm_nflib_nf_ready(nf_info);
/* not necessary honestly
Copy link
Member

@dennisafa dennisafa Oct 25, 2019

Choose a reason for hiding this comment

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

Nit: Remove commented code

netvmc->rx_ring = onvm_nflib_get_rx_ring(nf_info);
netvmc->tx_stats = onvm_nflib_get_tx_stats(nf_info);

/* these functions do not exist anymore
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Remove commented code

@@ -273,7 +331,7 @@ static int netvm_daq_start(void *handle)
//printf("->netvm_daq_start()\n");
netvm_daq_reset_stats(handle);

//nf_info->status = NF_RUNNING;
//nf->status = NF_RUNNING;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Remove commented code

for (j = 0; j < tx_batch_size; j++) {
rte_pktmbuf_free(pktsTX[j]);
}
} else {
netvmc->tx_stats->tx[info->instance_id] += tx_batch_size;
/* update to the new pointer, update name */
nf->stats.tx += tx_batch_size;
}
}
//printf("<-netvm_daq_acquire() - count\n");
Copy link
Member

Choose a reason for hiding this comment

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

Remove commented out print statements

@archit-p
Copy link
Contributor Author

There are extra print statements in several places. I believe, they were used when debugging daq_netvm.c.

@archit-p
Copy link
Contributor Author

There are too many commits in my repository right now, I wish to squash or fixup most of them.

Could you let me know once you approve of the changes? I'll make a force push with modified history.

Install.md Outdated
```sh
cd ../
./patching-Makefile.sh
```

4. Navigate to the src folder of snort and Make snort.
5. Navigate to the src folder of snort and Make snort.
Copy link
Member

Choose a reason for hiding this comment

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

Please add a step asking the user to run autoreconf -ivf in case of a version mismatch error, then to do step 3 again.
Also, I am getting this issue:
Screen Shot 2019-10-24 at 11 57 43 PM

Could you elaborate on -ldpdk a bit more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, looks like I forgot to add autoreconf -fvi. Thanks for pointing that out!

Copy link
Member

Choose a reason for hiding this comment

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

Hi Archit, please add this step to the install guide. Thank you.

@archit-p
Copy link
Contributor Author

archit-p commented Oct 25, 2019 via email

@dennisafa
Copy link
Member

-ldpdk is found in $RTE_SDK/$RTE_TARGET/lib From the screenshot it looks to me like your $RTE_SDK variable is still set to the dpdk-16.11 directory. You might need to set it again.

On Fri, 25 Oct 2019, 9:28 am Dennis Afanasev, @.> wrote: @.* commented on this pull request. ------------------------------ In Install.md <#8 (comment)>: > sh cd ../ ./patching-Makefile.sh -4. Navigate to the src folder of snort and Make snort. +5. Navigate to the src folder of snort and Make snort. Please add a step asking the user to run autoreconf -ivf in case of a version mismatch error, then to do step 3 again. Also, I am getting this issue: [image: Screen Shot 2019-10-24 at 11 57 43 PM] https://user-images.githubusercontent.com/32916582/67542278-2369d880-f6ba-11e9-80b7-dcceb6cb1cf6.png Could you elaborate on -ldpdk a bit more? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8?email_source=notifications&email_token=AHMM4LFSHMT2VGLOO7RECDLQQJVHTA5CNFSM4I76KWHKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJGANEA#pullrequestreview-306972304>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHMM4LGLPKGSNW5DDL46A6DQQJVHTANCNFSM4I76KWHA .

It looks like my $RTE_SDK output is correct (set to onvm-snort/openNetVM/dpdk) I will look into it.

@archit-p
Copy link
Contributor Author

Might be helpful to try running

sudo ldconfig

once DAQ is done installing.

Install.md Outdated Show resolved Hide resolved
@dennisafa
Copy link
Member

I managed to get a working build running with a few modifications to the install guide (see comments above)
After you change those, I'll approve the PR. Thank you!

ratnadeepb and others added 13 commits November 11, 2019 09:42
The snort NF will now print out processing time for every batch of packets and the related packets per second rate
patching-Makefile.sh has been modified to accept default values. This file is now called from inside setenv.sh. setenv.sh sets up all initial enviornment variables assuming defaults. The variables in this file can be changed if defaults are not used. The setenv.sh script is run from inside install.sh. This script essentially, sets up all environment variables, installs all packages and builds all the tools.
Flags needed to compile daq-2.0.6 with DPDK 18.11
daq-2.0.6/build.sh - automates daq build
snort-2.9.8.3/build.sh - automates snort build
.gitignore contains common patterns to ignore for C projects.
Removed extra print statements, added comments to daq_netvm.c
Removed patching-makefile.sh step, changed variable names
Maintains consistency with openNetVM 19.07
Using --disable-shared while building snort, does not build
dynamic preprocessors.
Steps to build onvm-snort with openNetVM 19.07
@archit-p
Copy link
Contributor Author

Hi @dennisafa

Thanks a lot for reviewing the changes. I've updated the pull request according to your suggestions above.

@archit-p
Copy link
Contributor Author

@dennisafa I've merged commits into a more manageable number. You could merge these if the changes are approved. Thanks.

@archit-p
Copy link
Contributor Author

archit-p commented Dec 3, 2019

Hi, does this merge request require anymore changes?

@dennisafa
Copy link
Member

Hi, does this merge request require anymore changes?

Hi Archit, I ran into a few issues when re-building on a fresh instance. I've had a lot of work pile up so I haven't gotten around to figuring out the exact issue quite yet. I will retry and submit a change to the install guide by this weekend. Thanks for your patience!

Copy link
Member

@dennisafa dennisafa left a comment

Choose a reason for hiding this comment

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

Hi Archit, thanks for being patient. I managed to get snort working by switching a couple commands in the install guide. Could you change those, and then squash the unnecessary commits? After that I will definitely approve.
Screen Shot 2019-12-07 at 5 01 54 PM

Install.md Outdated Show resolved Hide resolved
Install.md Outdated Show resolved Hide resolved
@dennisafa
Copy link
Member

Hey @archit-p let me know if you have any questions about the comments!

Updated installation instructions
@archit-p
Copy link
Contributor Author

archit-p commented Jan 8, 2020

Hi, apologies for being away for so long. I have made the requested changes and updated the pull request! Thanks for reviewing!

@archit-p
Copy link
Contributor Author

Hi @twood02! Could you look at the proposed changes?

@twood02
Copy link
Member

twood02 commented Jan 30, 2020

Thanks @archit-p! I'll be discussing this with Dennis tomorrow and expect to merge it after that. We already have other students who are interested in using your code so your contribution is greatly appreciated!

@twood02 twood02 merged commit e98b33d into sdnfv:master Feb 1, 2020
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.

4 participants