-
Notifications
You must be signed in to change notification settings - Fork 34
Conversation
lib/src/main/java/com/futurewei/alcor/common/entity/HostState.java
Outdated
Show resolved
Hide resolved
...manager/src/main/java/com/futurewei/alcor/portmanager/exception/AllocateIpAddrException.java
Outdated
Show resolved
Hide resolved
return "range1"; | ||
} | ||
|
||
private void allocateIpAddress(PortState portState, Stack<PortStateRollback> rollbacks) 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.
The idea of using a rollback stack to keep track of potential rollback call is good.
The issue, however, is that it might not work in current implementation. If an exception is thrown before rollbacks.push(ipAddressRollback), then the stack doesn't have ipAddressRollback therefore won't be able to trigger rollback during exception handling.
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.
Yes, we can consider it in two cases. First, if an exception occurs before receiving the rest response, there is no need to roll back. Second, if an exception occurs after receiving the rest response, it needs to be rolled back, so we need to ensure that there is no exception between the rest response and rollbacks.push (). In fact, rollbacks.push () is executed after receiving the rest response, so we can assume that there will be no exception between them.
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.
I want to deal with the rollbacks of various operations in a unified way by a common method. This method does not need to judge whether the operation needs to be rolled back, and does not need to care about what to be rolled back, this should be concerned by the operation itself. There may be a better way, do you have any good suggestions?
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.
@chenpiaoping I miss your previous comment in this thread. Unifying the rollback is a good idea but depending on how we are going to do it.
Regarding the first case, let us say Service A => Service B, and the modify (POST or PUT or DELETE) request has been sent but no yet received a response, an exception is received at this time. It is still necessary that Service A send roolback to Service B.
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.
Oh, this is indeed a big problem. Does subnet manager have the same problem?
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.
https://github.com/futurewei-cloud/alcor/pull/180/commits/0a4c96a05592534af52680abe72f9d5d6f364d28,This commit is going to solve this problem.
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.
Thanks let me check it.
} | ||
} | ||
|
||
private PortStateJson tryCreatePortState(String projectId, PortState portState, Stack<PortStateRollback> rollbacks) 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.
The calls into various downstream microservices remain serial and sync.
One recommended way is to async call into multiple microservices in parallel. See the implementation of Subnet Manager for an example.
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.
Okay, I will do that.
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.
Thanks.
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.
Nice work on support Async with a generic AsyncExecutor class. Like 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.
If this mechanism is approved by you, I will move it to the lib directory
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. Go for it :-)
lib/src/main/java/com/futurewei/alcor/common/entity/DeviceOwner.java
Outdated
Show resolved
Hide resolved
services/port_manager/src/main/java/com/futurewei/alcor/portmanager/utils/Ipv4AddrUtil.java
Outdated
Show resolved
Hide resolved
550316e
to
6fefe46
Compare
} | ||
|
||
private void tryCreatePortState(PortState portState, Stack<PortStateRollback> rollbacks) throws Exception { | ||
//Verify VPC ID |
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 should verify Subnet ID, instead of Vpc Id here.
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.
@xieus But port may not have a Subnet ID. In this case, we need to find a suitable subnet for port, and assign an ip address to port from that subnet.
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.
Correct. We have multiple cases to cover, most of the cases would require to verify subnet ID.
https://docs.openstack.org/api-ref/network/v2/?expanded=create-port-detail#ports
(1) If a user specifies a subnet ID in the optional fixed_ips field, then we have to verify that subnet Id
(2) If the user doesn't give any fixed_ips, we handle it as you suggested.
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.
@xieus For the first case, I think we need to get the range ID by Subnet ID (At this point, the Subnet ID will be verified), and then assign an ip address from the ip range. At present, Subnet manager have not provided an interface to obtain the Range ID by the Subnet ID. I discussed this with kevin, and he said he would think about 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.
I think we could make this verification simple by calling the regular Get /subnets/{subnet_id} and the response includes the range ids. @kevin-zhonghao
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.
One discipline is that we follow the same workflows for both case (1) and case (2).
You could either go to SubnetManager to verify subnetId or vpcId, retrieve the range id, and then go to IP manager,
OR, build a mapping in IP manager from VpcId to rangeID as well as SubnetId to RangeId, and only need go to IP manager.
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.
Yes, for the case of going to SubnetManager to retrieve a range id, Since SubnetManager do not know whether there is an available ip address in the range, we may need to go to SubnetManager many times to obtain a available range.
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.
@xieus Ip manager provides an interface that randomly assigns ip addresses according to vpcId. Is this solution okay? I'm ready to develop.
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.
@chenpiaoping Rethinking the issue, we might have to pick the option of " go to SubnetManager to verify subnetId or vpcId, retrieve the range id, and then go to IP manager".
The reason is that PortManager needs to visit both RouteManager and IPManager to retrieve routes and to allocate ip, respectively. SubnetManger is the source of truth to retrieve the routes under the subnet and to get the ip range id under the same subnet.
Therefore, the workflow is like this:
-
Port Manager => SubnetManager
1.1 If Subnet Id is known, send and verify subnet Id, retrieve routes under the subnet (as SubnetManager has the latest routes) and to get range Id
1.2 Otherwise, send vpc id to SubnetManager, SubnetManager picks one subnet, and the rest is the same as Step 1.1 -
Port Manager => Ip Manager, send the range id from Step 1, and allocate ip from the given range.
Currently, Step 1.1 needs a new API from SubnetManager. @kevin-zhonghao and I will work to create one in a parallel PR (#181).
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.
@xieus I have submitted the code, but it is a little different from what you described. Can you review it?
//Verify security group | ||
|
||
//If port binds host, to send it's information to host | ||
|
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.
Need two more calls:
(1) One into RouteManager to get the routing rules of the subnet that this port belong to. The list of routing rules is the superset of Neutron router and could include SNAT/DNAT rules.
For now, I would recommend you to look into Subnet implementation regarding how to call RouteManager.
Line 120 in 2ae412f
@Override |
(2) The other into NodeManager which is under development.
PR: https://github.com/futurewei-cloud/alcor/pull/185/files
The API you could be interested in is GET /nodes/{nodeid}.
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 I get route info from RouteManager or from SubnetManager? I found that RouteManager only has the interface to get route info by Vpc ID(/ vpcs/ {vpcId} / routes/ {routeId}), and there is no interface to get route info by Subnet ID. However, SubnetManager will store route info, which will be stored in RouteManager, VpcManager. Will it take up too much memory?
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.
For the interaction with node manager, Can i come back to do it after the development of node manager finish?
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.
@chenpiaoping Regarding NodeManager, PR #185 is under review and should be merged pretty soon so I would recommend to write the codes based on the current APIs. You could comment out for now and test only when NodeManager is merged. Should be soon.
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.
Regarding routing rules, I suggest to still go to RouteManager as that is the ultimate source of truth for routing information.
I think your point is very good. We possibly need to add a new API in RouteManager to allow query by SubnetId. Add @kevin-zhonghao for awareness.
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.
Regarding memory usage of storing routes in SubnetManager and VpcManager that might occupy too much memory, we haven't seen that case yet so storing routes in the upstream microservices could cut the latency for GET.
If you find such a case, let me know and we could quickly revisit this design.
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.
okay, xieus.
7edcb4e
to
8c4831b
Compare
Question: the latest MockServer will auto generate a new Ingite sub-directory under private_ip_manager after mvn test. What is it used for? We likely need to add the whole sub-directory to .gitignore. |
After turning on ip manager UTs (commit #5c598ab), some of UTs failed as it tried to connect to Redis. https://jenkins.alkaidcloud.io/job/alcor-controller-pr-test/298/console
[�[1;31mERROR�[m] �[1;31mTests �[0;1mrun: �[0;1m14�[m, Failures: 0, �[1;31mErrors: �[0;1;31m13�[m, Skipped: 0, Time elapsed: 3.438 s�[1;31m <<< FAILURE!�[m - in com.futurewei.alcor.privateipmanager.controller.�[1mIpAddrControllerTest�[m |
services/private_ip_manager/pom.xml
I have deleted the maven-surefire-plugin package in this commit: 8c4831b, do we need to add it back again? |
Yes, I annotated @ FixMethodOrder (MethodSorters.NAME_ASCENDING), and all the UT's name is prefixed with TESTxx_, which ensures the order they run. |
Is the working directory that contains information that ignite nodes need, let me add it to .gitignore. |
I tried to add maven-surefire-plugin back in the pom.xml file, it's ok in my IDEA env. |
dd5d2ea
to
b3f39f0
Compare
885c7ec
to
0a23e9a
Compare
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.
A few comments on the restWrap and rest classes. I like those classes in general.
import org.springframework.stereotype.Component; | ||
|
||
@Component | ||
public class BeanUtil implements ApplicationContextAware { |
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 would fit into the scope of AlcorLib. Let us move it there.
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.
yes, I thought too, but it didn't work.
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.
Is this because of "@component" that you will need to launch this bean within the scope of AlcorPortManager?
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.
Oh, maybe, Let me have a try.
web/src/main/java/com/futurewei/alcor/web/rest/AbstractRest.java
Outdated
Show resolved
Hide resolved
...s/port_manager/src/main/java/com/futurewei/alcor/portmanager/restwrap/IpAddressRestWrap.java
Outdated
Show resolved
Hide resolved
|
||
public List<IpAddrRequest> verifyIpAddresses(Object args) throws Exception { | ||
List<IpAddrRequest> ipAddrRequests = new ArrayList<>(); | ||
List<PortState.FixedIp> fixedIps = (List<PortState.FixedIp>)args; |
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 general, direct type casting is not recommended for downcasting.
Could you try cast() and isInstance() methods for safer downcasting?
A reference: https://www.baeldung.com/java-type-casting
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.
Don't worry, an exception will be thrown if there is a type conversion error, which is exactly what I expected when the type conversion failed.
List<IpAddrRequest> ipAddrRequests = new ArrayList<>(); | ||
PortState portState = (PortState)args; | ||
|
||
IpAddrRequest result = ipAddressRest.allocateIpAddress(IpVersion.IPV4, |
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 about Ipv6? Could we use sth like "portState.IpVersion" instead of fixing with IPv4?
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.
There is no ipVersion field in portState, do you think only ipv4 address is assigned to port by default, or both ipv4 and ipv6 addresses are assigned?
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.
I see. Ipv4v6 is something we need to support as well. Maybe put it in your backlog and do it when you get a chance.
if (portState.getFixedIps() == null) { | ||
executor.runAsync(ipAddressRestWrap::allocateIpAddress, portState); | ||
} else { | ||
executor.runAsync(ipAddressRestWrap::verifyIpAddresses, portState.getFixedIps()); |
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.
allocateIpAddress => allocateRandomIpAddress
verifyIpAddresses=> allocateFixedIpAddress
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.
sounds reasonable.
if (portState.getMacAddress() == null) { | ||
executor.runAsync(macAddressRestWrap::allocateMacAddress, portState); | ||
} else { | ||
executor.runAsync(macAddressRestWrap::verifyMacAddress, portState); |
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.
allocateMacAddress => allocateRandomMacAddress
verifyMacAddress => allocateFixedMacAddress
|
||
public MacStateJson verifyMacAddress(Object args) { | ||
//FIXME: Not support yet | ||
return null; |
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.
API (2) in Mac manager actually could set a fixed mac address in the rest body (the impl might not be there but at least the interface has that).
Ref: https://github.com/futurewei-cloud/alcor/blob/master/docs/design/mac_manager.adoc
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.
that would be nice.
portRepository.addItem(portState); | ||
} catch (Exception e) { | ||
/** | ||
When an exception occurs, we need to roll back all asynchronous operations, |
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.
Hmm, this is really tricky. We will need a good Timeout story to upper bound the rollback.
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.
One last comment: We need to add comments for each and every important method in Controller, Service, Service Impl and Repo.
RestWrap and Rollback is not straightforward therefore would require some level of comments as well.
|
||
} | ||
|
||
public PortStateJson updatePortState(String projectId, String portId, PortStateJson portStateJson) 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.
Looks like we'll need a bit more time for updatePortState. It is fine and let us do it in next PR.
import java.util.List; | ||
|
||
@Service | ||
public interface PortService { |
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 the bulk create API.
...ort_manager/src/test/java/com/futurewei/alcor/portmanager/controller/PortControllerTest.java
Show resolved
Hide resolved
...s/port_manager/src/main/java/com/futurewei/alcor/portmanager/restwrap/IpAddressRestWrap.java
Outdated
Show resolved
Hide resolved
5b84398
to
aa8c0f9
Compare
...es/port_manager/src/main/java/com/futurewei/alcor/portmanager/controller/PortController.java
Show resolved
Hide resolved
* Allocate a random ipv4 and ipv6 address from ip manager service | ||
* @param args PortState | ||
* @return A list of IpAddrRequest | ||
* @throws Exception Rest request 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.
@chenpiaoping, my previous comment may not be 100% clear. I meant that IPv4 only and Ipv4v6 should both be supported, with IPv4 only the default option.
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.
I see, should i revert 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.
Those are good change. Maybe by default we only trigger Ipv4 allocation, but add one more flag to trigger additional Ipv6 allocation.
Neutron may or may not have this field. Let me quickly check.
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.
I think if there is ipv6 subnet in the vpc, the ipv6 address will be assigned.
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.
My current implementation may not be very reasonable.
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 is totally fine, we do the development and design in an iterative/agile way: When we find it something not right, we change it quickly.
My current thinking is: This should be a consistent behavior on VPC level. If a VPC allows Ipv6, every of its Subnets will have Ipv6 ranges therefore trigger Ipv4v6 allocation here.
In our Subnet Object, there is a Ipv4RangeId and a Ipv6RangeId. Therefore our logic could be that if the Ipv6RangeId is not null, we allocate Ipv6 here. Does this make sense?
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.
Agree with you, but i think i need to make some changes in ip manager.
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's time for me to get off work, will do it tomorrow.
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.
I see. Might not be a small change considering here requires a full set of UTs.
Let us comment out Ipv6 allocation part in this PR, and continue to improve it in next PR.
@@ -40,11 +40,12 @@ public void releaseMacAddress(String macAddress) throws Exception { | |||
restTemplate.delete(url); | |||
} | |||
|
|||
public MacStateJson allocateMacAddress(String projectId, String vpcId, String portId) throws Exception { | |||
public MacStateJson allocateMacAddress(String projectId, String vpcId, String portId, String mac) 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.
is this a macAddress or macId?
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.
macAddress
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.
Let me rename 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.
No worries. I made changes to expedite the merge.
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.
Approved! This PR provides a solid foundation for port manager. We could continue to polish it for a few pending items, like ip allocation, bulk port creation, and update port.
@chenpiaoping UTs are partially turned on with six commented out for now. Please take a look. |
This PR contains the following features: