Skip to content

Add ability to load balance to different Grantors#438

Merged
AltraMayor merged 6 commits intoAltraMayor:masterfrom
cjdoucette:gt_load
Dec 20, 2020
Merged

Add ability to load balance to different Grantors#438
AltraMayor merged 6 commits intoAltraMayor:masterfrom
cjdoucette:gt_load

Conversation

@cjdoucette
Copy link
Collaborator

No description provided.

@cjdoucette cjdoucette added this to the First deployment milestone Nov 6, 2020
Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: add set of Grantors for Grantor rules

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: reflect multiple Grantors in FIB dump

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: allow multiple Grantors per entry

Please add an example of calling add_grantor_entry_lb() in file lua/examples/example_of_dynamic_config_request.lua.

We are still missing a function to update the grantors of a Grantor fib. This could come in a new patch to simplify the review.

@cjdoucette
Copy link
Collaborator Author

I can add a function for explicitly updating the entry, although I have just been calling add_fib_entry() or add_grantor_entry_lb() to overwrite the existing entry for a prefix.

Now that I'm looking at this aspect though, I am noticing that updating the FIB table using this method leaves dangling entries in the FIB. The new FIB entry gets added and the LPM table gets updated to reference the new FIB entry, while the old FIB entry still exists. This is an issue even before this patch -- calling add_fib_entry() twice for the same prefix will trigger this problem.

Should add_fib_entry() and its related functions fail if the entry to be added is for an existing prefix? Or should it replace the existing entry, cleaning up the old FIB entry? My preference would be to mimic what iproute2 does and only allow add/remove, not have a separate update function, and fail if the ID for the rule (the prefix) already exists.

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: allow multiple Grantors per entry

@AltraMayor
Copy link
Owner

AltraMayor commented Nov 10, 2020

I'm aligned with your preference of making add_fib_entry() and its related functions fail if the entry to be added is for an existing prefix. The bug triggered by calling add_fib_entry() on an existing prefix is serious enough for us to address it in another pull request and get that pull request merged before this one.

For this pull request, we will need a function to only update the grantor-gateway pairs of a Grantor entry. One can delete and add a new entry, but given that, for a brief moment, there would be no entry in the fib that protects the given network prefix. Moreover, the overhead to remove a Grantor fib entry is high (i.e. all GK blocks have to scan their flow tables to drop the associated flow entries). It's better to support the update of the grantor-gateway pairs with a dedicated function.

@cjdoucette cjdoucette force-pushed the gt_load branch 2 times, most recently from 64cb487 to 3480181 Compare November 13, 2020 16:15
@cjdoucette
Copy link
Collaborator Author

Ready for another review.

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: allow multiple Grantors per entry

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: allow Grantor entries to be updated

@cjdoucette cjdoucette force-pushed the gt_load branch 2 times, most recently from 8e6a9cd to 76b2d64 Compare November 13, 2020 20:58
@cjdoucette
Copy link
Collaborator Author

Ready for another review.

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: allow multiple Grantors per entry

@cjdoucette
Copy link
Collaborator Author

OK, ready for another review.

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: allow multiple Grantors per entry

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: add set of Grantors for Grantor rules

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: allow multiple Grantors per entry

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: allow multiple Grantors per entry

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: add set of Grantors for Grantor rules

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: separate prefix existence and security check

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: clean up FIB initialization code

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: allow Grantor entries to be updated

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: add set of Grantors for Grantor rules

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: reflect multiple Grantors in FIB dump

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: allow multiple Grantors per entry

@cjdoucette
Copy link
Collaborator Author

Ready for another review.

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: allow multiple Grantors per entry

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: separate prefix existence and security check

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: allow Grantor entries to be updated

@cjdoucette
Copy link
Collaborator Author

Ready for another review.

Copy link
Owner

@AltraMayor AltraMayor left a comment

Choose a reason for hiding this comment

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

fib: allow multiple Grantors per entry

@AltraMayor
Copy link
Owner

A rebase is needed.

To prepare for a patch that allows existing Grantor entries
to be updated, this patch separates (1) the checking of whether
a prefix already exists in the table from (2) the checking of
whether the prefix would insert a security hole. Updating an
entry that already exists doesn't require checking (2), so
these should be separate.
Replace the return types of the FIB initialization functions
to remove unnecessary variables, and add style fixes.
Allow an existing Grantor FIB entry to be updated. To do so,
this patch keeps add_fib_entry_numerical() mostly the same
and adds a separate function update_fib_entry_numerical().

Although there is some overlap between these functions, they
are kept separate because (1) add_fib_entry_numerical() is
exposed and has callers in cps/kni.c, and therefore adding
more parameters (int overwrite) to its prototype is undesirable,
and (2) it has different requirements than *updating* an entry,
which doesn't need to check for security holes and neighbor
FIB entries.
@cjdoucette
Copy link
Collaborator Author

Ready for review.

@AltraMayor AltraMayor merged commit 0cd13e6 into AltraMayor:master Dec 20, 2020
@cjdoucette cjdoucette deleted the gt_load branch May 12, 2021 19:30
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.

2 participants