Skip to content
This repository has been archived by the owner on Mar 31, 2023. It is now read-only.

[Feature] Adding support for port delete, also include L2/L3 neighbor delete #166

Merged
merged 17 commits into from Nov 20, 2020

Conversation

er1cthe0ne
Copy link
Contributor

@er1cthe0ne er1cthe0ne commented Nov 19, 2020

This PR added the ability to delete a port configuration on host. The change added:

  1. support for port delete
  2. support for L2 neighbor delete
  3. support for L3 neighbor delete
  4. test case for the above and confirm with traffic
  5. test refactoring for reuse and clean up
  6. new test cases of L3 neighbor

Also fixes:futurewei-cloud/alcor#103

@er1cthe0ne er1cthe0ne self-assigned this Nov 19, 2020
@er1cthe0ne er1cthe0ne added this to In progress in Alcor via automation Nov 19, 2020
@er1cthe0ne er1cthe0ne linked an issue Nov 19, 2020 that may be closed by this pull request
@er1cthe0ne
Copy link
Contributor Author

@zhangml @Zqy11 - FYI on this port delete PR.

include/aca_ovs_l3_programmer.h Show resolved Hide resolved
include/aca_ovs_l3_programmer.h Outdated Show resolved Hide resolved

case OperationType::UPDATE:
// only delete scenario is supported now
// VM was created with port specified, then delete the VM
Copy link
Contributor

Choose a reason for hiding this comment

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

We could discuss this option: In the VM update scenario, when PM receives the update request and determine it is a delete for the host, PM could issue a DEL. This could simplify ACA implementation. What do you think of?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea, keep in mind PM needs to determine that anyway because it needs to send down a neighbor table for the corresponding neighbor delete (same logic as port create and send down neighbor table).

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed. Let us use the same issue to track the PM change then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

port delete scenario tracking issue has been updated: futurewei-cloud/alcor#103

src/dp_abstraction/aca_dataplane_ovs.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@xieus xieus left a comment

Choose a reason for hiding this comment

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

Some more comments. Look pretty good in general as we know it has been tested 👍

src/dp_abstraction/aca_dataplane_ovs.cpp Outdated Show resolved Hide resolved
src/dp_abstraction/aca_dataplane_ovs.cpp Outdated Show resolved Hide resolved
"add-flow br-tun \"table=4, priority=1,tun_id=" + to_string(tunnel_id) +
" actions=mod_vlan_vid:" + to_string(internal_vlan_id) + ",output:\"patch-int\"\"",
culminative_time, overall_rc);
ACA_Vlan_Manager::get_instance().create_ovs_port(vpc_id, port_name, tunnel_id, culminative_time);

if (g_demo_mode) {
string cmd_string = "add-port br-int " + port_name +
Copy link
Contributor

Choose a reason for hiding this comment

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

My comment is to Line 194 where we start to execute the ovsdb command and system command.

This is one of the place we can start abstracting more dp related logics, for example, we have a one dp interface class with a run method, and many worker classes inheriting from this interface class with the real programming logics.

One step further, we could have one more layer of abstraction "on top of" the interface class to represent more general dp programming.

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, all the logic of parsing port configuration is already done at this point, and ready to do the dp programming. Another abstraction layer we can do is at line 191 ACA_Vlan_Manager::get_instance().create_ovs_port. If the other dp behavior is similar to ovs, we can simply call into another dataplane specific code here.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good. Let us plan for that and create a tracking item.

Alcor automation moved this from In progress to Reviewer approved Nov 20, 2020
@er1cthe0ne er1cthe0ne merged commit 8429dfb into futurewei-cloud:master Nov 20, 2020
Alcor automation moved this from Reviewer approved to Done Nov 20, 2020
@er1cthe0ne er1cthe0ne deleted the port_delete branch November 20, 2020 06:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Alcor
  
Done
Development

Successfully merging this pull request may close these issues.

Implement update and delete for port ovs programming
2 participants