Skip to content
This repository has been archived by the owner on Mar 31, 2023. It is now read-only.

Fix pseudo VMs lease cloning #150

Merged
merged 2 commits into from
May 9, 2017
Merged

Conversation

spodila
Copy link
Contributor

@spodila spodila commented May 9, 2017

  • Fix cloning of leases for pseudo VMs, update totals before use
  • unit test to replicate problem with cloned leases before this fix when using active VMs list

@spodila spodila requested review from corindwyer and tbak May 9, 2017 00:03
@spodila spodila merged commit 79404c2 into Netflix:master May 9, 2017
@spodila spodila deleted the fixPseudoLeaseCreation branch May 9, 2017 00:19
Copy link

@tbak tbak left a comment

Choose a reason for hiding this comment

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

Looks good.

if (currTotalLease == null)
logger.debug("Updated total lease is null");
else {
logger.debug("Updated total lease has cpu= " + currTotalLease.cpuCores() + ", mem=" + currTotalLease.memoryMB() +
Copy link

Choose a reason for hiding this comment

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

For better performance use '{}' template parameter.

avm.updateCurrTotalLease();
final VirtualMachineLease currTotalLease = avm.getCurrTotalLease();
if (currTotalLease == null)
Copy link

Choose a reason for hiding this comment

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

Is it ok for this to happen? If not lets change log level to warn, and add more information (AVM hostname?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this can happen for regular VMs (not pseudo VMs).

@@ -191,13 +200,49 @@ public boolean isShutdown() {
}
}
}
else if(pHostsAdded > 0) {
logger.debug("No pseudo assignments made, looking for failures");
Copy link

Choose a reason for hiding this comment

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

This is all about logging at debug level, so lets wrap it with if(logger.isDebugEnabled()).

@spodila
Copy link
Contributor Author

spodila commented May 9, 2017

Changes made in next PR except as noted above.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants