-
Notifications
You must be signed in to change notification settings - Fork 480
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
Add rake task that starts inactive adhoc instances #20422
Conversation
…pped and update the DNS to point to the new ip address.
Should this functionality just be a part of the "update" branch of the |
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 looks great! In the future I'd like us to refactor this CloudFormation
class to split out some of this code into more manageable modules (e.g., one module for base CloudFormation-API stuff, another module for some of the stack constants, another module for some of this adhoc-specific EC2-instance logic, ...etc...) - but for a first pass I think this looks good to be merged.
Some (optional) code-comments below.
lib/cdo/aws/cloud_formation.rb
Outdated
|
||
public_ip_address = instance.reload.public_ip_address | ||
dashboard_url = stack.outputs.select {|output| output.output_key == 'DashboardURL'}.first.output_value | ||
pegasus_url = stack.outputs.select {|output| output.output_key == 'PegasusURL'}.first.output_value |
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.
#select {[...]}.first
can be simplified to #detect
{[...]}
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.
Done!
hosted_zones. | ||
select {|zone| pegasus_domain_name.end_with?('.' + zone.name)}. | ||
first. | ||
id |
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.
Instead of iterating through all of the hosted zones, you might be able to use list_hosted_zones_by_name
, like:
zone_name = URI.parse(pegasus_url).host.split('.').last(2).join('.') + '.'
hosted_zone_id = route53_client.
list_hosted_zones_by_name(
dns_name: zone_name,
max_items: 1
).
hosted_zones.
first.
id
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 was avoiding this type of lookup in the unlikely event that a 3rd level domain (hourofcode.org.uk.
) was being used as the zone for the stack.
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.
ah, good point! looks like there's no general-purpose way to extract the domain from a hostname without querying central name-servers, so the current approach sounds good.
lib/cdo/aws/cloud_formation.rb
Outdated
CDO.log.info "Waiting 60 seconds for AWS Route53 to apply updated DNS records to all of its servers." | ||
sleep 60 # wait 60 seconds | ||
change_status = route53_client.get_change({id: change_id}).change_info.status | ||
CDO.log.info "DNS update status - #{change_status}" |
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.
You should be able to use the SDK's built-in :resource_record_sets_changed
waiter e.g.: route53_client.wait_until(:resource_record_sets_changed, id: change_id)
rather than an explicit sleep and call to get_change
here.
@@ -18,6 +18,8 @@ class CloudFormation | |||
TEMPLATE = ENV['TEMPLATE'] || raise('Stack template not provided in environment (TEMPLATE=[stack].yml.erb)') | |||
TEMPLATE_POLICY = TEMPLATE.split('.').tap {|s| s.first << '-policy'}.join('.') | |||
TEMP_BUCKET = ENV['TEMP_S3_BUCKET'] || 'cf-templates-p9nfb0gyyrpf-us-east-1' | |||
# number of seconds to configure as Time To Live for DNS record | |||
DNS_TTL = 60 |
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.
We set a constant TTL: 60
in cloud_formation_stack.yml.erb
in two places, that could be replaced with this defined constant (e.g.: <%= DNS_TTL %>
):
TTL: 60 |
TTL: 60 |
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.
Done!
Updates DNS entries to point to Instance's new public IP address after start is complete.