-
Notifications
You must be signed in to change notification settings - Fork 34
[Alcor Integration]Port CURD - Bug Fix and New Route Manager APIs #253
Conversation
… integration_fix
… integration_fix
xieus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevin-zhonghao Left a few comments. Please take a look.
| - key: application.properties | ||
| path: application.properties | ||
| containers: | ||
| - image: zhonghaolyu/repo:sgKube1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to add one more step of pushing container image to the "official" container registry in the CI workflow (for every PR merge) so that this yaml file could pull the latest "official" image.
Create an issue ##270 for tracking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure ~ when CI is finished, I'll change it
| - image: zhonghaolyu/repo:sgKube1 | ||
| #- image: fwnetworking/controller:node_manager-v0.3.0 | ||
| name: sgmanager-web | ||
| imagePullPolicy: IfNotPresent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How big is the image? We could want to change the policy to alwaysPull.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About several hundreds MB
| labels: | ||
| name: sgmanager-service | ||
| spec: | ||
| type: NodePort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ClusterIP or NodePort? Let us discuss.
services/port_manager/src/main/resources/application.properties
Outdated
Show resolved
Hide resolved
| method = GET, | ||
| value = {"/vpcs/{vpcId}/routes/{routeId}"}) | ||
| public RouteWebJson getRuleByVpcId(@PathVariable String vpcId, @PathVariable String routeId) throws Exception { | ||
| value = {"routes/vpcs/{vpcId}/get"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevin-zhonghao We may need to change the API doc if any.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we didn't have route API doc right now, I will add it after
...r/src/main/java/com/futurewei/alcor/route/service/Impl/RouteWithSubnetMapperServiceImpl.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/com/futurewei/alcor/route/service/Impl/RouteWithSubnetMapperServiceImpl.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public void addMapperByRouteEntity(String subnetId, RouteEntity routeEntity) throws DatabasePersistenceException { | ||
| try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Route Manager ServiceImpl and repo classes, I don't find usage of a transaction.
Please find the reference implementation:
alcor/services/port_manager/src/main/java/com/futurewei/alcor/portmanager/repo/PortRepository.java
Line 66 in 7c6f138
| public void createPortAndNeighbor(PortEntity portEntity, NeighborInfo neighborInfo) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix it ~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a ticket for tracking #274
...ager/src/main/java/com/futurewei/alcor/route/service/Impl/RouteWithVpcMapperServiceImpl.java
Show resolved
Hide resolved
# Conflicts: # README.md # docs/modules/ROOT/pages/comm_protocol/fast_path.adoc # docs/modules/ROOT/pages/comm_protocol/rescue_path.adoc # docs/modules/ROOT/pages/controller.adoc # docs/modules/ROOT/pages/db_services/data_store.adoc # docs/modules/ROOT/pages/deploy_related/integration_nova.adoc # docs/modules/ROOT/pages/high_level/system_flow.adoc # docs/modules/ROOT/pages/mgmt_services/private_ip_manager.adoc # docs/modules/ROOT/pages/mgmt_services/security_group_manager.adoc # docs/modules/ROOT/pages/mgmt_services/virtual_mac_manager.adoc # docs/modules/ROOT/pages/mgmt_services/vpc_manager.adoc # docs/modules/ROOT/pages/sys_monitoring/monitoring.adoc
…into integration sync up the commit.
# Conflicts: # README.md # docs/modules/ROOT/pages/comm_protocol/fast_path.adoc # docs/modules/ROOT/pages/comm_protocol/rescue_path.adoc # docs/modules/ROOT/pages/controller.adoc # docs/modules/ROOT/pages/db_services/data_store.adoc # docs/modules/ROOT/pages/deploy_related/deployment.adoc # docs/modules/ROOT/pages/deploy_related/integration_nova.adoc # docs/modules/ROOT/pages/high_level/system_flow.adoc # docs/modules/ROOT/pages/mgmt_services/private_ip_manager.adoc # docs/modules/ROOT/pages/mgmt_services/security_group_manager.adoc # docs/modules/ROOT/pages/mgmt_services/virtual_mac_manager.adoc # docs/modules/ROOT/pages/mgmt_services/vpc_manager.adoc # docs/modules/ROOT/pages/sys_monitoring/monitoring.adoc
…into integration
xieus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
| if (!subnetUniqueIds.contains(subnetId)) { | ||
| Long tunnelId = subnetEntity.getTenantId() != null ? Long.valueOf(subnetEntity.getTenantId()) : null; | ||
| // FIXME :subnetEntity.getVpcId().hashCode() need to be changed to segmentId | ||
| Long tunnelId = getTunnelId(subnetEntity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a new issue for tracking #275
|
|
||
| public interface RouteWithSubnetMapperService { | ||
|
|
||
| public SubnetToRouteMapper getBySubnetId (String subnetId) throws ResourceNotFoundException, ResourcePersistenceException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comments if possible.
| @JsonProperty("route_ids") | ||
| private List<String> routeIds; | ||
|
|
||
| @CreatedDate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think of moving these two to a common resource class.
| RouteWebJson routeWebJson = restTemplate.getForObject(url, RouteWebJson.class); | ||
| if (routeWebJson == null) { | ||
| public RoutesWebJson getRouteBySubnetId(String subnetId) throws Exception { | ||
| String url = routeManagerUrl + "/subnets/" + subnetId + "/get"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Url usually doesn't come with /get
This PR proposes the following changes: