Skip to content

Conversation

RobertLucian
Copy link
Member

@RobertLucian RobertLucian commented Apr 6, 2020

Closes #900, #950.

This PR includes support for public-accessible docker images (i.e. DockerHub) and ECR images under the cluster's AWS credentials.

Here is a set of errors that could possibly be returned by this new feature:

# some common ones that could appear due to setting an invalid value for image or tf_serve_image keys error: cortex.yaml: api-1: predictor: image: xxxxx.dkr.ecr.eu-central-1.amazonaws.com/cortexlabs/python-serve:custom-d is not accessible error: cortex.yaml: api-1: predictor: image: xxxxx.dkr.ecr.eu-central-1.amazonaws.com/cortexlabs/python-serve:custom:::dsfds is not a valid docker image resource error: cortex.yaml: api-1: predictor: image: robertlucian/private-sample-image is not accessible # some errors that should never happen (mostly internal use) error: cortex.yaml: api-1: predictor: image: can't extract ECR credentials error: cortex.yaml: api-1: predictor: image: index out of range: can't extract ECR credentials error: cortex.yaml: api-1: predictor: image: illegal base64 data at input byte 2504: can't extract ECR credentials error: cortex.yaml: api-1: predictor: failed to encode docker login credentials: json: <some error> error: cortex.yaml: api-1: predictor: image: ServerException: failed to retrieve ECR auth token: <insert actual error> 

checklist:

  • run make test and make lint
  • test manually (i.e. build/push all images, restart operator, and re-deploy APIs)
  • update docs and add any new files to summary.md (view in gitbook after merging)
Copy link
Member

@deliahu deliahu left a comment

Choose a reason for hiding this comment

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

@RobertLucian wow, amazing job on this!! I can see the effort you put in to match our conventions and find all code that should be updated, thank you for your attention to detail!

I reviewed all but the docker image accessibility check; I'll check that out soon. The main comment is that Vishal and I spoke, and we think it probably makes sense to remove the "user-facing" images from cluster configuration now that we have it in the API configuration, what do you think? I'd be happy to discuss

@RobertLucian RobertLucian requested a review from deliahu April 15, 2020 14:33
Copy link
Member

@deliahu deliahu left a comment

Choose a reason for hiding this comment

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

LGTM!

@deliahu deliahu merged commit 0493567 into cortexlabs:master Apr 15, 2020
@deliahu
Copy link
Member

deliahu commented Apr 15, 2020

@RobertLucian Thanks again for your great work on this!

@RobertLucian RobertLucian deleted the feature/overriden-base-images branch April 18, 2020 13:54
@RobertLucian RobertLucian mentioned this pull request Apr 23, 2020
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants