kmeshctl install by thebigbone · Pull Request #978 · kmesh-net/kmesh

thebigbone · 2024-10-23T08:58:52Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

adds install command for kmeshctl which will install all the resources for kmesh in a k8s cluster

Which issue(s) this PR fixes:
Fixes #552

Does this PR introduce a user-facing change?:

NONE

kmesh-bot · 2024-10-23T08:59:03Z

Welcome @thebigbone! It looks like this is your first PR to kmesh-net/kmesh 🎉

YaoZengzeng · 2024-10-24T07:15:57Z

ctl/main.go

 	rootCmd.AddCommand(waypoint.NewCmd())
 	rootCmd.AddCommand(version.NewCmd())
 	rootCmd.AddCommand(accesslog.NewCmd())
+	rootCmd.AddCommand(install.NewCmd())


Also need to update doc when this PR get merged.

YaoZengzeng · 2024-10-24T07:20:36Z

ctl/install/install.go

+				log.Fatal(err)
+			}
+
+			kubeconfig := os.Getenv("HOME") + "/.kube/config"


Use existing (tools)[https://github.com/kmesh-net/kmesh/blob/main/ctl/utils/utils.go#L31] to create k8s client direclty which has built-in method for applying yaml files.

So, I should remove the custom doServerSideApply function?

I think you could.

YaoZengzeng · 2024-10-24T07:21:38Z

ctl/install/install.go

@@ -0,0 +1,167 @@
+package install


Add copyright

YaoZengzeng · 2024-10-24T07:23:39Z

ctl/install/install.go

+	"log"
+	"os"
+
+	"k8s.io/apimachinery/pkg/api/meta"


Arrange the dependency order, put the standard lib together, put Kmesh lib together and put third-party dependencies together.

YaoZengzeng · 2024-10-24T07:24:33Z

ctl/install/install.go

+func NewCmd() *cobra.Command {
+	cmd := &cobra.Command{
+		Use:   "install",
+		Short: "install kmesh with all the resources",


Add examples, refer to other subcommands.

YaoZengzeng · 2024-10-24T07:30:03Z

ctl/install/install.go

+				panic(err.Error())
+			}
+
+			resources := []Resource{


Why not just apply all files in the yaml diretory instead of listing each files which may change in the future?

How would that work?

You could get all yaml contents and call this function

I don't know if there is an elegant way to direclty get the contents of all files under yaml directory or to know which files are in this directory and download them one by one.

I don't know if there is an elegant way to direclty get the contents of all files under yaml directory or to know which files are in this directory and download them one by one.

curl -s "https://api.github.com/repos/kmesh-net/kmesh/contents/deploy/yaml" | jq -r '.[] | .download_url'

The ApplyYAMLContents function works fine but it requires namespace argument. For now, I can separate the kmesh-system and istio but it can break in the future because there could be a new yaml file with a different namespace.

I think you could specify namespace as "", so that it will direcly use the ns specified in yaml.

Oh, that's helpful. I was parsing the namespace field in every file.

LiZhenCheng9527 · 2024-10-24T12:04:46Z

ctl/install/install.go

+				{Name: "serviceAccount", URL: getURL(version, "serviceaccount.yaml")},
+			}
+
+			fmt.Println("using version: ", version)


nit：install kmesh version:

codecov · 2024-10-24T14:04:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.46%. Comparing base (8c72e4c) to head (1e39cec).
Report is 168 commits behind head on main.

see 35 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 865f832...1e39cec. Read the comment docs.

LiZhenCheng9527 · 2024-10-26T02:20:22Z

ctl/install/install.go

+# Install a specific version of kmesh
+kmeshctl install --version 0.5`,
+		Run: func(cmd *cobra.Command, args []string) {
+			version, err := cmd.Flags().GetString("version")


We still have issue #949.
@YaoZengzeng Has this problem been fixed?

No, I hope this issue could be fixed with the solution of #892, but it seems that this problem is not being addressed now. If no one has dealt it by then, I will fix it.

YaoZengzeng · 2024-10-26T06:30:30Z

ctl/install/install.go

+
+			deployYaml, err := io.ReadAll(resp.Body)
+			if err != nil {
+				log.Fatalf("yamlFile.Get err   #%v ", err)


Suggested change

log.Fatalf("yamlFile.Get err #%v ", err)

log.Fatalf("yamlFile.Get err: %v ", err)

YaoZengzeng · 2024-10-26T06:31:49Z

ctl/install/install.go

+func NewCmd() *cobra.Command {
+	cmd := &cobra.Command{
+		Use:   "install",
+		Short: "install kmesh with all the resources",


Descriptive content use Kmesh, with the first letter capitalized.

YaoZengzeng · 2024-10-26T06:33:12Z

The overall framework seems fine for me, @hzxuzhonghu PTAL.

hzxuzhonghu · 2024-10-28T01:27:47Z

ctl/install/install.go

+func getYAMLFile(version string) string {
+	var url string
+	if version != "main" {
+		url = fmt.Sprintf("https://api.github.com/repos/kmesh-net/kmesh/contents/deploy/yaml?ref=release-%s", version)


I donot think this is quite correct, because?:

we didn't update the image tag

should not use the github branch, but use the tag instead

So in short should we use helm chart https://github.com/kmesh-net/kmesh/releases/download/v0.5.0/kmesh-helm-v0.5.0.tgz

or the OCI chart image https://github.com/orgs/kmesh-net/packages/container/package/kmesh-helm

It is using release tag? Using helm will introduce new dependency which is totally unnecessary.

See the released version above

ctl/install/install.go

YaoZengzeng · 2024-11-06T01:35:23Z

ping @hzxuzhonghu

hzxuzhonghu · 2024-11-06T08:24:59Z

please see above comment, when you set version-=0.5, then you will install kmesh with latest image https://github.com/kmesh-net/kmesh/blob/release-0.5/deploy/yaml/kmesh.yaml#L72

This is not expected, can you replace that image?

I would suggest we support v0.5.0 or 0.5.0 rather than 0.5, see our releases

thebigbone · 2024-11-06T13:15:59Z

please see above comment, when you set version-=0.5, then you will install kmesh with latest image https://github.com/kmesh-net/kmesh/blob/release-0.5/deploy/yaml/kmesh.yaml#L72

This is not expected, can you replace that image?

I would suggest we support v0.5.0 or 0.5.0 rather than 0.5, see our releases

Should I just replace the image and get the yaml files from main then?

hzxuzhonghu · 2024-11-07T01:53:14Z

Should I just replace the image and get the yaml files from main than?

This probably not work, think of the case that a yaml is updated because of some new features. The old image may cannot work with the new daemonset yaml

Signed-off-by: thebigbone <pacman@duck.com>

Co-authored-by: Tiger Xu / Zhonghu Xu <xuzhonghu@huawei.com> Signed-off-by: thebigbone <pacman@duck.com>

Signed-off-by: thebigbone <pacman@duck.com>

thebigbone · 2024-11-07T09:48:49Z

Removed the branch and instead using tags now.

hzxuzhonghu

/lgtm

YOu can fix it later

hzxuzhonghu · 2024-11-12T06:22:18Z

ctl/install/install.go

+			if err != nil {
+				log.Fatal(err)
+			}
+			defer resp.Body.Close()


nit: not use defer in a for loop

kmesh-bot · 2024-11-12T06:23:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [hzxuzhonghu]

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

kmesh-bot · 2024-11-13T07:23:46Z

New changes are detected. LGTM label has been removed.

kmesh-bot · 2024-11-13T07:23:48Z

Adding label do-not-merge/contains-merge-commits because PR contains merge commits, which are not allowed in this repository.
Use git rebase to reapply your commits on top of the target branch. Detailed instructions for doing so can be found here.

Details

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.

thebigbone · 2024-11-13T07:56:20Z

The kube package was changed so it's not able to find ApplyYamlContents

hzxuzhonghu · 2024-11-20T04:03:09Z

Can you implement this method? Or use helm client https://github.com/kurator-dev/kurator/blob/ac68f35c8799773aea22643e39ca032a20e65fb7/pkg/client/client.go#L66

LiZhenCheng9527 · 2024-12-03T07:47:38Z

@thebigbone This PR can be merged after you pass CI.

thebigbone · 2025-01-22T08:01:25Z

Can you implement this method? Or use helm client https://github.com/kurator-dev/kurator/blob/ac68f35c8799773aea22643e39ca032a20e65fb7/pkg/client/client.go#L66

Should I use the istio's implementation directly in kube/client.go?

thebigbone · 2025-01-27T06:23:29Z

ping @LiZhenCheng9527

LiZhenCheng9527 · 2025-01-27T06:34:20Z

ctl/install/install.go

+					log.Fatal(err)
+				}
+			} else {
+				err = cli.ApplyYAMLContents("", combinedYAMLFile)


There is no ApplyYamlContents in kmesh's kube pkg.
After fixing this problem, this PR can be merged.

Yes, which is why I asked if I should use the istio's implementation or the previous doServerSideApply function which I used at the beginning.

Perhaps we can implement applyYamlContents in Kmesh. If that doesn't work, we can switch back to the doServerSideApply function.

thebigbone · 2025-01-28T06:19:54Z

I have implemented the method @LiZhenCheng9527

kmesh-bot added the kind/feature label Oct 23, 2024

kmesh-bot requested review from bitcoffeeiux and supercharge-xsy October 23, 2024 08:59

kmesh-bot added the size/L label Oct 23, 2024

thebigbone force-pushed the main branch from 2056938 to 17252ca Compare October 23, 2024 08:59

YaoZengzeng reviewed Oct 24, 2024

View reviewed changes

ctl/install/install.go

@@ -0,0 +1,167 @@

package install

Copy link
Copy Markdown

Member

YaoZengzeng Oct 24, 2024

Choose a reason for hiding this comment

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

Add copyright

YaoZengzeng reviewed Oct 24, 2024

View reviewed changes

LiZhenCheng9527 reviewed Oct 24, 2024

View reviewed changes

thebigbone force-pushed the main branch from 15d34d9 to 64989de Compare October 24, 2024 13:50

kmesh-bot added the do-not-merge/contains-merge-commits label Oct 24, 2024

thebigbone force-pushed the main branch from adbc9d2 to 36088b8 Compare October 25, 2024 07:07

kmesh-bot removed the do-not-merge/contains-merge-commits label Oct 25, 2024

thebigbone force-pushed the main branch from 36088b8 to 830f8b1 Compare October 25, 2024 07:09

LiZhenCheng9527 reviewed Oct 26, 2024

View reviewed changes

YaoZengzeng reviewed Oct 26, 2024

View reviewed changes

hzxuzhonghu reviewed Oct 28, 2024

View reviewed changes

ctl/install/install.go Outdated Show resolved Hide resolved

thebigbone force-pushed the main branch from 0af3ac2 to f278040 Compare November 7, 2024 09:45

thebigbone and others added 8 commits November 7, 2024 15:16

minor changes

006932e

Signed-off-by: thebigbone <pacman@duck.com>

use utils for applying yaml

97e282a

Signed-off-by: thebigbone <pacman@duck.com>

example command

1cf4744

Signed-off-by: thebigbone <pacman@duck.com>

version check

9539d57

Signed-off-by: thebigbone <pacman@duck.com>

minor linting error

f557c4b

Signed-off-by: thebigbone <pacman@duck.com>

minor changes

07995b6

Signed-off-by: thebigbone <pacman@duck.com>

update ctl/install/install.go

661ff07

Co-authored-by: Tiger Xu / Zhonghu Xu <xuzhonghu@huawei.com> Signed-off-by: thebigbone <pacman@duck.com>

use tags for versions

1e39cec

Signed-off-by: thebigbone <pacman@duck.com>

thebigbone force-pushed the main branch from 691283d to 1e39cec Compare November 7, 2024 09:47

hzxuzhonghu previously approved these changes Nov 12, 2024

View reviewed changes

ctl/install/install.go

if err != nil {

log.Fatal(err)

}

defer resp.Body.Close()

Copy link
Copy Markdown

Member

hzxuzhonghu Nov 12, 2024

Choose a reason for hiding this comment

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

nit: not use defer in a for loop

kmesh-bot assigned hzxuzhonghu Nov 12, 2024

kmesh-bot added the lgtm label Nov 12, 2024

kmesh-bot added the approved label Nov 12, 2024

Merge branch 'main' into main

2438862

thebigbone dismissed hzxuzhonghu’s stale review via 2438862 November 13, 2024 07:23

kmesh-bot removed the lgtm label Nov 13, 2024

kmesh-bot added the do-not-merge/contains-merge-commits label Nov 13, 2024

LiZhenCheng9527 reviewed Jan 27, 2025

View reviewed changes

thebigbone and others added 2 commits January 27, 2025 14:01

Merge branch 'kmesh-net:main' into main

5119f9d

implement ApplyYAMLContents from istio

ab40442

	log.Fatalf("yamlFile.Get err #%v ", err)
	log.Fatalf("yamlFile.Get err: %v ", err)

Conversation

thebigbone commented Oct 23, 2024

Uh oh!

kmesh-bot commented Oct 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YaoZengzeng commented Oct 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thebigbone Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

YaoZengzeng commented Nov 6, 2024

Uh oh!

hzxuzhonghu commented Nov 6, 2024

Uh oh!

thebigbone commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hzxuzhonghu commented Nov 7, 2024

Uh oh!

thebigbone commented Nov 7, 2024

Uh oh!

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmesh-bot commented Nov 12, 2024

Uh oh!

kmesh-bot commented Nov 13, 2024

Uh oh!

codecov bot commented Oct 24, 2024 •

edited

Loading

thebigbone Oct 28, 2024 •

edited

Loading

thebigbone commented Nov 6, 2024 •

edited

Loading

thebigbone commented Jan 22, 2025 •

edited

Loading