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.

Sign up for GitHub

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

Jump to bottom

Add all-in-one installation script/docs #192

Merged
merged 7 commits into from
Sep 18, 2021
Merged

Add all-in-one installation script/docs #192

merged 7 commits into from
Sep 18, 2021

Conversation

llhuii
Copy link

@llhuii llhuii commented Sep 10, 2021

fix #181

@kubeedge-bot kubeedge-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 10, 2021
@llhuii llhuii marked this pull request as draft September 10, 2021 08:07
@kubeedge-bot kubeedge-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 10, 2021
Add all-in-one docs
8d515fb 
Signed-off-by: llhuii <[email protected]>
@llhuii llhuii marked this pull request as ready for review September 17, 2021 04:02
@llhuii
Copy link
Author

llhuii commented Sep 17, 2021

/hold cancel

@llhuii llhuii changed the title Add all-in-one installation script Add all-in-one installation script/docs Sep 17, 2021
@kubeedge-bot kubeedge-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 17, 2021
@JoeyHwong-gk
Copy link
Contributor

look good to me

MooreZheng
MooreZheng reviewed Sep 17, 2021
View reviewed changes
Copy link
Contributor

@MooreZheng MooreZheng left a comment

Choose a reason for hiding this comment

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

In general it looks fine but there are still some writing points to consider.

docs/setup/quick-start.md Outdated Show resolved Hide resolved
docs/setup/quick-start.md Outdated Show resolved Hide resolved
docs/setup/install.md Outdated Show resolved Hide resolved
docs/setup/quick-start.md Outdated Show resolved Hide resolved
llhuii added 2 commits September 17, 2021 16:09
MooreZheng
MooreZheng suggested changes Sep 17, 2021
View reviewed changes
Copy link
Contributor

@MooreZheng MooreZheng left a comment

Choose a reason for hiding this comment

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

It is a much improved version. Futher suggestion: Avoid ambigity and highlight the attractive local-up installation.

docs/setup/quick-start.md Outdated Show resolved Hide resolved
docs/setup/quick-start.md Outdated
There are some ways to set up Sedna, depends on your use case:
- If you have none Kubernetes environment and don't want to install Kubernetes manually, you can follow [the instruction](./all-in-one.md) to install all-in-one Sedna environment.
- Else you can follow [this instruction](./install.md) to install Sedna on existing Kubernetes cluster.
- Also [there is a local up script](./local-up.md) which is mainly used for developing Sedna, it boots a local Kubernetes cluster and installs Sedna based on local Sedna repository.
Copy link
Contributor

@MooreZheng MooreZheng Sep 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This version looks much more smooth. In fact, the local-up installation itself is very attractive. We can polish the sentences a little bit to make the local-up version stand out.

  1. Highlight the difference between local up and others. Add examples to show why a local version is needed.
  2. The first half is already a completed sentence where a period should be used.
  3. "used for developing Sedna" could be misleading. It implies "one cannot develop Sedna based on all-in-one or normal-install". Saying "easier development" would be fine.

Might the following is clearer (not very sure about the sayings and pls correct me if anything wrong):

"One more thing: we also have a local-up install script available for easier Sedna development, using only one machine to simulate a multi-container environment. That is, instead of setting up several machines, the local-up version boots a local Kubernetes cluster and installs Sedna based on a local repository.

[Example Use Case for Local-up Installation] When one is contributing new features for Sedna, codes like AI algorithms under testing can be frequently changed before final deployment. When coding in that case, s/he would suffer from tortured re-installations and frequent failures of the whole complicated system. To get rid of the torments, one can use the local-up installation, embraced the single-machine simulation for agiler development and testing."

Copy link
Author

Choose a reason for hiding this comment

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

Add the example use case for local-up into local-up.md

@kubeedge-bot
Copy link
Collaborator

@MooreZheng: changing LGTM is restricted to collaborators

In response to this:

It is a much improved version. Futher suggestion: Avoid ambigity and highlight the attractive local-up installation.

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.

@llhuii
Copy link
Author

llhuii commented Sep 18, 2021

@MooreZheng PTAL

all-in-one: make GM node schedulable
479d407 
When NUM_CLOUD_WORKERS >= 1, kind will taint master node with
key='node-role.kubernetes.io/master'.

By default, GM will be deployed at master node.
So make master node schedulable, and be able to run workloads, i.e. GM
here.

Signed-off-by: llhuii <[email protected]>
@llhuii llhuii force-pushed the allinone-script branch 2 times, most recently from 740f615 to e887b38 Compare September 18, 2021 07:24
@llhuii
Copy link
Author

llhuii commented Sep 18, 2021

Updated according to all review coment.
And tested, we can try it online on https://www.katacoda.com/kubeedge-sedna/scenarios/all-in-one

/approve

@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: llhuii

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

@kubeedge-bot kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 18, 2021
@llhuii
Copy link
Author

llhuii commented Sep 18, 2021

So let's merge it😄, @MooreZheng @JimmyYang20

@JimmyYang20
Copy link

/lgtm

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2021
@kubeedge-bot kubeedge-bot merged commit 3b75368 into kubeedge:main Sep 18, 2021
@MooreZheng MooreZheng mentioned this pull request May 5, 2022
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MooreZheng MooreZheng MooreZheng requested changes

@JensenJian JensenJian JensenJian left review comments

@JimmyYang20 JimmyYang20 Awaiting requested review from JimmyYang20

@TymonXie TymonXie Awaiting requested review from TymonXie

Assignees

@JimmyYang20 JimmyYang20

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

A local sedna (just like minikube for k8s) is needed
6 participants
@llhuii @JoeyHwong-gk @kubeedge-bot @JimmyYang20 @MooreZheng @JensenJian