Add ManageIQ alert profiles module #32354

elad661 · 2017-10-30T16:11:27Z

SUMMARY

ManageIQ is an open source management platform for Hybrid IT.

This change is adding:

manageiq_alert_profiles module, responsible for alert profiles management in ManageIQ.
(alert profiles are collections of alert policies).

All the modules, including docs, tests and usage examples can be found here

Currently, the only requirement for the module is manageiq-api-client-python,
the module also requires ManageIQ/manageiq-api#149 in the ManageIQ server.

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

manageiq_alert_profiles.py (module)

ANSIBLE VERSION

ansible 2.5.0

elad661 · 2017-10-30T16:11:34Z

cc @yaacov @cben - please review.

ansibot · 2017-10-30T16:12:48Z

cc @abellotti @gtanzillo @zgalor
click here for bot help

ansibot · 2017-10-30T16:20:33Z

@abellotti @cben @dkorn @gtanzillo @yaacov @zgalor

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

cben · 2017-10-31T11:51:20Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

+    description:
+      - absent - alert profile should not exist,
+      - present - alert profile should exist,
+      - list - return a list of alert policies.


typo? "list of alert profiles"?

yep, it was a typo. Fixed now. (I'll squash the commits before this PR will be merged)

yaacov · 2017-10-31T13:01:22Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

+
+        # assign / unassign the alert policies, if needed
+
+        if to_remove:


you may accidentally remove policies user did not explicitly specify state: "absent" for policies that currently exist but no the list the user gave.

for example:
if I have policies A and B
and I ask in the playbook to make sure A is present.
the play book may inadvertently delete policy B.

p.s.
you may want to specify new actions to add and remove policies from a profile
e.g. "assign and set policies" ?

cc @cben @joelddiaz ?

from my experience with configuration management tools similar to ansible, when you define something using the playbook you want it to act like this - to make sure the configuration on the server is 100% in sync with the automated configuration.

"absent" and "present" are the states of the profile itself, when the state is "present" that means "it needs to 1) exist 2) be configured exactly how it's described in the playbook".

that's my take on it, but I'll be happy to change it if the ops people decide they want it to behave differently.

as someone using this module, i would be surprised if the behavior when creating a new alert profile A, a separate unrelated profile B disappeared.

a different way of saying this is if the product ships with profiles X and Y, then while creating a new profile A, i shouldn't have to provide (or know about) X and Y to the ansible module call when all i want to do is create/edit/delete my own alert profile A.

I think you misunderstood the question. This has nothing to do with "unrelated alert profiles", let me paraphrase:

Assume you have manually added an alert profile X, and assigned the alerts A, B and C to it.
If run an ansible playbook that defines alert profile X with only A and B assigned, it will remove C from the existing profile. Is that a kind of behaviour you expect?

If you have a profile Y that is not defined in the playbook, the module will (obviously) not touch it.

I absolutely did misunderstand then.

For the primary use case, I think we do want an Ansible call to create/update profile X with alerts A and B to make sure that only alerts A & B are associated with profile X.

There is an edge case where we may want to surgically append/remove alerts from profile X, but I don't think it is critical to support this use case at this time.

Okay, so the primary use case is implemented by the module. The edge case of "surgically removing/appending" is not supported at the moment, but should be easy to add in the future if it'll ever be needed.

resmo · 2017-11-02T07:19:28Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

+      - absent - alert profile should not exist,
+      - present - alert profile should exist,
+      - list - return a list of alert profiles.
+    required: False


Not a blocker but required: False is the default, so no need to document it.

resmo · 2017-11-02T07:23:47Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

+            response = self.client.get(self.url + '?expand=alert_definitions,resources')
+        except Exception as e:
+            self.module.fail_json(msg="Failed to query alert profiles: {error}".format(error=e))
+        return response.get('resources', [])


If you like to replace None with [], it sugguest to change it to response.get('resources') or [] . The difference is, currently with this code, [] will only returned if there is no resources key in response

yaacov · 2017-11-02T10:13:15Z

shipit

yaacov

LGTM

cben

some technical comments. overall structure good.

cben · 2017-11-06T13:57:28Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

+
+EXAMPLES = '''
+- name: List alert policies in ManageIQ
+  manageiq_alert_policies:


typo, manageiq_alert_profiles

cben · 2017-11-06T13:58:57Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

+    default: 'present'
+  description:
+    description:
+      - The unique alert profile description in ManageIQ.


unique globally or within resource_type?

I thought it was globally unique, just like with alerts, but apparently alert profiles don't enforce uniqueness of the description. The module assumes its globally unique, but this is not necessarily true... I've sent an email to the relevant ManageIQ people to see if this can be changed.

So the new plan is to use name which is unique.
Creating from ansible, we can control both name and description.
Creating from UI only asks for description; it currently sets name to a GUID, will be changed to equal description.

cben · 2017-11-06T19:56:57Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

+
+
+# TODO: I copied this from yaacov's ansible/ansible PR 31233. It should be removed
+# from here once that PR is merged.


now in utils, can drop from here.

cben · 2017-11-06T19:58:21Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

+            self.module.fail_json(msg="Failed to query alert profiles: {error}".format(error=e))
+        return response.get('resources') or []
+
+    def get_alert_policies(self, alert_policies):


as discussed previously, no such thing "alert policies". every mention of "policy/ies" should be renamed...

cben · 2017-11-06T20:06:58Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

+
+        # figure out which policies we need to assign / unassign
+        # alert polices listed by the user:
+        new_alert_policies = set(self.get_alert_policies(new_profile['alert_policies']))


"new" may sound as "to be added", suggest "desired" instead.

cben · 2017-11-06T20:21:08Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

+            result = self.client.post(result['results'][0]['href'] + '/alert_definitions',
+                                      action="assign", resources=policies)
+        except Exception as e:
+            # TODO question - should we rollback the creation if assignment failed?


IMO no need to.
Each step is idempotent, we failed to reach desired state but we got closer.
But mostly, it's just easier ;-)

cben · 2017-11-06T20:21:31Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

+        """
+        # find all alert policies to add to the profile
+        # we do this first to fail early if one is missing.
+        policies = [dict(href=href) for href in self.get_alert_policies(profile['alert_policies'])]


this dict(href=href) building repeats everywhere... can you move it inside either assign_or_unassign? or get_alert_policies()?

cben · 2017-11-06T20:23:23Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

+        # now that it has been created, we can assign the policies
+        try:
+            result = self.client.post(result['results'][0]['href'] + '/alert_definitions',
+                                      action="assign", resources=policies)


any reason not to use assign_or_unassign() here?

cben · 2017-11-06T20:33:59Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

+                # existing profile has different notes
+                profile_dict['set_data'] = dict(notes=new_profile['notes'])
+
+        if new_profile['notes'] and ('set_data' not in old_profile or 'notes' not in old_profile['set_data']):


this condition, and previous one, are complicated.
partly caused by set_data asymmetry.
can you first compute 2 vars with existing notes and new notes (using None or '' where missing),
and then just check if old_notes != new_notes:?

Also, please open manageiq-api issues for each read vs write format asymmetry in the API. There are too many of these :-(

cben · 2017-11-06T21:35:17Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

+        existing_profile = manageiq.find_collection_resource_by("alert_definition_profiles",
+                                                                description=description)
+
+        # we need to add or update the alert policy


here again, and below L332, "alert policy" is not alert but typo for alert profile

jctanner · 2017-11-30T14:14:16Z

@cben can you re-review please?

cben · 2017-11-30T18:53:51Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

-            if old_profile['set_data']['notes'] != new_profile['notes']:
-                # existing profile has different notes
-                profile_dict['set_data'] = dict(notes=new_profile['notes'])
+            old_notes = old_profile['set_data']['notes']


above 3 lines can be written as:

old_notes = old_profile.get('set_data', {}).get('notes')

cben · 2017-11-30T19:01:15Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

+            old_notes = old_profile['set_data']['notes']
+
+        if desired_profile['notes'] != old_notes:
+            # existing profile has no notes, but we have notes


I think this comment is not precise now, this also happens when existing has some old_notes, new has empty string, or both old/new non-empty but different... Can just drop it, or "# notes needs to be updated".

elad661 · 2017-12-20T12:56:15Z

I addressed review feedback and squashed the commits. Now that ManageIQ/manageiq-api#149 is merged, this module is ready for a final review (and eventually for being merged).

@cben @yaacov please take another look when you have the time.

ansibot · 2017-12-20T13:03:20Z

The test ansible-test sanity --test no-assert [?] failed with the following error:

Command "test/sanity/code-smell/no-assert.py" returned exit status 1.
>>> Standard Output
Use of assert in production code is not recommended.
Python will remove all assert statements if run with optimizations
Alternatives:
    if not isinstance(value, dict):
        raise AssertionError("Expected a dict for value")
lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py:177:9:         assert action in ["assign", "unassign"]

click here for bot help

cben · 2017-12-21T09:28:55Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

+        alerts = []
+        for alert_description in alert_descriptions:
+            alert = self.manageiq.find_collection_resource_or_fail("alert_definitions",
+                                                                   description=alert_description)


~~I think this should now match by name (and method arg should be named alert_names)~~

Ignore this, I confused alert profile — matched by name, because that's the unique field — with alerts that are matched by description, which is unique in that table and more user friendly.

cben · 2017-12-21T09:43:24Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

+            response = self.client.get(self.url + '?expand=alert_definitions,resources')
+        except Exception as e:
+            self.module.fail_json(msg="Failed to query alert profiles: {error}".format(error=e))
+        return response.get('resources') or []


python nitpick: if you just want to return [] for missing 'resource' key, write .get('resources', []).
As currently written is effectively .get('resource', None) or [], which will also accept "resources": null and other any "falsey" values like "resources": "". Harmless but one day someone will wonder "is this intentional, can server return non-arrays here?"...

oops this is exactly opposite to what @resmo recommended previously #32354 (comment)
I don't really care, LGTM from me on either form.

cben

LGTM

cben · 2017-12-21T10:06:15Z

shipit

elad661 · 2017-12-21T11:39:11Z

bot_status

ansibot · 2017-12-21T11:49:39Z

Components

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py
support: community
maintainers: abellotti cben gtanzillo yaacov zgalor

Metadata

waiting_on: maintainer
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): 1
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 0
shipit_actors (maintainers or core team members): cben
shipit_actors_other: []

click here for bot help

yaacov · 2017-12-21T12:41:17Z

shipit 🚢

cben · 2018-01-04T19:25:03Z

@yaacov how can we move this and #32136 along?

Looks like bot ignored our shipits here? (I think we're ~~"community_shipits (namespace maintainers)"~~)
shipit

yaacov · 2018-01-04T19:36:58Z

bot_status

ansibot · 2018-01-04T19:43:09Z

Components

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py
support: community
maintainers: abellotti cben gtanzillo yaacov zgalor

Metadata

waiting_on: maintainer
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): 2
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 0
shipit_actors (maintainers or core team members): cben yaacov
shipit_actors_other: []

click here for bot help

yaacov · 2018-01-04T19:51:46Z

LGTM 👍

cben · 2018-01-04T20:06:57Z

@resmo could you review please?

resmo · 2018-01-04T20:56:58Z

~~LGTM~~

~~rebase_merge~~

Sorry to bother again

The module uses state=list, while there are some existing modules having a state=list, it is not something we accept anymore for new modules. This functionality should be implemented as a separate facts module.

resmo · 2018-01-04T21:09:26Z

needs_revision

elad661 · 2018-01-08T10:38:02Z

Okay, removed the state=list option.
@resmo @yaacov @cben, please take another look.

yaacov · 2018-01-08T10:40:53Z

lib/ansible/modules/remote_management/manageiq/manageiq_alert_profiles.py

+version_added: '2.5'
+author: Elad Alfassa (ealfassa@redhat.com)
+description:
+  - The manageiq_alert_profiles module supports listing, adding, updating and deleting alert profiles in ManageIQ.


yaacov · 2018-01-10T12:59:35Z

shipit

resmo · 2018-01-10T13:04:24Z

shipit

elad661 · 2018-01-10T15:51:39Z

bot_status

resmo · 2018-01-10T16:03:52Z

merging.

resmo · 2018-01-10T16:04:23Z

Thanks all of you for the hard work!

cben reviewed Oct 31, 2017

View reviewed changes

cben mentioned this pull request Oct 31, 2017

Add ManageIQ alerts module #32136

Merged

yaacov reviewed Oct 31, 2017

View reviewed changes

jborean93 removed the needs_triage Needs a first human triage before being processed. label Nov 1, 2017

resmo reviewed Nov 2, 2017

View reviewed changes

ansibot removed the new_contributor This PR is the first contribution by a new community member. label Nov 2, 2017

yaacov approved these changes Nov 2, 2017

View reviewed changes

elad661 mentioned this pull request Nov 6, 2017

module_utils/manageiq: add find_collection_resource_or_fail #32588

Merged

cben reviewed Nov 6, 2017

View reviewed changes

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 16, 2017

cben reviewed Nov 30, 2017

View reviewed changes

cben mentioned this pull request Dec 11, 2017

Incompatible changes in modules upstreamed into Ansible dkorn/manageiq-ansible-module#85

Open

7 tasks

ansibot removed community_review In order to be merged, this PR must follow the community review workflow. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Dec 20, 2017

cben reviewed Dec 21, 2017

View reviewed changes

cben approved these changes Dec 21, 2017

View reviewed changes

ansibot added shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels Dec 21, 2017

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed shipit This PR is ready to be merged by Core labels Jan 4, 2018

yaacov reviewed Jan 8, 2018

View reviewed changes

Add ManageIQ alert profiles module

64476f3

resmo merged commit 8c87c76 into ansible:devel Jan 10, 2018

ansible locked and limited conversation to collaborators Apr 26, 2019


		# assign / unassign the alert policies, if needed

		if to_remove:



		# TODO: I copied this from yaacov's ansible/ansible PR 31233. It should be removed
		# from here once that PR is merged.

Add ManageIQ alert profiles module #32354

Add ManageIQ alert profiles module #32354

Conversation

elad661 commented Oct 30, 2017

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

elad661 commented Oct 30, 2017

ansibot commented Oct 30, 2017

ansibot commented Oct 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaacov commented Nov 2, 2017

yaacov left a comment

Choose a reason for hiding this comment

cben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jctanner commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elad661 commented Dec 20, 2017

ansibot commented Dec 20, 2017

cben Dec 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cben left a comment

Choose a reason for hiding this comment

cben commented Dec 21, 2017

elad661 commented Dec 21, 2017

ansibot commented Dec 21, 2017

Components

Metadata

yaacov commented Dec 21, 2017

cben commented Jan 4, 2018 • edited

yaacov commented Jan 4, 2018

ansibot commented Jan 4, 2018

Components

Metadata

yaacov commented Jan 4, 2018

cben commented Jan 4, 2018

resmo commented Jan 4, 2018 • edited

resmo commented Jan 4, 2018

elad661 commented Jan 8, 2018

Choose a reason for hiding this comment

yaacov commented Jan 10, 2018

resmo commented Jan 10, 2018

elad661 commented Jan 10, 2018

resmo commented Jan 10, 2018

resmo commented Jan 10, 2018

cben Dec 21, 2017 •

edited

cben commented Jan 4, 2018 •

edited

resmo commented Jan 4, 2018 •

edited