cli: NS removed from mesh when uninstalling mesh #4555

mudit-01 · 2022-02-23T10:25:15Z

"osm uninstall mesh" removes osm-related labels and annotations from the monitored namespace, partially fixes #4444

Signed-off-by: mudit singh mudit.singh@india.nec.com

Description:

Testing done: yes(Manual) below are the screenshots

Affected area:

Functional Area
New Func	[ ]
CI System	[ ]
CLI Tool	[x ]
Certificate Management	[ ]
Control Plane	[ ]
Demo	[ ]
Documentation	[ ]
Egress	[ ]
Ingress	[ ]
Install	[ ]
Networking	[ ]
Observability	[ ]
Performance	[ ]
SMI Policy	[ ]
Security	[ ]
Sidecar Injection	[ ]
Tests	[ ]
Upgrade	[ ]
Other	[ ]

Please answer the following questions with yes/no.

Does this change contain code from or inspired by another project?
- Did you notify the maintainers and provide attribution?
Is this a breaking change?
Has documentation corresponding to this change been updated in the osm-docs repo (if applicable)?

"osm uninstall mesh" removes osm-related labels and annotations from the monitored namespace Signed-off-by: mudit singh <mudit.singh@india.nec.com>

Added error check for removing the namespace within a mesh Signed-off-by: mudit singh <mudit.singh@india.nec.com>

codecov-commenter · 2022-02-23T11:03:30Z

Codecov Report

Merging #4555 (d019aab) into main (1ae2578) will increase coverage by 0.02%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main    #4555      +/-   ##
==========================================
+ Coverage   68.10%   68.13%   +0.02%     
==========================================
  Files         195      195              
  Lines       15707    15718      +11     
==========================================
+ Hits        10697    10709      +12     
+ Misses       4957     4956       -1     
  Partials       53       53

Flag	Coverage Δ
unittests	`68.13% <83.33%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
cmd/cli/uninstall_mesh.go	`70.28% <83.33%> (+0.37%)`	⬆️
cmd/cli/util.go	`71.64% <0.00%> (+1.54%)`	⬆️

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

Added test for uninstalling osm when the mesh has a namespace added to it Signed-off-by: mudit singh <mudit.singh@india.nec.com>

Added enableMetrics argument in function call Signed-off-by: mudit singh <mudit.singh@india.nec.com>

mudit-01 · 2022-03-03T04:21:54Z

@snehachhabria @shashankram @jaellio @nojnhuh please suggest, if there are any changes for the PR.

draychev

@mudit-01 Thank you so much for helping with this project.
I left a small comment questioning the need to check whether namespaces is empty.

cmd/cli/uninstall_mesh.go

draychev · 2022-04-05T04:12:05Z

cmd/cli/uninstall_mesh.go

+
+					// Unable to remove namespace in mesh
+					if err != nil {
+						return err


I wonder if it would be helpful to log the reason for the inability to remove namespaces with log.Error().Err(err).Msgf("Error removing namespaces: %+v", namespaces.Items)
OR we could assume that the caller would do that for us based on the err they get.

I believe caller one is good.
what do other people suggest?

Can we mark this as resolved @draychev?

Removed one level of indentation Signed-off-by: mudit singh <mudit.singh@india.nec.com>

mudit-01 · 2022-05-04T13:56:41Z

Can we merge this PR?

shashankram · 2022-05-25T17:20:36Z

Hey @mudit-01, this PR has some conflicts with the version in main. Would you mind rebasing/merging your changes with the main branch.

mudit-01 · 2022-05-26T10:29:22Z

Thanks @shashankram I will work on resolving asap

Replaced spaces with tab Signed-off-by: mudit singh <mudit.singh@india.nec.com>

Removed outdated test and checks Signed-off-by: mudit singh <mudit.singh@india.nec.com>

mudit-01 · 2022-06-03T15:45:34Z

Hi @shashankram need help in resolving the lint error(cyclomatic complexity). How can I remove that currently it's 32 just 2 more than the threshold?

cmd/cli/uninstall_mesh.go

jaellio · 2022-07-20T00:24:21Z

cmd/cli/uninstall_mesh.go

@@ -171,6 +171,17 @@ func (d *uninstallMeshCmd) run() error {
 			}

 			if err == nil {
+				namespaces, _ := selectNamespacesMonitoredByMesh(m.name, d.clientSet)


@mudit-01 Could you add a unit test for the namespace cleanup on mesh uninstall? Thanks!

Sure @jaellio I will do that thanks for the review

@mudit-01 we are pending a updated unit test here.

osm/cmd/cli/uninstall_mesh_test.go

Line 631 in d2c37a6

assert.Equal(len(test.existingNamespaces), len(namespaces.Items))

You can check if the OSM related labels/annotations are removed from existing namespaces after uninstallation.

@jaellio @allenlsy added test for checking labels/annotations.

jaellio

Please see my above comments! Thank you!

As suggested Signed-off-by: mudit singh <mudit.singh@india.nec.com> Co-authored-by: Jackie Elliott <64559656+jaellio@users.noreply.github.com>

* chore(cmd/cli): Reduce cyclomatic complexity of uninstall command (#4825) * chore(cmd/cli): Reduce cyclomatic complexity of uninstall command Separate code into functions to reduce the cyclomatic complexity And adds unit tests to functions created Helps unblock #4555 Signed-off-by: Shalier Xia <shalierxia@microsoft.com> (cherry picked from commit d5d3a25) * Golang errors pkg (#4904) This PR partially resolves #4524 * Removes "github.com/pkg/errors" package from the repo * Wrap errors wherever possible rather than using the string format of the error as part of the message Signed-off-by: Allen Leigh <allenlsy@gmail.com> (cherry picked from commit 8030047) * fix golints and security scan issues (#4915) fix golints and security for: 1. G112 (ReadHeaderTimeout) 2. prometheus client_go version pinned to bad version Signed-off-by: Sean Teeling <seanteeling@microsoft.com> (cherry picked from commit f768f64) * Fix lints by removing superfluous var type from var instantiation (#4917) Remove unnecessary var types during instantiation to fix lints Signed-off-by: Sean Teeling <seanteeling@microsoft.com> (cherry picked from commit 9e9f712) Co-authored-by: Shalier Xia <69616256+shalier@users.noreply.github.com> Co-authored-by: allenlsy <allenlsy@gmail.com> Co-authored-by: steeling <seanteeling@microsoft.com>

Updated message for error Signed-off-by: mudit singh <mudit.singh@india.nec.com>

Updated command for removing lint error Signed-off-by: mudit singh <mudit.singh@india.nec.com>

Removed lint error Signed-off-by: mudit singh <mudit.singh@india.nec.com>

mudit-01 · 2022-08-17T17:53:25Z

Thanks! @allenlsy for the review :)

Added test for checking removed label and annotation of removed namespace while uninstalling Signed-off-by: mudit singh <mudit.singh@india.nec.com>

allenlsy · 2022-09-15T16:28:49Z

cmd/cli/uninstall_mesh_test.go

@@ -268,6 +286,16 @@ func TestUninstallCmd(t *testing.T) {

 				if test.userPromptsYes {
 					if test.meshExists {
+						if len(test.nsInMesh) != 0 {


Optional:

Using too many if...else is not preferred. Since removing these labels and annotation applies to the whole uninstall command, we can make the test case more general.

i.e.

tests := []struct { ... workloadNamespaces []string ... } // when setting up test env for workloadNs := range test.workloadNamespaces { // create workload namespace nsSpec := createNamespaceSpec(workloadNs, test.meshName, true, false, true) _, err = fakeClientSet.CoreV1().Namespaces().Create(context.TODO(), nsSpec, metav1.CreateOptions{}) assert.Nil(err) } // verifying in test case (line 289) for workloadNs := range test.workloadNamespaces { // check if labels and annotations are removed for workloadNs, same as your implementation from L290-L298 }

@allenlsy I have implemented the requested change, PTAL

Updated test to be more general Signed-off-by: mudit singh <mudit.singh@india.nec.com>

mudit-01 · 2022-09-29T04:24:53Z

Thanks!! @jaellio @keithmattix for the review

mudit-01 added 2 commits February 23, 2022 15:53

cli: NS removed from mesh when uninstalling mesh

e7947b6

"osm uninstall mesh" removes osm-related labels and annotations from the monitored namespace Signed-off-by: mudit singh <mudit.singh@india.nec.com>

Added check for namespace removal

72bea6e

Added error check for removing the namespace within a mesh Signed-off-by: mudit singh <mudit.singh@india.nec.com>

mudit-01 added 2 commits February 24, 2022 14:48

Added test for uninstalling mesh

274b57d

Added test for uninstalling osm when the mesh has a namespace added to it Signed-off-by: mudit singh <mudit.singh@india.nec.com>

Fixed argument in function call

fd76322

Added enableMetrics argument in function call Signed-off-by: mudit singh <mudit.singh@india.nec.com>

mudit-01 marked this pull request as ready for review February 24, 2022 11:44

mudit-01 requested review from shashankram, snehachhabria, nojnhuh, draychev and jaellio as code owners February 24, 2022 11:44

draychev reviewed Apr 5, 2022

View reviewed changes

Merge branch 'openservicemesh:main' into contrib

c9381a2

mudit-01 requested a review from trstringer as a code owner April 21, 2022 09:03

Removed indentation

9c4064e

Removed one level of indentation Signed-off-by: mudit singh <mudit.singh@india.nec.com>

mudit-01 marked this pull request as draft April 21, 2022 09:14

mudit-01 marked this pull request as ready for review April 21, 2022 09:42

mudit-01 requested a review from draychev April 21, 2022 09:48

Merge branch 'main' into contrib

ea0cb9a

mudit-01 marked this pull request as draft May 26, 2022 10:03

mudit-01 added 3 commits June 2, 2022 16:42

Merge branch 'openservicemesh:main' into contrib

f4189f7

Removed lint error

54bea0b

Replaced spaces with tab Signed-off-by: mudit singh <mudit.singh@india.nec.com>

Removed outdated tests

f7e3046

Removed outdated test and checks Signed-off-by: mudit singh <mudit.singh@india.nec.com>

mudit-01 marked this pull request as ready for review June 3, 2022 15:42

steeling reviewed Jun 15, 2022

View reviewed changes

cmd/cli/uninstall_mesh.go Show resolved Hide resolved

mudit-01 requested review from jaellio and steeling July 18, 2022 16:10

jaellio reviewed Jul 20, 2022

View reviewed changes

cmd/cli/uninstall_mesh.go Outdated Show resolved Hide resolved

jaellio reviewed Jul 20, 2022

View reviewed changes

jaellio suggested changes Jul 20, 2022

View reviewed changes

Update cmd/cli/uninstall_mesh.go

641bebd

As suggested Signed-off-by: mudit singh <mudit.singh@india.nec.com> Co-authored-by: Jackie Elliott <64559656+jaellio@users.noreply.github.com>

mudit-01 added 6 commits July 28, 2022 11:26

Merge branch 'openservicemesh:main' into contrib

f83917d

Merge branch 'openservicemesh:main' into contrib

9ae76ac

Updated message

1c6e975

Updated message for error Signed-off-by: mudit singh <mudit.singh@india.nec.com>

Merge branch 'openservicemesh:main' into contrib

c02efe2

Updated Message

7975de5

Updated command for removing lint error Signed-off-by: mudit singh <mudit.singh@india.nec.com>

Lint error removed

0ef3219

Removed lint error Signed-off-by: mudit singh <mudit.singh@india.nec.com>

allenlsy requested a review from jaellio August 17, 2022 17:49

allenlsy approved these changes Aug 17, 2022

View reviewed changes

Merge branch 'openservicemesh:main' into contrib

aaf696e

mudit-01 marked this pull request as draft September 15, 2022 05:48

Added test to check removed ns

b4d6da8

Added test for checking removed label and annotation of removed namespace while uninstalling Signed-off-by: mudit singh <mudit.singh@india.nec.com>

mudit-01 marked this pull request as ready for review September 15, 2022 07:24

allenlsy reviewed Sep 15, 2022

View reviewed changes

Merge branch 'openservicemesh:main' into contrib

d9777c1

mudit-01 marked this pull request as draft September 26, 2022 07:56

Updated test

d019aab

Updated test to be more general Signed-off-by: mudit singh <mudit.singh@india.nec.com>

mudit-01 marked this pull request as ready for review September 26, 2022 08:57

jaellio approved these changes Sep 28, 2022

View reviewed changes

keithmattix approved these changes Sep 28, 2022

View reviewed changes

keithmattix merged commit 8cf4c94 into openservicemesh:main Sep 28, 2022

mudit-01 mentioned this pull request Feb 25, 2023

Self-nomination for Maintainer role #5281

Closed

cli: NS removed from mesh when uninstalling mesh #4555

cli: NS removed from mesh when uninstalling mesh #4555

Uh oh!

Conversation

mudit-01 commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mudit-01 commented Mar 3, 2022

Uh oh!

draychev left a comment

Choose a reason for hiding this comment

Uh oh!

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!

mudit-01 commented May 4, 2022

Uh oh!

shashankram commented May 25, 2022

Uh oh!

mudit-01 commented May 26, 2022

Uh oh!

mudit-01 commented Jun 3, 2022

Uh oh!

Uh oh!

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!

jaellio left a comment

Choose a reason for hiding this comment

Uh oh!

mudit-01 commented Aug 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mudit-01 commented Sep 29, 2022

Uh oh!

Uh oh!

mudit-01 commented Feb 23, 2022 •

edited

Loading

codecov-commenter commented Feb 23, 2022 •

edited

Loading