VirtualMediaEject should not require image_url

smalleni commented 3 years ago

Summary

When using redfish_command (https://github.com/ansible-collections/community.general/blob/main/plugins/modules/remote_management/redfish/redfish_command.py) I want to eject all virtual media and looks like the module expects a value for image_url for this to work. There are cases when a user wants to eject all virtual media and it might not be possible to know the exact urls of the virtual media previously supported. It looks like redfish supports ejecting all virtual media without providig an image_url link (https://github.com/dell/iDRAC-Redfish-Scripting/issues/24#issuecomment-481563823), however I see an error like

TASK [boot-iso : DELL - Eject Virtual Media (if any)] *******************************************************************************************************************************
Wednesday 21 July 2021  12:48:53 -0500 (0:00:11.918)       0:02:23.541 ******** 
fatal: [e16-h12-b02-fc640.rdu2.scalelab.redhat.com]: FAILED! => {"changed": false, "msg": "image_url option required for VirtualMediaEject"}

if I try passing an empty dictionary to virtual_media

Issue Type

Bug Report

Component Name

redfish_command

Ansible Version

[smalleni@unknown74d83e950f42 jetlag]$ ansible --version
ansible 2.9.21
  config file = /home/smalleni/Documents/code/jetlag/ansible.cfg
  configured module search path = ['/home/smalleni/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python3.8/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 3.8.7 (default, Jan 20 2021, 00:00:00) [GCC 10.2.1 20201125 (Red Hat 10.2.1-9)]

Configuration

[smalleni@unknown74d83e950f42 jetlag]$ ansible-config dump --only-changed
DEFAULT_CALLBACK_WHITELIST(/home/smalleni/Documents/code/jetlag/ansible.cfg) = ['profile_tasks']

OS / Environment

Fedora 32

Steps to Reproduce

    - name: DELL - Eject Virtual Media (if any)
      community.general.redfish_command:
        category: Manager
        command: VirtualMediaEject
        baseuri: "{{ hostvars[item]['bmc_address'] }}"
        username: "{{ hostvars[item]['bmc_user'] }}"
        password: "{{ hostvars[item]['bmc_password'] }}"
        virtual_media:
          image_url: "http://{{ http_store_host }}:{{ http_store_port }}/{{ hostvars[item]['boot_iso'] }}"
        resource_id: iDRAC.Embedded.1
      ignore_errors: yes

Expected Results

I expect to be able to unmount all virtual media without being required to know the urls of all the virtualmedia previously mounted.

Actual Results

TASK [boot-iso : DELL - Eject Virtual Media (if any)] *******************************************************************************************************************************
Wednesday 21 July 2021  12:48:53 -0500 (0:00:11.918)       0:02:23.541 ******** 
fatal: [e16-h12-b02-fc640.rdu2.scalelab.redhat.com]: FAILED! => {"changed": false, "msg": "image_url option required for VirtualMediaEject"}

Code of Conduct

[X] I agree to follow the Ansible Code of Conduct

ansibullbot commented 3 years ago

Files identified in the description:

plugins/modules/remote_management/redfish/redfish_command.py

If these files are inaccurate, please update the component name section of the description or use the !component bot command.

click here for bot help

ansibullbot commented 3 years ago

cc @mraineri @renxulei @tomasg2012 @xmadsen click here for bot help

smalleni commented 3 years ago

Looks like there is an explicit check for image_url: https://github.com/ansible-collections/community.general/blob/ea822c7bdd9cbeccc4541c2f95280442c6f213ab/plugins/module_utils/redfish_utils.py#L2325-L2327

And it is used to to find the virtual media to eject at: https://github.com/ansible-collections/community.general/blob/ea822c7bdd9cbeccc4541c2f95280442c6f213ab/plugins/module_utils/redfish_utils.py#L2347

mraineri commented 3 years ago

Yes, at least with the behavior we've implemented today the image URL is needed since the expectation is the user is specifying the image URL for the ISO to eject. I think we need to consider some things first before making changes.

If the image URL is not given, then what exactly should be ejected? All virtual media your example indicates? This seems a bit heavy handed in my opinion, but others might like this behavior. Redfish itself doesn't support an "eject all" type of operation, and I suspect the script you're referencing is either using OEM actions or is just looping on all slots and ejecting everything.

Should a user be allowed specify an alternative identifier (such as the "Id" of the virtual media instance) in order to control what slot is ejected?

Certainly would like opinions from others for desired behavior. I do like the idea of keeping the mandatory argument list as minimal as possible, but would like to agree upon the desired behavior first.

smalleni commented 3 years ago

Yes, at least with the behavior we've implemented today the image URL is needed since the expectation is the user is specifying the image URL for the ISO to eject. I think we need to consider some things first before making changes.

If the image URL is not given, then what exactly should be ejected? All virtual media your example indicates? This seems a bit heavy handed in my opinion, but others might like this behavior. Redfish itself doesn't support an "eject all" type of operation, and I suspect the script you're referencing is either using OEM actions or is just looping on all slots and ejecting everything.

Should a user be allowed specify an alternative identifier (such as the "Id" of the virtual media instance) in order to control what slot is ejected?

Certainly would like opinions from others for desired behavior. I do like the idea of keeping the mandatory argument list as minimal as possible, but would like to agree upon the desired behavior first.

Thank you for taking a look quickly. This is especially useful in our situation where the hardware nodes get recycled quite a bit between tenants and we might have things lingering around from previous tenants. Hence the need to remove any virtualmedia. I thought redifsh supported this but perhaps not then? I thought I saw

Method: POST
URI: https://<idrac_ip_address>/redfish/v1/Managers/iDRAC.Embedded.1/VirtualMedia/CD/Actions/VirtualMedia.EjectMedia
BODY:
{}

mraineri commented 3 years ago

That particular example ejects a virtual media instance called "CD"; there are other virtual media instances in the collection. Essentially you'd replace the segment "CD" in the URI for the action with another instance identifier (I think "USB" is another one used by iDRAC).

smalleni commented 3 years ago

That particular example ejects a virtual media instance called "CD"; there are other virtual media instances in the collection. Essentially you'd replace the segment "CD" in the URI for the action with another instance identifier (I think "USB" is another one used by iDRAC).

Got it, but since it doesn't require an image url, maybe even in ansible we can support ejecting all media instance of a particular media type?

mraineri commented 3 years ago

Maybe an explicit "all" option just for clarity? I'd be concerned with having default behavior eject everything since the other slot could be in use for another user.

smalleni commented 3 years ago

Maybe an explicit "all" option just for clarity? I'd be concerned with having default behavior eject everything since the other slot could be in use for another user.

Sure, but it's really useful for sysadmins to be able to eject all virtual media. Either it is a default option when you don't provide image_url or you have to explicitly ask for it, either is fine.

At the end of the day I want to be able to eject virtual media (either of a particular type like CD or of all types) without the image_url since I may not always know the image_url used by a user who had used the hardware previouly.

mraineri commented 3 years ago

The group has discussed this a bit and is okay with adding a dedicated "eject all" command in order to keep the semantics of the existing eject command as is.

Is this something you'd be able to work on?

ansibullbot commented 2 years ago

cc @bhavya06 @rajeevkallur click here for bot help

ansibullbot commented 2 years ago

cc @jose-delarosa click here for bot help

ansibullbot commented 2 years ago

Files identified in the description: