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.

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

Conversation

arunmastermind
Copy link

Logged info whenever falling back to on-demand instance as this is very useful to track the logs

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

Logged info whenever falling back to on-demand instance as this is very useful to track the logs
@arunmastermind
Copy link
Author

Logged info whenever falling back to on-demand instance as i found this missing and there is no way to find out the reason for on demand instances.
we must be very focused on providing the logs as the end user will always judge the product as per these features.

@@ -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

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

*/
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

@@ -889,6 +889,7 @@ public Tenancy getTenancyAttribute() {
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.

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

Successfully merging this pull request may close these issues.

2 participants