Improve state reporting in kube checks by skonto · Pull Request #2082 · knative/pkg

skonto · 2021-04-02T11:26:16Z

Changes

Report state in kube checks

Fixes #996

codecov · 2021-04-02T11:30:50Z

Codecov Report

Merging #2082 (1ead025) into main (952fdd9) will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2082      +/-   ##
==========================================
- Coverage   67.36%   67.31%   -0.06%     
==========================================
  Files         215      215              
  Lines        9111     9092      -19     
==========================================
- Hits         6138     6120      -18     
+ Misses       2698     2697       -1     
  Partials      275      275

Impacted Files	Coverage Δ
injection/config.go	`0.00% <0.00%> (ø)`
reconciler/filter.go	`100.00% <0.00%> (ø)`
network/domain.go	`50.00% <0.00%> (+7.14%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 952fdd9...1ead025. Read the comment docs.

markusthoemmes · 2021-04-08T07:25:27Z

 	})
+
+	if waitErr != nil {
+		return fmt.Errorf("deployment %q is not in desired state, got: %+v: %w", name, lastState, waitErr)


Should we print the big structs via spew.Sprint. It resolves pointers and stuff. We use that in Serving wherever actual debugging output of full K8s resources is necessary.

markusthoemmes · 2021-04-08T09:17:04Z


 	if waitErr != nil {
-		return fmt.Errorf("deployment %q is not in desired state, got: %+v: %w", name, lastState, waitErr)
+		return errors.New(spew.Sprintf("deployment %q is not in desired state, got: %+v: %w", name, lastState, waitErr))


Why move away from Errorf here? I'd have kept that and would propose:

return fmt.Errorf("deployment %q is not in desired state, error: %w, state:\n%s", name, waitErr spew.Sprint(lastState))

Why not? It is another way to do it. It covers all other arguments too without thinking too much about them (their size etc). Anyway I can change.

I think I could have used spew.Errorf() to have only one call if I want to make spew parse all arguments btw.

skonto · 2021-04-08T10:05:05Z

TestMetricsExport again.

skonto · 2021-04-08T10:12:50Z

/retest

skonto · 2021-04-13T07:13:08Z

/retest

skonto · 2021-04-13T08:01:05Z

@markusthoemmes gentle ping. The test keeps failing but it is unrelated I have a PR to fix that infamous test here #2088 .

skonto · 2021-04-15T14:52:05Z

@markusthoemmes gentle ping for a merge.

markusthoemmes

LGTM apart from the nits. The only important one I believe is the logs output.

markusthoemmes · 2021-04-15T14:57:19Z

 		return strings.Contains(string(logs), content), nil
 	})
+	if waitErr != nil {
+		return fmt.Errorf("logs do not contain the desired content %q, got %q: %w", content, logs, waitErr)


Suggested change

return fmt.Errorf("logs do not contain the desired content %q, got %q: %w", content, logs, waitErr)

return fmt.Errorf("logs do not contain the desired content %q, got %q: %w", content, string(logs), waitErr)

I believe this gets us a messy byte output otherwise.

I checked that before, so the rules say for byte slices with %q: %q a double-quoted string safely escaped with Go syntax. Even %s works (but no quotes). Check here.

Oh wow interesting!

markusthoemmes · 2021-04-15T14:57:54Z

 	})
+
+	if waitErr != nil {
+		return fmt.Errorf("pod %q is not in desired state, got: %+v: %w", name, spew.Sprint(lastState), waitErr)


Suggested change

return fmt.Errorf("pod %q is not in desired state, got: %+v: %w", name, spew.Sprint(lastState), waitErr)

return fmt.Errorf("pod %q is not in desired state, got: %s: %w", name, spew.Sprint(lastState), waitErr)

just a nit, but for completeness sake.

markusthoemmes · 2021-04-15T14:58:02Z

 	})
+
+	if waitErr != nil {
+		return fmt.Errorf("pod list is not in desired state, got: %+v: %w", spew.Sprint(lastState), waitErr)


Suggested change

return fmt.Errorf("pod list is not in desired state, got: %+v: %w", spew.Sprint(lastState), waitErr)

return fmt.Errorf("pod list is not in desired state, got: %s: %w", spew.Sprint(lastState), waitErr)

markusthoemmes · 2021-04-15T14:58:17Z

 	})
+
+	if waitErr != nil {
+		return fmt.Errorf("deployment %q is not in desired state, got: %+v: %w", name, spew.Sprint(lastState), waitErr)


Suggested change

return fmt.Errorf("deployment %q is not in desired state, got: %+v: %w", name, spew.Sprint(lastState), waitErr)

return fmt.Errorf("deployment %q is not in desired state, got: %s: %w", name, spew.Sprint(lastState), waitErr)

markusthoemmes · 2021-04-15T15:00:20Z

/assign

Please assign me if you want a review, i'll see it pop up in my Github state.

skonto · 2021-04-15T15:01:07Z

/assign @markusthoemmes

markusthoemmes · 2021-04-15T15:14:50Z

/lgtm

skonto · 2021-04-16T07:50:50Z

@markusthoemmes gentle ping.

markusthoemmes

/lgtm
/approve

The PR won't merge without the unit test passing though

knative-prow-robot · 2021-04-16T07:55:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markusthoemmes

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:

~~test/OWNERS~~ [markusthoemmes]

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

skonto · 2021-04-16T16:08:30Z

/retest

improve state reporting in kube check

77bda6a

google-cla Bot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 2, 2021

knative-prow-robot added area/test-and-release size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 2, 2021

skonto changed the title ~~Improve state reporting in kube check~~ Improve state reporting in kube checks Apr 2, 2021

markusthoemmes reviewed Apr 8, 2021

View reviewed changes

use spew

a4af04a

markusthoemmes reviewed Apr 8, 2021

View reviewed changes

update

4c5a248

markusthoemmes approved these changes Apr 15, 2021

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 15, 2021

knative-prow-robot assigned markusthoemmes Apr 15, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2021

nits

1ead025

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2021

markusthoemmes approved these changes Apr 16, 2021

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 16, 2021

knative-prow-robot merged commit b80a192 into knative:main Apr 16, 2021

	return fmt.Errorf("logs do not contain the desired content %q, got %q: %w", content, logs, waitErr)
	return fmt.Errorf("logs do not contain the desired content %q, got %q: %w", content, string(logs), waitErr)

	return fmt.Errorf("pod %q is not in desired state, got: %+v: %w", name, spew.Sprint(lastState), waitErr)
	return fmt.Errorf("pod %q is not in desired state, got: %s: %w", name, spew.Sprint(lastState), waitErr)

	return fmt.Errorf("pod list is not in desired state, got: %+v: %w", spew.Sprint(lastState), waitErr)
	return fmt.Errorf("pod list is not in desired state, got: %s: %w", spew.Sprint(lastState), waitErr)

	return fmt.Errorf("deployment %q is not in desired state, got: %+v: %w", name, spew.Sprint(lastState), waitErr)
	return fmt.Errorf("deployment %q is not in desired state, got: %s: %w", name, spew.Sprint(lastState), waitErr)

Conversation

skonto commented Apr 2, 2021

Changes

Uh oh!

codecov Bot commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

skonto Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto commented Apr 8, 2021

Uh oh!

skonto commented Apr 8, 2021

Uh oh!

skonto commented Apr 13, 2021

Uh oh!

skonto commented Apr 13, 2021

Uh oh!

skonto commented Apr 15, 2021

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

markusthoemmes commented Apr 15, 2021

Uh oh!

skonto commented Apr 15, 2021

Uh oh!

markusthoemmes commented Apr 15, 2021

Uh oh!

skonto commented Apr 16, 2021

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Apr 16, 2021

Uh oh!

skonto commented Apr 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Apr 2, 2021 •

edited

Loading

skonto Apr 8, 2021 •

edited

Loading

skonto Apr 8, 2021 •

edited

Loading

skonto Apr 15, 2021 •

edited

Loading