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

JSON load balancer config file #328

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from
Open

JSON load balancer config file #328

wants to merge 19 commits into from

Conversation

andreaseno
Copy link

@andreaseno andreaseno commented Jul 26, 2022

Replaced the server.conf file inside the load balancer nf with a json file called server.json. Because the config file parser was written for a plaintext file rather than a json file, I had to rewrite the backend config file parse function. It now uses cjson to parse the config file.

Summary:

Usage:

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

Merging notes:

  • Dependencies: Should be merged after pull request Weighted Random Policy #327 for the weighted policy, since this code includes code from that pull request.

TODO before merging :

  • PR is ready for review

Test Plan:

I tested the changes by doing a full setup of a 2 server ONVM experiment, made sure the arp_response and load balancer nf's ran correctly without error, and used iperf to verify that the packets would still send correctly. The only other necessary testing I can think of off the top of my head is verifying it works with more than 2 servers.

Review:

(optional) << @-mention people who should review these changes >>

(optional) Subscribers: << @-mention people who probably care about these changes >>

Copy link
Member

@twood02 twood02 left a comment

Choose a reason for hiding this comment

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

some small changes to make. After #327 is updated you will need to merge it into your branch too.

examples/load_balancer/load_balancer.c Show resolved Hide resolved
examples/load_balancer/load_balancer.c Outdated Show resolved Hide resolved
examples/load_balancer/server.json Show resolved Hide resolved
examples/load_balancer/load_balancer.c Outdated Show resolved Hide resolved
examples/load_balancer/load_balancer.c Show resolved Hide resolved
examples/load_balancer/load_balancer.c Show resolved Hide resolved
examples/load_balancer/load_balancer.c Show resolved Hide resolved
examples/load_balancer/server.json Outdated Show resolved Hide resolved
@twood02
Copy link
Member

twood02 commented Aug 4, 2022

@andreaseno I merged #327 so now you will need to resolve conflicts.

Copy link
Member

@twood02 twood02 left a comment

Choose a reason for hiding this comment

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

A couple more edits.

@@ -600,6 +674,10 @@ main(int argc, char *argv[]) {
int arg_offset;
const char *progname = argv[0];

time_t t;
/* Intializes RANDOM number generator */
srand((unsigned) time(&t));
Copy link
Member

Choose a reason for hiding this comment

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

Check indentation

Copy link
Author

Choose a reason for hiding this comment

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

What is the issue with the indentation here?

Copy link
Member

Choose a reason for hiding this comment

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

looks like tabs not spaces. @catherinemeadows -- do you know if we have a working linter script that checks for this kind of formatting issue in ONVM? I thought our GitHub actions would do it automatically, but it seems not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, in /openNetVM/style, there is documentation about manually running our linter scripts. I think the GitHub actions weren't triggered yet because there are still merge conflicts.

{
"Info" : {
"list_size": 2,
"policy": "random"
Copy link
Member

Choose a reason for hiding this comment

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

Should this be all caps?

@@ -555,6 +628,7 @@ packet_handler(struct rte_mbuf *pkt, struct onvm_pkt_meta *meta,
for (i = 0; i < RTE_ETHER_ADDR_LEN; i++) {
flow_info->s_addr_bytes[i] = ehdr->s_addr.addr_bytes[i];
}
if(debug_mode) printf("New connection made with server %d.\n",flow_info->dest);
Copy link
Member

Choose a reason for hiding this comment

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

Can we have this display the src IP and port? Something like:
192.168.1.100:4256 assigned to server 2

@twood02
Copy link
Member

twood02 commented Aug 21, 2022

@andreaseno @xwedea I added a couple more changes. If you can fix them sometime in the next few weeks - great! If not I'll get another student to help out later.

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.

None yet

4 participants