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

Updates on CloudSigma driver #1558

Merged
merged 1 commit into from Oct 22, 2021
Merged

Conversation

dimgal1
Copy link
Contributor

@dimgal1 dimgal1 commented Mar 1, 2021

Updates on CloudSigma driver

Description

  • Add all supported regions
  • Use example sizes currently provided by CloudSigma
  • Change methods parameters to use actual cores to define CPUs instead of MHz
  • Change Drive methods parameters to use GBs instead of bytes
  • Implement SSH key related methods ( list_key_pairs, get_key_pair, create_key_pair, import_key_pair_from_string, delete_key_pair), add ssh keys param on create_node
  • Implement ex_attach_drive, ex_detach_drive, ex_destroy_drive, reboot_node, ex_get_node
  • Make deleting all attached drives on destroy_node optional
  • Add tests on methods added
  • Add volume methods which call the appropriate ex_drive method ( since CloudSigma drives are pretty much volumes in the context of libcloud, I thought this makes sense)

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)

@@ -1138,10 +1148,13 @@ def create_node(self, name, size, image, ex_metadata=None,
# ide 0:0
data = {}
data['name'] = name
data['cpu'] = size.cpu
data['cpu'] = size.cpu * 2000
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to add a comment on why we need to do * 2000 here.

@@ -1194,19 +1207,55 @@ def create_node(self, name, size, image, ex_metadata=None,

return node

def destroy_node(self, node):
def destroy_node(self, node, delete_drives=False):
Copy link
Member

Choose a reason for hiding this comment

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

Since it's not a standard argument, please prefix it with ex_.

stopped = True

if not stopped:
raise CloudSigmaException(
Copy link
Member

Choose a reason for hiding this comment

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

I guess there is no way to retrieve a better error - aka why stopping node failes?

Copy link
Contributor Author

@dimgal1 dimgal1 May 9, 2021

Choose a reason for hiding this comment

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

I wasn't sure about this one, I used the return value only because the base NodeDriver returns a boolean so I handled the case of False.
Practically stopped was never False, CloudSigma returns 403 status code if the action cannot be done so an exception is raised or the action is completed and stopped is True.
Let me know if I should remove this completely

image = '{} {}'.format(drive.extra.get('distribution', ''),
drive.extra.get('version', ''))
break
# try to find if node size is from example sizes given by CloudSigma
Copy link
Member

Choose a reason for hiding this comment

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

It seems somewhat inefficient to run this on every to _to_node() method call (even if that list_nodes() method doesn't result in a HTTP request and just filters a local list).

Is there any way we could handle this better / more efficiently?

Perhaps we could build a module level dictionary constant where the key would be a tuple of (cpus, memory, drive_size) and the value would be NodeSize. Then we can just do a simple dictionary lookup.

I think this should be fine since there isn't a lot of pre-defined sizes,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used a dictionary as you said, now we could remove completely INSTANCE_TYPES and use only the new dictionary's values to list sizes as they are basically the same. Let me know what you think.

@Kami
Copy link
Member

Kami commented May 2, 2021

Thanks for the contribution.

I added some comments, nothing major.

@dimgal1 dimgal1 force-pushed the cloudsigma-updates branch 2 times, most recently from 1ebba0a to 948c938 Compare May 9, 2021 17:49
- add all supported regions
- update example sizes
- implement SSH key methods, reboot_node, ex_get_node, attach/detach
disk
- Use CPU cores to define CPU size instead of MHz and GBs to define
drive size
Copy link
Member

@Kami Kami left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all the comments. LGTM 👍

asfgit pushed a commit that referenced this pull request Oct 22, 2021
@asfgit asfgit merged commit 42c6780 into apache:trunk Oct 22, 2021
d-mo pushed a commit to mistio/libcloud that referenced this pull request Nov 15, 2021
d-mo pushed a commit to mistio/libcloud that referenced this pull request Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants