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

Add ec2_spot_pricing_history_facts module #52222

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@danihodovic
Copy link
Contributor

danihodovic commented Feb 14, 2019

SUMMARY

Adds a module for gathering spot pricing facts. This allows the end user to create the cheapest instances possible.
https://docs.aws.amazon.com/cli/latest/reference/ec2/describe-spot-price-history.html

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

ec2_spot_pricing_facts

ADDITIONAL INFORMATION
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 14, 2019

@danihodovic, just so you are aware we have a dedicated Working Group for aws.
You can find other people interested in this in #ansible-aws on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@danihodovic

This comment has been minimized.

Copy link
Contributor Author

danihodovic commented Feb 14, 2019

If anyone knows how I can pass the sanity checks I would be very grateful. The default value for the dates is dynamically created at runtime based on the time right now.

Sanity checks fail because the generated value does not match the default value in the docs:

Run command: /usr/bin/python test/sanity/validate-modules/validate-modules --format json --arg-spec lib/ansible/modules/cloud/amazon/ec2_spot_pricing_his ...
ERROR: Found 2 validate-modules issue(s) which need to be resolved:
ERROR: lib/ansible/modules/cloud/amazon/ec2_spot_pricing_history_facts.py:0:0: E324 Value for "default" from the argument_spec ('2019-02-14T10:08:24') for "end_time" does not match the documentation ('ansible_date_time.iso8601')
ERROR: lib/ansible/modules/cloud/amazon/ec2_spot_pricing_history_facts.py:0:0: E324 Value for "default" from the argument_spec ('2019-02-14T10:08:24') for "start_time" does not match the documentation ('ansible_date_time.iso8601')

Additionally I'm getting IAM authentication errors on CI.

00:27 fatal: [testhost]: FAILED! => {
00:27     "changed": false, 
00:27     "error": {
00:27         "code": "AuthFailure", 
00:27         "message": "AWS was not able to validate the provided access credentials"
00:27     }, 
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 14, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 3 errors:

lib/ansible/modules/cloud/amazon/ec2_spot_pricing_history_facts.py:0:0: E307 version_added should be '2.8'. Currently '2.7'
lib/ansible/modules/cloud/amazon/ec2_spot_pricing_history_facts.py:0:0: E324 Value for "default" from the argument_spec ('2019-02-14T10:07:39') for "end_time" does not match the documentation ('ansible_date_time.iso8601')
lib/ansible/modules/cloud/amazon/ec2_spot_pricing_history_facts.py:0:0: E324 Value for "default" from the argument_spec ('2019-02-14T10:07:39') for "start_time" does not match the documentation ('ansible_date_time.iso8601')

click here for bot help

)

try:
import dateutil.parser

This comment has been minimized.

@mattclay

mattclay Feb 14, 2019

Member

Avoid the extra dependency on dateutil and use the built-in Python parsing methods instead.

filters=dict(type="list", default=[]),
start_time=dict(
type="str",
default=datetime.datetime.now().replace(microsecond=0).isoformat(),

This comment has been minimized.

@mattclay

mattclay Feb 14, 2019

Member

Defaults need to be static values. Values of None or "now" could be used to indicate the current time.

short_description: Gather facts about ec2 spot pricing history in AWS
description:
- Gather historical facts on the spot prices of different instance types
version_added: "2.7"

This comment has been minimized.

@mattclay

mattclay Feb 14, 2019

Member

This needs to be the current version under development, which is currently 2.8.

@mattclay

This comment has been minimized.

Copy link
Member

mattclay commented Feb 14, 2019

@danihodovic Our CI system will require a permissions update before the tests for this module can be executed. I've labeled the PR so it is on our list for permissions updates.

@danihodovic

This comment has been minimized.

Copy link
Contributor Author

danihodovic commented Feb 15, 2019

Thanks for the quick review @mattclay !

@ansibot ansibot removed the ci_verified label Feb 15, 2019

@danihodovic

This comment has been minimized.

Copy link
Contributor Author

danihodovic commented Feb 23, 2019

@mattclay how can I move this PR forward?

@mattclay

This comment has been minimized.

Copy link
Member

mattclay commented Feb 28, 2019

@danihodovic We have a bit of a backlog of PRs needing updates to our CI system:

https://github.com/ansible/ansible/labels/needs_ci_update

We recently had to make some significant changes to our system because we were exceeding maximum policy size limits. That issue has now been resolved, but we have quite a few permissions updates to deal with now.

One thing you could do to help on the testing side of things would be to identify the minimal set of permissions needed to create a policy for running the tests. There is an open PR to add some new tools to document this process and make it easier. You might want to take a look: #49312

@ansibot ansibot added the stale_ci label Feb 28, 2019

@danihodovic

This comment has been minimized.

Copy link
Contributor Author

danihodovic commented Mar 9, 2019

@mattclay

The following policy is required when running these tests:

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "VisualEditor0",
            "Effect": "Allow",
            "Action": [
                "ec2:DescribeSpotPriceHistory",
                "ec2:DescribeAvailabilityZones"
            ],
            "Resource": "*"
        }
    ]
}

ec2:DescribeSpotPriceHistory is a global action and cannot be constrained to a particular AWS resource.

aws_connection_info: &aws_connection_info
aws_access_key: "{{ aws_access_key }}"
aws_secret_key: "{{ aws_secret_key }}"
region: "{{ aws_region }}"

This comment has been minimized.

@mattclay

mattclay Mar 19, 2019

Member

I've updated our CI system to allow the actions required by this PR. You'll need one additional change:

security_token: "{{ security_token }}"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.