[Microservice] Route Manager Refactor to Support Neutron Routers #361

kevin-zhonghao · 2020-08-21T23:17:11Z

Implement Neutron routers in route manager:

CRUD APIs
Add/remove interface to Neutron routers
Add/remove routes to Neutron routers
Get connected subnets

…thController into docs/design

xieus

@kevin-zhonghao Some early feedback, mostly on AlcorWeb change.

xieus · 2020-08-24T22:05:24Z

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

+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.futurewei.alcor.web.entity.port.PortEntity;
+
+public class FixedIp {


FixedIp is also defined in PortEntity

alcor/web/src/main/java/com/futurewei/alcor/web/entity/port/PortEntity.java

Line 251 in d823316

public static class FixedIp {

Could you check if we could reuse it?

Yes ~ I see the FixedIp is the internal class in PortEntity, could we move the entity to the general directory? Because some other class also need it

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

xieus · 2020-08-24T22:08:28Z

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

+import org.jetbrains.annotations.NotNull;
+
+@Data
+public class RouteEntry extends CustomerResource {


I found there are two files, both named RouteEntity in this PR. Very unusual. Could you pls double check?

One is the RouteEntity, it's used for our previous version of route manager; the other one is RouteEntry, it's used for our new version.

Got it. Looked so similar, time to upgrade my glass :-)

@kevin-zhonghao @xieus Should we remove the previous version of APIs from this PR if we don't need it?

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

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

xieus · 2020-08-24T22:14:59Z

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


-public class RouterState {
-    private String tenantId;
+import lombok.Data;


@kevin-zhonghao Forgot to mention this to you earlier. Going forward, let us not use lombok.Data, instead, let us write setter and getter explicitly.

NP ~ I will remember that and use setter and getter directorly, but in this PR, most of web class used @DaTa. Should I fix them all?

@kevin-zhonghao It is okay. Plan to change it when you get bored :-)

xieus · 2020-08-24T22:19:03Z

web/src/main/java/com/futurewei/alcor/web/entity/vpc/VpcEntity.java

    @JsonProperty("routes")
    private List<RouteEntity> routeEntities;

+    @JsonProperty("router")


@kevin-zhonghao @cj-chung

For VPCEntity, could use extract the common fields of VPCEntity and NetworkEntity (not existed yet) to a common class? VPCEntity and NetworkEntity could inherit this common class. This way, we could clearly see the differences in two different scenarios.

Not high priority for this PR. We could discuss and plan in next PR.

That's a good idea ~ we could discuss later

@kevin-zhonghao @xieus Agree, this is a fundamental change, we should wait until next PR.

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

services/vpc_manager/src/main/java/com/futurewei/alcor/vpcmanager/service/VpcService.java

xieus

@kevin-zhonghao Completed review of NeutronRouterController. The web layer is well written for db access. As discussed, next step is to close the contract with PM.

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

xieus · 2020-08-26T05:50:24Z

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

+//        RouterExtraAttribute routerExtraAttribute = null;
+
+        try {
+            RestPreconditionsUtil.verifyParameterNotNullorEmpty(routerid);


@kevin-zhonghao maybe it is time to start thinking about a generic validator for all microservices.

Yes I think so

we could put all validator of microservices into one Util class

Correct. @kevin-zhonghao

That is one way - putting all the util methods together in a utii class, and those methods are set as static methods.

Alternative is to use OOD for validator - a base validator class defining the validate() interface, and a few more child classes inheriting from the base and implement the actual validate method.

Each child class is responsible of one specific class that it wants to validate.

Yes ~ could we do this in next PR?

@kevin-zhonghao This is not urgent. Instead, we will need design the validator for the ground up and apply it to the web layers of all microservices (of course, starting with small, e.g., from vpc mgr).

Tracking the item in an issue #374 and assign to you and me :-)

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

xieus · 2020-08-26T06:05:50Z

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

+            neutronRouterWebRequestObject = resource.getRouter();
+            String id = neutronRouterWebRequestObject.getId();
+
+            if (id == null || StringUtils.isEmpty(id)) {


Could we consolidate the validation into one?

we may do this in next PR

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

xieus · 2020-08-26T06:10:52Z

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

+            RestPreconditionsUtil.verifyParameterNotNullorEmpty(projectid);
+
+            // check resource
+            if (!RouteManagerUtil.checkNeutronRouterWebResourceIsValid(resource)) {


what if the resource.getid() != routerid?

User are not allowed to change the router_id

xieus

Some more minor comments.

web/src/main/java/com/futurewei/alcor/web/entity/subnet/SubnetEntity.java

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

xieus · 2020-08-26T06:25:10Z

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

+import org.jetbrains.annotations.NotNull;
+
+@Data
+public class RouteEntry extends CustomerResource {


Got it. Looked so similar, time to upgrade my glass :-)

services/vpc_manager/src/main/java/com/futurewei/alcor/vpcmanager/service/VpcService.java

services/vpc_manager/src/main/java/com/futurewei/alcor/vpcmanager/controller/VpcController.java

…o new_master

# Conflicts: # services/vpc_manager/src/main/java/com/futurewei/alcor/vpcmanager/controller/VpcController.java

xieus

@kevin-zhonghao A few more comments. Major comment is about four empty controllers, do we plan to implement something there?

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

xieus · 2020-09-02T16:56:03Z

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

+//        RouterExtraAttribute routerExtraAttribute = null;
+
+        try {
+            RestPreconditionsUtil.verifyParameterNotNullorEmpty(routerid);


Correct. @kevin-zhonghao

That is one way - putting all the util methods together in a utii class, and those methods are set as static methods.

Alternative is to use OOD for validator - a base validator class defining the validate() interface, and a few more child classes inheriting from the base and implement the actual validate method.

Each child class is responsible of one specific class that it wants to validate.

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

...s/route_manager/src/main/java/com/futurewei/alcor/route/controller/RouteEntryController.java

cj-chung · 2020-09-02T21:21:51Z

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

+        if (routerExtraAttribute != null) {
+            BeanUtils.copyProperties(routerExtraAttribute, neutronRouterWebRequestObject);
+            neutronRouterWebRequestObject.setId(routerId);
+        }


Why you need to set routerID when routerExtraAttribute != null?
The routerID should be set when the router has or has not extra attribute.

The ID of NeutronRouterWebRequestObject should be router_id, but I used the BeanUtils.copyProperties method twice. The first time I used it, NeutronRouterWebRequestObject_id = router_id; But the second time I used it, route_extra_attribute_id would overwrite the previous value, so I had to change it set back

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

cj-chung · 2020-09-02T22:29:30Z

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

+import org.jetbrains.annotations.NotNull;
+
+@Data
+public class RouteEntry extends CustomerResource {


@kevin-zhonghao @xieus Should we remove the previous version of APIs from this PR if we don't need it?

cj-chung · 2020-09-02T22:32:58Z

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

+@Data
+public class Router extends CustomerResource {
+
+    @JsonProperty("routetable")


@kevin-zhonghao There are multiple routetables for a router. It's better rename 'routetable' to 'routetables'

Actually our vpc and subnet still use the previous APIs of route manager, is that correct?

'routetable' to 'routetables' issue has been fixed

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

cj-chung · 2020-09-02T22:45:14Z

web/src/main/java/com/futurewei/alcor/web/entity/vpc/VpcEntity.java

    @JsonProperty("routes")
    private List<RouteEntity> routeEntities;

+    @JsonProperty("router")


@kevin-zhonghao @xieus Agree, this is a fundamental change, we should wait until next PR.

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

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

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

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

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

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

cj-chung

LGTM

xieus

Look great! Thanks @kevin-zhonghao and @cj-chung

xieus · 2020-09-04T22:22:42Z

services/route_manager/src/test/java/com/futurewei/alcor/route/NeutronRouterTests.java

+    }
+
+    @Test
+    public void getConnectedSubnets_pass () throws Exception {


Really like the comprehensive set of tests in this file 💯

xieus and others added 30 commits September 30, 2019 18:24

Update design documents and table of content

d745b47

Update README.md

1fc253e

Add cloud-native key word

9b0efb6

Merge branch 'docs/design' of https://github.com/futurewei-cloud/Alio…

db0a699

…thController into docs/design

Start docs for comm and controller

424dd5b

Add design sketch

b41edb9

Design spec round 1

3e54c57

Rename fast path file and start feature comparison

d7d8ad9

Update rescue path doc [DRAFT]

4c6588e

upload rescue_path.GIF

cceaa68

update rescue path needed change table

9785656

rescue_path.GIF

3e2b63e

rescue_path.adoc

f8fc472

Design spec round 2

2480dd1

Monitoring design outline

68eda0b

Update comm docs

c1c9cc7

Update data storage design

79e91fe

Merge branch 'docs/design' of https://github.com/futurewei-cloud/Alio…

ca69892

…thController into docs/design

Update data store

8b154ab

Complete review of existing dbs

4561d1e

Update review table

ca4610e

Adjust table column width

24b4a36

Update feature comparsion table

45774da

Finalize review section in data store

18e500c

Add design diagram

cc99d14

Add a design diagram

a9c8232

Add files via upload

2823921

fix "design" session level

47b7e45

Update monitoring.adoc

8c9c004

Update monitoring.adoc

4180450

xieus requested review from cj-chung and xieus August 24, 2020 17:58

xieus self-assigned this Aug 24, 2020

xieus added enhancement New feature or request feature feature development labels Aug 24, 2020

xieus added this to the Version 0.8.2020.08.30 milestone Aug 24, 2020

xieus changed the title ~~Feature/refactor route manager~~ [Microservice] Route Manager Refactor to Support Neutron Routers Aug 24, 2020

xieus suggested changes Aug 24, 2020

View reviewed changes

xieus reviewed Aug 24, 2020

View reviewed changes

services/vpc_manager/src/main/java/com/futurewei/alcor/vpcmanager/service/VpcService.java Outdated Show resolved Hide resolved

kevin-zhonghao added 3 commits August 25, 2020 14:41

update

22c32ce

update

3159e8e

update

b387260

xieus reviewed Aug 26, 2020

View reviewed changes

kevin-zhonghao added 6 commits August 27, 2020 17:16

update

3783756

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

88b02b7

…o new_master

Merge branch 'new_master' into feature/refactor_route_manager

b4c259f

# Conflicts: # services/vpc_manager/src/main/java/com/futurewei/alcor/vpcmanager/controller/VpcController.java

update UTs and fix comments

0cedccb

new API

a909f88

update

06c30f4

xieus reviewed Sep 2, 2020

View reviewed changes

cj-chung reviewed Sep 2, 2020

View reviewed changes

web/src/main/java/com/futurewei/alcor/web/entity/route/NeutronRouterWebRequestObject.java Show resolved Hide resolved

cj-chung reviewed Sep 3, 2020

View reviewed changes

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

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

cj-chung reviewed Sep 3, 2020

View reviewed changes

fix comments

1c59b86

cj-chung approved these changes Sep 4, 2020

View reviewed changes

xieus approved these changes Sep 4, 2020

View reviewed changes

xieus merged commit 1a478a5 into futurewei-cloud:master Sep 4, 2020

[Microservice] Route Manager Refactor to Support Neutron Routers #361

[Microservice] Route Manager Refactor to Support Neutron Routers #361

Uh oh!

Conversation

kevin-zhonghao commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

xieus left a comment

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!

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xieus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevin-zhonghao commented Aug 21, 2020 •

edited

Loading