Prober should not consider `Failed` or `Terminating` Machines, `Unhealthy` Nodes. #124

rishabh-11 · 2024-07-01T06:25:26Z

What this PR does / why we need it:
This PR does the following:-

Prober will no longer consider Failed or Terminating Machines in candidate lease calculation.
Prober will no longer consider Unhealthy Nodes in candidate lease calculation. The criteria for a node to be Unhealthy is as follows:-
a.) The Shoot resource has a section to define node Conditions per worker to be checked by MCM for marking a machine Unknown See:- https://github.com/gardener/gardener/blob/98fb871eb6045f69715d9f8b39dd84f9637f1fa0/example/90-shoot.yaml#L138. If not specified DWD will use the default node conditions that MCM uses (See :- https://github.com/gardener/machine-controller-manager/blob/11c2983db70c3c13a7a95d51f9de9896e69d4599/pkg/util/provider/app/options/options.go#L60).
b.) If any of these node conditions is set to True, then DWD will consider the node Unhealthy and not consider it in candidate lease calculation.
Replace mocks with fake clients for unit tests.
If there is only one candidate lease present in the cluster, DWD probe will not perform any scaling operations.
Enabled Pprof endpoint to allow profiling of DWD.

Which issue(s) this PR fixes:
Fixes #123 #126
Fixes part of #110

Special notes for your reviewer:

Release note:

Replaced mocks with fake clients for unit tests

DWD prober no longer considers `Failed` or `Terminating` Machines in scaling decisions for the dependent resources.

DWD prober no longer considers `Unhealthy` Nodes in scaling decisions for the dependent resources.

DWD prober no longer performs scaling operations for clusters with only one candidate lease.

…ator

rishabh-11 · 2024-07-01T09:24:18Z

/kind enhancement

rishabh-11 · 2024-07-01T09:40:12Z

/test pull-dependency-watchdog-check-vulnerabilities

gardener-ci-robot · 2024-11-02T06:15:39Z

The Gardener project currently lacks enough active contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:

After 15d of inactivity, lifecycle/stale is applied
After 15d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 7d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Mark this PR as rotten with /lifecycle rotten
Close this PR with /close

/lifecycle stale

gardener-ci-robot · 2024-11-17T06:29:38Z

The Gardener project currently lacks enough active contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:

After 15d of inactivity, lifecycle/stale is applied
After 15d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 7d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close

/lifecycle rotten

unmarshall · 2024-11-25T11:56:11Z

controllers/cluster/clusterpredicate_test.go

@@ -58,30 +60,30 @@ func testDeletePredicateFunc(g *WithT, numWorkers int) bool {
 func TestUpdatePredicateFunc(t *testing.T) {
 	tests := []struct {
 		title          string
-		oldNumWorkers  int
-		newNumWorkers  int
+		oldWorkerNames []string


Just write a small function that generates the worker names and retain the old way as that is more flexible and easier to change. Same elsewhere as well.

unmarshall · 2024-11-25T12:03:10Z

controllers/endpoint/endpoints_controller_test.go

@@ -150,9 +150,10 @@ func testWeederDedicatedEnvTest(t *testing.T) {
 	for _, test := range tests {
 		ctx, cancelFn := context.WithCancel(context.Background())
 		testEnv, reconciler := setupWeederEnv(ctx, t, test.apiServerFlags)
-		ns := testutil.CreateTestNamespace(ctx, g, reconciler.Client, strings.ToLower(test.name))
+		testNs := testutil.GenerateRandomAlphanumericString(g, 4)


You do not need to use this utility function any longer. We should perhaps remove this now.
Use https://pkg.go.dev/k8s.io/apimachinery/pkg/util/rand#String instead.
Change this at all places where this utility function is used.

unmarshall · 2024-11-25T12:07:16Z

internal/docs/e2e-tests.md

+* Check the role/rolebinding in the shoot for the dependency-watchdog-probe related service account in the `kube-system`
+  namespace. Add rules for listing leases in the `kube-node-lease` namespace.
+* Scale down the DWD prober deployment in the garden namespace (in the shooted-seed) and start a local DWD process by
+  providing the prober config and the kubeconfig of the shooted seed as command line flags -


you need to add a new line for commands that are enclosed within bash else the formatting is not going to be correct.

unmarshall · 2024-11-25T12:20:36Z

internal/docs/e2e-tests.md

+> process. To do that you will have to the following:
+> * Create a new `DNSRecord` containing the new shoot Kube API server endpoint. This will create a provider specific
+    route (e.g. In case of AWS it will create a AWS-Route53 endpoint)
+> * To ensure that a call to this endpoint is routed to the Kube API server of the shoot do the following:


Do we need all this? Please revisit and change the docs.

unmarshall · 2024-11-26T05:30:06Z

internal/prober/errors/errors.go

+// ProbeError is the error type for probe errors. It contains the error code, the cause of the error, and the error message.
+// It is used by prober to record its last error and is currently only used for unit tests.
+type ProbeError struct {
+	Code    ErrorCode


add doc strings for all exposed fields

unmarshall · 2024-11-26T05:38:26Z

internal/prober/fakes/k8s/client.go

+
+func (c *fakeClient) getRecordedObjectCollectionError(method ClientMethod, namespace string, labelSelector labels.Selector, objGVK schema.GroupVersionKind) error {
+	for _, errRecord := range c.errorRecords {
+		if errRecord.method == method && errRecord.resourceNamespace == namespace {


you can combine these into a single IF condition and perhaps add some comments as well.

if errRecord.method == method && errRecord.resourceNamespace == namespace && (errRecord.resourceGVK == objGVK || // if it is a direct GVK match (labelSelector == nil && errRecord.labels == nil) || // if there is no label selector and no labels in the error record labelSelector != nil && labelSelector.Matches(errRecord.labels)) { // if there is a label selector, and it matches the labels in the error record return errRecord.err }

unmarshall · 2024-11-26T06:00:48Z

internal/prober/prober_test.go

 }

 func TestAPIServerProbeFailure(t *testing.T) {
-	testCases := []probeTestCase{
-		{name: "Unknown error is returned by api server", discoveryError: errFoo},


you seems to have removed this test case.

It is not needed. The other cases are sufficient.

unmarshall · 2024-11-26T06:09:04Z

internal/test/miscellaneous.go

+}
+
+// GenerateRandomAlphanumericString generates a random alphanumeric string of the given length.
+func GenerateRandomAlphanumericString(g *WithT, length int) string {


this function is no longer required.

unmarshall

/lgtm

gardener-prow · 2024-11-26T16:32:46Z

LGTM label has been added.

Git tree hash: e83c81d405913128845165aff0a9e9367cc3f0f8

gardener-prow · 2024-11-26T16:32:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: unmarshall

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [unmarshall]

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

rishabh-11 and others added 10 commits June 20, 2024 12:28

[WIP] refactoring to solve some existing issues

f30b785

add test for starting probe with correct worker node condition mapping

c0acd16

WIP commmit, enhanced ShootClientCreator, added fake client support

c1312e1

WIP commit for prober changes adapting to changes in shoot client cre…

cebd997

…ator

[WIP] create fakes for discovery and shoot client

e923864

WIP changing package structure and tests

532a50f

restructured fakes to avoid cyclic dependency

dab1cf1

[WIP] add prober test for successful scaleup

4aa4d91

[WIP] add prober tests

5ccef19

replace usage of mocks with fake clients

03e608e

rishabh-11 requested a review from unmarshall July 1, 2024 06:25

gardener-prow bot added the do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Jul 1, 2024

gardener-prow bot requested a review from ashwani2k July 1, 2024 06:25

gardener-prow bot added cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 1, 2024

gardener-robot-ci-3 added the reviewed/ok-to-test label Jul 1, 2024

gardener-ci-robot removed the reviewed/ok-to-test label Jul 1, 2024

fix make check errors

84d4f43

gardener-robot-ci-2 added the reviewed/ok-to-test label Jul 1, 2024

gardener-ci-robot removed the reviewed/ok-to-test label Jul 1, 2024

test package constants in controller tests

7e9a0f4

rishabh-11 force-pushed the refactor branch from d9fabc6 to 7e9a0f4 Compare July 1, 2024 08:20

gardener-robot-ci-3 added the reviewed/ok-to-test label Jul 1, 2024

gardener-ci-robot removed the reviewed/ok-to-test label Jul 1, 2024

gardener-prow bot added kind/enhancement Enhancement, improvement, extension and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jul 1, 2024

update e2e test doc

57b906b

gardener-robot-ci-3 added the reviewed/ok-to-test label Jul 1, 2024

gardener-prow bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 17, 2024

rishabh-11 removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Oct 18, 2024

gardener-prow bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 2, 2024

gardener-prow bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 17, 2024

rishabh-11 removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Nov 18, 2024

pass correct ns when creating new scaler, fix e2e test docs

d50a034

gardener-robot-ci-3 added the reviewed/ok-to-test label Nov 19, 2024

gardener-ci-robot removed the reviewed/ok-to-test label Nov 19, 2024

unmarshall requested changes Nov 25, 2024

View reviewed changes

unmarshall requested changes Nov 26, 2024

View reviewed changes

gardener-robot-ci-1 added the reviewed/ok-to-test label Nov 26, 2024

rishabh-11 force-pushed the refactor branch from b4a4cf6 to 26f9fdb Compare November 26, 2024 10:37

gardener-ci-robot removed the reviewed/ok-to-test label Nov 26, 2024

address review comments - part 2

b5fca63

rishabh-11 force-pushed the refactor branch from 09ae8b5 to b5fca63 Compare November 26, 2024 11:58

gardener-robot-ci-3 added the reviewed/ok-to-test label Nov 26, 2024

gardener-ci-robot removed the reviewed/ok-to-test label Nov 26, 2024

unmarshall approved these changes Nov 26, 2024

View reviewed changes

gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2024

gardener-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 26, 2024

unmarshall merged commit e54d69f into gardener:master Nov 26, 2024
6 of 7 checks passed

gardener-robot-ci-2 added the reviewed/ok-to-test label Nov 26, 2024

rishabh-11 deleted the refactor branch November 26, 2024 17:54

rishabh-11 mentioned this pull request Nov 30, 2024

Skip prober scaling for clusters with only one active node #126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prober should not consider `Failed` or `Terminating` Machines, `Unhealthy` Nodes. #124

Prober should not consider `Failed` or `Terminating` Machines, `Unhealthy` Nodes. #124

rishabh-11 commented Jul 1, 2024 •

edited

Loading

rishabh-11 commented Jul 1, 2024

rishabh-11 commented Jul 1, 2024

gardener-ci-robot commented Nov 2, 2024

gardener-ci-robot commented Nov 17, 2024

unmarshall Nov 25, 2024

unmarshall Nov 25, 2024

unmarshall Nov 25, 2024

unmarshall Nov 25, 2024

unmarshall Nov 26, 2024

unmarshall Nov 26, 2024

unmarshall Nov 26, 2024

rishabh-11 Nov 26, 2024

unmarshall Nov 26, 2024

unmarshall left a comment

gardener-prow bot commented Nov 26, 2024

gardener-prow bot commented Nov 26, 2024

Prober should not consider Failed or Terminating Machines, Unhealthy Nodes. #124

Prober should not consider Failed or Terminating Machines, Unhealthy Nodes. #124

Conversation

rishabh-11 commented Jul 1, 2024 • edited Loading

rishabh-11 commented Jul 1, 2024

rishabh-11 commented Jul 1, 2024

gardener-ci-robot commented Nov 2, 2024

gardener-ci-robot commented Nov 17, 2024

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

unmarshall left a comment

Choose a reason for hiding this comment

gardener-prow bot commented Nov 26, 2024

gardener-prow bot commented Nov 26, 2024

Prober should not consider `Failed` or `Terminating` Machines, `Unhealthy` Nodes. #124

Prober should not consider `Failed` or `Terminating` Machines, `Unhealthy` Nodes. #124

rishabh-11 commented Jul 1, 2024 •

edited

Loading