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

External redis crash fix #571

Closed
HarryAtMove opened this issue Sep 13, 2018 · 4 comments
Closed

External redis crash fix #571

HarryAtMove opened this issue Sep 13, 2018 · 4 comments

Comments

@HarryAtMove
Copy link

HarryAtMove commented Sep 13, 2018

When specifying the redis_ip for the DockerContainerManager constructor, then connecting to Clipper an exception will be thrown similiar to the following.

    clipper_conn.start_clipper()
  File "/app/anaconda3/envs/ml-serve/lib/python3.6/site-packages/clipper_admin-develop-py3.6.egg/clipper_admin/clipper_admin.py", line 126, in start_clipper
    num_frontend_replicas)
  File "/app/anaconda3/envs/ml-serve/lib/python3.6/site-packages/clipper_admin-develop-py3.6.egg/clipper_admin/docker/docker_container_manager.py", line 246, in start_clipper
    self.connect()
  File "/app/anaconda3/envs/ml-serve/lib/python3.6/site-packages/clipper_admin-develop-py3.6.egg/clipper_admin/docker/docker_container_manager.py", line 265, in connect
    self.redis_port = all_labels[CLIPPER_DOCKER_PORT_LABELS['redis']]
KeyError: 'ai.clipper.redis.port'

The fix for this is to skip port look up and use the value of self.redis_port by replacing line 261 of clipper/clipper_admin/clipper_admin/docker/docker_container_manager.py with the following:

        if not self.external_redis:
            self.redis_port = all_labels[CLIPPER_DOCKER_PORT_LABELS['redis']]
@rkooo567
Copy link
Collaborator

@simon-mo Is it fixed in the current develop?

@DNCoelho
Copy link
Contributor

DNCoelho commented Jun 3, 2019

@rkooo567 I had this issue today while working with a fresh install from develop. I fixed it by simply performing a key check before the assignment. This is what I got at line 312 of DockerContainerManager:

if CLIPPER_DOCKER_PORT_LABELS['redis'] in all_labels:
    self.redis_port = all_labels[CLIPPER_DOCKER_PORT_LABELS['redis']]

By doing this the redis_port is not reassigned when an external value is passed, while still behaving normally in case it isn't.

@withsmilo
Copy link
Collaborator

@DNCoelho Good point. I think it's missed check. Would you please create a new PR?

@DNCoelho
Copy link
Contributor

DNCoelho commented Jun 4, 2019

@withsmilo Done 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants