inference-model-manager icon indicating copy to clipboard operation
inference-model-manager copied to clipboard

Published 20 hours ago • verified publisher icon intel

Suggestions to improve installer

Open poussa opened this issue 5 years ago • 9 comments

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

  • [x] On Mac, don’t update brew every time the script is run
  • [x] Add option to specify external S3 storage
  • [x] Install ldap from helm subdir, not from tests
  • [x] Add option to uninstall (able to start over)
  • [x] Add (or document) the option not to use kops (case bare metal)
  • [ ] Add option to skip certificate generation, or document to reuse existing certs
  • [x] List -t option in help
  • [x] Make 3 min wait after ldap install optional or tunable
  • [x] Make the validation (at the end) optional
  • [ ] If helm minio is installed, create also the persistent volume so minio becomes usable

poussa avatar Mar 18 '19 09:03 poussa

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 ?

kbalka avatar Mar 18 '19 17:03 kbalka

@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 avatar Mar 18 '19 18:03 poussa

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

poussa avatar Mar 18 '19 18:03 poussa

@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

kbalka avatar Mar 19 '19 10:03 kbalka

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

  • List -t option in help
  • Add option to skip minio install

mareklevv avatar Mar 20 '19 13:03 mareklevv

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

kbalka avatar Mar 21 '19 12:03 kbalka

@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?

mareklevv avatar Mar 21 '19 13:03 mareklevv

  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?

poussa avatar Mar 21 '19 13:03 poussa

@poussa 2. 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.

mareklevv avatar Mar 21 '19 14:03 mareklevv