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 module to configure Pure Storage FlashArray Directory Services #37350

Merged
merged 1 commit into from
Apr 11, 2018

Conversation

sdodsley
Copy link
Contributor

SUMMARY

Add a new module to configure the Directory Service settings on a Pure
Storage FlashArray.
Create, enable, disable and delete are enabled.
There is no facility to set SSL certs in this modules

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

storage/purestorage/purefa_ds

ANSIBLE VERSION
2.5

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. labels Mar 13, 2018
@ryansb ryansb removed the needs_triage Needs a first human triage before being processed. label Mar 13, 2018
@ansibot 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 Mar 21, 2018
Copy link

@theraggedcoder theraggedcoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Does what it says. Thanks for this @sdodsley

@ansibot ansibot removed 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 Mar 22, 2018
@ansibot 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 Mar 30, 2018
@ansibot ansibot removed 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 Apr 4, 2018
from ansible.module_utils.pure import get_system, purefa_argument_spec


def update_ds(module, array):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function doesn't appear to do anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is here as a placeholder for future functionality

changed = True
except:
changed = False
module.exit_json(changed=changed)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think things like this will traceback in check-mode because changed hasn't been set to a default value (Should default to True).

Also, for this particular module, check_mode doesn't seem to be doing anything so you probably just want to say supports_check_mode=False when creating the AnsibleModule.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a module does not support check mode, ansible always reports that it would have changed something which is the safest assumption for the user.

array.enable_directory_service()
changed = True
except:
changed = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to catch any exception here and hide it from the user? It seems like we'd need to see the exception messages in order to debug problems rather than silently thinking that the module succeeded and did not have to make any changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no exception to show. It's a flag that we are setting. If it fails then something else is wrong in the configuration, however the configuration has been validated as much as possible at creation time in the create_ds function. We can't say what would be wrong with the config as it could be a typo with a password, username, password etc.

@abadger
Copy link
Contributor

abadger commented Apr 10, 2018 via email

@sdodsley sdodsley force-pushed the purefa_ds branch 2 times, most recently from b010d03 to f132340 Compare April 10, 2018 18:57
@sdodsley
Copy link
Contributor Author

@abadger Good idea. Thanks. Implemented

changed = True
except:
changed = False
module.fail_json(msg='Enable Directory Service failed: Check configuration')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should look like this:

from ansible.module_utils._text import to_native
[...]

def enable_ds(module, array):
    """Enable Directory Service"""
    try:
        array.enable_directory_service()
        changed = True
    except Exception as e:
        module.fail_json(msg='Enable Directory Service failed: Check configuration: %s' % to_native(e))
    module.exit_json(changed=changed)

One thing I'm not sure about is how we tell if array.enable_directory_service() made a change or not. Our users generally define Ansible's idempotence as:

  • they describe the state they want in a playbook task
  • the module makes the state match what is in the playbook
  • the module returns changed=True if something had to be changed to achieve the desired state or changed=False if the state was already as desired.

This code probably does the first two but it's not doing the last one. I'm not sure how to fix it because I don't know what array.enable_directory_service() does. If it returns a specifc exception if the directory_service was already enabled, then we catch that specific exception and set changed=True for that. If it returns a status code telling whether it had to make a change or not, then we can use that to set changed. If neither of those, we would have to query the current state of the toggle first. If the toggle is already enabled, then set changed=False and call module.exit_json(). If the toggle is not yet enabled, call array.enable_directory_service(), set changed=True, and then call module.exit_json().

(Also note, these notes on changes also apply to the other verbs you have in this module: disable_ds, delete_ds, and create_ds)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all checked for in the main() function. It can't get into these functions if the DS isn't already in the correct state for these functions to operate on.

@maxamillion maxamillion merged commit d9b3b3f into ansible:devel Apr 11, 2018
@sdodsley sdodsley deleted the purefa_ds branch April 11, 2018 13:53
@ansible ansible locked and limited conversation to collaborators Apr 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants