intel / inference-model-manager

Inference Model Manager for Kubernetes
Apache License 2.0
46 stars 8 forks source link

Suggestions to improve installer #58

Open poussa opened 5 years ago

poussa commented 5 years ago

The scripts in ./installer could be enhanced / fixed with the following:

kbalka commented 5 years ago

Hi, there are updates on master branch which are resolving some of the issues (checked). Option to skip kops cluster creation (-s) is documented in https://github.com/IntelAI/inference-model-manager/blob/master/installer/README.md. Maybe we should make it default ?

poussa commented 5 years ago

@kbalka About kops vs no kops option. Can we make it so, if you give the -k option it means use kops and if you don't it means no kops. With -s the -k is not needed even though the doc says it is required option. With single -k option we can cleanly handle both cases (and get rid of one option).

poussa commented 5 years ago

Hmm, 2nd thoughts. The does not work since you may want to use -k and -s at the same time (e.g., GCE case)

kbalka commented 5 years ago

@poussa I've checked the code, option -s is disabling -k. -k is required parameter if -s is not present. It think we can use -k for cluster installation with kops, and if -k is not present, it will expect cluster to be available before installation - so we can get rid of -s

mareklevv commented 5 years ago

@poussa In PR #70 i solved two of your issues:

kbalka commented 5 years ago

@poussa Waiting after LDAP is updated wtih #72 , now it checks for Ldap pod availability, is this solution ok ?

mareklevv commented 5 years ago

@poussa I prepared 2 pull requests which covered 3 of your proposed features. I hope they will be merged soon #70 #71 .

Also i have some questions about rest of proposed features. Can you explain this Add option to provide external IP address? Because now i can't see for why this should be implemented or i have wrong point of view. About Add option to skip certificate generation. Do you want to provide possibility to skip certificate generation and pass previously generated using envs? There is another option, where we can read content of directory with certificates and if we won't found expected certs, we can generate them. Do you want to allow users to skip generation of all external certs or you only want to pass certificate used to create tenant?

poussa commented 5 years ago
  1. external IP: this maybe solved with proper LB usage. I don't have none at the moment so I always patch the ingress service kubectl patch svc ingress-nginx -n ingress-nginx -p '{"spec":{"externalIPs": ["10.237.72.190"],"type": "LoadBalancer"}}' to get the install script progressing. I looped at the MetalLB today bit I can't use it at the office since I don't have a pool of IP addresses.

  2. If we can read the existing certificates from a dir, then we don't need the skip option. btw, what is that option?

mareklevv commented 5 years ago

@poussa

  1. I mean we can achieve skipping certificates generation in 2 ways:
    • Using envs with paths to certificates
    • Read directory with certificates(e.g. installer/certs), but this will required to put certificates to this directory with appropriate names.