You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@libcloud.apache.org by GitBox <gi...@apache.org> on 2020/07/21 16:52:32 UTC
[GitHub] [libcloud] Eis-D-Z opened a new pull request #1477: Add expose ports through services for VMs, extra ports and services d…
Eis-D-Z opened a new pull request #1477:
URL: https://github.com/apache/libcloud/pull/1477
## Add capability of exposing ports for VMs
### Description
Added methods to create services through which ports can be exposed for VMs, each VM will have one service for each type through which multiple ports can be exposed.
### Status
- done, ready for review
### Checklist (tick everything that applies)
- [x] [Code linting](http://libcloud.readthedocs.org/en/latest/development.html#code-style-guide) (required, can be done after the PR checks)
- [ ] Documentation
- [x] [Tests](http://libcloud.readthedocs.org/en/latest/testing.html)
- [x] [ICLA](http://libcloud.readthedocs.org/en/latest/development.html#contributing-bigger-changes) (required for bigger changes)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] Kami commented on pull request #1477: Add expose ports through services for VMs, extra ports and services d…
Posted by GitBox <gi...@apache.org>.
Kami commented on pull request #1477:
URL: https://github.com/apache/libcloud/pull/1477#issuecomment-683431311
Thanks for addressing the feedback - I merged changes into trunk.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] Eis-D-Z commented on pull request #1477: Add expose ports through services for VMs, extra ports and services d…
Posted by GitBox <gi...@apache.org>.
Eis-D-Z commented on pull request #1477:
URL: https://github.com/apache/libcloud/pull/1477#issuecomment-682354847
Thanks for pointing out those issues. They have been fixed!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] Kami commented on pull request #1477: Add expose ports through services for VMs, extra ports and services d…
Posted by GitBox <gi...@apache.org>.
Kami commented on pull request #1477:
URL: https://github.com/apache/libcloud/pull/1477#issuecomment-667665888
Thanks for the contribution.
I added some comments in-line.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] Kami commented on a change in pull request #1477: Add expose ports through services for VMs, extra ports and services d…
Posted by GitBox <gi...@apache.org>.
Kami commented on a change in pull request #1477:
URL: https://github.com/apache/libcloud/pull/1477#discussion_r464069120
##########
File path: libcloud/compute/drivers/kubevirt.py
##########
@@ -167,10 +165,17 @@ def destroy_node(self, node):
"""
namespace = node.extra['namespace']
name = node.name
- # stop the vmi first
+ # find and delete services for this VM only
+ services = self.ex_list_services(namespace=namespace, node_name=name)
+ for service in services:
+ service_type = service['spec']['type']
+ self.ex_create_service(node=node, ports=[],
Review comment:
I think it would be better to have another ``ex_delete_service`` method which can call ``ex_create_service`` underneath with those arguments.
Also, from the method docstring it looks like you may need to specify ``override_existing_ports`` argument. Or is it not needed?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] Eis-D-Z commented on a change in pull request #1477: Add expose ports through services for VMs, extra ports and services d…
Posted by GitBox <gi...@apache.org>.
Eis-D-Z commented on a change in pull request #1477:
URL: https://github.com/apache/libcloud/pull/1477#discussion_r475517680
##########
File path: libcloud/compute/drivers/kubevirt.py
##########
@@ -167,10 +165,17 @@ def destroy_node(self, node):
"""
namespace = node.extra['namespace']
name = node.name
- # stop the vmi first
+ # find and delete services for this VM only
+ services = self.ex_list_services(namespace=namespace, node_name=name)
+ for service in services:
+ service_type = service['spec']['type']
+ self.ex_create_service(node=node, ports=[],
Review comment:
Hey,
An empty ports will result in the service being deleted. The `override_existing_ports` argument was added because there is no general standard in case there already are some ports forwarded and a user requests additional ports to be opened. Sometimes he might want to keep the previous, sometimes not. I thought it would be better to spare the user from manually deleting in the second case (also I was not brilliant enough to have an `ex_delete_service` which makes a lot of sense).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] asfgit merged pull request #1477: Add expose ports through services for VMs, extra ports and services d…
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #1477:
URL: https://github.com/apache/libcloud/pull/1477
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] Kami commented on a change in pull request #1477: Add expose ports through services for VMs, extra ports and services d…
Posted by GitBox <gi...@apache.org>.
Kami commented on a change in pull request #1477:
URL: https://github.com/apache/libcloud/pull/1477#discussion_r464069328
##########
File path: libcloud/compute/drivers/kubevirt.py
##########
@@ -1022,3 +1096,143 @@ def _to_node(self, vm, is_stopped=False):
driver=driver, size=size,
image=image, extra=extra,
created_at=created_at)
+
+ def ex_list_services(self, namespace='default', node_name=None,
+ service_name=None):
+ '''
+ If node_name is given then the services returned will be those that
+ concern the node
+ '''
+ params = None
+ if service_name is not None:
+ params = {'fieldSelector': 'metadata.name={}'.format(service_name)}
+ req = ROOT_URL + '/namespaces/{}/services'.format(namespace)
+ result = self.connection.request(req, params=params).object['items']
+ if node_name:
+ res = []
+ for service in result:
+ if node_name in service['metadata'].get('name', ""):
+ res.append(service)
+ return res
+ return result
+
+ def ex_create_service(self, node, ports, service_type="NodePort",
+ cluster_ip=None, load_balancer_ip=None,
+ override_existing_ports=False):
+ '''
+ Each node has a single service of one type on which the exposed ports
+ are described. If a service exists then the port declared will be
+ exposed alongside the existing ones, set override_existing_ports=True
+ to delete existing exposed ports and expose just the ones in the port
+ variable.
+
+ param node: the libcloud node for which the ports will be exposed
+ type node: libcloud `Node` class
+
+ param ports: a list of dictionaries with keys --> values:
+ 'port' --> port to be exposed on the service
+ 'target_port' --> port on the pod/node, optional
+ if empty then it gets the same
+ value as 'port' value
+ 'protocol' ---> either 'UDP' or 'TCP', defaults to TCP
+ 'name' --> A name for the service
+ If ports is an empty `list` and a service exists of this
+ type then the service will be deleted.
+ type ports: `list` of `dict` where each `dict` has keys --> values:
+ 'port' --> `int`
+ 'target_port' --> `int`
+ 'protocol' --> `str
+ 'name' --> str
+
+ param service_type: Valid types are ClusterIP, NodePort, LoadBalancer
+ type service_type: `str`
+
+ param cluster_ip: This can be set with an IP string value if you want
+ manually set the service's internal IP. If the value
+ is not correct the method will fail, this value can't
+ be updated.
+ type cluster_ip: `str`
+
+ param override_existing_ports: Set to True if you want to delete the
+ existing ports exposed by the service
+ and keep just the ones declared in the
+ present ports argument.
+ By default it is false and if the
+ service already exists the ports will be
+ added to the existing ones.
+ type override_existing_ports: `boolean`
+ '''
+ # check if service exists first
+ namespace = node.extra.get('namespace', 'default')
+ service_name = 'service-{}-{}'.format(service_type.lower(), node.name)
+ service_list = self.ex_list_services(namespace=namespace,
+ service_name=service_name)
+
+ ports_to_expose = []
+ # if ports has a falsey value like None or 0
+ if not ports:
+ ports = []
+ for port_group in ports:
+ if not port_group.get('target_port', None):
+ port_group['target_port'] = port_group['port']
+ if not port_group.get('name', ""):
+ port_group['name'] = 'port-{}'.format(port_group['port'])
+ ports_to_expose.append(
+ {'protocol': port_group.get('protocol', 'TCP'),
+ 'port': int(port_group['port']),
+ 'targetPort': int(port_group['target_port']),
+ 'name': port_group['name']})
+ headers = None
+ data = None
+ if len(service_list) > 0:
Review comment:
Per my comment above, I think it would be better to be explicit and have another ``ex_delete_service`` method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] Kami commented on a change in pull request #1477: Add expose ports through services for VMs, extra ports and services d…
Posted by GitBox <gi...@apache.org>.
Kami commented on a change in pull request #1477:
URL: https://github.com/apache/libcloud/pull/1477#discussion_r464069228
##########
File path: libcloud/compute/drivers/kubevirt.py
##########
@@ -181,7 +186,8 @@ def destroy_node(self, node):
# only has container disk support atm with no persistency
def create_node(self, name, image, location=None, ex_memory=128, ex_cpu=1,
ex_disks=None, ex_network=None,
- ex_termination_grace_period=0):
+ ex_termination_grace_period=0,
+ ports={}):
Review comment:
Please don't use mutable defaults - https://florimond.dev/blog/articles/2018/08/python-mutable-defaults-are-the-source-of-all-evil/#:~:text=In%20Python%2C%20when%20passing%20a,or%20even%20a%20class%20instance.
Doing ``ports=None`` and then inside method ``ports = ports or {}`` is better and safer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org