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

Adding image field to gcp_compute inventory script #52688

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@rambleraptor
Copy link
Contributor

rambleraptor commented Feb 20, 2019

SUMMARY

Adding an image field to gcp_compute inventory script.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

gcp_compute.py

ADDITIONAL INFORMATION

In reference to #51884

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 20, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 20, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/plugins/inventory/gcp_compute.py:320:15: singleton-comparison Comparison to True should be just 'expr' or 'expr is True'

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/plugins/inventory/gcp_compute.py:320:33: E712 comparison to True should be 'if cond is True:' or 'if cond:'

click here for bot help

@AlanCoding

This comment has been minimized.

Copy link
Member

AlanCoding commented Feb 21, 2019

I tried this, and it returned "image": null for all the hosts. But...

I dropped a pdb shell inside your method _get_image, and then tinkered...

config_data = self._read_config_data('private/gce/gcp_compute.yml')  # my file
config_data['scopes'] = ['https://www.googleapis.com/auth/compute']
module = GcpMockModule(config_data)
auth = GcpSession(module, 'compute')
resp = auth.get(item['disks'][0]['source'])
print resp.json()['sourceImage']

This is a string like

https://www.googleapis.com/compute/v1/projects/centos-cloud/global/images/centos-7-v20171025

Where centos-7-v20171025 here corresponds to gce_image from the old script.

You're welcome to work this into this code if you want.

A few more comments about getting this is:

  • a changelog fragment would be good to have, I definitely want to keep a backport on the table
  • since my method involves an additional request, it would slow down the plugin, so it might be something that needs explicit enablement by a new parameter in the inventory file, and this is fine with me

As an example of such a parameter, see expand_hostvars for openstack:

https://github.com/ansible/ansible/blob/af9ff07c74b53798edff132aa8f8099773e97a91/lib/ansible/plugins/inventory/openstack.py

@AlanCoding

This comment has been minimized.

Copy link
Member

AlanCoding commented Feb 21, 2019

Here's some code for that

https://github.com/ansible/ansible/compare/devel...AlanCoding:gcp_image_request?expand=1

It takes a really long time for it to run. For whatever reason, the API takes a very long time to do a request. We would probably need to change to that old approach where a request was done to list disks, instead of a separate request for each disk.

@rambleraptor

This comment has been minimized.

Copy link
Contributor Author

rambleraptor commented Feb 22, 2019

Does the contrib/gce.py inventory script successfully get the image currently? I have no issues adding a flag which may slow down the inventory script, but I want to make sure that the old inventory script hasn't found a better way to fetch the image name

(Disclaimer: I think your way is the only way to get the image name successfully)

@AlanCoding

This comment has been minimized.

Copy link
Member

AlanCoding commented Feb 23, 2019

Yes, I had some notes here: #51884 (comment)

the script hits /v1/projects/project-name/aggregated/disks?maxResults=500, whereas the branch I linked hits the URL for the specific disk, which looks like /v1/projects/project-name/zones/us-east1-d/disks/image-name.

Sometimes the API is very very slow. Doing a request for each disk is almost certainly too slow. Instead, if we can use the list of disks, and then loop over and associate them to the hosts (like the script), that would be better. However, the plugin makes requests for instances in a zone-specific way, so that scoping should probably also be respected. Presumably, we would want to get the list endpoint for disks, specific to each zone (don't know what the URL for that would be). Or maybe not, I'm not sure.

@ansibot ansibot added the stale_ci label Mar 3, 2019

@maxamillion

This comment has been minimized.

Copy link
Contributor

maxamillion commented Mar 18, 2019

@rambleraptor any update here? I'd be happy to take this over if you're no longer interested in the patch.

needs_info

@rambleraptor

This comment has been minimized.

Copy link
Contributor Author

rambleraptor commented Mar 19, 2019

Hey @maxamillion and all. Sorry, I completely forgot about this PR. If you don't mind taking it over, that would be fantastic. Happy to provide whatever help I can!

@ansibot ansibot added community_review and removed needs_info labels Mar 19, 2019

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.