Bug 1986375: adding check for node exporter daemon set #1279

prashbnair · 2021-07-13T09:30:53Z

removing operator log

I added CHANGELOG entry for this change.
No user facing changes, so no entry in CHANGELOG was needed.

arajkumar · 2021-07-13T12:08:09Z

pkg/client/client.go

+				if condition.Type == v1.NodeReady {
+					if condition.Status != v1.ConditionTrue {


nit

Suggested change

if condition.Type == v1.NodeReady {

if condition.Status != v1.ConditionTrue {

if condition.Type == v1.NodeReady && condition.Status != v1.ConditionTrue {

simonpasquier

It might be worth looking if other fields in the daemonset's status wouldn't bring us the same information? In particular numberAvailable and numberReady...

https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.21/#daemonsetstatus-v1-apps

simonpasquier · 2021-07-19T13:23:51Z

pkg/client/client.go

+		nodes := nodeList.Items
+		for _, node := range nodes {


(nit) saving one line

Suggested change

nodes := nodeList.Items

for _, node := range nodes {

for _, node := range nodeList.Items {

simonpasquier · 2021-07-20T10:15:29Z

It might be worth looking if other fields in the daemonset's status wouldn't bring us the same information? In particular numberAvailable and numberReady...

Scratch that, it doesn't seem to provide the information we need.

simonpasquier · 2021-07-20T10:21:16Z

pkg/client/client.go

+			}
+		}
+
+		if d.Status.NumberUnavailable != 0 && d.Status.NumberUnavailable != nodeNotReadyCount {


another way to look at it would be to compare the number of ready nodes against NumberAvailable. If there's a difference, CMO could log something like expected %d ready pods for %q daemonset, got only %d

simonpasquier · 2021-07-23T14:02:49Z

pkg/client/client.go

 			lastErr = errors.Errorf("got %d unavailable nodes",
 				d.Status.NumberUnavailable)


Suggested change

lastErr = errors.Errorf("got %d unavailable nodes",

d.Status.NumberUnavailable)

lastErr = errors.Errorf("expected %d ready pods for %q daemonset, got only %d",

nodeReadyCount, ds.GetName(), d.Status.NumberAvailable)

dgrisonnet · 2021-07-26T13:31:29Z

pkg/client/client.go

-				d.Status.NumberUnavailable)
+
+		var nodeReadyCount int32
+		nodeList, err := c.kclient.CoreV1().Nodes().List(context.TODO(), metav1.ListOptions{})


let's pass the context ctx here instead of TODO()

dgrisonnet · 2021-07-26T13:33:22Z

pkg/client/client.go

+		}
+
+		if d.Status.NumberAvailable != nodeReadyCount {
+			lastErr = errors.Errorf("expected %d ready pods for %q daemon set, got %d ",


Suggested change

lastErr = errors.Errorf("expected %d ready pods for %q daemon set, got %d ",

lastErr = errors.Errorf("expected %d ready pods for %q daemonset, got %d ",

removing operator log code review comments correcting log message changing to use context instead of todo

prashbnair · 2021-07-27T07:19:43Z

/test e2e-agnostic

openshift-ci · 2021-07-27T07:31:56Z

@prashbnair: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-single-node	`57df962`	link	`/test e2e-aws-single-node`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

dgrisonnet · 2021-07-27T11:48:12Z

/lgtm

openshift-bot · 2021-07-27T12:03:26Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-07-27T12:39:27Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-07-27T13:03:37Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

prashbnair · 2021-07-27T15:55:43Z

/bugzilla refresh

openshift-ci · 2021-07-27T15:55:44Z

@prashbnair: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2021-07-27T15:56:38Z

@prashbnair: This pull request references Bugzilla bug 1986375, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @juzhao

In response to this:

Bug 1986375: adding check for node exporter daemon set

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

simonpasquier

/lgtm

openshift-ci · 2021-07-27T16:12:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrisonnet, prashbnair, simonpasquier

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~~ [dgrisonnet,prashbnair,simonpasquier]

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

openshift-ci · 2021-07-27T17:58:51Z

@prashbnair: All pull requests linked via external trackers have merged:

openshift/cluster-monitoring-operator#1279

Bugzilla bug 1986375 has been moved to the MODIFIED state.

In response to this:

Bug 1986375: adding check for node exporter daemon set

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci bot requested review from arajkumar and raptorsun July 13, 2021 09:31

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 13, 2021

prashbnair changed the title ~~adding check for node exporter daemon set~~ MON-1292: adding check for node exporter daemon set Jul 13, 2021

arajkumar reviewed Jul 13, 2021

View reviewed changes

simonpasquier reviewed Jul 19, 2021

View reviewed changes

simonpasquier reviewed Jul 20, 2021

View reviewed changes

prashbnair force-pushed the mon-1292 branch from 515d8f9 to f3c95cc Compare July 23, 2021 11:45

simonpasquier reviewed Jul 23, 2021

View reviewed changes

prashbnair force-pushed the mon-1292 branch from f3c95cc to ec69852 Compare July 26, 2021 07:08

dgrisonnet reviewed Jul 26, 2021

View reviewed changes

adding check for node exporter daemon set

57df962

removing operator log code review comments correcting log message changing to use context instead of todo

prashbnair force-pushed the mon-1292 branch from ec69852 to 57df962 Compare July 27, 2021 05:08

openshift-ci bot assigned dgrisonnet Jul 27, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2021

prashbnair changed the title ~~MON-1292: adding check for node exporter daemon set~~ Bug 1986375 :adding check for node exporter daemon set Jul 27, 2021

prashbnair changed the title ~~Bug 1986375 :adding check for node exporter daemon set~~ Bug 1986375: adding check for node exporter daemon set Jul 27, 2021

openshift-ci bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jul 27, 2021

openshift-ci bot requested a review from juzhao July 27, 2021 15:56

simonpasquier reviewed Jul 27, 2021

View reviewed changes

openshift-ci bot assigned simonpasquier Jul 27, 2021

openshift-merge-robot merged commit 1959ea9 into openshift:master Jul 27, 2021

prashbnair deleted the mon-1292 branch December 1, 2021 05:10

slashpai mentioned this pull request Nov 22, 2022

OCPBUGS-1998: pkg/client: Update daemonset degrade condition #1812

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1986375: adding check for node exporter daemon set #1279

Bug 1986375: adding check for node exporter daemon set #1279

prashbnair commented Jul 13, 2021

arajkumar Jul 13, 2021

prashbnair Jul 23, 2021

simonpasquier left a comment

simonpasquier Jul 19, 2021

prashbnair Jul 23, 2021

simonpasquier commented Jul 20, 2021

simonpasquier Jul 20, 2021

prashbnair Jul 23, 2021

simonpasquier Jul 23, 2021

prashbnair Jul 26, 2021

dgrisonnet Jul 26, 2021

prashbnair Jul 27, 2021

dgrisonnet Jul 26, 2021

prashbnair Jul 27, 2021

prashbnair commented Jul 27, 2021

openshift-ci bot commented Jul 27, 2021 •

edited

dgrisonnet commented Jul 27, 2021

openshift-bot commented Jul 27, 2021

openshift-bot commented Jul 27, 2021

openshift-bot commented Jul 27, 2021

prashbnair commented Jul 27, 2021

openshift-ci bot commented Jul 27, 2021

openshift-ci bot commented Jul 27, 2021

simonpasquier left a comment

openshift-ci bot commented Jul 27, 2021

openshift-ci bot commented Jul 27, 2021

		if condition.Type == v1.NodeReady {
		if condition.Status != v1.ConditionTrue {

	if condition.Type == v1.NodeReady {
	if condition.Status != v1.ConditionTrue {
	if condition.Type == v1.NodeReady && condition.Status != v1.ConditionTrue {

	nodes := nodeList.Items
	for _, node := range nodes {
	for _, node := range nodeList.Items {

		lastErr = errors.Errorf("got %d unavailable nodes",
		d.Status.NumberUnavailable)

	lastErr = errors.Errorf("expected %d ready pods for %q daemon set, got %d ",
	lastErr = errors.Errorf("expected %d ready pods for %q daemonset, got %d ",

Bug 1986375: adding check for node exporter daemon set #1279

Bug 1986375: adding check for node exporter daemon set #1279

Conversation

prashbnair commented Jul 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier commented Jul 20, 2021

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

prashbnair commented Jul 27, 2021

openshift-ci bot commented Jul 27, 2021 • edited

dgrisonnet commented Jul 27, 2021

openshift-bot commented Jul 27, 2021

openshift-bot commented Jul 27, 2021

openshift-bot commented Jul 27, 2021

prashbnair commented Jul 27, 2021

openshift-ci bot commented Jul 27, 2021

openshift-ci bot commented Jul 27, 2021

simonpasquier left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 27, 2021

openshift-ci bot commented Jul 27, 2021

openshift-ci bot commented Jul 27, 2021 •

edited