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

Lldp lshw upload status #123

Merged
merged 7 commits into from
Mar 4, 2014
Merged

Lldp lshw upload status #123

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2c2cbbc
support features.allowedServerUpdateStatuses to accept lldp/lshw in c…
byxorna Feb 21, 2014
b64a44c
remove errant log statement
byxorna Feb 21, 2014
3951376
add default values to allowedServerUpdateStatuses to reflect business…
byxorna Feb 21, 2014
c030b61
whoops, revert change to style
byxorna Feb 28, 2014
e380285
always allow updates in maintenance mode
byxorna Feb 28, 2014
8b6d621
rename updateAnyServer, remove updateMaintenanceServer
byxorna Feb 28, 2014
f02af2b
rename updateAnyServer to updateServerHardwareMeta for clarity
byxorna Feb 28, 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
42 changes: 34 additions & 8 deletions app/models/AssetLifecycle.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,11 @@ object AssetLifecycle {
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Call me crazy - is this just a cosmetic change? Did you actually change any functionality here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No functionality change. Just did it out of OCD. I can revert this change if you would like, as it is not necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I totally understand OCD / having consistent, clearer code. I just wanted to make sure I wasn't missing a subtle diff. Leave it.

def updateAsset(asset: Asset, options: Map[String,String]): Status[Boolean] = asset.isServerNode match {
case true => updateServer(asset, options)
case false => updateOther(asset, options)
def updateAsset(asset: Asset, options: Map[String,String]): Status[Boolean] = {
asset.isServerNode match {
case true => updateServer(asset, options)
case false => updateOther(asset, options)
}
}

protected def updateOther(asset: Asset, options: Map[String,String]): Status[Boolean] = {
Expand All @@ -106,7 +108,7 @@ object AssetLifecycle {
} else if (asset.isMaintenance) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@byxorna i can't see any benefit for leaving this status check in here or leaving the updateMaintenanceServer around. please consider removing it and renaming the method you have created as i have suggested below.

updateMaintenanceServer(asset, options)
} else {
Left(new Exception("Only updates for Incomplete, New, and Maintenance servers are currently supported"))
updateOtherStatusServer(asset, options)
}
}

Expand Down Expand Up @@ -155,6 +157,28 @@ object AssetLifecycle {
}.left.map(e => handleException(asset, "Error updating status/state for asset", e))
}

protected def updateOtherStatusServer(asset: Asset, options: Map[String,String]): Status[Boolean] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should really just be called updateServer

// if asset's status is in the allowed statuses for updating, do it
if (Feature.allowedServerUpdateStatuses.contains(asset.getStatus())) {
// we will allow updates to lshw/lldp while the machine is in these statuses
allCatch[Boolean].either {
Asset.inTransaction {
options.get("lshw").foreach{lshw =>
parseLshw(asset, new LshwParser(lshw)).left.foreach{throw _}
InternalTattler.informational(asset, None, "Parsing and storing LSHW data succeeded")
Copy link
Contributor

Choose a reason for hiding this comment

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

nice use of the tattler

}
options.get("lldp").foreach{lldp =>
parseLldp(asset, new LldpParser(lldp)).left.foreach{throw _}
InternalTattler.informational(asset, None, "Parsing and storing LLDP data succeeded")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@byxorna this should also support updating the CHASSIS_TAG here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

roger.

true
}
}.left.map(e => handleException(asset, "Exception updating asset", e))
} else {
Left(new Exception("Only updates for servers in statuses " + Feature.allowedServerUpdateStatuses.mkString(", ") + " are currently supported"))
}
}

protected def updateNewServer(asset: Asset, options: Map[String,String]): Status[Boolean] = {
val units = PowerUnits()
val requiredKeys = Set(RackPosition.toString) ++ PowerUnits.keys(units)
Expand Down Expand Up @@ -217,7 +241,7 @@ object AssetLifecycle {
MetaWrapper.createMeta(asset, filtered ++ Map(AssetMeta.Enum.ChassisTag.toString -> chassis_tag))
val newAsset = asset.copy(status = Status.New.map(_.id).getOrElse(0), updated = Some(new Date().asTimestamp))
Asset.partialUpdate(newAsset, newAsset.updated, Some(newAsset.status), State.New)
InternalTattler.informational(newAsset, None, "Parsing and storing LSHW data succeeded")
InternalTattler.informational(newAsset, None, "Parsing and storing LSHW/LLDP data succeeded")
Copy link
Contributor

Choose a reason for hiding this comment

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

@byxorna good catch

true
}
}.left.map(e => handleException(asset, "Exception updating asset", e))
Expand All @@ -232,15 +256,17 @@ object AssetLifecycle {

allCatch[Boolean].either {
Asset.inTransaction {
options.get("lshw").foreach{lshw =>
options.get("lshw").foreach{lshw =>
parseLshw(asset, new LshwParser(lshw)).left.foreach{throw _}
InternalTattler.informational(asset, None, "Parsing and storing LSHW data succeeded")
}
options.get("lldp").foreach{lldp =>
parseLldp(asset, new LldpParser(lldp)).left.foreach{throw _}
InternalTattler.informational(asset, None, "Parsing and storing LLDP data succeeded")
}
options.get("CHASSIS_TAG").foreach{chassis_tag =>
options.get("CHASSIS_TAG").foreach{chassis_tag =>
MetaWrapper.createMeta(asset, Map(AssetMeta.Enum.ChassisTag.toString -> chassis_tag))
}
}
true
}
}.left.map(e => handleException(asset, "Exception updating asset", e))
Expand Down
5 changes: 4 additions & 1 deletion app/util/config/Feature.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package util
package config

import models.{Asset, AssetMeta}
import models.{Asset, AssetMeta, Status}
import models.logs.LogMessageType

/**
Expand All @@ -17,6 +17,9 @@ object Feature extends Configurable {
override val referenceConfigFilename = "features_reference.conf"

def allowTagUpdates = getStringSet("allowTagUpdates")
def allowedServerUpdateStatuses = getStringSet("allowedServerUpdateStatuses").union(Set("NEW", "INCOMPLETE", "MAINTENANCE"))
Copy link
Contributor

Choose a reason for hiding this comment

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

@byxorna this doesn't make sense to me why you have NEW and INCOMPLETE here, those statuses always allow server asset updates, this feature is to control what other statuses allow the update (like MAINTENANCE)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set this so the messages that were returned via API when updating an asset in a status that doesnt allow updates would be clear. I agree though, Ill remove those two, and set MAINTENANCE as a permanent member of this set.

.map { m => Status.findByName(m) }
.filter(_.isDefined).map(_.get)
def defaultLogType = {
val lts = getString("defaultLogType", "Informational").toUpperCase
try {
Expand Down