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

Extract modelmesh part in helm chart #2704

Merged
merged 3 commits into from
Apr 23, 2023
Merged

Extract modelmesh part in helm chart #2704

merged 3 commits into from
Apr 23, 2023

Conversation

hhk7734
Copy link
Contributor

@hhk7734 hhk7734 commented Feb 19, 2023

What this PR does / why we need it:

  • For serverless installation without modelmesh

Which issue(s) this PR fixes :

x

Type of changes

x

Feature/Issue validation/testing:

helm template kserve ./charts/kserve-resources \
  -n kserve \
  --set kserve.modelmesh.enabled=false \
  > kserve-helm.yaml

kserve-helm.yaml and kserve.yaml should be approximately the same without CRDs.

Special notes for your reviewer:

x

Checklist:

  • Have you added unit/e2e tests that prove your fix is effective or that this feature works?
  • Has code been commented, particularly in hard-to-understand areas?
  • Have you made corresponding changes to the documentation?

Release note:

Add kserve.modelmesh.enabled option to kserve-resources helm chart

charts/kserve-resources/templates/clusterrolebinding.yaml Outdated Show resolved Hide resolved
charts/kserve-resources/templates/deployment.yaml Outdated Show resolved Hide resolved
charts/kserve-resources/templates/modelmesh/role.yaml Outdated Show resolved Hide resolved
charts/kserve-resources/templates/role.yaml Outdated Show resolved Hide resolved
charts/kserve-resources/templates/rolebinding.yaml Outdated Show resolved Hide resolved
@yuzisun
Copy link
Member

yuzisun commented Feb 28, 2023

@hhk7734 Thanks for the PR and it looks good ! just a few nits for fixing the new lines

@sukumargaonkar
Copy link
Contributor

should we make the modelmesh resources as a subchart?
that way we can enable or disable their creation via variables in values.yaml

Random example form google
Eg: https://stackoverflow.com/questions/54032974/helm-conditionally-install-subchart

@hhk7734
Copy link
Contributor Author

hhk7734 commented Mar 12, 2023

@yuzisun Please review again :)

@yuzisun
Copy link
Member

yuzisun commented Mar 16, 2023

@yuzisun Please review again :)

@hhk7734 have you considered the sub chart idea @sukumargaonkar commented ? This avoids all the if checks

@hhk7734
Copy link
Contributor Author

hhk7734 commented Mar 16, 2023

@yuzisun Please review again :)

@hhk7734 have you considered the sub chart idea @sukumargaonkar commented ? This avoids all the if checks

If this is turned into a subchart, version control for the modelmesh chart should also be considered. I am wondering if the kserve organization plans to manage this.

@yuzisun
Copy link
Member

yuzisun commented Mar 25, 2023

@yuzisun Please review again :)

@hhk7734 have you considered the sub chart idea @sukumargaonkar commented ? This avoids all the if checks

If this is turned into a subchart, version control for the modelmesh chart should also be considered. I am wondering if the kserve organization plans to manage this.

@hhk7734 modelmesh is always released together with kserve, we do not have separate cadence for modelmesh.

@hhk7734
Copy link
Contributor Author

hhk7734 commented Mar 27, 2023

@yuzisun If you want to apply subchart, I'll make the following changes. Do you want to use subchart?

CC. @sukumargaonkar


Directory layout

charts/
└── kserve-resources/
    ├── Chart.yaml
    ├── charts/
    │   └── modelmesh/
    │       ├── Chart.yaml
    │       └── templates/
    │           ├── clusterrole.yaml
    │           ├── clusterrolebinding.yaml
    │           ├── clusterservingruntimes.yaml
    │           ├── configmap.yaml
    │           ├── deployment.yaml
    │           ├── networkpolicy.yaml
    │           ├── role.yaml
    │           ├── rolebinding.yaml
    │           └── serviceaccount.yaml
    ├── templates/
    │   ├── certificate.yaml
    │   ├── clusterrole.yaml
    │   ├── clusterrolebinding.yaml
    │   ├── clusterservingruntimes.yaml
    │   ├── configmap.yaml
    │   ├── deployment.yaml
    │   ├── role.yaml
    │   ├── rolebinding.yaml
    │   ├── service.yaml
    │   ├── serviceaccount.yaml
    │   └── webhookconfiguration.yaml
    └── values.yaml

Add modelmesh Chart.yaml

apiVersion: v1
name: modelmesh
version: v0.10.0
description: Helm chart for deploying kserve resources
keywords:
  - kserve
  - modelmesh
sources:
  - http://github.com/kserve/kserve

Add modelmesh dependency to kserve-resources Chart.yaml

...
dependencies:
  - name: modelmesh
    version: v0.10.0
    condition: modelmesh.enabled

Change the hierarchy of kserve-resources values.yaml

kserve:
  ...
  modelmesh:
    enabled: true
    ...
  ...

to

kserve:
  ...
modelmesh:
  enabled: true
  ...

Change the modemesh values

        image: {{ .Values.kserve.modelmesh.controller.image }}:{{ .Values.kserve.modelmesh.controller.tag }}

to

        image: {{ .Values.controller.image }}:{{ .Values.controller.tag }}

@hhk7734
Copy link
Contributor Author

hhk7734 commented Apr 4, 2023

@yuzisun

How about reflecting this PR without modification first for backwards compatibility, and later if a subchart is needed?

@hhk7734 hhk7734 requested a review from yuzisun April 10, 2023 13:36
Signed-off-by: Hyeonki Hong <hhk7734@gmail.com>
Signed-off-by: Hyeonki Hong <hhk7734@gmail.com>
Signed-off-by: Hyeonki Hong <hhk7734@gmail.com>
@hhk7734
Copy link
Contributor Author

hhk7734 commented Apr 23, 2023

@yuzisun

Reflected changes in the master chart.

@yuzisun
Copy link
Member

yuzisun commented Apr 23, 2023

Thanks @hhk7734 !

/lgtm
/approve

@kserve-oss-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hhk7734, yuzisun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kserve-oss-bot kserve-oss-bot merged commit e3ecb47 into kserve:master Apr 23, 2023
52 checks passed
Iamlovingit pushed a commit to Iamlovingit/kserve that referenced this pull request Oct 1, 2023
* Extract modelmesh part in helm chart

Signed-off-by: Hyeonki Hong <hhk7734@gmail.com>

* Add last newline to helm chart templates

Signed-off-by: Hyeonki Hong <hhk7734@gmail.com>

* reflect 2b5a770 changes (kserve#2627)

Signed-off-by: Hyeonki Hong <hhk7734@gmail.com>

---------

Signed-off-by: Hyeonki Hong <hhk7734@gmail.com>
Signed-off-by: iamlovingit <freecode666@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants