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

Update README #327

Closed
wants to merge 1 commit into from
Closed

Conversation

thandayuthapani
Copy link
Contributor

Add steps to download helm and setup helm with appropriate serviceaccount

For Issue: #310
#310 (comment)

@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: thandayuthapani
To complete the pull request process, please assign hex108
You can assign the PR to them by writing /assign @hex108 in a comment when ready.

The full list of commands accepted by this bot can be found 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

@volcano-sh-bot volcano-sh-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 11, 2019
@@ -63,7 +63,30 @@ You can watch industry experts talking about Volcano in different International

The easiest way to deploy Volcano is to use the Helm chart. Volcano can be deployed by cloning code and also by adding helm repo.

## Using Volcano Helm Repo
## Install Helm
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about make this under quick start guide and change it to Install Volcano?

Copy link
Contributor

@TommyLike TommyLike Jul 11, 2019

Choose a reason for hiding this comment

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

Why not we provide the helm document link here and note about the RBAC issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TommyLike Sure, will change that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TommyLike Done

Copy link
Member

Choose a reason for hiding this comment

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

it's better to highlight chart version/branch if using volcano/chart. We're going to update master when releasing.

@hzxuzhonghu
Copy link
Collaborator

Can you also remove the ### 2. Helm charts under ## Cloning Code, because the installer/helm/chart/volcano does not exist at all.

@hzxuzhonghu
Copy link
Collaborator

/cc @TommyLike

@volcano-sh-bot volcano-sh-bot requested a review from TommyLike July 11, 2019 10:12
@thandayuthapani
Copy link
Contributor Author

thandayuthapani commented Jul 11, 2019

Can you also remove the ### 2. Helm charts under ## Cloning Code, because the installer/helm/chart/volcano does not exist at all.

Since volcano-sh/charts is added as submodule, we will have to clone using --recursive tag, then only we can find those files under installer/helm directory

@TommyLike
Copy link
Contributor

Can you also remove the ### 2. Helm charts under ## Cloning Code, because the installer/helm/chart/volcano does not exist at all.

It's a submodule

@thandayuthapani thandayuthapani force-pushed the readme branch 2 times, most recently from 906c935 to a17c3a0 Compare July 11, 2019 11:12

### Download Helm

Install helm by following official guide - https://github.com/helm/helm/blob/master/docs/install.md
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is too complicated, as a newer, he would not want to install A, and then B, before actually install volcano, i would prefer moving this to a simple scripts.

No one like complicated thing. If possible, can we remove dependency on helm totally? @k82cn @TommyLike

Copy link
Contributor

Choose a reason for hiding this comment

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

@hzxuzhonghu maybe you can bring the practice from istio?

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with that. IMO, it's complex because of security, e.g. RBAC; maybe we can provide a quick guidance which ignore security to make it simpler.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI:
istio provides a generated istio-demo.yaml, which allows users to install easily(https://istio.io/docs/setup/kubernetes/install/kubernetes/). And also provides helm install https://istio.io/docs/setup/kubernetes/install/helm/.

And when it comes to kubernetes, we can run ./hack/local-ip-cluster.sh to start a cluster for developing and testing.

@TravisBuddy
Copy link

Travis tests have failed

Hey @thandayuthapani,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 0deba470-a3d1-11e9-8928-81693ac37660

@TommyLike
Copy link
Contributor

/retest

@TravisBuddy
Copy link

Travis tests have failed

Hey @thandayuthapani,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 31b89a80-a3e9-11e9-8928-81693ac37660

@hzxuzhonghu
Copy link
Collaborator

/lgtm

Since this is a bug fix, should be in ASAP. But we should make a install improvement later @thandayuthapani .

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2019
thandayuthapani pushed a commit to thandayuthapani/volcano that referenced this pull request Jul 15, 2019
@k82cn
Copy link
Member

k82cn commented Jul 15, 2019

Please update README according to #334 :)

@TommyLike
Copy link
Contributor

According to the latest response from end user, I guess we need to add some prerequisites for installing volcano in order to reduce confusion. For example, from what I have tested on my environments, we have this requirement at least:

# Prerequisites
Kubernetes 1.12 or newer cluster.

Why: Since we enabled subresource in our CRDs, end user will have this issue when using elder version:
ISSUE: kubernetes/kubernetes#65357
NOTES: https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG-1.12.md#api-changes (65357)

Free feel to add more prerequisites

@hzxuzhonghu @k82cn

@hex108
Copy link
Contributor

hex108 commented Jul 15, 2019

@TommyLike CRD status could be enabled manually since v1.10(https://blog.openshift.com/kubernetes-custom-resources-grow-up-in-v1-10/).

@hzxuzhonghu
Copy link
Collaborator

Also golang version, should be at least 1.11+, because we use go module.

@TommyLike
Copy link
Contributor

TommyLike commented Jul 15, 2019

@TommyLike CRD status could be enabled manually since v1.10(https://blog.openshift.com/kubernetes-custom-resources-grow-up-in-v1-10/).

@hex108 Hmm, so that is another requirement since we need the feature on?

@volcano-sh-bot volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2019
@volcano-sh-bot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@thandayuthapani
Copy link
Contributor Author

Please update README according to #334 :)

Updated

@volcano-sh-bot volcano-sh-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2019
### Setup Helm

Since Tiller needs cluster-admin RBAC access, create `ServiceAccount` and `ClusterRoleBinding` by following the offical guide - https://github.com/helm/helm/blob/master/docs/rbac.md

Copy link
Member

Choose a reason for hiding this comment

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

let's give an example here; I don-t think a new user will go through the whole doc of rbac for volcano installation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Will add step to add serviceaccount creation and such

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@k82cn Have added steps to create serviceaccount and clusterrolebinding for tiller in README

Copy link
Member

@k82cn k82cn Jul 16, 2019

Choose a reason for hiding this comment

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

hm... if the following steps about serviceaccount is the correct final solution, why do we need helm rbac installation guidance? If it's only an example, please highlight that.

@volcano-sh-bot volcano-sh-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 16, 2019
@hzxuzhonghu
Copy link
Collaborator

I think with #344, the install guide in this readme can be removed, and just add a link to installer/README.md

@k82cn
Copy link
Member

k82cn commented Jul 17, 2019

I think with #344, the install guide in this readme can be removed, and just add a link to installer/README.md

so where's the yaml file?

@hzxuzhonghu
Copy link
Collaborator

The yaml generate should be a step, it is with default options and new images built.

@TommyLike
Copy link
Contributor

Since we have different concerns on this section, I drafted the install section with the latest process of installing. @k82cn @thandayuthapani @hzxuzhonghu please take a look and share your idea.

===============================================================

Quick Start Guide

Prerequisites

  • Kubernetes 1.12+ with CRD support
  • (Optional) Helm is needed if you'd like use volcano via helm charts. Please follow official
    guide to complete installation <Need Metion RBAC issue here?>

Install release from github

The easiest way to install volcano is to use release yaml, please refer to the README file inside release folder after
download from website

Install release from helm repo

We also support helm chart, install with the command below

# Add volcano offical helm chart
helm repo add volcano https://volcano-sh.github.io/charts
# Install a release wanted
helm install volcano/volcano --namespace <namespace> --name <name>  --version  <release-version>

NOTE:
Since tiller server will not manage CRD resource, therefore try install command with --no-crd-hook option when reinstalling.

Install from source code

  1. Clone the repo to your local path:
# mkdir -p $GOPATH/src/volcano.sh/
# cd $GOPATH/src/volcano.sh/
# git clone --recursive https://github.com/volcano-sh/volcano.git
  1. Build docker images and load them into your k8s cluster
cd $GOPATH/src/volcano.sh/volcano
make images

## Verify your images
# docker images
REPOSITORY                      TAG                 IMAGE ID            CREATED             SIZE
volcanosh/vc-admission          latest              a83338506638        8 seconds ago       41.4MB
volcanosh/vc-scheduler          latest              faa3c2a25ac3        9 seconds ago       49.6MB
volcanosh/vc-controllers        latest              7b11606ebfb8        10 seconds ago      44.2MB

  1. generate the yaml file and install
make generate-yaml    
# The generated yaml file would be located in _output/release/volcano-latest.yaml
kubectl apply -f _output/release/volcano-latest.yaml
# Create default queue resource after installation.
kubectl apply -f installer/helm/chart/volcano/templates/default-queue.yaml

Install verify

To verify your installation run the following commands:

#1. Verify the Running Pods
# kubectl get pods --namespace <namespace>
NAME                                                 READY   STATUS    RESTARTS   AGE
<specified-name>-admission-84fd9b9dd8-9trxn          1/1     Running   0          43s
<specified-name>-controllers-75dcc8ff89-42v6r        1/1     Running   0          43s
<specified-name>-scheduler-b94cdb867-89pm2           1/1     Running   0          43s
<specified-name>--admission-init-qbtmb               0/1     Completed 0          43s

#2. Verify the Services
# kubectl get services --namespace <namespace>
NAME                                     TYPE        CLUSTER-IP     EXTERNAL-IP   PORT(S)   AGE
<specified-name>-admission-service       ClusterIP   10.105.78.53   <none>        443/TCP   91s

@hzxuzhonghu
Copy link
Collaborator

hzxuzhonghu commented Jul 17, 2019

As install guide is very long, i suggest moving it to docs repo and link it here.
If we put a very long install procedure here, makes users feel bad.
BTW, all docs and example guides(example yaml should keep in this repo) should be there i think.

@k82cn
Copy link
Member

k82cn commented Jul 18, 2019

@TommyLike @hzxuzhonghu , can you summary current status about installation? I'm lost in your PR and discussion, and I don-t know when it'll be finished :(

@thandayuthapani
Copy link
Contributor Author

Since PR #346 has been merged, this PR can be closed, @k82cn Please close this PR when discussion this PR if finished.

@volcano-sh-bot
Copy link
Contributor

@thandayuthapani: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k82cn k82cn closed this Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. retest-not-required-docs-only size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants