[bugfix]: add pre-check that If the cluster already exists #2023

Stevent-fei · 2023-02-10T07:24:26Z

Describe what this PR does / why we need it

When the sealer runs, it will check in advance whether there is a deployed cluster image. If there is, and the version is inconsistent, then an error will be reported. If the version is consistent, then skip. If not, install normally

exist：

not exist：

Does this pull request fix one issue?

Fixes #2022

Describe how you did it

Describe how to verify it

Special notes for reviews

VinceCui · 2023-02-17T09:12:53Z

cmd/sealer/cmd/cluster/run.go

+					return err
+				}
+
+				if ok := strings.Contains(string(context), args[0]); !ok {


No need check if cluster image same, if not same, the user also can't run cluster.

Uer API to marshal it.

kakaZhou719 · 2023-02-28T05:17:55Z

@Stevent-fei i have two questions:

how to determine there is an existed cluster?
should we add this pre-check logic to all create process?

kakaZhou719 · 2023-02-28T05:20:18Z

cmd/sealer/cmd/cluster/run.go

@@ -115,6 +117,30 @@ func NewRunCmd() *cobra.Command {
 					app, extension, nil, imageEngine, runFlags.Mode)
 			}

+			if exist := osi.IsFileExist(common.GetDefaultClusterfile()); exist {


not use file existing to do this pre-check, try get it from clusterfile.NewClusterFile(nil),
if not found, no cluster.
if have ,return run error

codecov-commenter · 2023-02-28T07:27:47Z

Codecov Report

Patch coverage has no change and project coverage change: -5.80 ⚠️

Comparison is base (a51d0ec) 18.89% compared to head (f2eff17) 13.10%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2023      +/-   ##
==========================================
- Coverage   18.89%   13.10%   -5.80%     
==========================================
  Files          96      260     +164     
  Lines        8918    22236   +13318     
==========================================
+ Hits         1685     2913    +1228     
- Misses       7003    18938   +11935     
- Partials      230      385     +155

Flag	Coverage Δ
e2e-tests	`9.22% <ø> (?)`
unit-tests	`18.87% <0.00%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/sealer/cmd/utils/cluster.go	`32.04% <0.00%> (-1.68%)`	⬇️

... and 164 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

starnop

LGTM

starnop · 2023-03-01T09:52:00Z

cmd/sealer/cmd/cluster/run.go

@@ -427,6 +433,19 @@ func installApplication(appImageName string, envs []string, app *v2.Application,
 	return nil
 }

+func checkClusterIsExists(cf clusterfile.Interface) error {
+	rawCluster := cf.GetCluster()


rawCluster/desiredCluster/g

starnop · 2023-03-01T09:52:26Z

cmd/sealer/cmd/cluster/run.go

+func checkClusterIsExists(cf clusterfile.Interface) error {
+	rawCluster := cf.GetCluster()
+	file, err := clusterfile.NewClusterFile(nil)
+	if err == nil {


if err!=nil && match some regular，return nil

kakaZhou719 · 2023-03-10T01:33:43Z

cmd/sealer/cmd/cluster/run.go

@@ -510,3 +517,16 @@ func prepareMaterials(infraDriver infradriver.InfraDriver, imageEngine imageengi
 	}
 	return nil
 }
+
+func checkClusterIsExists(cf clusterfile.Interface) error {


not good use err to identify the check result. i assume we could return check result and error.

kakaZhou719 · 2023-03-10T01:36:41Z

@Stevent-fei . it ok for me to do pre-check, such as cluster image . however , that is just one item need to check . it is better that if we add a check list to do this.

kakaZhou719 · 2023-03-10T03:51:07Z

cmd/sealer/cmd/cluster/run.go

@@ -510,3 +518,16 @@ func prepareMaterials(infraDriver infradriver.InfraDriver, imageEngine imageengi
 	}
 	return nil
 }
+
+func checkClusterIsExists(cf clusterfile.Interface) (string, error) {


func checkClusterIsExists(cluster v2.cluster) (string, error) {

kakaZhou719 · 2023-03-10T03:51:51Z

cmd/sealer/cmd/cluster/run.go

@@ -510,3 +518,16 @@ func prepareMaterials(infraDriver infradriver.InfraDriver, imageEngine imageengi
 	}
 	return nil
 }
+
+func checkClusterIsExists(cf clusterfile.Interface) (string, error) {


move this func to different file Or utils, and add UT test.

kakaZhou719 · 2023-03-10T03:53:07Z

cmd/sealer/cmd/cluster/run.go

+	if err == nil {
+		cluster := clusterFile.GetCluster()
+		if cluster.Spec.Image != rawCluster.Spec.Image {
+			return "exist", fmt.Errorf("the cluster image already exists, please uninstall the current cluster and install another version of the cluster image")


"exist" is not good, you should use const such as "SameImageName" Or something to identity it

kakaZhou719 · 2023-03-10T03:53:53Z

cmd/sealer/cmd/cluster/run.go

@@ -235,6 +237,12 @@ func runWithClusterfile(clusterFile string, runFlags *types.RunFlags) error {
 }

 func runClusterImage(imageEngine imageengine.Interface, cf clusterfile.Interface, imageSpec *imagev1.ImageSpec, mode string) error {
+	//check cluster
+	checkClusterFile, err := checkClusterIsExists(cf)
+	if checkClusterFile == "exist" && err != nil {


why &&? Return err if it is not nil.

kakaZhou719 · 2023-03-14T09:50:53Z

cmd/sealer/cmd/cluster/run.go

@@ -236,6 +238,11 @@ func runWithClusterfile(clusterFile string, runFlags *types.RunFlags) error {

 func runClusterImage(imageEngine imageengine.Interface, cf clusterfile.Interface, imageSpec *imagev1.ImageSpec, mode string) error {
 	cluster := cf.GetCluster()
+	//check cluster
+	checkClusterFile, err := utils.CheckClusterIsExists(cluster)


where is err?

kakaZhou719 · 2023-03-14T09:52:22Z

cmd/sealer/cmd/utils/cluster.go

@@ -256,3 +259,15 @@ func GetClusterClient() *k8s.Client {
 	}
 	return nil
 }
+
+func CheckClusterIsExists(cluster v2.Cluster) (string, error) {
+	clusterFile, _, err := clusterfile.GetActualClusterFile()


should retrun err fast

kakaZhou719 · 2023-03-14T09:53:42Z

cmd/sealer/cmd/utils/cluster.go

+	if err == nil {
+		newClusterfile := clusterFile.GetCluster()
+		if cluster.Spec.Image != newClusterfile.Spec.Image {
+			return clusterExists, fmt.Errorf("the cluster image already exists, please uninstall the current cluster and install another version of the cluster image")


should not return fmt.Errorf

Stevent-fei force-pushed the fix_sealer_run_bug branch from ce8e3bd to 58fedfa Compare February 10, 2023 07:46

Stevent-fei linked an issue Feb 10, 2023 that may be closed by this pull request

[sealer run]check if there is a running cluster, if exist, should not mount new image. #2022

Open

starnop requested review from kakaZhou719 and VinceCui February 13, 2023 01:45

VinceCui reviewed Feb 17, 2023

View reviewed changes

Stevent-fei force-pushed the fix_sealer_run_bug branch from 6699f98 to fe1f2d5 Compare February 17, 2023 10:45

kakaZhou719 reviewed Feb 28, 2023

View reviewed changes

kakaZhou719 changed the title ~~feature: check if there is a running cluster, if exist, should not mount new image~~ [bugfix]: add pre-check that If the cluster already exists Feb 28, 2023

Stevent-fei force-pushed the fix_sealer_run_bug branch 3 times, most recently from 635ae1a to c594c3b Compare February 28, 2023 09:51

starnop approved these changes Mar 1, 2023

View reviewed changes

Stevent-fei force-pushed the fix_sealer_run_bug branch from c594c3b to ef20494 Compare March 1, 2023 09:37

starnop reviewed Mar 1, 2023

View reviewed changes

Stevent-fei force-pushed the fix_sealer_run_bug branch 2 times, most recently from 1d0af38 to 777cd49 Compare March 9, 2023 08:39

kakaZhou719 reviewed Mar 10, 2023

View reviewed changes

Stevent-fei force-pushed the fix_sealer_run_bug branch 2 times, most recently from a5ed0e2 to ae156e5 Compare March 10, 2023 02:43

kakaZhou719 reviewed Mar 10, 2023

View reviewed changes

Stevent-fei force-pushed the fix_sealer_run_bug branch 2 times, most recently from 384a336 to e383428 Compare March 10, 2023 05:32

kakaZhou719 reviewed Mar 14, 2023

View reviewed changes

[bugfix]: add pre-check that If the cluster already exists

f2eff17

Stevent-fei force-pushed the fix_sealer_run_bug branch from e383428 to f2eff17 Compare March 31, 2023 03:23

Stevent-fei closed this Jul 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bugfix]: add pre-check that If the cluster already exists #2023

[bugfix]: add pre-check that If the cluster already exists #2023

Stevent-fei commented Feb 10, 2023 •

edited by kakaZhou719

Loading

VinceCui Feb 17, 2023

VinceCui Feb 17, 2023

kakaZhou719 commented Feb 28, 2023

kakaZhou719 Feb 28, 2023 •

edited

Loading

codecov-commenter commented Feb 28, 2023 •

edited

Loading

starnop left a comment

starnop Mar 1, 2023

starnop Mar 1, 2023

kakaZhou719 Mar 10, 2023 •

edited

Loading

kakaZhou719 commented Mar 10, 2023

kakaZhou719 Mar 10, 2023

kakaZhou719 Mar 10, 2023

kakaZhou719 Mar 10, 2023

kakaZhou719 Mar 10, 2023

kakaZhou719 Mar 14, 2023

kakaZhou719 Mar 14, 2023

kakaZhou719 Mar 14, 2023 •

edited

Loading

[bugfix]: add pre-check that If the cluster already exists #2023

[bugfix]: add pre-check that If the cluster already exists #2023

Conversation

Stevent-fei commented Feb 10, 2023 • edited by kakaZhou719 Loading

Describe what this PR does / why we need it

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kakaZhou719 commented Feb 28, 2023

kakaZhou719 Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Feb 28, 2023 • edited Loading

Codecov Report

starnop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kakaZhou719 Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

kakaZhou719 commented Mar 10, 2023

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

kakaZhou719 Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Stevent-fei commented Feb 10, 2023 •

edited by kakaZhou719

Loading

kakaZhou719 Feb 28, 2023 •

edited

Loading

codecov-commenter commented Feb 28, 2023 •

edited

Loading

kakaZhou719 Mar 10, 2023 •

edited

Loading

kakaZhou719 Mar 14, 2023 •

edited

Loading