karmadactl: Fix karmada-data directory not inilization isssue #2548

jwcesign · 2022-09-20T07:28:37Z

Signed-off-by: jwcesign jiangwei115@huawei.com

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR cause following error: #2297

jw@ecs-3fa1 [02:58:20 PM] [~/workspace/git/karmada-diff/karmada] [release-1.3]
-> % kubectl karmada init  --karmada-data=/home/jw/workspace/karmada_data
I0920 14:58:26.229852 3193825 deploy.go:145] kubeconfig file: /home/jw/.kube/karmada.config, kubernetes: https://192.168.1.237:42213
I0920 14:58:26.244413 3193825 deploy.go:165] karmada apiserver ip: [172.18.0.6]
I0920 14:58:26.620751 3193825 cert.go:229] Generate ca certificate success.
I0920 14:58:26.845036 3193825 cert.go:229] Generate karmada certificate success.
I0920 14:58:27.031043 3193825 cert.go:229] Generate apiserver certificate success.
I0920 14:58:27.088533 3193825 cert.go:229] Generate front-proxy-ca certificate success.
I0920 14:58:27.435088 3193825 cert.go:229] Generate front-proxy-client certificate success.
I0920 14:58:27.610710 3193825 cert.go:229] Generate etcd-ca certificate success.
I0920 14:58:27.725861 3193825 cert.go:229] Generate etcd-server certificate success.
I0920 14:58:27.844586 3193825 cert.go:229] Generate etcd-client certificate success.
I0920 14:58:27.844724 3193825 deploy.go:251] download crds file name: /home/jw/workspace/karmada_data/crds.tar.gz
error: prepare karmada failed.open /home/jw/workspace/karmada_data/crds.tar.gz: no such file or directory

With release-1.2 works

Which issue(s) this PR fixes:
Fixes bugs about dir lost

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmadactl`: Fixed `--karmada-data` directory not initialization issue in `init` command.

XiShanYongYe-Chang

@jwcesign, can you help update the release note, the format can be as follows:

{component name}: {fix error information}

XiShanYongYe-Chang · 2022-09-20T08:28:40Z

/cc @lonelyCZ @carlory can you help take a review?

jwcesign · 2022-09-20T08:31:10Z

release

done

jwcesign · 2022-09-20T12:20:06Z

cc @RainbowMango

RainbowMango · 2022-09-20T12:27:27Z

pkg/karmadactl/cmdinit/kubernetes/deploy.go

@@ -177,7 +177,7 @@ func (i *CommandInitOption) Complete() error {
 		}
 	}

-	return nil
+	return os.MkdirAll(i.KarmadaDataPath, 0755)


Can you remind me why just removes the KarmadaDataPath and create it again?

based on this PR: #1455

When the current host uses init to install karmada multiple times, the installation fails because the data directory of karmada is not empty.

After remove it, it's not create it again then it will has error here:

karmada/pkg/karmadactl/cmdinit/kubernetes/deploy.go

Line 252 in ab88988

if err := utils.DownloadFile(i.CRDs, filename); err != nil {

I just thought about that it is better not to remove whole directory, if users set a important existed directory, the command will remove it, which is dangerous. Like kubeamd reset, it only remove the files that created by kubeadm init. Perhaps, we can enhence it in a new PR.

For this pr, I think it is fine.

lonelyCZ · 2022-09-20T13:22:17Z

pkg/karmadactl/cmdinit/kubernetes/deploy.go

@@ -171,13 +171,17 @@ func (i *CommandInitOption) Complete() error {
 	}

 	// Determine whether KarmadaDataPath exists, if so, delete it
+	// Related issue: https://github.com/karmada-io/karmada/pull/1455


we don't need to explain it in code. It can be recorded by git commit

Yes, that's exactly what I want to say. We don't need to describe the history of the code, we just need to explain why we do it.

lonelyCZ · 2022-09-20T13:22:59Z

pkg/karmadactl/cmdinit/kubernetes/deploy.go

 	if utils.IsExist(i.KarmadaDataPath) {
 		if err := os.RemoveAll(i.KarmadaDataPath); err != nil {
 			return err
 		}
 	}

-	return nil
+	// Create this dir, next steps will download crds to this dir


We can add comments here.

U mean: add comment like TODO: enhence this operation like kubeadm reset?

No. The comment could be shorter and clearer. Like above comments

// When the current host uses init to install karmada multiple times, the installation fails // because the data directory of karmada is not empty.

It is not necessary to describe the install karmada multiple times that should be in PR description rather than in code.

how about this:

// The installation will fail because of the history data, so delete it and create it again if utils.IsExist(i.KarmadaDataPath) { if err := os.RemoveAll(i.KarmadaDataPath); err != nil { return err } } return os.MkdirAll(i.KarmadaDataPath, os.FileMode(0755)) }

cc @lonelyCZ

RainbowMango · 2022-09-21T06:29:07Z

/assign

RainbowMango · 2022-09-21T09:10:04Z

pkg/karmadactl/cmdinit/kubernetes/deploy.go

+	// When the current host uses init to install karmada multiple times, the installation fails
+	// because the data directory of karmada is not empty.
 	if utils.IsExist(i.KarmadaDataPath) {
 		if err := os.RemoveAll(i.KarmadaDataPath); err != nil {
 			return err
 		}
 	}

-	return nil
+	// Create this dir, next steps will download crds to this dir
+	return os.MkdirAll(i.KarmadaDataPath, os.FileMode(0755))


My suggestion:

// Initialize karmada-data directory. // If the directory already exists, just clean it up. if utils.IsExist(i.KarmadaDataPath) { if err := os.RemoveAll(i.KarmadaDataPath); err != nil { return err } } if err := os.MkdirAll(i.KarmadaDataPath, os.FileMode(0755)); err != nil { return fmt.Errorf("failed to create directory: %s, error: %v", i.KarmadaDataPath, err) } return nil

cyclomatic complexity execced, how about extract one function:

} } return cleanupDataDirectory(i.KarmadaDataPath) } // cleanupDataDirectory clean up the karamda data directory func cleanupDataDirectory(karmadaDataPath string) error { // Initialize karmada-data directory. // If the directory already exists, just clean it up. if utils.IsExist(karmadaDataPath) { if err := os.RemoveAll(karmadaDataPath); err != nil { return err } } if err := os.MkdirAll(karmadaDataPath, os.FileMode(0755)); err != nil { return fmt.Errorf("failed to create directory: %s, error: %v", karmadaDataPath, err) } return nil }

What we are going to do is ensure a directory exists and is clean, more than cleanup.

RainbowMango · 2022-09-21T11:27:34Z

pkg/karmadactl/cmdinit/kubernetes/deploy.go

+
+// initializeDirectory clean up the directory path if it exists, then recreate it
+func initializeDirectory(path string) error {
+	// Initialize karmada-data directory.


It's ok to introduce a function here, but the comment might be inappropriate, as it's not dedicated to karmada-data now. It's a common function now.

I guess we can add a unit test for it.

fixed, and add ut case

RainbowMango · 2022-09-21T11:31:36Z

pkg/karmadactl/cmdinit/kubernetes/deploy.go

+	return initializeDirectory(i.KarmadaDataPath)
+}
+
+// initializeDirectory clean up the directory path if it exists, then recreate it


Suggested change

// initializeDirectory clean up the directory path if it exists, then recreate it

// initializeDirectory initializes a directory and makes sure it's empty.

jwcesign · 2022-09-22T03:15:14Z

cc @RainbowMango

RainbowMango · 2022-09-22T03:23:38Z

Please fixed the lint issue.

Signed-off-by: jwcesign <jiangwei115@huawei.com>

RainbowMango

/lgtm
/approve

Please help to cherry-pick this to release-1.3.

karmada-bot · 2022-09-22T03:47:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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:

~~pkg/karmadactl/cmdinit/OWNERS~~ [RainbowMango]

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

…-upstream-release-1.3 Automated cherry pick of #2548: Fix bugs about karmada-data dir lost

karmada-bot added the kind/bug Categorizes issue or PR as related to a bug. label Sep 20, 2022

karmada-bot requested review from lonelyCZ and prodanlabs September 20, 2022 07:28

karmada-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 20, 2022

XiShanYongYe-Chang reviewed Sep 20, 2022

View reviewed changes

XiShanYongYe-Chang mentioned this pull request Sep 20, 2022

Update karmada's k8s components to 1.25.2 #2539

Merged

RainbowMango reviewed Sep 20, 2022

View reviewed changes

jwcesign force-pushed the fix-dir-lost branch from 6c730a4 to 7cb0b4f Compare September 20, 2022 12:49

lonelyCZ reviewed Sep 20, 2022

View reviewed changes

jwcesign force-pushed the fix-dir-lost branch from 7cb0b4f to 0bce8fc Compare September 21, 2022 01:52

karmada-bot assigned RainbowMango Sep 21, 2022

RainbowMango changed the title ~~Fix bugs about karmada-data dir lost~~ karmadactl: Fix karmada-data directory not inilization isssue Sep 21, 2022

RainbowMango reviewed Sep 21, 2022

View reviewed changes

jwcesign force-pushed the fix-dir-lost branch 3 times, most recently from a6b6697 to b992a3d Compare September 21, 2022 09:29

karmada-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 21, 2022

jwcesign force-pushed the fix-dir-lost branch from b992a3d to 9a1a425 Compare September 21, 2022 09:39

RainbowMango reviewed Sep 21, 2022

View reviewed changes

jwcesign force-pushed the fix-dir-lost branch from 9a1a425 to 8b0e9f3 Compare September 22, 2022 03:14

karmada-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. labels Sep 22, 2022

Fix bugs about karmada-data dir lost

cdaa934

Signed-off-by: jwcesign <jiangwei115@huawei.com>

jwcesign force-pushed the fix-dir-lost branch from 8b0e9f3 to cdaa934 Compare September 22, 2022 03:23

RainbowMango approved these changes Sep 22, 2022

View reviewed changes

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2022

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2022

karmada-bot merged commit 6102f41 into karmada-io:master Sep 22, 2022

jwcesign mentioned this pull request Sep 22, 2022

Automated cherry pick of #2548: Fix bugs about karmada-data dir lost #2563

Merged

karmada-bot added a commit that referenced this pull request Sep 22, 2022

Merge pull request #2563 from jwcesign/automated-cherry-pick-of-#2548…

d44b18b

…-upstream-release-1.3 Automated cherry pick of #2548: Fix bugs about karmada-data dir lost

jwcesign mentioned this pull request Oct 17, 2022

REQUEST: New membership for jwcesign karmada-io/community#12

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

karmadactl: Fix karmada-data directory not inilization isssue #2548

karmadactl: Fix karmada-data directory not inilization isssue #2548

jwcesign commented Sep 20, 2022 •

edited by RainbowMango

Loading

XiShanYongYe-Chang left a comment

XiShanYongYe-Chang commented Sep 20, 2022

jwcesign commented Sep 20, 2022

jwcesign commented Sep 20, 2022

RainbowMango Sep 20, 2022

jwcesign Sep 20, 2022

lonelyCZ Sep 20, 2022

lonelyCZ Sep 20, 2022

jwcesign Sep 21, 2022

RainbowMango Sep 21, 2022

lonelyCZ Sep 20, 2022

jwcesign Sep 21, 2022

lonelyCZ Sep 21, 2022

jwcesign Sep 21, 2022

RainbowMango commented Sep 21, 2022

RainbowMango Sep 21, 2022

jwcesign Sep 21, 2022

RainbowMango Sep 21, 2022

jwcesign Sep 21, 2022

RainbowMango Sep 21, 2022

RainbowMango Sep 21, 2022

jwcesign Sep 22, 2022

RainbowMango Sep 21, 2022

jwcesign Sep 22, 2022

jwcesign commented Sep 22, 2022

RainbowMango commented Sep 22, 2022

RainbowMango left a comment

karmada-bot commented Sep 22, 2022

	// initializeDirectory clean up the directory path if it exists, then recreate it
	// initializeDirectory initializes a directory and makes sure it's empty.

karmadactl: Fix karmada-data directory not inilization isssue #2548

karmadactl: Fix karmada-data directory not inilization isssue #2548

Conversation

jwcesign commented Sep 20, 2022 • edited by RainbowMango Loading

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

XiShanYongYe-Chang commented Sep 20, 2022

jwcesign commented Sep 20, 2022

jwcesign commented Sep 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RainbowMango commented Sep 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwcesign commented Sep 22, 2022

RainbowMango commented Sep 22, 2022

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Sep 22, 2022

jwcesign commented Sep 20, 2022 •

edited by RainbowMango

Loading