[Fundamental] Sub-level programming through Subnet and Route Mgr #468

kevin-zhonghao · 2020-11-10T02:59:50Z

The implementation of Sub-level programming mainly includes the following features:

Update of route rule, including VPC route rule and this Subnet route rule;
Integration of route manager and PM
Integration of route manager and DPM
Fixed issues related to gateway IP update (tracked by Issue [Microservice] Port Mgr Support L3 Routing with Routing Rules #446)

Test scenarios:

Create a VPC (success)
Create a Subnet. Subnet get gateway port from PM (comment the step of sending info to DPM) (success)
Create a Port (success)
SM Path: Create/Update/Delete subnet routing rule -> Create/Update/Delete subnet routetable in RM. (success)
RM Path - For Neutron Router，Add/Remove extra routes -> Update/Delete HostRoute in SubnetEntity in SM (success)
RM Path - For VPC router， Create/Update/Delete subnet routetable -> Create/Update/Delete HostRoute in SubnetEntity in SM (pending)

…o new_master

services/route_manager/src/main/java/com/futurewei/alcor/route/controller/RouterController.java

...oute_manager/src/main/java/com/futurewei/alcor/route/controller/NeutronRouterController.java

services/route_manager/src/main/java/com/futurewei/alcor/route/controller/RouterController.java

...es/route_manager/src/main/java/com/futurewei/alcor/route/service/Impl/RouterServiceImpl.java

...net_manager/src/main/java/com/futurewei/alcor/subnet/service/implement/SubnetServiceImp.java

cj-chung · 2020-11-12T16:42:17Z

...net_manager/src/main/java/com/futurewei/alcor/subnet/service/implement/SubnetServiceImp.java

+    public void deleteSubnetRoutingRuleInRM(String projectId, String subnetId) throws SubnetIdIsNull {
+
+        if (subnetId == null) {
+            throw new SubnetIdIsNull();
+        }
+
+        String routeManagerServiceUrl = routeUrl + "project/" + projectId + "/subnets/" + subnetId + "/routetable";
+        restTemplate.delete(routeManagerServiceUrl, ResponseId.class);
+
+    }
+
+    @Override
+    public void updateSubnetRoutingRuleInRM(String projectId, String subnetId, SubnetEntity subnetEntity) throws SubnetIdIsNull {


deleteSubnetRoutingRuleInRM, updateSubnetRoutingRuleInRM, and createSubnetRoutingRuleInRM three functions are doing pretty similar task, should we consider to consolidate them into one function?

Actually they should call different rest API and the same thing is to construct RouteTable in updateSubnetRoutingRuleInRM, createSubnetRoutingRuleInRM. We could put this process in one method, but I think sperate these three method is okay. What do you think?

...net_manager/src/main/java/com/futurewei/alcor/subnet/service/implement/SubnetServiceImp.java

cj-chung · 2020-11-12T17:37:06Z

...net_manager/src/main/java/com/futurewei/alcor/subnet/service/implement/SubnetServiceImp.java

+
+                for (int i = 0 ; i < hostRoutes.size(); i ++) {
+                    HostRoute hostRoute = hostRoutes.get(i);
+                    String subnetDestination = hostRoute.getDestination();


Do you think it costs too much to iterate all routes for each UPDATE route?

HostRoute dont have its own repo, so I think we could do in this way, I think subnet dont have many hostRoutes, right?

cj-chung · 2020-11-12T17:39:40Z

...net_manager/src/main/java/com/futurewei/alcor/subnet/service/implement/SubnetServiceImp.java

+                Iterator<HostRoute> iterator = hostRoutes.iterator();
+                while (iterator.hasNext()) {
+                    HostRoute hostRoute = iterator.next();
+                    String subnetDestination = hostRoute.getDestination();
+                    if (subnetDestination == null) {
+                        continue;
+                    }


same comment above, can we lookup hostRoute by destination?

the response above

...e_manager/src/main/java/com/futurewei/alcor/route/service/Impl/NeutronRouterServiceImpl.java

…o new_master

# Conflicts: # services/data_plane_manager/src/main/java/com/futurewei/alcor/dataplane/service/ovs/DataPlaneServiceImplNew.java

codecov-io · 2020-11-19T00:42:20Z

Codecov Report

Merging #468 (c61733f) into master (ff19c6b) will decrease coverage by 0.89%.
The diff coverage is 23.04%.

@@             Coverage Diff              @@
##             master     #468      +/-   ##
============================================
- Coverage     36.59%   35.69%   -0.90%     
- Complexity     1162     1166       +4     
============================================
  Files           453      467      +14     
  Lines         10869    11311     +442     
  Branches       1393     1456      +63     
============================================
+ Hits           3977     4037      +60     
- Misses         6350     6721     +371     
- Partials        542      553      +11

Impacted Files	Coverage Δ	Complexity Δ
...r/route/exception/CanNotFindRouteTableByOwner.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
.../alcor/route/exception/DPMFailedHandleRequest.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...ewei/alcor/route/exception/DestinationInvalid.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...lcor/route/exception/HostRoutesToSubnetIsNull.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...te/exception/OwnerInNeutronRouteTableNotFound.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...ption/PortWebBulkJsonOrPortEntitiesListIsNull.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...route/exception/PortWebJsonOrPortEntityIsNull.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...wei/alcor/route/exception/RouteTableNotUnique.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...ute/service/Impl/VpcRouterToSubnetServiceImpl.java	`14.28% <0.00%> (-5.72%)`	`1.00 <0.00> (ø)`
.../futurewei/alcor/route/utils/RouteManagerUtil.java	`50.72% <0.00%> (-4.84%)`	`9.00 <0.00> (ø)`
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff19c6b...c61733f. Read the comment docs.

...e_manager/src/main/java/com/futurewei/alcor/route/service/Impl/NeutronRouterServiceImpl.java

web/src/main/java/com/futurewei/alcor/web/entity/route/RoutesToNeutronWebRequest.java

...oute_manager/src/main/java/com/futurewei/alcor/route/controller/NeutronRouterController.java

web/src/main/java/com/futurewei/alcor/web/entity/route/UpdateRoutingRuleResponse.java

...oute_manager/src/main/java/com/futurewei/alcor/route/controller/NeutronRouterController.java

services/route_manager/src/main/java/com/futurewei/alcor/route/config/ConstantsConfig.java

services/route_manager/src/main/java/com/futurewei/alcor/route/controller/RouterController.java

...e_manager/src/main/java/com/futurewei/alcor/route/service/Impl/NeutronRouterServiceImpl.java

cj-chung · 2020-11-25T23:33:48Z

...e_manager/src/main/java/com/futurewei/alcor/route/service/Impl/NeutronRouterServiceImpl.java

+        routingRuleExtraInfo.setDestinationType(VpcRouteTarget.LOCAL);
+
+        if (route == null) {
+            internalRoutingRule.setId(route.getId());


Is that possible for route == null, that's mean existing route doesn't exist?
If it's null, how can you getId() from a null?

Yes, route == null is possible I think. And the two lines of code here are indeed in reverse order

cj-chung · 2020-11-25T23:52:17Z

...e_manager/src/main/java/com/futurewei/alcor/route/service/Impl/NeutronRouterServiceImpl.java

+        if (newRouteRequest != null) {
+            internalRoutingRule.setDestination(newRouteRequest.getDestination());
+        } else {
+            internalRoutingRule.setDestination(route.getDestination());


Why not just set these properties based on the operation type?
if (operationType == CREATE), the routing rule's properties use new route's properties.
if (operationType == UPDATE), only nexthop use new route's property, others use existing route's properties.
if (operationType == DELETE), use existing route's properties.

Actually it is same as the logic here, since we have to set RouteEntry route and NewRoutesRequest newRouteRequest in params

...e_manager/src/main/java/com/futurewei/alcor/route/service/Impl/NeutronRouterServiceImpl.java

cj-chung · 2020-11-26T00:05:46Z

...oute_manager/src/main/java/com/futurewei/alcor/route/service/Impl/RouterToPMServiceImpl.java

+        for (String gatewayPortId : gatewayPorts) {
+            String portManagerServiceUrl = portUrl + "/project/" + projectid + "/ports/" + gatewayPortId;
+            PortWebJson portResponse = restTemplate.getForObject(portManagerServiceUrl, PortWebJson.class);


Does PM has bulk create/query by a list of IP addresses?
If there is, we can just make an API call and do a batch process.

Yes, PM has bulk create/query API. But here we query PortEntity by port id, right?

...c/main/java/com/futurewei/alcor/subnet/service/implement/SubnetToPortManagerServiceImpl.java

web/src/main/java/com/futurewei/alcor/web/entity/route/InternalRouterConfiguration.java

xieus

LGTM

xieus · 2020-12-02T01:31:13Z

.../port_manager/src/main/java/com/futurewei/alcor/portmanager/processor/FixedIpsProcessor.java

+            if (portEntity.isGatewayPort()) {
+                continue;
+            }


Add comments.

xieus · 2020-12-02T01:32:33Z

.../port_manager/src/main/java/com/futurewei/alcor/portmanager/processor/FixedIpsProcessor.java

        }

-        getSubnetAndRoute(context, new ArrayList<>(subnetIds));
+        //getSubnetAndRoute(context, new ArrayList<>(subnetIds));


@chenpiaoping Could you pls review this change? I raised a question on Slack channel regarding why we need to reevaluate subnet and route after allocating IP address.

xieus

This PR includes quite a few fundamental changes in Route Manager, Subnet Manager and Web Entities. The new features added by this PR get in an important piece of puzzle for Alcor and enable more comprehensive routing support. 👍

Thank you @kevin-zhonghao and @cj-chung

xieus · 2020-12-02T03:20:58Z

...ces/subnet_manager/src/main/java/com/futurewei/alcor/subnet/controller/SubnetController.java

+//            CompletableFuture<MacStateJson> macFuture = CompletableFuture.supplyAsync(() -> {
+//                try {
+//                    return this.subnetService.allocateMacAddressForGatewayPort(projectId, vpcId, portId);
+//                } catch (Exception e) {
+//                    throw new CompletionException(e);
+//                }
+//            }, ThreadPoolExecutorUtils.SELECT_POOL_EXECUTOR).handle((s, e) -> {
+//                macResponseAtomic.set(s);
+//                if (e != null) {
+//                    throw new CompletionException(e);
+//                }
+//                return s;
+//            });


Please remove and cleanup the commented codes. @kevin-zhonghao

xieus · 2020-12-02T03:21:13Z

...ces/subnet_manager/src/main/java/com/futurewei/alcor/subnet/controller/SubnetController.java

+//            MacState macState = macResponse.getMacState();
+//            if (macState != null) {
+//                inSubnetEntity.setGatewayMacAddress(macState.getMacAddress());
+//            }


xieus · 2020-12-02T03:26:08Z

services/route_manager/src/main/resources/application.properties

 #####Microservice url configuration######
 microservices.subnet.service.url=http://localhost:9002
 microservices.vpc.service.url=http://localhost:9001
+microservices.dpm.service.url=http://localhost:9010


For the new configurations in RM and SM, don't forget to add them in K8s yaml file :-)

xieus · 2020-12-03T08:47:45Z

Item 4 resolves Issue #407.

kevin-zhonghao added 16 commits October 7, 2020 14:49

commit message

24b9b76

Merge branch 'master' of https://github.com/futurewei-cloud/alcor int…

08ec421

…o new_master

Merge branch 'master' of https://github.com/futurewei-cloud/alcor int…

2507b8f

…o new_master

Merge branch 'master' of https://github.com/futurewei-cloud/alcor int…

8cab6bf

…o new_master

Merge branch 'master' of https://github.com/futurewei-cloud/alcor int…

d9d2a58

…o new_master

Merge branch 'master' of https://github.com/futurewei-cloud/alcor int…

daba98e

…o new_master

Merge branch 'master' of https://github.com/futurewei-cloud/alcor int…

b813735

…o new_master

Merge branch 'master' of https://github.com/futurewei-cloud/alcor int…

fd9a984

…o new_master

Merge branch 'master' of https://github.com/futurewei-cloud/alcor int…

e44875a

…o new_master

Merge branch 'master' of https://github.com/futurewei-cloud/alcor int…

98dcf0d

…o new_master

Merge branch 'master' of https://github.com/futurewei-cloud/alcor int…

a9c3a47

…o new_master

Merge branch 'master' of https://github.com/futurewei-cloud/alcor int…

f058bd1

…o new_master

Merge branch 'master' of https://github.com/futurewei-cloud/alcor int…

08d4a6f

…o new_master

update

0e8fa34

update

6e787b5

update

15ff2f5

cj-chung self-requested a review November 12, 2020 15:49

cj-chung assigned kevin-zhonghao Nov 12, 2020

cj-chung requested a review from xieus November 12, 2020 15:51

cj-chung suggested changes Nov 12, 2020

View reviewed changes

services/route_manager/src/main/java/com/futurewei/alcor/route/controller/RouterController.java Outdated Show resolved Hide resolved

cj-chung suggested changes Nov 12, 2020

View reviewed changes

...e_manager/src/main/java/com/futurewei/alcor/route/service/Impl/NeutronRouterServiceImpl.java Show resolved Hide resolved

kevin-zhonghao added 8 commits November 12, 2020 17:27

update

170c425

update

57bb593

update

17cfb31

update

dbe2f9f

Merge branch 'master' of https://github.com/futurewei-cloud/alcor int…

ef0b4fe

…o new_master

Merge branch 'new_master' into feature/sub_level_programming

7aa6973

# Conflicts: # services/data_plane_manager/src/main/java/com/futurewei/alcor/dataplane/service/ovs/DataPlaneServiceImplNew.java

update

6052d2e

update

f5a703f

update

e602e29

xieus added enhancement New feature or request feature feature development labels Nov 21, 2020

xieus added this to the Version 1.0.2020.11.30 milestone Nov 21, 2020

update

590eea3

cj-chung suggested changes Nov 25, 2020

View reviewed changes

cj-chung suggested changes Nov 26, 2020

View reviewed changes

kevin-zhonghao added 3 commits November 30, 2020 14:53

update

0b67d82

update

ff129d1

update

9736111

xieus approved these changes Dec 2, 2020

View reviewed changes

update

c61733f

cj-chung approved these changes Dec 2, 2020

View reviewed changes

xieus approved these changes Dec 2, 2020

View reviewed changes

xieus changed the title ~~[Fundamental] Sub-level programming through Subnet Manager and Route Manager~~ [Fundamental] Sub-level programming through Subnet and Route Mgr Dec 2, 2020

xieus merged commit c6011f3 into futurewei-cloud:master Dec 2, 2020

xieus mentioned this pull request Dec 2, 2020

[Data-Plane Mgr] Add Local Cache Support to DPM #486

Merged

xieus mentioned this pull request Dec 3, 2020

[Fundamental] Unify GW ports and Non-GW ports #407

Closed

[Fundamental] Sub-level programming through Subnet and Route Mgr #468

[Fundamental] Sub-level programming through Subnet and Route Mgr #468

Uh oh!

Conversation

kevin-zhonghao commented Nov 10, 2020 • edited by xieus Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xieus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xieus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xieus commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

kevin-zhonghao commented Nov 10, 2020 •

edited by xieus

Loading

codecov-io commented Nov 19, 2020 •

edited

Loading

xieus commented Dec 3, 2020 •

edited

Loading