libcloud icon indicating copy to clipboard operation
libcloud copied to clipboard

chore: Remove storage and volume interface implementation

Open aayushrangwala opened this issue 2 years ago • 6 comments

Changes Title (replace this with a logical title for your changes)

Remove unused and unsupported apis from equinix metal

Description

  • Remove implementation of storage APIs as they were removed from Packet years ago. there is no direct replacement. Equinix Metal offers Pure and NetApp storage devices today but there is not equivalent functionality as far as the Equinix Metal API is concerned with directly attaching storage to servers and equinix metal doesnot supports only 2 storage providers and

For more information on contributing, please see Contributing section of our documentation.

Status

Ready For Review

Checklist (tick everything that applies)

  • [x] Code linting (required, can be done after the PR checks)
  • [x] Documentation
  • [x] Tests
  • [x] ICLA (required for bigger changes)

aayushrangwala avatar Nov 12 '23 09:11 aayushrangwala

Reviewer note: The Elastic Block storage feature was removed from Equinix Metal in June 2021. https://web.archive.org/web/20210620005004/https://metal.equinix.com/developers/docs/storage/elastic-block-storage/#elastic-block-storage

displague avatar Nov 13 '23 12:11 displague

@Kami is this something that you can review and merge? There are three Equinix Metal PRs up. I have reviewed them on their Equinix Metal merits.

displague avatar Nov 15 '23 11:11 displague

Thanks for the contribution.

Since this is a breaking change, can you please add an entry in upgrade notes file (docs/upgrade_notes.rst)? Thanks.

Kami avatar Dec 03 '23 10:12 Kami

Codecov Report

Merging #1972 (86ff77e) into trunk (dfbb595) will increase coverage by 0.06%. Report is 10 commits behind head on trunk. The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##            trunk    #1972      +/-   ##
==========================================
+ Coverage   83.20%   83.25%   +0.06%     
==========================================
  Files         353      353              
  Lines       81423    81252     -171     
  Branches     8591     8559      -32     
==========================================
- Hits        67742    67645      -97     
+ Misses      10874    10820      -54     
+ Partials     2807     2787      -20     
Files Coverage Δ
libcloud/compute/drivers/equinixmetal.py 70.54% <ø> (+6.18%) :arrow_up:
libcloud/test/compute/test_equinixmetal.py 92.89% <ø> (+2.76%) :arrow_up:

codecov-commenter avatar Dec 03 '23 10:12 codecov-commenter

@aayushrangwala Can you please sync up this branch with a latest trunk? I wanted to wrap it up myself locally, but I noticed more work is needed to make this change complete - it appears create_node() still calls create_volume() / attach_volume(), etc.

Please let me know when that has been addressed and when all checks are passing and I will have a look again.

On that note, it would also be good to update PR description with some context and why this change is needed (I know that context may be available in other PR, but we should also update this PR description).

Thanks.

Kami avatar Dec 03 '23 11:12 Kami

@Kami can you please review again, Ive rebased and updated.

aayushrangwala avatar Jan 18 '24 16:01 aayushrangwala

@antoinebourayne I had a look and it looks like this comment is still relevant - https://github.com/apache/libcloud/pull/1972#issuecomment-1837447866

I see create_node() method is still calling volume related methods which have been removed. create_node() should be updated to remove those calls and disk + disk_size argument should be removed as well (since it doesn't make sense / it's unused without corresponding volume management methods).

Kami avatar Apr 18 '24 16:04 Kami

I made and pushed the following changes myself:

  • Remove disk and disk_size argument from create_node method - daddbae3fe699dcc708e2f7585a5d6865875ce7a
  • Add upgrade notes entry - 70a5f872c97d771dab2c5fe4e66a9b8a1d3aaf8a
  • Remove volumes from list_resources_async() - fc48cacde37b21498e4c7f1d597392df238c3c3f
  • Fix lint - 84b68be73045532110aa92eaa0b05873663ed994

With those changes, the code has now also been merged into trunk.

Kami avatar Apr 18 '24 16:04 Kami