-
Notifications
You must be signed in to change notification settings - Fork 34
[Microservice API] VPC Mgr and Subnet Mgr Issue Fix #307
Conversation
Docs/design
# 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
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.
This is very timely fix. Thank you @kevin-zhonghao.
I left a few comments. Please take a look.
| return null; | ||
| } | ||
|
|
||
| String[] highIps = highIp.split("\\."); |
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.
Should we add the logic here?
cidrToFirstIpAndLastIp is used somewhere else that actually asks the first and last Ip, right? If so, then this new logic is going to break codes in those places.
I think we should have a new method that handles the cut explicitly.
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.
This method is used for creating Ip Address Range, and Ip Address Range may also need this fix, right? let us talk about this.
...net_manager/src/main/java/com/futurewei/alcor/subnet/service/implement/SubnetServiceImp.java
Outdated
Show resolved
Hide resolved
services/subnet_manager/src/test/java/com/futurewei/alcor/subnet/SubnetControllerTests.java
Show resolved
Hide resolved
...ices/vpc_manager/src/main/java/com/futurewei/alcor/vpcmanager/config/DefaultValueConfig.java
Outdated
Show resolved
Hide resolved
services/vpc_manager/src/main/java/com/futurewei/alcor/vpcmanager/controller/VpcController.java
Outdated
Show resolved
Hide resolved
...net_manager/src/main/java/com/futurewei/alcor/subnet/service/implement/SubnetServiceImp.java
Outdated
Show resolved
Hide resolved
...es/vpc_manager/src/main/java/com/futurewei/alcor/vpcmanager/service/Impl/VpcServiceImpl.java
Outdated
Show resolved
Hide resolved
...es/vpc_manager/src/main/java/com/futurewei/alcor/vpcmanager/service/Impl/VpcServiceImpl.java
Show resolved
Hide resolved
| response.setStatus(NetworkStatusEnum.ACTIVE.getNetworkStatus()); | ||
| } | ||
|
|
||
| // mtu |
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 //TDDO in the code to remind ourselves and community that the current logic will be updated and the plan
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. Signed off with some minor comments.
This PR fixes four identified issues in VPC mgr and Subnet Mgr: