-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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.
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
eureka-1131: handle spot instance termination #1132
Conversation
@@ -63,6 +63,8 @@ | |||
availabilityZone("availability-zone", "placement/"), | |||
publicHostname("public-hostname"), | |||
publicIpv4("public-ipv4"), | |||
spotInstanceTerminationTime("termination-time", "spot/"), |
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.
very minor nit: mind calling this spotTerminationTime
? That aligns more with how the rest of the enums are named.
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. Changed to spotTerminationTime
.
@@ -50,7 +50,7 @@ | |||
}; | |||
|
|||
private final AmazonInfoConfig amazonInfoConfig; | |||
private final RefreshableAmazonInfoProvider amazonInfoHolder; | |||
/* visible for testing */ RefreshableAmazonInfoProvider amazonInfoHolder; |
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 would appreciate this be kept final to better ensure it is always correctly instantiated. An alternative for your test can be to add a new constructor that can be used to set RefreshableAmazonInfoProvider
, with test visibility?
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.
Fixed.
DataCenterInfo dataCenterInfo = config.getDataCenterInfo(); | ||
if (dataCenterInfo instanceof AmazonInfo) { | ||
AmazonInfo amazonInfo = (AmazonInfo) dataCenterInfo; | ||
String newSpotInstanceAction = amazonInfo.getMetadata().get(AmazonInfo.MetaDataKey.spotInstanceAction.name); |
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 unfortunate that the API for AmazonInfo
is a bit leaky, but the preferred API to extract data is
public String get(MetaDataKey key) { |
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.
Got it. Fixed.
existingSpotInstanceAction, | ||
newSpotInstanceAction)); | ||
InstanceInfo.Builder builder = new InstanceInfo.Builder(instanceInfo); | ||
builder.setDataCenterInfo(config.getDataCenterInfo()); |
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 duplication of code here with code on line 218/219/220, mind consolidating them? 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.
Fixed.
@@ -219,6 +219,21 @@ public void refreshDataCenterInfoIfRequired() { | |||
builder.setHostName(newAddress).setIPAddr(newIp).setDataCenterInfo(config.getDataCenterInfo()); | |||
instanceInfo.setIsDirty(); | |||
} | |||
|
|||
String existingSpotInstanceAction = instanceInfo.getMetadata().get(AmazonInfo.MetaDataKey.spotInstanceAction.name); |
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.
(mostly due to our bad naming) I think this is the wrong metadata map to get here, you want the metadata from the instanceInfo.getDataCenterInfo(), if it is an instance of AmazonInfo.
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.
Right. Fixed.
AmazonInfo amazonInfo = (AmazonInfo) dataCenterInfo; | ||
String newSpotInstanceAction = amazonInfo.getMetadata().get(AmazonInfo.MetaDataKey.spotInstanceAction.name); | ||
if (newSpotInstanceAction != null && !newSpotInstanceAction.equals(existingSpotInstanceAction)) { | ||
logger.warn(String.format("The spot instance termination action changed from: %s => %s", |
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.
This is more you use case, is this change worth a WARN level log? Assuming spot termination actions can change often, this'll get noisy.
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 have changed to INFO. If you think that DEBUG is even better here, let me know. In our case, we don't have so much spot-instances termination.
@qiangdavidliu thanks for looking into this. I have fixed all comments. |
Thanks @krutsko |
No description provided.