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.

Sign up for GitHub

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

Jump to bottom

[#438] Fix accessing unknown deployments #440

Merged
merged 13 commits into from
Sep 18, 2018
Merged

[#438] Fix accessing unknown deployments #440

merged 13 commits into from
Sep 18, 2018

Conversation

kirillmakhonin
Copy link
Member

This closes #438

@kirillmakhonin kirillmakhonin added the bug [Fixed] for any bug fixes. label Sep 7, 2018
@kirillmakhonin kirillmakhonin self-assigned this Sep 7, 2018
@kirillmakhonin kirillmakhonin requested review from akharlamov and a team as code owners September 7, 2018 09:12
@kirillmakhonin
Copy link
Member Author

ok to test

@legion-bot
Copy link
Collaborator

Build status: FAILURE

@kirillmakhonin
Copy link
Member Author

ok to test

@legion-bot
Copy link
Collaborator

Build status: FAILURE

@kirillmakhonin
Copy link
Member Author

Code should be replaced in order to 1) add retries to kube API reading function 2) raise error if cannot find deployment

@kirillmakhonin
Copy link
Member Author

ok to test

ablatov
ablatov previously approved these changes Sep 10, 2018
View reviewed changes
@legion-bot
Copy link
Collaborator

Build status: FAILURE

1 similar comment
@legion-bot
Copy link
Collaborator

Build status: FAILURE

@kirillmakhonin
Copy link
Member Author

ok to test

@legion-bot
Copy link
Collaborator

Build status: FAILURE

1 similar comment
@legion-bot
Copy link
Collaborator

Build status: FAILURE

@kirillmakhonin
Copy link
Member Author

ok to test

@legion-bot
Copy link
Collaborator

Build status: FAILURE

@legion-bot
Copy link
Collaborator

Build status: SUCCESS

@kirillmakhonin
Copy link
Member Author

Code should be replaced to unified function with API for iterable function call (retries)

@kirillmakhonin
Copy link
Member Author

ok to test

1 similar comment
@kirillmakhonin
Copy link
Member Author

ok to test

@legion-bot
Copy link
Collaborator

Build status: FAILURE

@kirillmakhonin
Copy link
Member Author

previous build has been aborted

@legion-bot
Copy link
Collaborator

Build status: SUCCESS

1 similar comment
@legion-bot
Copy link
Collaborator

Build status: SUCCESS

ablatov
ablatov previously approved these changes Sep 12, 2018
View reviewed changes
akharlamov
akharlamov requested changes Sep 13, 2018
View reviewed changes
Copy link
Member

@akharlamov akharlamov left a comment

Choose a reason for hiding this comment

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

Quick changes?

legion/legion/utils.py Outdated
timeout))
time.sleep(timeout)

raise Exception('No retries left')
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather return False here in case if the internal function does not succed. Then you can handle the error on a caller's side an provide more meaningful error message.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, will do

legion/legion/utils.py Outdated
@@ -588,3 +591,30 @@ def deduce_model_file_name(model_id, model_version):
return os.path.join(default_prefix, file_name)

return file_name


def retry_function_call(function_to_call, retries, timeout):
Copy link
Member

Choose a reason for hiding this comment

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

ensure_function_succed(...) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why?

@legion-bot
Copy link
Collaborator

Build status: FAILURE

@kirillmakhonin
Copy link
Member Author

ok to test

@legion-bot
Copy link
Collaborator

Build status: FAILURE

@kirillmakhonin
Copy link
Member Author

ok to test

@legion-bot
Copy link
Collaborator

Build status: SUCCESS

kirillmakhonin
kirillmakhonin commented Sep 17, 2018
View reviewed changes
legion/legion/k8s/services.py Outdated
client = legion.k8s.utils.build_client()

extension_api = kubernetes.client.ExtensionsV1beta1Api(client)

all_deployments = extension_api.list_namespaced_deployment(self._k8s_service.metadata.namespace)
model_deployments = [deployment for deployment in all_deployments.items
Copy link
Member Author

Choose a reason for hiding this comment

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

Change to next(

legion/legion/k8s/services.py Outdated
return

self._deployment = ensure_function_succeed(self._load_deployment_data_logic,
LOAD_DATA_ITERATIONS, LOAD_DATA_TIMEOUT)
self._deployment_data_loaded = True
Copy link
Member Author

Choose a reason for hiding this comment

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

Add logging

dsuslov
dsuslov previously approved these changes Sep 17, 2018
View reviewed changes
Copy link

@dsuslov dsuslov left a comment

Choose a reason for hiding this comment

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

👍

@kirillmakhonin kirillmakhonin force-pushed the feat/438-fix-model-service-deployment branch from b28e0b7 to 6066ca0 Compare September 18, 2018 06:42
@kirillmakhonin
Copy link
Member Author

ok to test (after rebase and adding some new logic)

dsuslov
dsuslov requested changes Sep 18, 2018
View reviewed changes
legion/legion/k8s/services.py Outdated
extension_api = kubernetes.client.ExtensionsV1beta1Api(client)

all_deployments = extension_api.list_namespaced_deployment(self._k8s_service.metadata.namespace)
try:
Copy link

Choose a reason for hiding this comment

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

the default value - None for the function next() would be better than try/except approach. See https://docs.python.org/2/library/functions.html#next

@legion-bot
Copy link
Collaborator

Build status: SUCCESS

dsuslov
dsuslov approved these changes Sep 18, 2018
View reviewed changes
Copy link

@dsuslov dsuslov left a comment

Choose a reason for hiding this comment

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

👍

@legion-bot
Copy link
Collaborator

Build status: FAILURE

@kirillmakhonin
Copy link
Member Author

ok to test

@dsuslov dsuslov merged commit bd686bf into develop Sep 18, 2018
@kirillmakhonin kirillmakhonin deleted the feat/438-fix-model-service-deployment branch September 19, 2018 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akharlamov akharlamov akharlamov requested changes

@dsuslov dsuslov dsuslov approved these changes

@ablatov ablatov ablatov left review comments

Assignees

@kirillmakhonin kirillmakhonin

Labels
bug [Fixed] for any bug fixes. critical
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inspect fails if cannot find deployment
5 participants
@kirillmakhonin @legion-bot @akharlamov @dsuslov @ablatov