홍길동 / onos

Go to a project
Toggle navigation Toggle navigation pinning
Madan Jampani
Committed by Gerrit Code Review
56dcdce9 1 parent 1f0202cf

Fixes deadlock in dhcp ip assignment logic caused by nesting calls to distributed primitives. 

Change-Id: I25acd37cbf3800ad260c672c99e9f630435f0f88
Inline Side-by-side
Showing 1 changed file with 20 additions and 27 deletions
...@@ -25,13 +25,11 @@ import org.apache.felix.scr.annotations.Service; ...@@ -25,13 +25,11 @@ import org.apache.felix.scr.annotations.Service;
25 import org.onlab.packet.Ip4Address; 25 import org.onlab.packet.Ip4Address;
26 import org.onlab.packet.MacAddress; 26 import org.onlab.packet.MacAddress;
27 import org.onlab.util.KryoNamespace; 27 import org.onlab.util.KryoNamespace;
28 -import org.onlab.util.Tools;
29 import org.onosproject.dhcp.DhcpStore; 28 import org.onosproject.dhcp.DhcpStore;
30 import org.onosproject.dhcp.IpAssignment; 29 import org.onosproject.dhcp.IpAssignment;
31 import org.onosproject.net.HostId; 30 import org.onosproject.net.HostId;
32 import org.onosproject.store.serializers.KryoNamespaces; 31 import org.onosproject.store.serializers.KryoNamespaces;
33 import org.onosproject.store.service.ConsistentMap; 32 import org.onosproject.store.service.ConsistentMap;
34 -import org.onosproject.store.service.ConsistentMapException;
35 import org.onosproject.store.service.DistributedSet; 33 import org.onosproject.store.service.DistributedSet;
36 import org.onosproject.store.service.Serializer; 34 import org.onosproject.store.service.Serializer;
37 import org.onosproject.store.service.StorageService; 35 import org.onosproject.store.service.StorageService;
...@@ -44,7 +42,6 @@ import java.util.Map; ...@@ -44,7 +42,6 @@ import java.util.Map;
44 import java.util.List; 42 import java.util.List;
45 import java.util.HashMap; 43 import java.util.HashMap;
46 import java.util.Objects; 44 import java.util.Objects;
47 -import java.util.concurrent.atomic.AtomicBoolean;
48 45
49 /** 46 /**
50 * Manages the pool of available IP Addresses in the network and 47 * Manages the pool of available IP Addresses in the network and
...@@ -172,15 +169,11 @@ public class DistributedDhcpStore implements DhcpStore { ...@@ -172,15 +169,11 @@ public class DistributedDhcpStore implements DhcpStore {
172 List<Ip4Address> addressList) { 169 List<Ip4Address> addressList) {
173 log.debug("Assign IP Called w/ Ip4Address: {}, HostId: {}", ipAddr.toString(), hostId.mac().toString()); 170 log.debug("Assign IP Called w/ Ip4Address: {}, HostId: {}", ipAddr.toString(), hostId.mac().toString());
174 171
175 - AtomicBoolean assigned = Tools.retryable(() -> { 172 + Versioned<IpAssignment> currentAssignment = allocationMap.get(hostId);
176 - AtomicBoolean result = new AtomicBoolean(false); 173 + IpAssignment newAssignment = null;
177 - allocationMap.compute( 174 + if (currentAssignment == null) {
178 - hostId,
179 - (h, existingAssignment) -> {
180 - IpAssignment assignment = existingAssignment;
181 - if (existingAssignment == null) {
182 if (rangeNotEnforced) { 175 if (rangeNotEnforced) {
183 - assignment = IpAssignment.builder() 176 + newAssignment = IpAssignment.builder()
184 .ipAddress(ipAddr) 177 .ipAddress(ipAddr)
185 .timestamp(new Date()) 178 .timestamp(new Date())
186 .leasePeriod(leaseTime) 179 .leasePeriod(leaseTime)
...@@ -191,21 +184,26 @@ public class DistributedDhcpStore implements DhcpStore { ...@@ -191,21 +184,26 @@ public class DistributedDhcpStore implements DhcpStore {
191 .routerAddress((Ip4Address) addressList.toArray()[2]) 184 .routerAddress((Ip4Address) addressList.toArray()[2])
192 .domainServer((Ip4Address) addressList.toArray()[3]) 185 .domainServer((Ip4Address) addressList.toArray()[3])
193 .build(); 186 .build();
194 - result.set(true); 187 +
195 } else if (freeIPPool.remove(ipAddr)) { 188 } else if (freeIPPool.remove(ipAddr)) {
196 - assignment = IpAssignment.builder() 189 + newAssignment = IpAssignment.builder()
197 .ipAddress(ipAddr) 190 .ipAddress(ipAddr)
198 .timestamp(new Date()) 191 .timestamp(new Date())
199 .leasePeriod(leaseTime) 192 .leasePeriod(leaseTime)
200 .assignmentStatus(IpAssignment.AssignmentStatus.Option_Assigned) 193 .assignmentStatus(IpAssignment.AssignmentStatus.Option_Assigned)
201 .build(); 194 .build();
202 - result.set(true); 195 + } else {
196 + return false;
203 } 197 }
204 - } else if (Objects.equals(existingAssignment.ipAddress(), ipAddr) && 198 + return allocationMap.putIfAbsent(hostId, newAssignment) == null;
199 + // TODO: handle the case where map changed.
200 + } else {
201 + IpAssignment existingAssignment = currentAssignment.value();
202 + if (Objects.equals(existingAssignment.ipAddress(), ipAddr) &&
205 (existingAssignment.rangeNotEnforced() || ipWithinRange(ipAddr))) { 203 (existingAssignment.rangeNotEnforced() || ipWithinRange(ipAddr))) {
206 switch (existingAssignment.assignmentStatus()) { 204 switch (existingAssignment.assignmentStatus()) {
207 case Option_RangeNotEnforced: 205 case Option_RangeNotEnforced:
208 - assignment = IpAssignment.builder() 206 + newAssignment = IpAssignment.builder()
209 .ipAddress(ipAddr) 207 .ipAddress(ipAddr)
210 .timestamp(new Date()) 208 .timestamp(new Date())
211 .leasePeriod(existingAssignment.leasePeriod()) 209 .leasePeriod(existingAssignment.leasePeriod())
...@@ -216,39 +214,34 @@ public class DistributedDhcpStore implements DhcpStore { ...@@ -216,39 +214,34 @@ public class DistributedDhcpStore implements DhcpStore {
216 .routerAddress(existingAssignment.routerAddress()) 214 .routerAddress(existingAssignment.routerAddress())
217 .domainServer(existingAssignment.domainServer()) 215 .domainServer(existingAssignment.domainServer())
218 .build(); 216 .build();
219 - result.set(true);
220 break; 217 break;
221 case Option_Assigned: 218 case Option_Assigned:
222 case Option_Requested: 219 case Option_Requested:
223 - assignment = IpAssignment.builder() 220 + newAssignment = IpAssignment.builder()
224 .ipAddress(ipAddr) 221 .ipAddress(ipAddr)
225 .timestamp(new Date()) 222 .timestamp(new Date())
226 .leasePeriod(leaseTime) 223 .leasePeriod(leaseTime)
227 .assignmentStatus(IpAssignment.AssignmentStatus.Option_Assigned) 224 .assignmentStatus(IpAssignment.AssignmentStatus.Option_Assigned)
228 .build(); 225 .build();
229 - result.set(true);
230 break; 226 break;
231 case Option_Expired: 227 case Option_Expired:
232 if (freeIPPool.remove(ipAddr)) { 228 if (freeIPPool.remove(ipAddr)) {
233 - assignment = IpAssignment.builder() 229 + newAssignment = IpAssignment.builder()
234 .ipAddress(ipAddr) 230 .ipAddress(ipAddr)
235 .timestamp(new Date()) 231 .timestamp(new Date())
236 .leasePeriod(leaseTime) 232 .leasePeriod(leaseTime)
237 .assignmentStatus(IpAssignment.AssignmentStatus.Option_Assigned) 233 .assignmentStatus(IpAssignment.AssignmentStatus.Option_Assigned)
238 .build(); 234 .build();
239 - result.set(true);
240 } 235 }
241 break; 236 break;
242 default: 237 default:
243 break; 238 break;
244 } 239 }
240 + return allocationMap.replace(hostId, currentAssignment.version(), newAssignment);
241 + } else {
242 + return false;
243 + }
245 } 244 }
246 - return assignment;
247 - });
248 - return result;
249 - }, ConsistentMapException.class, MAX_RETRIES, MAX_BACKOFF).get();
250 -
251 - return assigned.get();
252 } 245 }
253 246
254 @Override 247 @Override
......