Add support for Linode's API v4 #1504

dimgal1 · 2020-10-13T11:12:30Z

Add Linode Compute Driver for APIv4

Description

This Pull Request adds Linode Compute Driver compatible with Linode's API v4 while simultaneously supporting the previous API version.

Status

done, ready for review

Checklist (tick everything that applies)

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

support for v3.

- Cast StorageVolume id to str - Change params in attach_volume to be in the same order as base class

Kami · 2020-11-05T22:07:28Z

Sorry for the delay.

This is a relatively large PR so it will take a while to review.

micafer · 2020-11-13T09:51:48Z

HI @dimgal1 and @Kami,

I'm interested in this new version.

I tested some of the functionality of the compute driver:

list_nodes
list_sizes
list_locations
list_volumes
create_node
create_volume
ex_resize_node
start_node
stop_node
reboot_node
destroy_node
destroy_volume

I found some minor issues I will put inline.

micafer · 2020-11-13T09:55:37Z

libcloud/compute/drivers/linode.py

+        'migrating': NodeState.MIGRATING,
+        'rebuilding': NodeState.UPDATING,
+        'cloning': NodeState.MIGRATING,
+        'restoring': NodeState.PENDING,


There is a missing state 'resizing'. I checked the API documentation and it does not appear. But I made some tests and it does exist. May be a good map value could be:
'resizing': NodeState.RECONFIGURING

micafer · 2020-11-13T09:57:38Z

libcloud/compute/drivers/linode.py

+            'location': data['region'],
+            'linode_id': data['linode_id'],
+            'linode_label': data['linode_label'],
+            'state': self.LINODE_VOLUME_STATES[data['status']]


filesystem_path' is an important value that is not returned in the extra fields:

'filesystem_path': data['filesystem_path']

dimgal1 · 2020-11-15T13:04:09Z

Hi @micafer,
made the recommendations you mentioned.
Thanks for the feedback!

codecov-io · 2020-11-16T21:39:58Z

Codecov Report

Merging #1504 (6b253ff) into trunk (c0c4743) will increase coverage by 0.05%.
The diff coverage is 88.33%.

@@            Coverage Diff             @@
##            trunk    #1504      +/-   ##
==========================================
+ Coverage   83.00%   83.06%   +0.05%     
==========================================
  Files         390      392       +2     
  Lines       83701    84667     +966     
  Branches     8882     8997     +115     
==========================================
+ Hits        69480    70332     +852     
- Misses      11222    11272      +50     
- Partials     2999     3063      +64

Impacted Files	Coverage Δ
libcloud/compute/drivers/linode.py	`66.98% <71.48%> (+4.24%)`	⬆️
libcloud/dns/drivers/linode.py	`82.80% <78.18%> (-5.03%)`	⬇️
libcloud/common/linode.py	`87.62% <95.74%> (+7.62%)`	⬆️
libcloud/test/dns/test_linode_v4.py	`97.08% <97.08%> (ø)`
libcloud/test/compute/test_linode_v4.py	`98.29% <98.29%> (ø)`
libcloud/test/compute/test_linode.py	`98.05% <100.00%> (ø)`
libcloud/test/dns/test_linode.py	`95.73% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0c4743...6b253ff. Read the comment docs.

Kami · 2020-12-06T16:27:02Z

@micafer Since you originally reviewed this PR, can you please have a look again when you get a chance?

And if everything looks OK, feel free to add a CHANGELOG entry and merge it into master.

micafer · 2020-12-07T07:25:17Z

@micafer Since you originally reviewed this PR, can you please have a look again when you get a chance?

And if everything looks OK, feel free to add a CHANGELOG entry and merge it into master.

OK, I will take a look.

micafer · 2020-12-07T09:07:12Z

libcloud/common/linode.py

+        status = int(self.status)
+        data = self.parse_body()
+        # Use only the first error, as there'll be only one most of the time
+        error = data['errors'][0]


Why not:
error = " ".join(data['errors'])
In this case all error messages will be returned, not only the first one.

data['errors'] is a list of dictionaries so I think this will not work.
Maybe it could be something like this?

Suggested change

error = data['errors'][0]

error_list = []

for error in data['errors']:

reason = error.get('reason')

# The field in the request that caused this error

field = error.get('field')

if field is not None:

msg = '%s-%s' % (reason, field)

else:

msg = reason

error_list.append(msg)

error_msg = ' '.join(error_list)

Yes, you are right. May be you can leave the original one. As you said in the first error is the most important one.

micafer · 2020-12-07T10:37:32Z

libcloud/compute/drivers/linode.py

+        :type volume: :class:`StorageVolume`
+
+        :param volume: Node to attach the volume to(required)
+        :type volume: :class:`Node`


It should be:
:param node:
:type node:

micafer · 2020-12-07T10:38:30Z

libcloud/compute/drivers/linode.py

+        :type volume: :class:`Node`
+
+        :keyword persist_across_boots: Node to attach the volume to(required)
+        :type volume: :class:`Node`


It should be:
:type persist_across_boots: bool

micafer · 2020-12-07T10:40:11Z

libcloud/compute/drivers/linode.py

+        :type node: :class:`Node`
+
+        :keyword address_type: Type of IP address
+        :type address_type: `bool`


It should be:
:type address_type: str

micafer · 2020-12-07T10:45:02Z

libcloud/compute/drivers/linode.py

+                                           method='POST').object
+        return self._to_address(response)
+
+    def ex_share_address(self, node, addresses=[]):


Why setting addresses as an optional keyword?
It is required to share an address.
I think it shoul be better:

def ex_share_address(self, node, addresses):

micafer · 2020-12-07T10:45:41Z

libcloud/compute/drivers/linode.py

+        :type node: :class:`Node`
+
+        :param size: the size of the new node
+        :type node: :class:`NodeSize`


It should be:
:type size:

micafer · 2020-12-07T11:30:41Z

Adding other methods as:
ex_get_node or ex_get_volume will be very useful.

dimgal1 · 2020-12-07T18:17:49Z

Thanks for the feedback @micafer .
Fixed the issues and added these two methods you mentioned.

micafer · 2020-12-09T09:13:24Z

LGTM.

Kami · 2020-12-13T19:01:26Z

Looks like that merging this "broke" automatically generated provider tables - it shows Linode DNS driver doesn't implement any methods.

I believe that's related to the way driver is now structured - our table generation script only changes methods on the base class.

I will work on a workaround.

Kami · 2020-12-13T19:10:22Z

Here we go - 85d6bbb :)

dimgal1 added 4 commits October 13, 2020 13:19

Add support for Linode's APIv4 and use it by default while maintaining

2e52fa1

support for v3.

Add tests for Linode's APIv4 driver

55e5e43

Add DNS Driver for Linode APIV4

8d2c9d3

Fix StorageVolume id and reverse params in attach_volume

1709d4f

- Cast StorageVolume id to str - Change params in attach_volume to be in the same order as base class

Kami added api: compute drivers: linode labels Nov 5, 2020

micafer reviewed Nov 13, 2020

View reviewed changes

micafer added the api: dns label Nov 13, 2020

Add resizing to node states and filesystem path on volumes

416004f

micafer reviewed Dec 7, 2020

View reviewed changes

micafer and others added 5 commits December 7, 2020 13:09

Add changelog entry

61653af

Merge branch 'trunk' into linode-api-v4

503c26c

Fix docstrings and update ex_share_address definition

2f906d8

Add ex_get_node, ex_get_volume methods and tests

ed8b0a6

Fix flake8 issues

6b253ff

micafer merged commit 137d41d into apache:trunk Dec 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Linode's API v4 #1504

Add support for Linode's API v4 #1504

dimgal1 commented Oct 13, 2020

Kami commented Nov 5, 2020

micafer commented Nov 13, 2020 •

edited

micafer Nov 13, 2020

micafer Nov 13, 2020

dimgal1 commented Nov 15, 2020

codecov-io commented Nov 16, 2020 •

edited

Kami commented Dec 6, 2020

micafer commented Dec 7, 2020

micafer Dec 7, 2020

dimgal1 Dec 7, 2020

micafer Dec 9, 2020

micafer Dec 7, 2020

micafer Dec 7, 2020

micafer Dec 7, 2020

micafer Dec 7, 2020

micafer Dec 7, 2020

micafer commented Dec 7, 2020

dimgal1 commented Dec 7, 2020 •

edited

micafer commented Dec 9, 2020

Kami commented Dec 13, 2020

Kami commented Dec 13, 2020

-        error = data['errors'][0]
+        error_list = []
+        for error in data['errors']:
+            reason = error.get('reason')
+            # The field in the request that caused this error
+            field = error.get('field')
+            if field is not None:
+                msg = '%s-%s' % (reason, field)
+            else:
+                msg = reason
+            error_list.append(msg)
+        error_msg = ' '.join(error_list)

Add support for Linode's API v4 #1504

Add support for Linode's API v4 #1504

Conversation

dimgal1 commented Oct 13, 2020

Add Linode Compute Driver for APIv4

Description

Status

Checklist (tick everything that applies)

Kami commented Nov 5, 2020

micafer commented Nov 13, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimgal1 commented Nov 15, 2020

codecov-io commented Nov 16, 2020 • edited

Codecov Report

Kami commented Dec 6, 2020

micafer commented Dec 7, 2020

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

micafer commented Dec 7, 2020

dimgal1 commented Dec 7, 2020 • edited

micafer commented Dec 9, 2020

Kami commented Dec 13, 2020

Kami commented Dec 13, 2020

micafer commented Nov 13, 2020 •

edited

codecov-io commented Nov 16, 2020 •

edited

dimgal1 commented Dec 7, 2020 •

edited