Gateway Manager Micorservice #541

songxiaoyan · 2021-01-15T09:20:13Z

Current support CREATE,UPDATE,DELETE(for zeta gateway)，continued development based on the design will follow

merge

# Conflicts: # services/port_manager/src/main/java/com/futurewei/alcor/portmanager/processor/FixedIpsProcessor.java # services/port_manager/src/main/java/com/futurewei/alcor/portmanager/request/RequestManager.java # services/private_ip_manager/src/main/java/com/futurewei/alcor/privateipmanager/controller/IpAddrController.java # services/private_ip_manager/src/main/java/com/futurewei/alcor/privateipmanager/repo/IpAddrRangeRepo.java # services/private_ip_manager/src/main/java/com/futurewei/alcor/privateipmanager/service/implement/IpAddrServiceImpl.java

# Conflicts: # services/port_manager/src/main/java/com/futurewei/alcor/portmanager/request/UpdatePortIpAddressRequest.java

…to gateway_manager_micorservice # Conflicts: # services/gateway_manager/src/main/java/com/futurewei/alcor/gatewaymanager/service/GatewayService.java

codecov-io · 2021-01-15T09:37:04Z

Codecov Report

Merging #541 (9232f36) into master (91e71bd) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #541      +/-   ##
============================================
- Coverage     34.95%   34.90%   -0.05%     
+ Complexity     1189     1183       -6     
============================================
  Files           473      473              
  Lines         11799    11799              
  Branches       1526     1526              
============================================
- Hits           4124     4119       -5     
- Misses         7112     7113       +1     
- Partials        563      567       +4

Impacted Files	Coverage Δ	Complexity Δ
...nager/service/implement/NetworkAclServiceImpl.java	`87.73% <ø> (ø)`	`29.00 <0.00> (ø)`
...ewei/alcor/subnet/controller/SubnetController.java	`54.18% <ø> (ø)`	`13.00 <0.00> (ø)`
...cor/subnet/service/implement/SubnetServiceImp.java	`1.10% <ø> (ø)`	`1.00 <0.00> (ø)`
...com/futurewei/alcor/macmanager/utils/MacUtils.java	`56.52% <0.00%> (-8.70%)`	`7.00% <0.00%> (-1.00%)`
...i/alcor/portmanager/processor/RouterProcessor.java	`79.41% <0.00%> (-5.89%)`	`9.00% <0.00%> (-1.00%)`
...rewei/alcor/portmanager/processor/PortContext.java	`68.91% <0.00%> (-2.71%)`	`31.00% <0.00%> (-2.00%)`
...alcor/elasticipmanager/dao/ElasticIpAllocator.java	`63.81% <0.00%> (+0.27%)`	`48.00% <0.00%> (ø%)`

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 91e71bd...9232f36. Read the comment docs.

cj-chung

@songxiaoyan I cannot find UT codes in this PR. Could you add UT in this PR to verify basic functionalities especially for DB (create/update/delete/query) operations?

cj-chung · 2021-01-18T22:55:39Z

...y_manager/src/main/java/com/futurewei/alcor/gatewaymanager/controller/GatewayController.java

+    public void getGatewaysByVpcId(@PathVariable("projectid") String projectId) {
+


@songxiaoyan The gateway here should be project-scope, not vpc-scope. So, it should be listed all available gateways in the current project.

This is a reserved interface, which will be adjusted according to actual business in the future

cj-chung · 2021-01-18T23:29:33Z

...way_manager/src/main/java/com/futurewei/alcor/gatewaymanager/dao/GWAttachmentRepository.java

+public class GWAttachmentRepository implements ICacheRepository<GWAttachment> {
+


@songxiaoyan do we need cache for GWAttachment?

I think we need to create the Cache Cache of GatewayEntity and GwAttachment in GM's DB for later interface query

cj-chung · 2021-01-18T23:30:07Z

.../gateway_manager/src/main/java/com/futurewei/alcor/gatewaymanager/dao/GatewayRepository.java

+public class GatewayRepository implements ICacheRepository<GatewayEntity> {
+


@songxiaoyan Same here. Do we need cache for Gateway?

I agree to have cache for GM's DB access.

cj-chung · 2021-01-18T23:30:50Z

...teway_manager/src/main/java/com/futurewei/alcor/gatewaymanager/dao/RouteTableRepository.java

+package com.futurewei.alcor.gatewaymanager.dao;
+
+import org.springframework.stereotype.Repository;
+
+@Repository
+public class RouteTableRepository {
+}


@songxiaoyan We probably don't need it.

ok, I can deleted reserved interface if don't need

cj-chung · 2021-01-18T23:36:26Z

.../gateway_manager/src/main/java/com/futurewei/alcor/gatewaymanager/entity/GatewayWebJson.java

+public class GatewayWebJson {
+


If this class is the return for "list all available gateway", it's better rename it "GatewaysWebJson"

cj-chung · 2021-01-18T23:41:15Z

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

+server.port=9015
+


@xieus Does this port for GM? please confirm.

I am fine with 9015 and K8s cluster port of 30015. Keep in mind that 9014 remain available therefore we can keep it for other microservice.

cj-chung · 2021-01-18T23:48:45Z

...gateway_manager/src/main/java/com/futurewei/alcor/gatewaymanager/service/GatewayService.java

+
+        //TODO check whether the project type is zeta by projectId
+
+        // create GatewayInfo entity in the DB


@songxiaoyan Please re-phrase the comment here. We are not creating a GatewayInfo enity in GM's DB.

xieus · 2021-01-15T18:05:31Z

pom.xml

        <module>services/security_group_manager</module>
        <module>services/elastic_ip_manager</module>
        <module>services/quota_manager</module>
 		<module>services/network_acl_manager</module>


Irrelevant comment :-) Can we replace tab with space?

cj-chung · 2021-01-19T01:19:18Z

web/src/main/java/com/futurewei/alcor/web/entity/gateway/GatewayIpJson.java

+public class GatewayIpJson {
+


Should we include "zeta" in the class name? since it's dedicated for zeta gateway only.

cj-chung · 2021-01-19T01:31:40Z

...gateway_manager/src/main/java/com/futurewei/alcor/gatewaymanager/service/GatewayService.java

+                log.info("update GatewayEntity status to READY failed, detail message: {}", e.getMessage());
+                //TODO how to handle this exception? rollback all?


If the execution goes to this far, that's mean DPM already has an gatewayinfo entry for zeta with status = PENDING. In that case, when port creation comes to DPM, DPM should wait for the status become READY for 30s. If the status still PENDING or FAILED, DPM will switch to OVS plugin. So, should we let the updateDPMCacheGateway try some times and if still failed rollback GM's DB and ask Zeta to remove the corresponding gateway resources.

if update DPM's cache status to READY is fail,we need retry some times(3 times?),if still failed,we should rollback all the request resource, is this mean?

Yes. that's what I think.

cj-chung · 2021-01-19T01:32:30Z

...gateway_manager/src/main/java/com/futurewei/alcor/gatewaymanager/service/GatewayService.java

+                //TODO if rollback failed,how we should handle it?
+            }


@songxiaoyan If rollback failed, we should raise an alarm or error.

cj-chung · 2021-01-19T02:00:20Z

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

+import com.futurewei.alcor.web.entity.route.RouteWebJson;
 import com.futurewei.alcor.web.entity.subnet.*;


@songxiaoyan why your code will touch the import in the SubnetController.java?

cj-chung · 2021-01-19T02:03:50Z

services/subnet_manager/src/main/java/com/futurewei/alcor/subnet/service/SubnetService.java

-import com.futurewei.alcor.web.entity.route.InternalRouterInfo;
+import com.futurewei.alcor.web.entity.route.RouteWebJson;
 import com.futurewei.alcor.web.entity.subnet.NewHostRoutes;
 import com.futurewei.alcor.web.entity.subnet.SubnetEntity;


@songxiaoyan Your IDE seems re-arrange the import statements in the code.

Merge code and resolve conflicts, it automatically import, I tried to turn it off.

cj-chung · 2021-01-19T02:05:31Z

web/src/main/java/com/futurewei/alcor/web/entity/gateway/GatewayEntity.java

+    private List<String> tags;
+    private String owner;
+    private Map<String, String> options;
+}


@songxiaoyan Do we need constructor here?

you mean it should be constructor in the GM entity package?

I mean such the code like this:

public GatewayEntity() { } public GatewayEntity(GatewayType type, String status, List<GatewayIp> ips, private List<String> attachments, List<String> routetables, ...) { ... }

oh,I get your mean,I use the set method to assign values, if necessary, I can add the construction method

songxiaoyan · 2021-01-19T02:30:28Z

@songxiaoyan I cannot find UT codes in this PR. Could you add UT in this PR to verify basic functionalities especially for DB (create/update/delete/query) operations?

I'll make it up as soon as possible

chenpiaoping · 2021-01-19T06:18:05Z

...anager/src/main/java/com/futurewei/alcor/gatewaymanager/controller/AttachmentController.java

+     * @param gatewayId
+     * @param gwAttachment
+     */
+    @PostMapping("/project/{projectid}/gateways/{gateway_id}/attachments")


Underscore style is recommended: project_id

chenpiaoping · 2021-01-19T06:31:17Z

...eway_manager/src/main/java/com/futurewei/alcor/gatewaymanager/service/AttachmentService.java

+        if (!gatewayId.equals(attachment.getGatewayId())) {
+            throw new Exception(ExceptionMsgConfig.GATEWAY_NOT_ASSOCIATED_ATTACHMENT.getMsg());
+        }
+        gatewayEntity.getAttachments().removeIf(attachId::equals);


We also need to persist the updated gatewayEntity to db here, linke this: gatewayRepository.addItem(gatewayEntity)

Have updates in the code below

chenpiaoping · 2021-01-19T06:32:41Z

...eway_manager/src/main/java/com/futurewei/alcor/gatewaymanager/service/AttachmentService.java

+        gatewayEntity.getAttachments().removeIf(attachId::equals);
+
+        //delete attachment and update GatewayEntity in the DB
+        gwAttachmentRepository.deleteItem(attachId,gatewayEntity);


Is a transaction required to ensure the consistency of gatewayRepository and gwAttachmentRepository?

There are transactions in the method

chenpiaoping · 2021-01-19T06:34:06Z

...eway_manager/src/main/java/com/futurewei/alcor/gatewaymanager/service/AttachmentService.java

+
+
+    public List<GWAttachment> getAllAttachments(String gatewayId) throws CacheException {
+        Map<String, GWAttachment> attachmentMap = gwAttachmentRepository.findAllItems();


I suggest using findAllItems(Map < String, Object [] > queryParams) instead of findAllItems() here.

chenpiaoping · 2021-01-19T06:43:18Z

...y_manager/src/main/java/com/futurewei/alcor/gatewaymanager/controller/GatewayController.java

+        RestPreconditionsUtil.verifyParameterNotNullorEmpty(projectId);
+        RestPreconditionsUtil.verifyParameterNotNullorEmpty(vpcId);
+        gatewayService.deleteGatewayInfoForZeta(projectId, vpcId);
+        log.info("GatewayInfo deleted success,the resource_id is: {}", vpcId);


Do we need to initialize log with module class? linke this: private static final Logger log = LoggerFactory.getLogger(GatewayController.class);

I used the @slf4j annotation,do don't need to initialize log.

chenpiaoping · 2021-01-19T06:56:11Z

...gateway_manager/src/main/java/com/futurewei/alcor/gatewaymanager/service/GatewayService.java

+        //TODO check whether the project type is zeta by projectId
+
+        GatewayEntity gatewayEntity = null;
+        Map<String, GWAttachment> attachmentsMap = gwAttachmentRepository.findAllItems();


It is recommended to use the method of findAllItems (Map < String, Object [] > queryParams).

chenpiaoping · 2021-01-19T07:01:27Z

...gateway_manager/src/main/java/com/futurewei/alcor/gatewaymanager/service/GatewayService.java

+        Map<String, GWAttachment> attachmentsMap = gwAttachmentRepository.findAllItems();
+        for (GatewayEntity newGatewayEntity : newGatewayInfo.getGatewayEntities()) {
+            for (GWAttachment attachment : attachmentsMap.values()) {
+                if (attachment.getResourceId().equals(newGatewayInfo.getResourceId())) {


Should be attachment.getGatewayId() == newGatewayInfo.getResourceId() ?

String parameters,I think use the equals to compare is better

attachment.getGatewayId(), not attachment.getResourceId() ?

attachment.getGatewayId() is the gatewayId, attachment.getResourceId() and newGatewayInfo.getResourceId() is the vpcId

chenpiaoping · 2021-01-19T07:03:51Z

...y_manager/src/main/java/com/futurewei/alcor/gatewaymanager/controller/GatewayController.java

+     */
+    @PutMapping("/project/{projectid}/gatewayinfo/{resource_id}")
+    @ResponseStatus(HttpStatus.NO_CONTENT)
+    public ResponseId updateGatewayInfoForZeta(@PathVariable("projectid") String projectId, @PathVariable("resource_id") String vpcId, @RequestBody GatewayInfoJson gatewayInfoJson) throws Exception {


Who will call this interface? vpc-manager?

Not sure, now it’s just providing this interface

DPM will call this interface if there is no gatewayinfo entry in the DPM's cache for a vpc.

chenpiaoping · 2021-01-19T07:05:07Z

...gateway_manager/src/main/java/com/futurewei/alcor/gatewaymanager/service/GatewayService.java

+                        throw new Exception(ExceptionMsgConfig.GATEWAY_ENTITY_NOT_FOUND.getMsg());
+                    }
+                    if (gatewayEntity.getType().equals(newGatewayEntity.getType())) {
+                        gatewayEntity.setStatus(newGatewayEntity.getStatus());


need gatewayRepository.addItem(gatewayEntity) here ?

Have updates in the code below

cj-chung · 2021-01-22T17:24:49Z

...gateway_manager/src/main/java/com/futurewei/alcor/gatewaymanager/service/GatewayService.java

+                log.info("update GatewayEntity status to READY failed, detail message: {}", e.getMessage());
+                // rollback GM's DB and ask Zeta to remove the corresponding gateway resources
+            }
+
+            try {
+                rollback(result, gatewayInfo, gatewayEntity, projectId);
+            } catch (Exception e) {
+                log.info("rollback failed, detail message: {}", e.getMessage());
+                // If rollback failed, we should raise an alarm or error.
+            }
+            return null;
+        });
+        return gatewayInfo;
+    }


We can leave rollback to another PR and figure out a better generic way for rollback.

cj-chung

The current code looks good for zeta gateway integration. The rollback part can leave for another PR after we figure out a better generic way to handle the rollback.

songxiaoyan and others added 29 commits December 8, 2020 19:12

Merge pull request #1 from futurewei-cloud/master

134441d

merge

Merge remote-tracking branch 'upstream/master'

a97ec5c

[New API Request] Private IP Manager to support IP address replacement …

cc9781c

…futurewei-cloud#218

[New API Request] Private IP Manager to support IP address replacement …

ae97d93

…futurewei-cloud#218

fix some problems

80691ec

resolve conflict

1fedc02

resolve conflict

30c5956

Merge remote-tracking branch 'upstream/master'

331c46c

Merge branch 'master' into master

41a0012

Support Batch Insertion futurewei-cloud#226

0b7f1fa

Merge remote-tracking branch 'origin/master'

e8e768f

Fix and perfect some problems for futurewei-cloud#218

075d075

Fix and perfect some problems for futurewei-cloud#218

8e26629

Merge remote-tracking branch 'origin/master'

f9c4fda

# Conflicts: # services/port_manager/src/main/java/com/futurewei/alcor/portmanager/request/UpdatePortIpAddressRequest.java

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

7d0c586

Support Batch Insertion futurewei-cloud#226

b0c1b9d

add new module [gateway-manager]

0a94845

Add interface implementation

180e9d4

merge and resolve conflict

85d95a0

Function Development And Improved

cb87d1a

Function Development And Improved

cec541c

Function Development And Improved

c4a3d78

merge and resolve conflict

44479ee

Function Development And Improved

424cd41

Function Development And Improved

d821aed

Merge remote-tracking branch 'origin/gateway_manager_micorservice' in…

87cfb81

…to gateway_manager_micorservice # Conflicts: # services/gateway_manager/src/main/java/com/futurewei/alcor/gatewaymanager/service/GatewayService.java

Merge branch 'master' into gateway_manager_micorservice

82280e0

merge and resolve conflict

9232f36

songxiaoyan requested review from chenpiaoping, cj-chung and xieus January 15, 2021 09:40

xieus assigned songxiaoyan Jan 15, 2021

xieus added the feature feature development label Jan 15, 2021

xieus added this to the Version 1.0.2021.02.28 milestone Jan 15, 2021

cj-chung suggested changes Jan 18, 2021

View reviewed changes

cj-chung reviewed Jan 18, 2021

View reviewed changes

xieus reviewed Jan 18, 2021

View reviewed changes

cj-chung reviewed Jan 19, 2021

View reviewed changes

chenpiaoping reviewed Jan 19, 2021

View reviewed changes

add UT and perfect code

6db4cf9

cj-chung reviewed Jan 22, 2021

View reviewed changes

cj-chung approved these changes Jan 22, 2021

View reviewed changes

xieus merged commit e07d52e into futurewei-cloud:master Jan 23, 2021

		public void getGatewaysByVpcId(@PathVariable("projectid") String projectId) {

		public class GWAttachmentRepository implements ICacheRepository<GWAttachment> {

		public class GatewayRepository implements ICacheRepository<GatewayEntity> {


		//TODO check whether the project type is zeta by projectId

		// create GatewayInfo entity in the DB

		log.info("update GatewayEntity status to READY failed, detail message: {}", e.getMessage());
		//TODO how to handle this exception? rollback all?

		import com.futurewei.alcor.web.entity.route.RouteWebJson;
		import com.futurewei.alcor.web.entity.subnet.*;



		public List<GWAttachment> getAllAttachments(String gatewayId) throws CacheException {
		Map<String, GWAttachment> attachmentMap = gwAttachmentRepository.findAllItems();

		server.port=9015

Gateway Manager Micorservice #541

Gateway Manager Micorservice #541

Uh oh!

Conversation

songxiaoyan commented Jan 15, 2021

Uh oh!

codecov-io commented Jan 15, 2021

Codecov Report

Uh oh!

cj-chung left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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!

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!

songxiaoyan commented Jan 19, 2021

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!

songxiaoyan Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

cj-chung left a comment •

edited

Loading

songxiaoyan Jan 19, 2021 •

edited

Loading