Change k8s ServiceTypes to be changeable #667
Conversation
When creating a KubernetesManager object, you could define own ServiceTypes. For example,
cm = KubernetesContainerManager(
....,
service_types={
'redis': 'NodePort',
'management': 'LoadBalancer',
'query': 'LoadBalancer',
'query-rpc': 'ClusterIP',
'metric': 'LoadBalancer'})
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
|
@withsmilo Thanks for this patch. However, I think we also need to patch how |
| @@ -168,6 +192,18 @@ def __init__(self, | |||
| self.cluster_identifier = "{ns}-{cluster}".format( | |||
| ns=self.k8s_namespace, cluster=self.cluster_name) | |||
|
|
|||
| self.service_types = DEFAULT_CLIPPER_SERVICE_TYPES | |||
There was a problem hiding this comment.
Why don't we make this part as a function and add some tests?
There was a problem hiding this comment.
@rkooo567 , OK. Let me refactor this part.
There was a problem hiding this comment.
@rkooo567 , What tests are we needed here in additional?
There was a problem hiding this comment.
I thought just a simple unit test? Like if this function raises Exception correctly when wrong input is given and etc. I could find that the current CI fails on line 200 (when you raise ClipperException), so it can be good if we can write some unit tests inside one of integration test files.
There was a problem hiding this comment.
@withsmilo @rkooo567 can we add
def set_clipper_svc_types(self, svctype, svcname):
for key in self.OFFICIAL_K8S_SERVICE_TYPE:
if(svctype.lower()==key.lower()):
self.DEFAULT_CLIPPER_SERVICE_TYPES[svcname]=key
|
@simon-mo , I will try to add something to need. |
|
Test FAILed. |
|
Test PASSed. |
|
Test PASSed. |
|
|
||
| query_frontend_ports = self._k8s_v1.read_namespaced_service( | ||
| if self.service_types['management'] in [CLUSTER_IP, NODE_PORT]: |
There was a problem hiding this comment.
Why don't we read service_types from our Kubernetes definitions (yaml file) in the connect function? Otherwise we should always specify the correct self.service_types whenever we initiate KubernetesContainerManager.
It shouldn't matter when we initiate KubernetesContainerManager for the first time for start_clipper. But, let's say we already started clipper and just wanted to connect to the cluster using ClipperConnection(KubernetesContainerManager()).connect(). If you forget to write service_types, it will be set to be None and this part might be broken.
There was a problem hiding this comment.
@rkooo567 You're right. Thank you for your kind advice. I will fix it as soon as possible.
There was a problem hiding this comment.
@rkooo567 : According to your advice, I added a new mechanism to read/write service_types from/to a yaml file. You can connect to the executing Clipper cluster easily with service_types(None), which will be loaded automatically from a yaml file saved when we started it. If you call 'stop_all()', the yaml file will be removed.
There was a problem hiding this comment.
Thank you! Everything looks good to me :)
|
Test PASSed. |
|
Test FAILed. |
|
Test PASSed. |
|
Test PASSed. |
|
Test PASSed. |
|
@simon-mo : Please review this PR. Can we use 'LoadBalancer' service type with Travis CI? |
|
Test PASSed. |
|
This PR looks good to me. But since it is not a simple PR, I will wait for @simon-mo's approval. |
|
Test PASSed. |
When creating a KubernetesManager object, you could define own ServiceTypes. For example,