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

Weighted Random Policy #327

Merged
merged 23 commits into from
Aug 4, 2022
Merged

Weighted Random Policy #327

merged 23 commits into from
Aug 4, 2022

Conversation

xwedea
Copy link

@xwedea xwedea commented Jul 26, 2022

This PR adds 2 new policies to the load balancer: Random and Weighted Random.

After this PR, there are 3 policies that can be specified in the configuration file as:

  • RROBIN
  • RANDOM
  • WEIGHTED_RANDOM

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
Dependency updates
Web stats updates

Merging notes:

  • Dependencies: None

TODO before merging :

  • PR is ready for review

Test Plan:

Review:

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

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

@xwedea xwedea changed the title Weighted Weighted Random Policy Jul 26, 2022
@xwedea xwedea closed this Jul 26, 2022
@twood02 twood02 reopened this Jul 26, 2022
@twood02 twood02 marked this pull request as draft July 26, 2022 15:13
@xwedea
Copy link
Author

xwedea commented Jul 31, 2022

@twood02 we allocate and free lb->weights and lb->policy using standard malloc and free functions. However, in the previous of the code dpdk's function rte_calloc and rte_free are used. Should I edit this code to use those functions? I am not sure how to free them later in that case.

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.

Good start! I've marked a few things which can be improved.

examples/load_balancer/load_balancer.c Show resolved Hide resolved
examples/load_balancer/load_balancer.c Outdated Show resolved Hide resolved
examples/load_balancer/load_balancer.c Outdated 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 Outdated Show resolved Hide resolved
examples/load_balancer/load_balancer.c Outdated Show resolved Hide resolved
run_manager.sh Outdated Show resolved Hide resolved
examples/load_balancer/load_balancer.c Outdated Show resolved Hide resolved

/* structures to store server weights */
int *weights;
int total_weight;
Copy link
Member

Choose a reason for hiding this comment

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

your data structure here will allow us to represent weighted load balancers, but there could be other policies we could come up with in the future that might need totally different data. For now this is fine, but you should think about how you would make this data structure generic so it works for all LBs (but still allows customization by each policy in some way)

@twood02
Copy link
Member

twood02 commented Aug 1, 2022

@twood02 we allocate and free lb->weights and lb->policy using standard malloc and free functions. However, in the previous of the code dpdk's function rte_calloc and rte_free are used. Should I edit this code to use those functions? I am not sure how to free them later in that case.

The main differences between normal malloc and rte_malloc are:

  • rte_malloc allocates memory in the DPDK shared memory pool. This allows all NFs and the manager to access the data. That can either be a good thing or a bad thing
  • rte_malloc is much faster than malloc if you are frequently allocating and freeing objects (if you want to understand why, read about DPDK memory allocation and pools here)

For your use case, you are allocating memory which will only be used by the LB NF and you are only doing it once when the NF starts. So it is fine to use normal malloc here.

@twood02 twood02 marked this pull request as ready for review August 3, 2022 15:20
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 bit more cleanup then this is ready

examples/load_balancer/load_balancer.c Show resolved Hide resolved
examples/load_balancer/load_balancer.c Outdated Show resolved Hide resolved
@xwedea
Copy link
Author

xwedea commented Aug 4, 2022

@twood02 I removed the commented code and did a final check with all policies. this should be good to go

@twood02
Copy link
Member

twood02 commented Aug 4, 2022

@xwedea - can you put a description of what this PR adds at the top of this page. When we make our release notes for each ONVM version we go back to the PRs to get short descriptions of each new feature.

Otherwise this looks good and I will merge it into develop. Then next week we'll go through a full review process with @andreaseno's PR where he will run the NFs and demonstrate it working for me.

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.

Looks good!

The only issue is that it requires all config files to have weights specified even if they aren't used, but this will be resolved by PR #328 that changes to using JSON config files.

@xwedea
Copy link
Author

xwedea commented Aug 4, 2022

@twood02 I put a description.

@twood02 twood02 merged commit e04dcf6 into sdnfv:develop Aug 4, 2022
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.

3 participants