Wait until the manager is actually gone before continuing the upgrade #1635

ANeumann82 · 2020-08-06T09:53:49Z

Signed-off-by: Andreas Neumann aneumann@mesosphere.com

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

alenkacz

I think we need to wait for the webhook as well, right? UninstallWebHook

pkg/kudoctl/kudoinit/manager/manager.go

kensipe

agree with @alenkacz
Also... I much prefer to move k8s specific functionality to that package. What we are deleting or waiting for should be declared here (manager) but the logic of how to delete or wait should be encapsulated in a kube / kubernetes package. Not a blocker as there is a prior art... but a kind loving nug :)

ANeumann82 · 2020-08-06T14:31:36Z

agree with @alenkacz
Also... I much prefer to move k8s specific functionality to that package. What we are deleting or waiting for should be declared here (manager) but the logic of how to delete or wait should be encapsulated in a kube / kubernetes package. Not a blocker as there is a prior art... but a kind loving nug :)

Good point. I've started out that way, but that was a bit too generic: In the kubernetes package I'd like to have a generic version of this, which would need the dynamic.Interface client which we currently don't have in the kubectl client.

I'm not opposed to adding it there though. Especially as we have to wait for the webhook resource as well, I think it makes sense to generalise this.

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Waiting for Webhook is now in, and Alena doesn't have time for a full re-review today

kensipe

nothing blocking... but I'm challenged by the verbosity levels and the name "step" in the initializer code.

kensipe · 2020-08-07T13:47:46Z

pkg/kubernetes/client.go

+	// Wait for resources to be deleted.
+	return wait.PollImmediate(250*time.Millisecond, 30*time.Second, func() (done bool, err error) {
+		err = c.Get(context.TODO(), key, obj.DeepCopyObject())
+		clog.V(8).Printf("Fetched %s/%s to wait for delete: %v", key.Namespace, key.Name, err)


questioning this verbose levels... level 7-9 tend to be deconstruction of serialized objects or deep http calls. These seem like level 2 and 3 type stuff (obviously subjective)

I've adjusted them a bit to 6 - This one especially is in a tight loop and can get quite spammy that's why I didn't want to have it in the higher ones

kensipe · 2020-08-07T13:49:30Z

pkg/kubernetes/client.go

@@ -15,3 +24,47 @@ func GetDiscoveryClient(mgr manager.Manager) (*discovery.DiscoveryClient, error)
 	}
 	return dc, nil
 }
+
+// DeleteAndWait deletes the given runtime object and waits until it is fully deleted
+func DeleteAndWait(c client.Client, obj runtime.Object, options ...client.DeleteOption) error {


what do you think about a Delete(c, obj, wait bool, options) ?

I'd rather add another func

Delete(c client.Client, obj runtime.Object, options ...client.DeleteOption) error

which does not wait. I think having the "AndWait" in the name makes the intention more clear than a bool param.

kensipe · 2020-08-07T13:53:18Z

pkg/kudoctl/kudoinit/manager/manager.go

@@ -33,8 +34,8 @@ type Initializer struct {
 }

 // NewInitializer returns the setup management object
-func NewInitializer(options kudoinit.Options) Initializer {
-	return Initializer{
+func NewInitializer(options kudoinit.Options) *Initializer {


is there a reason for this change?
makes me wonder if the methods need adjustment as well... meaning
func (m Initializer) UninstallService(client *kube.Client) error { ->
func (m *Initializer) UninstallService(client *kube.Client) error { ?

Mostly to make the usage of this more clear in setup.go. And i've changed the receiver to a pointer. It's not required to be mutable right now, but it might as well be in the future and can be hard to find these kind of bugs :-/

kensipe · 2020-08-07T13:53:53Z

pkg/kudoctl/kudoinit/manager/manager.go

+}
+
+func (m Initializer) UninstallService(client *kube.Client) error {
+	clog.V(4).Printf("Uninstall KUDO manager service")


this number seems high... I would expect 2

kensipe · 2020-08-07T13:55:34Z

pkg/kudoctl/kudoinit/setup/setup.go

@@ -33,14 +36,22 @@ func NewInstaller(options kudoinit.Options, crdOnly bool) *Installer {
 		}
 	}

+	// This is a bit cumbersome - we need to access some funcs from these two
+	// steps for the upgrade process, that's why they are initialized here.
+	// This should be cleaned up


could you elaborate (in code comments) what is meant by cleaned up?

I've removed the "Cleaned up" part and changed the comment a bit.

I think the whole setup.go with the []kudoinit.Step slice should probably refactored. It was a good idea in the beginning, but doesn't work that well with upgrades when we need to access specific elements from the slice.

kensipe · 2020-08-07T13:57:17Z

pkg/kudoctl/kudoinit/setup/setup.go

@@ -21,6 +21,9 @@ var _ kudoinit.InstallVerifier = &Installer{}
 type Installer struct {
 	options kudoinit.Options
 	steps   []kudoinit.Step
+
+	managerStep *manager.Initializer


really don't like the name... step is a meaningful thing in kudo, and this doesn't align... it would be ok if it was some kind of step, but I don't see that... it is a manager Initializer..

I've renamed it to managerInitializer, same for the webhook

kensipe · 2020-08-07T13:58:37Z

pkg/kudoctl/kudoinit/setup/setup.go

@@ -21,6 +21,9 @@ var _ kudoinit.InstallVerifier = &Installer{}
 type Installer struct {
 	options kudoinit.Options
 	steps   []kudoinit.Step


this is likely where the managerStep gets it's name... but why do we see these as steps? These are an array of initializers... the renaming to step loses that context.

I've renamed the slice as well. I've left the kudoinit.Step type for now, but I agree that this should probably be renamed in another PR

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Wait until the manager is actually gone before continuing the upgrade

ec0058f

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 requested review from alenkacz, gerred, kensipe, nfnt and zen-dog as code owners August 6, 2020 09:53

alenkacz previously requested changes Aug 6, 2020

View reviewed changes

pkg/kudoctl/kudoinit/manager/manager.go Outdated Show resolved Hide resolved

kensipe reviewed Aug 6, 2020

View reviewed changes

ANeumann82 added 5 commits August 6, 2020 18:55

Use generic DeleteAndWait functions, cleanup

88af6ca

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Fixed tests

97e2876

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Fixed more tests

d5eb597

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Fixed wrong return in DeleteAndWait

4fc3b6f

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Fixed comments

708f9b4

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

alenkacz self-requested a review August 7, 2020 13:10

kensipe approved these changes Aug 7, 2020

View reviewed changes

Review requests

a41019a

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 merged commit 9a08e27 into main Aug 17, 2020

ANeumann82 deleted the an/wait-for-delete-uninstall-manager branch August 17, 2020 08:16

ANeumann82 mentioned this pull request Aug 18, 2020

Panic while running kudo init --upgrade #1632

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wait until the manager is actually gone before continuing the upgrade #1635

Wait until the manager is actually gone before continuing the upgrade #1635

ANeumann82 commented Aug 6, 2020 •

edited

Loading

alenkacz left a comment

kensipe left a comment

ANeumann82 commented Aug 6, 2020

kensipe left a comment

kensipe Aug 7, 2020

ANeumann82 Aug 7, 2020

kensipe Aug 7, 2020

ANeumann82 Aug 7, 2020

kensipe Aug 7, 2020

ANeumann82 Aug 7, 2020

kensipe Aug 7, 2020

kensipe Aug 7, 2020

ANeumann82 Aug 7, 2020

kensipe Aug 7, 2020

ANeumann82 Aug 7, 2020

kensipe Aug 7, 2020

ANeumann82 Aug 7, 2020

Wait until the manager is actually gone before continuing the upgrade #1635

Wait until the manager is actually gone before continuing the upgrade #1635

Conversation

ANeumann82 commented Aug 6, 2020 • edited Loading

alenkacz left a comment

Choose a reason for hiding this comment

kensipe left a comment

Choose a reason for hiding this comment

ANeumann82 commented Aug 6, 2020

kensipe left a 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

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

ANeumann82 commented Aug 6, 2020 •

edited

Loading