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

Logged info whenever falling back to on-demand instance #658

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+2 −0
Open

Logged info whenever falling back to on-demand instance #658

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 2 additions & 0 deletions src/main/java/hudson/plugins/ec2/SlaveTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -889,6 +889,7 @@ public List<EC2AbstractSlave> provision(int number, EnumSet<ProvisionOptions> pr
return provisionSpot(image, number, provisionOptions);
return Collections.emptyList();
}
LOGGER.info("No Configuration Spot, falling back to on-demand instance.");
Copy link
Contributor

Choose a reason for hiding this comment

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

I see no point to this, this is clearly configured without spot

Copy link
Author

Choose a reason for hiding this comment

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

yeah thats correct but the end user is not getting any information or reason for "falling back to on demand"
This is a quick change to get some logging which would help the end user.

Copy link
Contributor

Choose a reason for hiding this comment

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

The user should have all the information that its not spot because he did not configure spot in the first place. This code respects the setting the user has selected and not due to an edge case we have to fall back to anything.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the information. My issue here is, how would i be able to know if i have been assigned OnDemand Instance because of lower bidding or because of exhausted Spot Instances.

Copy link
Contributor

Choose a reason for hiding this comment

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

If its exhausted with bidprice you should see There is no spot capacity available matching your request, falling back to on-demand instance.

This change clearly doesn't answer if too low bid or exhausted instances happens because its provisioning ondemand due to lack of configuration. Exactly what the user configured.

return provisionOndemand(image, number, provisionOptions);
}

Expand Down Expand Up @@ -1352,6 +1353,7 @@ private void setupCustomDeviceMapping(List<BlockDeviceMapping> deviceMappings) {
private List<EC2AbstractSlave> provisionSpot(Image image, int number, EnumSet<ProvisionOptions> provisionOptions)
throws IOException {
if (!spotConfig.useBidPrice) {
LOGGER.info("No Bidding for Spot, falling back to on-demand instance.");
Copy link
Contributor

Choose a reason for hiding this comment

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

This actually creates a spot instance

Copy link
Author

Choose a reason for hiding this comment

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

Not exactly,
this check for "spotConfig.useBidPrice" is checking if the bid price is available or not and if not then its provisioning OnDemand (provisionOndemand).
here again, the end user is not getting any information or reason about getting OnDemand Instance rather than a spot.
I want the user should be having a clear reason for getting an OnDemand Instance.

Regards
AK

Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite, the fourth parameter is spotWithoutBidPrice so it is still spot.
See

ec2-plugin/src/main/java/hudson/plugins/ec2/SlaveTemplate.java

Lines 1092 to 1143 in b4a6e1e

private List<EC2AbstractSlave> provisionOndemand(Image image, int number, EnumSet<ProvisionOptions> provisionOptions, boolean spotWithoutBidPrice, boolean fallbackSpotToOndemand)
throws IOException {
AmazonEC2 ec2 = getParent().connect();
logProvisionInfo("Considering launching");
HashMap<RunInstancesRequest, List<Filter>> runInstancesRequestFilterMap = makeRunInstancesRequestAndFilters(image, number, ec2);
Map.Entry<RunInstancesRequest, List<Filter>> entry = runInstancesRequestFilterMap.entrySet().iterator().next();
RunInstancesRequest riRequest = entry.getKey();
List<Filter> diFilters = entry.getValue();
DescribeInstancesRequest diRequest = new DescribeInstancesRequest().withFilters(diFilters);
logProvisionInfo("Looking for existing instances with describe-instance: " + diRequest);
DescribeInstancesResult diResult = ec2.describeInstances(diRequest);
List<Instance> orphansOrStopped = findOrphansOrStopped(diResult, number);
if (orphansOrStopped.isEmpty() && !provisionOptions.contains(ProvisionOptions.FORCE_CREATE) &&
!provisionOptions.contains(ProvisionOptions.ALLOW_CREATE)) {
logProvisionInfo("No existing instance found - but cannot create new instance");
return null;
}
wakeOrphansOrStoppedUp(ec2, orphansOrStopped);
if (orphansOrStopped.size() == number) {
return toSlaves(orphansOrStopped);
}
riRequest.setMaxCount(number - orphansOrStopped.size());
List<Instance> newInstances;
if (spotWithoutBidPrice) {
InstanceMarketOptionsRequest instanceMarketOptionsRequest = new InstanceMarketOptionsRequest().withMarketType(MarketType.Spot);
if (getSpotBlockReservationDuration() != 0) {
SpotMarketOptions spotOptions = new SpotMarketOptions().withBlockDurationMinutes(getSpotBlockReservationDuration() * 60);
instanceMarketOptionsRequest.setSpotOptions(spotOptions);
}
riRequest.setInstanceMarketOptions(instanceMarketOptionsRequest);
try {
newInstances = ec2.runInstances(riRequest).getReservation().getInstances();
} catch (AmazonEC2Exception e) {
if (fallbackSpotToOndemand && e.getErrorCode().equals("InsufficientInstanceCapacity")) {
logProvisionInfo("There is no spot capacity available matching your request, falling back to on-demand instance.");
riRequest.setInstanceMarketOptions(new InstanceMarketOptionsRequest());
newInstances = ec2.runInstances(riRequest).getReservation().getInstances();
} else {
throw e;
}
}
} else {

Copy link
Author

Choose a reason for hiding this comment

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

Thats correct but my commit is in here

ec2-plugin/src/main/java/hudson/plugins/ec2/SlaveTemplate.java

Lines 1351 to 1357 in b4a6e1e

*/
private List<EC2AbstractSlave> provisionSpot(Image image, int number, EnumSet<ProvisionOptions> provisionOptions)
throws IOException {
if (!spotConfig.useBidPrice) {
return provisionOndemand(image, 1, provisionOptions, true, spotConfig.getFallbackToOndemand());
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but ultimately the statement No Bidding for Spot, falling back to on-demand instance. is wrong because it will provision a spot instance and if that fails it should log There is no spot capacity available matching your request, falling back to on-demand instance

return provisionOndemand(image, 1, provisionOptions, true, spotConfig.getFallbackToOndemand());
}

Expand Down