Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix getNextAvailableAddress to use local maximums instead of just last a... #168

Merged
merged 6 commits into from
Jun 23, 2014
Merged

fix getNextAvailableAddress to use local maximums instead of just last a... #168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1ec999d
fix getNextAvailableAddress to use local maximums instead of just las…
byxorna Jun 20, 2014
8a5408e
use sorted set instead
byxorna Jun 21, 2014
eee92c3
use headOption and if instead
byxorna Jun 21, 2014
5b878c0
use more elegant flatMap and for comprehension
byxorna Jun 21, 2014
3be2e75
use lazy streams
byxorna Jun 23, 2014
f706731
make the function actually work now
byxorna Jun 23, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/models/IpAddresses.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ object IpAddresses extends IpAddressStorage[IpAddresses] with IpAddressCacheMana
case failed =>
logger.info("Address cache dirty. Failed cache lookup so using DB")
populateCacheIfNeeded(scope, true)
getCurrentMaxAddress(calc.minAddressAsLong, calc.maxAddressAsLong)
getCurrentLowestLocalMaxAddress(calc.minAddressAsLong, calc.maxAddressAsLong)
}
val address: Long = calc.nextAvailableAsLong(currentMax)
(gateway, address, netmask)
Expand Down
45 changes: 36 additions & 9 deletions app/models/shared/IpAddressable.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import util.{IpAddress, IpAddressCalc}
import org.squeryl.Schema
import play.api.Logger
import java.sql.SQLException
import collection.immutable.SortedSet

trait IpAddressable extends ValidatedEntity[Long] {

Expand All @@ -27,7 +28,7 @@ trait IpAddressable extends ValidatedEntity[Long] {
def getId(): Long = id
def getAssetId(): Long = asset_id
def getAsset(): Asset = Asset.findById(getAssetId()).get

}

trait IpAddressStorage[T <: IpAddressable] extends Schema with AnormAdapter[T] {
Expand Down Expand Up @@ -75,15 +76,15 @@ trait IpAddressStorage[T <: IpAddressable] extends Schema with AnormAdapter[T] {
}

def getNextAvailableAddress(overrideStart: Option[String] = None)(implicit scope: Option[String]): Tuple3[Long,Long,Long] = {
//this is used by ip allocation without pools (i.e. IPMI)
val network = getNetwork
val startAt = overrideStart.orElse(getStartAddress)
val calc = IpAddressCalc(network, startAt)
val gateway: Long = getGateway().getOrElse(calc.minAddressAsLong)
val netmask: Long = calc.netmaskAsLong
val currentMax: Option[Long] = getCurrentMaxAddress(
calc.minAddressAsLong, calc.maxAddressAsLong
)
val address: Long = calc.nextAvailableAsLong(currentMax)
// look for the local maximum address (i.e. the last used address in a continuous sequence from startAddress)
val localMax: Option[Long] = getCurrentLowestLocalMaxAddress(calc.minAddressAsLong, calc.maxAddressAsLong)
val address: Long = calc.nextAvailableAsLong(localMax)
(gateway, address, netmask)
}

Expand All @@ -95,7 +96,7 @@ trait IpAddressStorage[T <: IpAddressable] extends Schema with AnormAdapter[T] {
var i = 0
do {
try {
res = Some(f(i))
res = Some(f(i))
} catch {
case e: SQLException =>
logger.info("createAddressWithRetry attempt %d: %s".format((i + 1), e.getMessage))
Expand All @@ -115,13 +116,39 @@ trait IpAddressStorage[T <: IpAddressable] extends Schema with AnormAdapter[T] {
)
}

protected def getCurrentMaxAddress(minAddress: Long, maxAddress: Long)(implicit scope: Option[String]): Option[Long] = inTransaction {
from(tableDef)(t =>
/*
* returns the lowest last used address in a continuous sequence from minAddress
* If maxAddress is the same as the lowest last used address, we return None which signifies
* that you should start allocating addresses from the start of the range.
*
* Ex: For a range 0L..20L, used addresses List(1,2,3,4,5,13,14,15,19,20), the result will be Some(5)
* For a range 0L..20L, used addresses List(5,6,7,8,19,20), the result will be Some(8)
* For a range 0L..20L, used addresses List(17,18,19,20), the result will be None (allocate from beginning)
*/
protected def getCurrentLowestLocalMaxAddress(minAddress: Long, maxAddress: Long)(implicit scope: Option[String]): Option[Long] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably the way to write this entire function

scala> val x:Seq[Long]=List(1,2,3,8,9)
x: Seq[Long] = List(1, 2, 3, 8, 9)

scala> val first = for { i <- 0 to x.size -2
     |   curr = x(i)
     |   next = x(i+1)
     |   if (next > curr + 1)
     | } yield  curr
first: scala.collection.immutable.IndexedSeq[Long] = Vector(3)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I use a SortedSet, this doesnt work, as apply() on a SortedSet is a boolean (contains). I suppose though there isnt a need for a sorted set though, so Ill use your solution with normal lists that are sorted by the DB.

O(n) is better than O(nlog(n)) :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this comprehension doesnt cover the case where sortedAddresses.size<=2 (meaning the range is 0..0) when using Stream.range, because it is not inclusive. Interestingly:

scala> Range(0,0)
res4: scala.collection.immutable.Range = Range()
scala> 0 to 0
res8: scala.collection.immutable.Range.Inclusive = Range(0)
scala> Stream.range(0,0)
res23: scala.collection.immutable.Stream[Int] = Stream()

I havent been able to find out how to make a Stream range be inclusive. Any ideas? This current iteration of code eats it after allocating the first address, because it thinks there are no localMaxima still :(

val sortedAddresses = from(tableDef)(t =>
where(
(t.address gte minAddress) and
(t.address lte maxAddress)
)
compute(max(t.address))
select(t.address)
orderBy(t.address asc)
).toSeq

lazy val localMaximaAddresses = for {
i <- Range(0, sortedAddresses.size-1).inclusive.toStream
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does that have to be size - 2? either that or remove the inclusive

scala> x
Array[Int] = Array(1, 2, 3)

scala> Range(0, x.size - 1)
scala.collection.immutable.Range = Range(0, 1)

scala> Range(0, x.size - 1).inclusive
scala.collection.immutable.Range = Range(0, 1, 2)

scala> x.size
Int = 3

scala> x(2)
Int = 3

scala> x(2+1)
java.lang.ArrayIndexOutOfBoundsException: 3

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoke with gabe. lgtm

curr = sortedAddresses(i)
next = sortedAddresses.lift(i+1)
if (next.map{_ > curr + 1}.getOrElse(true))
} yield curr

localMaximaAddresses.headOption.flatMap(localMax =>
if (localMax == maxAddress) {
//if we are at the end of our range, start from the beginning
None
} else {
Some(localMax)
}
)
}

Expand Down