[wmco] Improve error handling #42

sebsoto · 2020-04-21T19:50:12Z

This PR includes various commits to clean up the how the operator handles errors both in the creation of the controller, and the creation of nodes.

VaishnaviHire

Thanks for working on this @sebsoto . Please address my comments

pkg/controller/windowsmachineconfig/windowsmachineconfig_controller.go

aravindhp

Thanks for working on this, @sebsoto. Please address my comments.

pkg/controller/windowsmachineconfig/windowsmachineconfig_controller.go

aravindhp · 2020-04-22T17:12:20Z

pkg/controller/windowsmachineconfig/windowsmachineconfig_controller.go

+	windowsVMs := make(map[types.WindowsVM]bool)
+	vmTracker, err := tracker.NewTracker(clientset, windowsVMs)
+	if err != nil {
+		return nil, errors.Wrap(err, "tracker instantiation failed")


failed to instantiate tracker

pkg/controller/windowsmachineconfig/windowsmachineconfig_controller.go

mansikulkarni96

Thanks for the PR @sebsoto. PTAL at my comments.

pkg/controller/windowsmachineconfig/windowsmachineconfig_controller.go

mansikulkarni96 · 2020-04-22T19:40:43Z

pkg/controller/windowsmachineconfig/windowsmachineconfig_controller.go

+	}
+
+	// Terminate the instance via its instance id
+	id := vm.GetCredentials().GetInstanceId()


nit: rename id to instancedID for specific naming

This commit changes the creation of the WMC controller to be more diligent about handling errors. This commit changes two things * If the kubernetes clientset cannot be created, that error will be handled instead of just being logged. * Error messages around the previous addition have been made more verbose

This commit moves the creation of our node tracker out of the reconcile function to where we create the reconciler. This is being done to create a distinction between the setup of the controller and the functionality of the controller, allowing for cleaner error handling.

aravindhp · 2020-04-22T21:59:56Z

/retest

Testing openshift/release#8323

aravindhp · 2020-04-22T23:31:02Z

@sebsoto when you get a chance please introduce a failure in one of the e2e tests. I want to see if we will get to see the test output in that case. It looks like with the step registry e2e test output is not show on success.

mansikulkarni96 · 2020-04-24T17:21:48Z

LGTM

pkg/controller/windowsmachineconfig/windowsmachineconfig_controller.go

aravindhp · 2020-04-24T14:56:18Z

pkg/controller/windowsmachineconfig/windowsmachineconfig_controller.go


-// deleteWindowsVMs deletes the required number of Windows VMs from the cluster and returns a bool indicating the
-// status of deletion. This method will return false if any of the VMs fail to get deleted.
+// remove removes the given vm from the list of VMs and terminates the underlying VM


To be pedantic I would change order. i.e. terminate first and then remove from the list.

How about renaming the function to removeWorkerNode() or removeWorker and sync with rest of the functions? addWorker addWorkers etc.

I also think we don't need to call out that it is Windows as it obvious given our operator is WMCO :-) But I am open to suggestions.

aravindhp · 2020-04-24T14:57:19Z

pkg/controller/windowsmachineconfig/windowsmachineconfig_controller.go

 // TODO: This method should return a slice of errors that we collected.
 //		Jira story: https://issues.redhat.com/browse/WINC-266
-func (r *ReconcileWindowsMachineConfig) deleteWindowsVMs(count int) bool {
+func (r *ReconcileWindowsMachineConfig) removeWindowsWorkerNodes(count int) bool {


As per the previous comment this would become removeWorkerNodes

aravindhp · 2020-04-24T14:59:03Z

pkg/controller/windowsmachineconfig/windowsmachineconfig_controller.go

 }

-// createWindowsVMs creates the required number of windows Windows VM and configure them to make
+// addWindowsWorker creates a new Windows VM and configures it, adding it as a node object to the cluster


addWorkerNode()

aravindhp · 2020-04-24T14:59:26Z

pkg/controller/windowsmachineconfig/windowsmachineconfig_controller.go

+	if err != nil {
+		return nil, errors.Wrap(err, "error creating windows VM")
+	}
+	log.V(1).Info("created a new Windows VM", "ID", vm.GetCredentials().GetInstanceId())


I would remove this as the next debug message implies the VM was created.

aravindhp · 2020-04-24T15:13:19Z

pkg/controller/windowsmachineconfig/windowsmachineconfig_controller.go

 // TODO: This method should return a slice of errors that we collected.
 //		Jira story: https://issues.redhat.com/browse/WINC-266
-func (r *ReconcileWindowsMachineConfig) createWindowsWorkerNodes(count int) bool {
+func (r *ReconcileWindowsMachineConfig) addWindowsWorkerNodes(count int) bool {


addWorkerNode

aravindhp · 2020-04-24T15:18:19Z

go.mod

+	github.com/openshift/api v3.9.1-0.20190924102528-32369d4db2ad+incompatible
+	github.com/openshift/client-go v0.0.0-20190923180330-3b6373338c9b


We cannot be pinned to 3.9. That version is too old.

aravindhp · 2020-04-24T15:20:06Z

pkg/controller/windowsmachineconfig/windowsmachineconfig_controller.go

 	// tracker is used to track all the Windows nodes created via WMCO
 	tracker *tracker.Tracker
+	// nodeConfigurer is what is used to configure the created Windows VMs
+	nodeConfigurer nodeconfig.Interface


Please call this nodeConfig

aravindhp · 2020-04-24T15:22:10Z

pkg/controller/windowsmachineconfig/nodeconfig/nodeconfig.go

 )

+type Interface interface {
+	Configure(types.WindowsVM) error


Why do you need to pass in types.WindowsVM? Can't you create a mocked version of nodeConfig?

I do create a mocked version, but in order to use the mocked version within the reconciler we need to pass in the nodeconfig entity in some way. As it was before the regular nodeconfig was being created within the reconciliation process. That doesn't give us any way to switch to using the mocked one.

By making the nodeconfig part of the reconciler we can switch between using the regular and mocked version. Passing in the types.WindowsVM becomes necessary as we will now be using the same nodeconfig for multiple VMs, instead of a new one for every VM. We need to pass in the VM to select which VM will be configured.

aravindhp · 2020-04-24T17:28:49Z

pkg/controller/windowsmachineconfig/windowsmachineconfig_controller.go

+			k8sclientset:   clientset,
+			tracker:        vmTracker,
+			windowsVMs:     windowsVMs,
+			nodeConfigurer: nodeconfig.NewNodeConfig(clientset),


I am not able to understand why you need to do this. I don't see the utility of having generic node configuration entities

I think I explained this in the above message, let me know if this is a separate issue.

aravindhp

Looks good, @sebsoto. Just a few changes in the error messages.

pkg/controller/windowsmachineconfig/windowsmachineconfig_controller.go

Currently VMs that fail to become a node are left orphaned. This commit changes that, so that if a VM fails to become a node via bootstrapping it is deleted properly. This is being done so that we can properly track the status of the VMs we create.

aravindhp

/approve

Thanks for adding this, @sebsoto

aravindhp · 2020-04-28T16:17:54Z

/hold

aravindhp · 2020-04-28T16:18:03Z

/lgtm cancel

ravisantoshgudimetla

/lgtm

Thanks for working on this PR @sebsoto

openshift-ci-robot · 2020-04-28T16:25:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aravindhp, ravisantoshgudimetla, sebsoto

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~~ [aravindhp,ravisantoshgudimetla]

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

sebsoto · 2020-04-28T17:19:47Z

/hold cancel

[logging] Install Fluentd on Windows instance

openshift-ci-robot requested a review from aravindhp April 21, 2020 19:50

aravindhp changed the title ~~Operator error handling~~ [wmco] Improve error handling Apr 21, 2020

sebsoto force-pushed the operator_changes branch 2 times, most recently from 8ee7ea7 to 3251715 Compare April 22, 2020 14:45

VaishnaviHire reviewed Apr 22, 2020

View reviewed changes

aravindhp suggested changes Apr 22, 2020

View reviewed changes

openshift-ci-robot assigned aravindhp Apr 22, 2020

mansikulkarni96 reviewed Apr 22, 2020

View reviewed changes

sebsoto force-pushed the operator_changes branch from 3251715 to 83e7f53 Compare April 22, 2020 20:38

sebsoto added 2 commits April 22, 2020 17:07

sebsoto force-pushed the operator_changes branch from 83e7f53 to 3b67eb1 Compare April 22, 2020 21:07

sebsoto changed the title ~~[wmco] Improve error handling~~ [WIP] [wmco] Improve error handling Apr 23, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 23, 2020

sebsoto force-pushed the operator_changes branch 3 times, most recently from e3569ae to 7f0f8ba Compare April 23, 2020 20:24

sebsoto changed the title ~~[WIP] [wmco] Improve error handling~~ [wmco] Improve error handling Apr 23, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 23, 2020

sebsoto force-pushed the operator_changes branch 2 times, most recently from 8671abb to 260d1e8 Compare April 24, 2020 14:23

aravindhp suggested changes Apr 24, 2020

View reviewed changes

sebsoto force-pushed the operator_changes branch from 38d9a9e to bd40393 Compare April 27, 2020 20:42

aravindhp suggested changes Apr 28, 2020

View reviewed changes

sebsoto force-pushed the operator_changes branch from bd40393 to dbb8a21 Compare April 28, 2020 14:57

aravindhp approved these changes Apr 28, 2020

View reviewed changes

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 28, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 28, 2020

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 28, 2020

ravisantoshgudimetla approved these changes Apr 28, 2020

View reviewed changes

openshift-ci-robot assigned ravisantoshgudimetla Apr 28, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 28, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 28, 2020

openshift-merge-robot merged commit 52a7d27 into openshift:master Apr 28, 2020

wgahnagl pushed a commit to wgahnagl/windows-machine-config-operator that referenced this pull request Mar 6, 2023

Merge pull request openshift#42 from suhanime/winc35

5f89d9e

[logging] Install Fluentd on Windows instance

		github.com/openshift/api v3.9.1-0.20190924102528-32369d4db2ad+incompatible
		github.com/openshift/client-go v0.0.0-20190923180330-3b6373338c9b

[wmco] Improve error handling #42

[wmco] Improve error handling #42

Uh oh!

Conversation

sebsoto commented Apr 21, 2020

Uh oh!

VaishnaviHire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aravindhp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mansikulkarni96 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aravindhp commented Apr 22, 2020

Uh oh!

aravindhp commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mansikulkarni96 commented Apr 24, 2020

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!

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!

aravindhp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aravindhp left a comment

Choose a reason for hiding this comment

Uh oh!

aravindhp commented Apr 28, 2020

Uh oh!

aravindhp commented Apr 28, 2020

Uh oh!

ravisantoshgudimetla left a comment

Choose a reason for hiding this comment

Uh oh!

aravindhp commented Apr 22, 2020 •

edited

Loading