Skip to content

zFCP improvements#650

Merged
joseivanlopez merged 6 commits intoagama-project:masterfrom
joseivanlopez:zfcp-improvements
Jul 6, 2023
Merged

zFCP improvements#650
joseivanlopez merged 6 commits intoagama-project:masterfrom
joseivanlopez:zfcp-improvements

Conversation

@joseivanlopez
Copy link
Copy Markdown
Contributor

@joseivanlopez joseivanlopez commented Jul 5, 2023

Problem

Two things to fix/improve:

  • Automatically activated zFCP disks are not probed after activating a controller.
  • There is no info about allow_lun_scan when the option is disabled.

Solution

  • Delay zFCP probing in order to give some time to disk activation.
  • The system is marked as deprecated after probing, if needed.
  • allow_lun_scan status is always informed.
  • The allow_lun_scan explanation is improved.

Bonus: members of a device are sorted in the disk selector.

Testing

  • Adapted unit tests
  • Tested manually

Screenshots

Show/hide

localhost_8080_ (13)

localhost_8080_ (14)

@coveralls
Copy link
Copy Markdown

coveralls commented Jul 5, 2023

Coverage Status

Changes unknown
when pulling f51afe1 on joseivanlopez:zfcp-improvements
into ** on openSUSE:master**.

@joseivanlopez joseivanlopez marked this pull request as ready for review July 5, 2023 14:23
Copy link
Copy Markdown
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

Just some minor comments on the changes file format.

About the fixes, they look good. We might need to find a better way than a call to sleep, but we do not have any now.

What does it happen if it takes more than 2 seconds? Would the user eventually see the list of disks? And, in that case, can the user select one of them for installation?

@joseivanlopez joseivanlopez force-pushed the zfcp-improvements branch 2 times, most recently from 4f6f8cd to e15f5d9 Compare July 5, 2023 16:14
- zFCP disk changes are detected with each probing and callbacks
  are run if needed.
@joseivanlopez
Copy link
Copy Markdown
Contributor Author

joseivanlopez commented Jul 6, 2023

What does it happen if it takes more than 2 seconds? Would the user eventually see the list of disks? And, in that case, can the user select one of them for installation?

According to our tests, 2 seconds should be more than enough. Anyway, if exporting disks takes longer and the disks are not read, then zFCP has to be probed again. Right now there is nothing in the UI for doing the reprobing. To force a zFCP probing you have to leave the zFCP page (going back) and visit it again. The system is properly marked as deprecated (if needed) once the list of zFCP disks is read.

As said, this should not happen with the sleep delay. And I would avoid adding a button for manually re-read zFCP only just in case. What do you think?

Update: After discussing it, this issue should be solved in a more generic way, see #652.


return unless disks_changed?(previous_disks)

@on_disks_change_callbacks&.each { |c| c.call(disks) }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

np: Is it possible that @on_disks_change_callbacks is nil? Not something to change in this PR, but I rather prefer to have an empty array.

Copy link
Copy Markdown
Contributor Author

@joseivanlopez joseivanlopez Jul 6, 2023

Choose a reason for hiding this comment

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

Yes, because it is initialize on assignement, see https://github.com/openSUSE/agama/blob/master/service/lib/agama/storage/zfcp/manager.rb#L51. We could avoid it by initializing the list in the constructor or by adding a #on_disk_change_callbacks method.

@joseivanlopez joseivanlopez merged commit e8be9bb into agama-project:master Jul 6, 2023
@imobachgs imobachgs mentioned this pull request Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants