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

Red Hat Satellite subscription module #52578

Open
wants to merge 12 commits into
base: devel
from

Conversation

@stroobl
Copy link

stroobl commented Feb 19, 2019

SUMMARY

This is an Ansible module to manage subscriptions (licenses) for content hosts in a Red Hat Satellite 6 server (Katello). The subscriptions can be assigned with variables in the inventory. The module will search for a free subscription in the pool that matches your search query and assign it to a host. The module can also be used to check if virtual guests only have subscriptions from the hypervisor attached.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

Red Hat Satellite subcription (license) mangement module

ADDITIONAL INFORMATION

The satellite.py module_utils file contains functions also used by another Satellite (reporting) module that we want to contribute.

@ansibot

This comment was marked as resolved.

Copy link
Contributor

ansibot commented Feb 19, 2019

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

lib/ansible/module_utils/satellite.py:19:14: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/module_utils/satellite.py:31:31: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/module_utils/satellite.py:38:0: dangerous-default-value Dangerous default value {} as argument

The test ansible-test sanity --test pylint [explain] failed with 18 errors:

lib/ansible/modules/packaging/os/satellite_subscription.py:201:18: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/packaging/os/satellite_subscription.py:204:37: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/packaging/os/satellite_subscription.py:215:28: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/packaging/os/satellite_subscription.py:216:18: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/packaging/os/satellite_subscription.py:228:14: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/packaging/os/satellite_subscription.py:246:14: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/packaging/os/satellite_subscription.py:256:14: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/packaging/os/satellite_subscription.py:299:14: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/packaging/os/satellite_subscription.py:341:26: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/packaging/os/satellite_subscription.py:346:49: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/packaging/os/satellite_subscription.py:363:22: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/packaging/os/satellite_subscription.py:372:57: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/packaging/os/satellite_subscription.py:378:57: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/packaging/os/satellite_subscription.py:385:22: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/packaging/os/satellite_subscription.py:390:26: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/packaging/os/satellite_subscription.py:396:54: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/packaging/os/satellite_subscription.py:408:26: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/packaging/os/satellite_subscription.py:432:53: ansible-format-automatic-specification Format string contains automatic field numbering specification

click here for bot help

Luc Stroobant
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 20, 2019

@AnderEnder @DJMuggs @FlossWare @JayKayy @JoergFiedler @KimNorgaard @L2G @MacLemon @MorrisA @Tatsh @albertomurillo @alxgu @andrew-d @andytom @angristan @barnabycourt @berenddeboer @berenddeschouwer @brian-brazil @brontitall @danieljaouen @davx8342 @dermute @dinoocch @eest @enriclluelles @evgkrsk @fishman @flynn1973 @gforster @giovannisciortino @hacosta @hnanni @ignatenkobrain @indrajitr @jasperla @jcftang @jirutka @jlaska @jpdasma @jtyr @jvantuyl @kahowell @kairoaraujo @kustodian @kyleabenson @marvin-sinister @matburt @mator @matze @mavit @mekanix @melodous @mgwilliams @molekuul @obirvalger @oolongbrothers @opoplawski @overhacked @pmakowski @ramooncamacho @robinro @sashka @scathatheworm @seandst @skinp @sysadmind @szinck @tchernomax @tdtrask @the-maldridge @troy2914 @tuxillo @vaygr @vcarceler @verm666 @vincentvdk @vritant @wltjr @wtcross @xen0l

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@kyleabenson

This comment has been minimized.

Copy link

kyleabenson commented Feb 20, 2019

@stroobl thanks for taking the time to work on this and contribute. I'm trying to understand how this differs from the redhat_subscription module, which also allows for subscriptions to be applied from a Satellite 6 server? Is there a compelling use case I'm missing that isn't included in that existing module?

https://docs.ansible.com/ansible/latest/modules/redhat_subscription_module.html

@stroobl

This comment has been minimized.

Copy link
Author

stroobl commented Feb 20, 2019

@kyleabenson I know the Red Hat subscription module, but it's only useful to manage the subscriptions on the client side (with client I mean the system consuming the subscription here). This module works on the Satellite server side. It's a tool to ease the subscription management for the Satellite server admins.

It can be used to replace (expired) subscriptions on hosts or to manage subscriptions on systems where you don't have access on the client side (you can't use redhat_subscription on VMware hypervisors for example). This is the most important use case why I wrote the module: it's an easy way to assign subscriptions to a large amount of hypervisors. When you move an existing VMware guest to a hypervisor with a Virtual Datacenter subscription assigned, the module can be used to remove that one an replace it with a Virtual Datacenter guest subscription. Doing this on the client side is almost impossible, since you can't detect on which hypervisor a guest is running there.

@denraf

denraf approved these changes Feb 20, 2019

@kahowell

This comment has been minimized.

Copy link
Contributor

kahowell commented Feb 20, 2019

@stroobl, thanks for contributing this PR!

Ideally, there should be only one (Red Hat) subscription module. What do folks think of trying to refactor this into redhat_subscription? redhat_subscription could have the content_host parameter from this module added, and if specified, use the API instead of subscription-manager.

Personally, I'd rather see this logic incorporated into the redhat_subscription module.

If we decide not to incorporate this logic/code into the redhat_subscription module, can we please at least make sure that the parameters match those in redhat_subscription where possible? That would make it easier to switch from one module to the other.

Specifically:

  • satellite_hostname -> server_hostname
  • organization -> org_id
  • verify_ssl -> server_insecure
  • instead of state: autoattach, it should be auto_attach: yes/no for consistency.

Also, some of the logic here could be expressed in a more idiomatic or more generally useful way:

  • Some of the subscription/host searching logic could be refactored into lookup modules.
  • state: vdcguests logic could be rewritten as an example playbook.
  • state: optimize logic could be rewritten as an example playbook.
@stroobl

This comment has been minimized.

Copy link
Author

stroobl commented Feb 20, 2019

On the proposal to rewrite this a part of redhat_subscription I want to comment this: we use redhat_subscription to subscribe our guests. This module works completely different and has a different use case and target audience: it's to manage the subscriptions during the lifecycle of the system AFTER it has been subscribed. More than 90% of the users of redhat_subscription won't need it and won't understand what it does.
Rewriting vdcguest in a playbook is impossible, it relies on the data gathered in the Satellite API calls. You're basicly asking me to do a full rewrite here to match the conventions set by a module that has a completely different use case. It will never be possible to do that.

@dagwieers

This comment has been minimized.

Copy link
Member

dagwieers commented Feb 20, 2019

I agree, these are two different things. And we don't want to complicate a simple module like redhat-subscription with the functionality and possibilities of the Satellite interface. It's a lot better to have 2 distinct modules in this case.

@gforster

This comment has been minimized.

Copy link
Contributor

gforster commented Feb 20, 2019

As long as it is clear in the description/documentation (which I think it is), I agree. May need to modify the redhat-subscription documentation to show its difference.

@kahowell

This comment has been minimized.

Copy link
Contributor

kahowell commented Feb 20, 2019

On the proposal to rewrite this a part of redhat_subscription I want to comment this: we use redhat_subscription to subscribe our guests. This module works completely different and has a different use case and target audience: it's to manage the subscriptions during the lifecycle of the system AFTER it has been subscribed. More than 90% of the users of redhat_subscription won't need it and won't understand what it does.

Ideally, it should not be necessary to use two different modules to manage your subscriptions. Given the significant differences you point out, I'm happy whether this is incorporated into redhat_subscription or not.

Rewriting vdcguest in a playbook is impossible, it relies on the data gathered in the Satellite API calls. You're basicly asking me to do a full rewrite here to match the conventions set by a module that has a completely different use case. It will never be possible to do that.

Apologies, I did not mean to imply you must rewrite a bunch of stuff. I only meant to suggest that you have very useful logic in this module, and that it'd be interesting if we considered pulling some of it out into lookup modules or inventory modules (which could be done outside the scope of this PR).

Overall I like the PR though!

I agree, these are two different things. And we don't want to complicate a simple module like redhat-subscription with the functionality and possibilities of the Satellite interface. It's a lot better to have 2 distinct modules in this case.

For what it's worth, underneath, they ultimately hit the same underlying subscription service (Candlepin). Because the module in this PR is hitting Satellite-specific APIs (Katello and the like), I can accept the desire for a separate module.

@dagwieers

This comment has been minimized.

Copy link
Member

dagwieers commented Feb 20, 2019

We definitely want to add a seealso: section in both modules referring the other module given how they relate, see https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html#documentation-fields

@dagwieers
Copy link
Member

dagwieers left a comment

First review.

Show resolved Hide resolved lib/ansible/module_utils/satellite.py Outdated
Show resolved Hide resolved lib/ansible/module_utils/satellite.py
Show resolved Hide resolved lib/ansible/modules/packaging/os/satellite_subscription.py Outdated
Show resolved Hide resolved lib/ansible/modules/packaging/os/satellite_subscription.py Outdated
Show resolved Hide resolved lib/ansible/modules/packaging/os/satellite_subscription.py Outdated
Show resolved Hide resolved lib/ansible/modules/packaging/os/satellite_subscription.py Outdated
Show resolved Hide resolved lib/ansible/modules/packaging/os/satellite_subscription.py Outdated
Show resolved Hide resolved lib/ansible/modules/packaging/os/satellite_subscription.py Outdated
Show resolved Hide resolved lib/ansible/modules/packaging/os/satellite_subscription.py Outdated
Show resolved Hide resolved lib/ansible/modules/packaging/os/satellite_subscription.py Outdated
@dagwieers

This comment has been minimized.

Copy link
Member

dagwieers commented Feb 21, 2019

Also, I don't think this is correctly placed. Red Hat Satellite either belongs in remote_management (this is where foreman is located too) or web_infrastructure (this is where you can find ansible_tower).

So one of these:

  • lib/ansible/modules/remote_management/satellite/satellite_subscription.py
  • lib/ansible/modules/web_infrastructure/satellite/satellite_subscription.py

I find it also strange that there are no other Red Hat Satellite modules in Ansible at this time.

@gforster

This comment has been minimized.

Copy link
Contributor

gforster commented Feb 21, 2019

Also, I don't think this is correctly placed. Red Hat Satellite either belongs in remote_management (this is where foreman is located too) or web_infrastructure (this is where you can find ansible_tower). ...

@dagwieers there are the rhn_register and rhn_channel modules as well.

Sent with GitHawk

@dagwieers

This comment has been minimized.

Copy link
Member

dagwieers commented Feb 21, 2019

@gforster Sure, and those modules target the OS, this targets Satellite, which like Foreman and Tower are Web-based management frameworks. So it is best placed in a directory together with other (potential) Satellite modules. And as I said, I find it very strange there are no other Satellite modules.

Update lib/ansible/module_utils/satellite.py
Add coding.

Co-Authored-By: stroobl <stroobl@users.noreply.github.com>

dagwieers and others added some commits Feb 21, 2019

Update lib/ansible/modules/packaging/os/satellite_subscription.py
update short description

Co-Authored-By: stroobl <stroobl@users.noreply.github.com>
Update lib/ansible/modules/packaging/os/satellite_subscription.py
Update description

Co-Authored-By: stroobl <stroobl@users.noreply.github.com>
Apply suggestions from code review
Review remarks Dag

Co-Authored-By: stroobl <stroobl@users.noreply.github.com>
Luc Stroobant
@stroobl

This comment has been minimized.

Copy link
Author

stroobl commented Feb 21, 2019

I tried to handle most review remarks. I agree that's is probably better to move the module to another section. That will also help to avoid confusion with redhat_subscription. If that helps to decide where to put it: I have one other Satellite module that I want to contribute. It generates a (csv) overview of the used subscriptions in the Satellite and totals of the free/consumed subscriptions (which is currently almost impossible to see in the Satellite if you have multiple contracts per subscription type).

@dagwieers
Copy link
Member

dagwieers left a comment

Review #2

Show resolved Hide resolved lib/ansible/modules/packaging/os/satellite_subscription.py Outdated
Show resolved Hide resolved lib/ansible/modules/packaging/os/satellite_subscription.py
Show resolved Hide resolved lib/ansible/modules/packaging/os/satellite_subscription.py Outdated
Show resolved Hide resolved lib/ansible/modules/packaging/os/satellite_subscription.py Outdated
- Set to true to remove all other subscriptions attached to the host
type: bool
default: False
verify_ssl:

This comment has been minimized.

@dagwieers

dagwieers Feb 21, 2019

Member

This is marked as resolved, but the documentation still lists verify_ssl.

Show resolved Hide resolved lib/ansible/modules/packaging/os/satellite_subscription.py
Show resolved Hide resolved lib/ansible/modules/packaging/os/satellite_subscription.py Outdated
Show resolved Hide resolved lib/ansible/modules/packaging/os/satellite_subscription.py Outdated
Show resolved Hide resolved lib/ansible/modules/packaging/os/satellite_subscription.py Outdated
Show resolved Hide resolved lib/ansible/modules/packaging/os/satellite_subscription.py Outdated

Luc Stroobant and others added some commits Feb 21, 2019

Luc Stroobant
Apply suggestions from code review
Review remarks

Co-Authored-By: stroobl <stroobl@users.noreply.github.com>
@ansibot

This comment was marked as resolved.

Copy link
Contributor

ansibot commented Feb 21, 2019

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/packaging/os/satellite_subscription.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/packaging/os/satellite_subscription.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/packaging/os/satellite_subscription.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/packaging/os/satellite_subscription.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/packaging/os/satellite_subscription.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test docs-build [explain] failed with the error:

Command "/usr/bin/python test/sanity/code-smell/docs-build.py" returned exit status 1.
>>> Standard Error
Command 'make singlehtmldocs' failed with status code: 2
--> Standard Output
cat _themes/srtd/static/css/theme.css | sed -e 's/^[ 	]*//g; s/[ 	]*$//g; s/\([:{;,]\) /\1/g; s/ {/{/g; s/\/\*.*\*\///g; /^$/d' | sed -e :a -e '$!N; s/\n\(.\)/\1/; ta' > _themes/srtd/static/css/theme.min.css
PYTHONPATH=../../lib ../bin/dump_config.py --template-file=../templates/config.rst.j2 --output-dir=rst/reference_appendices/ -d ../../lib/ansible/config/base.yml
mkdir -p rst/cli
PYTHONPATH=../../lib ../bin/generate_man.py --template-file=../templates/cli_rst.j2 --output-dir=rst/cli/ --output-format rst ../../lib/ansible/cli/*.py
PYTHONPATH=../../lib ../bin/dump_keywords.py --template-dir=../templates --output-dir=rst/reference_appendices/ -d ./keyword_desc.yml
PYTHONPATH=../../lib ../bin/plugin_formatter.py -t rst --template-dir=../templates --module-dir=../../lib/ansible/modules -o rst/modules/ 
Evaluating module files...
Makefile:93: recipe for target 'modules' failed
--> Standard Error
Traceback (most recent call last):
  File "../bin/plugin_formatter.py", line 774, in <module>
    main()
  File "../bin/plugin_formatter.py", line 729, in main
    plugin_info, categories = get_plugin_info(options.module_dir, limit_to=options.limit_to, verbose=(options.verbosity > 0))
  File "../bin/plugin_formatter.py", line 294, in get_plugin_info
    doc, examples, returndocs, metadata = plugin_docs.get_docstring(module_path, fragment_loader, verbose=verbose)
  File "/root/ansible/lib/ansible/utils/plugin_docs.py", line 103, in get_docstring
    data = read_docstring(filename, verbose=verbose, ignore_errors=ignore_errors)
  File "/root/ansible/lib/ansible/parsing/plugin_docs.py", line 59, in read_docstring
    data[varkey] = AnsibleLoader(child.value.s, file_name=filename).get_single_data()
  File "/usr/local/lib/python3.6/dist-packages/yaml/constructor.py", line 35, in get_single_data
    node = self.get_single_node()
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 36, in get_single_node
    document = self.compose_document()
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 55, in compose_document
    node = self.compose_node(None, None)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 84, in compose_node
    node = self.compose_mapping_node(anchor)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 133, in compose_mapping_node
    item_value = self.compose_node(node, item_key)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 84, in compose_node
    node = self.compose_mapping_node(anchor)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 133, in compose_mapping_node
    item_value = self.compose_node(node, item_key)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 84, in compose_node
    node = self.compose_mapping_node(anchor)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 133, in compose_mapping_node
    item_value = self.compose_node(node, item_key)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 82, in compose_node
    node = self.compose_sequence_node(anchor)
  File "/usr/local/lib/python3.6/dist-packages/yaml/composer.py", line 110, in compose_sequence_node
    while not self.check_event(SequenceEndEvent):
  File "/usr/local/lib/python3.6/dist-packages/yaml/parser.py", line 98, in check_event
    self.current_event = self.state()
  File "/usr/local/lib/python3.6/dist-packages/yaml/parser.py", line 393, in parse_block_sequence_entry
    "expected <block end>, but found %r" % token.id, token.start_mark)
yaml.parser.ParserError: while parsing a block collection
  in "<unicode string>", line 19, column 7:
          - The name or IP address of the  ... 
          ^
expected <block end>, but found '?'
  in "<unicode string>", line 20, column 7:
          type: str
          ^
make: *** [modules] Error 1

The test ansible-test sanity --test ansible-doc --python 3.8 [explain] failed with 1 error:

lib/ansible/modules/packaging/os/satellite_subscription.py:0:0: has a documentation error formatting or is missing documentation.

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

lib/ansible/modules/packaging/os/satellite_subscription.py:0:0: E324 Argument 'state' in argument_spec defines default as ('autoattach') but documentation defines default as (None)
lib/ansible/modules/packaging/os/satellite_subscription.py:0:0: E324 Argument 'validate_certs' in argument_spec defines default as (True) but documentation defines default as (False)
lib/ansible/modules/packaging/os/satellite_subscription.py:0:0: E326 Argument 'state' in argument_spec defines choices as (['absent', 'autoattach', 'disable_autoattach', 'optimize', 'present', 'vdcguests']) but documentation defines choices as ([])
lib/ansible/modules/packaging/os/satellite_subscription.py:33:7: E302 DOCUMENTATION is not valid YAML

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

lib/ansible/modules/packaging/os/satellite_subscription.py:33:7: error DOCUMENTATION: syntax error: expected <block end>, but found '?'

click here for bot help

Luc Stroobant
__metaclass__ = type

try:
import requests

This comment has been minimized.

@sivel

sivel Feb 21, 2019

Member

Just as a heads up. We will not accept this change switching to requests. requests is a blacklisted import and we require use of open_url.

I see the conversation below, but that is not sufficient.

Requests has too many versions that may not be compatible, there are likely to be issues with fullfilling pinned dependencies, and this adds an extra non-necessary dependency.

This comment has been minimized.

@stroobl

stroobl Feb 21, 2019

Author

In that case the module (and specially the reporting module that we also want to contribute) becomes unusable for our use case. I can't spend on rewriting with the internal Ansible url modules if that bug is not resolved.

This comment has been minimized.

@dagwieers

dagwieers Feb 21, 2019

Member

@sivel Maybe we should have a check for this in Shippable CI, as I see more and more modules using requests.

[dag@moria ansible.git]$ grep -rlE '(import requests|from requests)' lib/ansible | wc -l
35

This comment has been minimized.

@sivel

sivel Feb 21, 2019

Member

We do actually, it's done in validate-modules. However validate-modules doesn't do a verification in module_utils. A code-smell may not be bad. However there is also strange logic for checking this, as a few modules snuck in after the re-combining core and extras into the main repo, when validate-modules had some broken functionality. So we would have to add skip lists.

There are also a few modules which by extension of another dependency use requests, such as some cloud modules, so they import to get access to exceptions.

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.