Enhancement freeze logic #180

prashanth26 · 2018-10-22T14:22:36Z

What this PR does / why we need it:
We need to enhance the freeze logic of safety-controller when APIServers are not reachable to freeze the machine controller from doing any wrong/unwanted reconcilations

Which issue(s) this PR fixes:
Fixes #155

Special notes for your reviewer:
The commits have been split into the following

(Un)freezing of machine controller based on APIServer availability - Actual logic for freezing
Controller test suite now supports control and target clients, Testing Framework now supports faking of object tracker returns/delays - Enhancements to testing framework
Added test cases for machine controller freezing - Adding unit test cases to test changes.

Release note:

(Un)Freezing of machine controller based on APIServer availability

- Machine Controller is now frozen when either the control (or) target APIServer is unreachable/down - It is unfrozen when they are reachable again - When unfreeze happens, the machine objects are re-queued and any machine object's who health check was being processed is re-initialized

- Supports faking of object tracker returns/delays - Controller test suite now supports control and target clients

amshuman-kr

Thanks, Prashanth! Overall, this looks very good!

I have added some comments and requested some changes.

Makefile

cmd/machine-controller-manager/app/options/options.go

amshuman-kr · 2018-10-22T14:44:33Z

pkg/controller/machine_safety.go

+	glog.V(3).Infof("reconcileClusterMachineSafetyAPIServer: Start")
+	defer glog.V(3).Infof("reconcileClusterMachineSafetyAPIServer: Stop")
+
+	if c.safetyOptions.MachineControllerFrozen {


Shouldn't the logic inside the if-else block run regardless of whether machine controller is frozen?

If condition (When controller is frozen) contains changes that updates machine status when from moving from health-check resetting (Unknown-->Running). Hence it makes sense to run this only when we unfreezing the controller and hence this check.

For the else logic (When controller is not frozen) sets the timer to check APIServer inactivity, and this checks will not be required if the controller is already frozen hence have added this check here.

Keeping the above two reasons in mind, I hope that the conditions make sense.

Ok. Understood.

But I thought inverting the nesting of checks for frozen and isServerUp might lead to leaner generated code. Please ignore. Not important enough ;-)

amshuman-kr · 2018-10-22T14:51:22Z

pkg/controller/machine_safety.go

+			c.safetyOptions.APIserverInactiveStartTime = time.Time{}
+			glog.V(2).Infof("reconcileClusterMachineSafetyAPIServer: UnFreezing Machine Controller")
+		}
+	} else {


Same as above.

Same as above ;-)

pkg/controller/machine_safety_test.go

pkg/controller/machine_test.go

pkg/fakeclient/client.go

prashanth26 · 2018-10-23T03:26:59Z

Thanks for your review @amshuman-kr . I shall make the necessary changes.

ggaurav10

Does this scale? I mean - for large clusters of sizes 100-200 machines:

How much time does isAPIServerUp() take in listing all nodes and all machines
How much time does it take to set/reset the status field of all machines in both cases of freeze and unfreeze

Can we do a sample run, and get these numbers. That would give some confidence into the design.
Also how much do we want it to scale. 1000 machines?
In case, we find that this doesn't scale, then does below method provide some improvement:

isAPIServerUp() lists all machinesets or machinedeployments
Instead of setting/resetting status of all machines, does it make more sense to set status on machinedpeloyment and stop machine deployment controller from reconciling status from machineset

amshuman-kr · 2018-10-23T09:34:04Z

Perhaps we can use a get call to a known non-existing resource where we expect a standard error response?

Or simply use the REST client instead?

prashanth26 · 2018-10-23T10:17:38Z

Or maybe I could run a simple GET("dummy_name") call on a dummy machine/node object. Thereby only trying to fetch a single object at most.

prashanth26 · 2018-10-24T10:08:19Z

I have made changes to adapt to most of the changes suggested by @amshuman-kr .

With respect to the valid concern raised by @ggaurav10 regarding LIST() of objects, i have changed the checks to GET() instead of LIST(). Refer - 53ad7ff#diff-65daa4cabb1be8c69955ab239204630bR154

pkg/controller/machine_safety.go

prashanth26 · 2018-10-26T09:30:09Z

@ggaurav10 Thanking you 👍 @amshuman-kr I am waiting to hear back from you on this PR.

amshuman-kr

LGTM

Only some questions.

amshuman-kr · 2018-10-30T05:29:23Z

pkg/controller/machine_safety.go

+	glog.V(3).Infof("reconcileClusterMachineSafetyAPIServer: Start")
+	defer glog.V(3).Infof("reconcileClusterMachineSafetyAPIServer: Stop")
+
+	if c.safetyOptions.MachineControllerFrozen {


Ok. Understood.

But I thought inverting the nesting of checks for frozen and isServerUp might lead to leaner generated code. Please ignore. Not important enough ;-)

amshuman-kr · 2018-10-30T05:30:02Z

pkg/controller/machine_safety.go

+			c.safetyOptions.APIserverInactiveStartTime = time.Time{}
+			glog.V(2).Infof("reconcileClusterMachineSafetyAPIServer: UnFreezing Machine Controller")
+		}
+	} else {


Same as above ;-)

amshuman-kr · 2018-10-30T05:31:05Z

pkg/controller/machine_safety_test.go

-			c, w := createController(stop, namespace, objects, nil)
-			defer w.Stop()
+			c, trackers := createController(stop, namespace, controlMachineObjects, nil, nil)
+			defer trackers.Stop()


amshuman-kr · 2018-10-30T05:40:18Z

pkg/controller/machine_safety.go

+
+// isAPIServerUp returns true if APIServers are up
+// Both control and target APIServers
+func (c *controller) isAPIServerUp() bool {


Do we need some timeouts or retries here before fail and freeze?

We are using timeouts before freezing, I assume this is what you mean by the above statement - https://github.com/gardener/machine-controller-manager/pull/180/files/53ad7ff54c570cab93137d15d8d038ce9036b0bd?utf8=%E2%9C%93&diff=unified#diff-65daa4cabb1be8c69955ab239204630bR135

So what we have that we have MachineSafetyAPIServerStatusCheckPeriod (default: 1min) and MachineSafetyAPIServerStatusCheckTimeout (default: 30sec). So the MCM polls every 1min and checks if APIServer is down, and if it is then it waits for 30secs during which it polls every 30/5 = 6seconds before it freezes.

I meant timeouts for the Get calls. I guess we are relying on the defaults there from client-co, right? But I guess, that might not matter. Please ignore.

amshuman-kr

LGTM

1. Blank object fetch is causing issue in newer versions of kubernetes 2. Added comments in CI tests

hardikdr

lgtm

prashanth26 requested review from ggaurav10 and amshuman-kr October 22, 2018 14:22

prashanth26 requested a review from a team as a code owner October 22, 2018 14:22

prashanth26 added 2 commits October 22, 2018 19:59

Testing Framework now supports faking of object tracker returns/delays

5c751f5

- Supports faking of object tracker returns/delays - Controller test suite now supports control and target clients

Added test cases for machine controller freezing

adef455

prashanth26 force-pushed the enhancement/enhance-freeze branch from 653e786 to adef455 Compare October 22, 2018 14:29

amshuman-kr suggested changes Oct 22, 2018

View reviewed changes

prashanth26 changed the title ~~Enhancement/enhance freeze~~ Enhancement freeze logic Oct 23, 2018

ggaurav10 reviewed Oct 23, 2018

View reviewed changes

Minor fixes to enhance machine controller freeze

53ad7ff

ggaurav10 suggested changes Oct 26, 2018

View reviewed changes

pkg/controller/machine_safety.go Show resolved Hide resolved

pkg/controller/machine_safety.go Show resolved Hide resolved

pkg/controller/machine_safety.go Show resolved Hide resolved

ggaurav10 approved these changes Oct 26, 2018

View reviewed changes

amshuman-kr reviewed Oct 30, 2018

View reviewed changes

amshuman-kr approved these changes Oct 30, 2018

View reviewed changes

prashanth26 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 31, 2018

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 31, 2018

gardener-robot-ci-1 added the needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 31, 2018

prashanth26 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 31, 2018

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 31, 2018

Get dummy_name objects instead of blank

bdf03bf

1. Blank object fetch is causing issue in newer versions of kubernetes 2. Added comments in CI tests

prashanth26 force-pushed the enhancement/enhance-freeze branch from 9d1dd3c to bdf03bf Compare October 31, 2018 12:16

hardikdr approved these changes Oct 31, 2018

View reviewed changes

hardikdr merged commit 35fd0d5 into gardener:master Oct 31, 2018

prashanth26 deleted the enhancement/enhance-freeze branch November 2, 2018 05:05

ghost added the component/mcm Machine Controller Manager (including Node Problem Detector, Cluster Auto Scaler, etc.) label Mar 7, 2020

himanshu-kun mentioned this pull request May 20, 2022

MCM marking Unknown machines Running on Unfreezing #680

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement freeze logic #180

Enhancement freeze logic #180

prashanth26 commented Oct 22, 2018 •

edited

Loading

amshuman-kr left a comment

amshuman-kr Oct 22, 2018

prashanth26 Oct 23, 2018

amshuman-kr Oct 30, 2018

amshuman-kr Oct 22, 2018

prashanth26 Oct 23, 2018

amshuman-kr Oct 30, 2018

prashanth26 commented Oct 23, 2018 •

edited

Loading

ggaurav10 left a comment •

edited

Loading

amshuman-kr commented Oct 23, 2018 •

edited

Loading

prashanth26 commented Oct 23, 2018

prashanth26 commented Oct 24, 2018

prashanth26 commented Oct 26, 2018 •

edited

Loading

amshuman-kr left a comment

amshuman-kr Oct 30, 2018

amshuman-kr Oct 30, 2018

amshuman-kr Oct 30, 2018

amshuman-kr Oct 30, 2018

prashanth26 Oct 30, 2018 •

edited

Loading

amshuman-kr Oct 30, 2018

amshuman-kr left a comment

hardikdr left a comment

Enhancement freeze logic #180

Enhancement freeze logic #180

Conversation

prashanth26 commented Oct 22, 2018 • edited Loading

amshuman-kr 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

prashanth26 commented Oct 23, 2018 • edited Loading

ggaurav10 left a comment • edited Loading

Choose a reason for hiding this comment

amshuman-kr commented Oct 23, 2018 • edited Loading

prashanth26 commented Oct 23, 2018

prashanth26 commented Oct 24, 2018

prashanth26 commented Oct 26, 2018 • edited Loading

amshuman-kr 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

prashanth26 Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amshuman-kr left a comment

Choose a reason for hiding this comment

hardikdr left a comment

Choose a reason for hiding this comment

prashanth26 commented Oct 22, 2018 •

edited

Loading

prashanth26 commented Oct 23, 2018 •

edited

Loading

ggaurav10 left a comment •

edited

Loading

amshuman-kr commented Oct 23, 2018 •

edited

Loading

prashanth26 commented Oct 26, 2018 •

edited

Loading

prashanth26 Oct 30, 2018 •

edited

Loading