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

[Bug] hot fix to enable L3 E2E #160

Merged
merged 10 commits into from
Nov 4, 2020

Conversation

er1cthe0ne
Copy link
Contributor

@er1cthe0ne er1cthe0ne commented Nov 2, 2020

This is the change to enable L3 E2E, with this change, we confirmed the L3 routing E2E is working through the horizon UI.

This PR includes the follow changes:

  1. added setup_ovs_bridges_if_need in aca_main which will work nicely when there is no ovs bridge when aca starts
  2. allow neighbor state to be create/update/info, since we are getting neighbor state update during L3 E2E
  3. enable process of neighbor state with fixed_ips_size > 1
  4. fixed the path issue with ovs-ctl

@er1cthe0ne er1cthe0ne added bug Something isn't working documentation Improvements or additions to documentation labels Nov 2, 2020
@er1cthe0ne er1cthe0ne self-assigned this Nov 2, 2020
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.

@er1cthe0ne Thanks for pushing the remaining fixes promptly. I left a few minor comments to the coding part.

Regarding the openflow table doc, let us review it together today.

src/dp_abstraction/aca_dataplane_ovs.cpp Show resolved Hide resolved
ACA_LOG_ERROR("PortConfiguration.fixed_ips_size: %d.\n",
current_PortConfiguration.fixed_ips_size());
throw std::invalid_argument("PortConfiguration.fixed_ips_size is less than zero");
}
virtual_ip_address = current_PortConfiguration.fixed_ips(0).ip_address();
Copy link
Contributor

Choose a reason for hiding this comment

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

Although we allow more than fixed IPs, we process the first one only as the primary IP. As a next step, it would be awesome to start thinking about storing the secondary IPs so that when customers add IPs in the VM (manually as limited by current networking stack), Control Plane has the knowledge to grant or reject customers' traffic, depending on whether customers configure the VM in the right way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it. The current port creation code doesn't consume the virtual IP address(s). It would be consumed in security group code to allow traffic come out from the VM with certain virtual IPs and virtual mac (it also should look at the allow_address_pairs in PortConfiguration).

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me rephrase the question: if an interface has more than 1 ports, the current ACA would only process and store the first ip, and other ips are ignored. We will need to handle other ips too.

src/dp_abstraction/aca_dataplane_ovs.cpp Outdated Show resolved Hide resolved
src/dp_abstraction/aca_dataplane_ovs.cpp Outdated Show resolved Hide resolved
src/dp_abstraction/aca_dataplane_ovs.cpp Outdated Show resolved Hide resolved

ovs-ofctl add-flow br-tun "table=0, priority=1,in_port="patch-int" actions=resubmit(,2)"
ovs-ofctl add-flow br-tun "table=2, priority=0 actions=resubmit(,22)"
if [ "$1" == "delete-bridges" ]; 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.

@cj-chung please take a look to confirm this approach is okay for our environment.

@xieus xieus self-requested a review November 4, 2020 22:19
@er1cthe0ne er1cthe0ne removed the documentation Improvements or additions to documentation label Nov 4, 2020
@er1cthe0ne er1cthe0ne merged commit e55cc1f into futurewei-cloud:master Nov 4, 2020
@er1cthe0ne er1cthe0ne deleted the openflow_rules branch November 4, 2020 23:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants