search

eclipse-openj9 / openj9-utils

Other
16 stars 29 forks source link

Initial commit for the OpenJ9 JITServer Helm Chart #8

Closed chrisc66 closed 3 years ago

chrisc66 commented 3 years ago

As a continuous part of JITServer technology deployment discussion, this PR contains the OpenJ9 JITServer Helm Chart that allows JITServer instances to be deployed and managed on K8s and OpenShift clusters.

This PR includes:

Reference: https://github.com/eclipse/openj9/issues/9443

Signed-off-by: Chris Chong Zichun.Chong@ibm.com

chrisc66 commented 3 years ago

Additional information on licensing.

The current helm chart template is created by the helm CLI with this command:

helm create CHART_NAME

The documentation for helm and helm create https://helm.sh/docs/ https://helm.sh/docs/helm/helm_create/

If there is any concern about the license, the helm CLI repo is using Apache 2.0. Everything generated by the helm CLI should be also under the same license, including the chart template and we can add other licenses on top. https://github.com/helm/charts/blob/master/LICENSE

chrisc66 commented 3 years ago

As discussed in https://github.com/eclipse/openj9/issues/11587, I will first create a separate PR that stores the binary helm charts in a separate branch. Then remove binary charts and update helm repo URL in README in this PR.

chrisc66 commented 3 years ago

I have pushed the latest commit with helm repo URL and removed binary charts from this PR. Before merging this PR, I would like to create a new branch helmcharts in openj9-utils that stores binary helm charts. Since the latest commit sets https://raw.githubusercontent.com/eclipse/openj9-utils/helmcharts/ as the helm repo, which doesn't exist before creating the helmcharts branch.

I created a new branch in my personal forked repo, however not able to create a new branch in this upstream repo. https://github.com/chrisc66/openj9-utils/tree/helmcharts

mpirvu commented 3 years ago

@chrisc66 I created the "helmcharts" branch

chrisc66 commented 3 years ago

Thanks, created a new PR into helmcharts branch. Please review and merge #11, then I will double check the helm repo URL for the final round before merging this PR.

chrisc66 commented 3 years ago

As discussed in the helm chart issue, let's take the GitHub release approach and I have added index.yaml back. The other binary helm chart PR #11 isn't needed anymore.

In summary, this PR contains below files and includes:

$ tree helm-chart/
helm-chart/
├── index.yaml
└── openj9-jitserver-chart
    ├── Chart.yaml
    ├── enable-jitserver.md
    ├── README.md
    ├── templates
    │   ├── deployment.yaml
    │   ├── _helpers.tpl
    │   ├── NOTES.txt
    │   ├── serviceaccount.yaml
    │   ├── service.yaml
    │   └── tests
    │       └── test-connection.yaml
    └── values.yaml
  1. a helm chart (helm-chart/openj9-jitserver-chart), and
  2. an index.yaml (helm-chart/index.yaml) for the helm CLI
  3. a binary helm chart (included at the beginning of this PR as a file)

I have verified hosting binary charts on GitHub release and storing index.yaml in repo is possible and doesn't confuse helm, which was my concern of this approach. I created a new repo to test it out, everything worked as expected. This repo is temporary and will be deleted after this PR is merged. https://github.com/chrisc66/helmcharts

chrisc66 commented 3 years ago

Two final questions before being ready to merge.

  1. Are we going to host binary charts in openj9 (the main repo) release or openj9-utils (this repo) release? The URL in index.yaml needs to be updated once the binary chart is released.
  2. Are we going to include this chart as part of the 0.24 release or later? The current chart version is 0.24.0 which is fine if included in 0.24. Otherwise, I will need to update the chart version to 0.25.
pshipton commented 3 years ago

My vote is to host the binary charts in this repo. See also https://github.com/eclipse/openj9/issues/11587#issuecomment-760237740. They will be hosted by attaching them to an Issue/PR. Just drag and drop them to the Issue/PR to get a link.

chrisc66 commented 3 years ago

Thanks for the above suggestion, the binary helm chart (tar.gz) has been uploaded to the PR description at the top of this page.

Looks like all issues are solved for now. Once the CQ is done, this PR should be ready to be merged. Please let me know if there are other concerns.

mpirvu commented 3 years ago

I created an Eclipse CQ: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=22984

mpirvu commented 3 years ago

CQ was approved: "--- Comment #3 from Shawn Kilpatrick shawn.kilpatrick@eclipse-foundation.org 2021-01-21 08:27:33 --- We have completed a high level triage of the attachment and you may now check the content into your project’s source code repository."

mpirvu commented 3 years ago

I am ok with this HelmChart PR. @DanHeidinga I'll be waiting for your approval before merging.

DanHeidinga commented 3 years ago

Some minor nitpicks and two questions about licensing. The licensing questions should be resolved before committing but the nitpicks can be handled separately if that's easier

mpirvu commented 3 years ago

@DanHeidinga Dan, if you are ok with the latest changes from @chrisc66 , could you please approve this PR? Thank you