🎁 Deterministic exit code for zap's Fatal for `sharedmain` #2496

cardil · 2022-04-22T16:35:56Z

Changes

🎁 Deterministic exit code for zap's Fatal for sharedmain

/kind enhancement

knative-prow · 2022-04-22T16:36:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cardil
To complete the pull request process, please assign julz after the PR has been reviewed.
You can assign the PR to them by writing /assign @julz in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

codecov · 2022-04-22T16:42:29Z

Codecov Report

Merging #2496 (e5e9a4a) into main (12be060) will increase coverage by 0.02%.
The diff coverage is 80.00%.

❗ Current head e5e9a4a differs from pull request most recent head 682c51d. Consider uploading reports for the commit 682c51d to get more accurate results

@@            Coverage Diff             @@
##             main    #2496      +/-   ##
==========================================
+ Coverage   81.71%   81.74%   +0.02%     
==========================================
  Files         163      165       +2     
  Lines        9653     9688      +35     
==========================================
+ Hits         7888     7919      +31     
- Misses       1529     1533       +4     
  Partials      236      236

Impacted Files	Coverage Δ
logging/exiting_zapcore.go	`76.66% <76.66%> (ø)`
system/posix/retcode/retcode.go	`100.00% <100.00%> (ø)`
controller/controller.go	`88.78% <0.00%> (+0.71%)`	⬆️

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 12be060...682c51d. Read the comment docs.

cardil · 2022-04-25T13:29:25Z

Github's Unit test fail looks like an unrelated flake.

cardil · 2022-04-26T10:45:05Z

/assign @knative/technical-oversight-committee

cardil · 2022-04-26T10:48:26Z

/assign @dprotaso
/assign @rhuss
/assign @julz
/assign @vagababov
/assign @vaikas
/assign @knative/knative-release-leads
/assign @nader-ziada
/assign @ikvmw

dprotaso · 2022-04-26T20:15:57Z

injection/sharedmain/main.go

@@ -282,7 +282,7 @@ func MainWithConfig(ctx context.Context, component string, cfg *rest.Config, cto
 }

 func flush(logger *zap.SugaredLogger) {
-	logger.Sync()
+	_ = logger.Sync() // we care about stdout and stderr only


I don't understand this change

dprotaso · 2022-04-26T20:17:57Z

zap's godoc says the Fatal exit code is 1

See: https://pkg.go.dev/go.uber.org/zap#Logger.Fatal

Are you seeing something different?

dprotaso · 2022-04-26T20:29:59Z

I'm not particularly sold on this change - as the exit code is dependent on the log message.

Plus a range of exit codes are meant to reserved:
https://man7.org/linux/man-pages/man1/exit.1p.html#RATIONALE

evankanderson · 2022-04-26T17:57:20Z

logging/exiting_zapcore.go

+	if entry.Level >= zapcore.DPanicLevel {
+		return ce.AddCore(entry, r)
+	}


Do we need to do this separately from the base?

evankanderson · 2022-04-26T17:58:11Z

logging/exiting_zapcore.go

+	if err := r.base.Write(entry, fields); err != nil {
+		return err
+	}


Other implementations won't exit during Write, correct?

evankanderson · 2022-04-26T17:59:27Z

logging/exiting_zapcore.go

+})
+
+type retcodeCore struct {
+	base   zapcore.Core


How much would you get for free from embedding zapcore.Core?

type retcodeCore struct { zapcore.Core fields []zapcore.Field }

evankanderson · 2022-04-26T18:00:08Z

logging/exiting_zapcore.go

+
+type retcodeCore struct {
+	base   zapcore.Core
+	fields []zapcore.Field


Can you add a comment on why we're tracking the fields here specially?

evankanderson · 2022-05-02T22:34:26Z

logging/exiting_zapcore.go

+	}
+	code := r.calculateRetcode(entry, fields)
+	_ = r.Sync()
+	exit.Exit(code)


Do you want to call go.uber.org/zap/internal/exit to enable working with zap's testing spy?

It looks like you've copied the zap spy into the pkg codebase, though I don't understand why we need that rather than adopting / improving the zap one.

evankanderson · 2022-05-02T22:43:40Z

system/posix/retcode/retcode.go

+
+import "hash/crc32"
+
+// Calc will calculate an POSIX retcode from an error.


I'm not sure why we want to do this -- it looks like we're collapsing all errors into a range of code 1-255.

https://unix.stackexchange.com/a/418802 suggests that the range 1-125 might be more reasonable, or 1 - (2^31-1) if you're willing to work with waitid and some of the newer posix calls.

evankanderson · 2022-05-02T22:44:28Z

system/posix/retcode/retcode_test.go

+)
+
+func TestCalc(t *testing.T) {
+	cases := testCases()


Why not inline this declaration (as we do elsewhere)?

cases := []struct { name string err error want int }{{ name: "nil", err: nil, want: 0, }, { ... }}

evankanderson · 2022-05-02T22:52:45Z

logging/exiting_zapcore_test.go

+	assert.Check(t, cmp.Contains(logs, `"message":"foo"`))
+	assert.Check(t, cmp.Contains(logs, `"error":"bar"`))


I'm slightly concerned about adding a new dependency here to save a line of code:

if ! strings.Contains(logs, `"message":"foo"`) { t.Errorf("Expected %q to contain message tag", logs) }

(To put it simply, the cost of adding the dependency seems higher than the value that it adds in terms of clarity.)

knative-prow-robot · 2022-06-22T15:21:25Z

@cardil: PR needs rebase.

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.

rhuss · 2022-06-30T09:33:12Z

Where do we stand with this PR ? Seems to be dormant since quite some time.

dprotaso · 2022-06-30T14:10:38Z

I don't think we should merge this change - changing the exit code based on log messages I think will just make things more confusing as people google exit code values and they have different meaning.

We should probably write the correct log line to the k8s termination path

https://kubernetes.io/docs/tasks/debug/debug-application/determine-reason-pod-failure/

dprotaso · 2022-07-27T00:35:11Z

Going to close this out

knative-prow bot added kind/enhancement size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 22, 2022

cardil force-pushed the feature/retcode-from-error-message branch from 8e3b800 to e5e9a4a Compare April 22, 2022 16:38

Deterministic calc of POSIX return code for zap's Fatal for sharedmain

682c51d

cardil force-pushed the feature/retcode-from-error-message branch from e5e9a4a to 682c51d Compare April 22, 2022 17:12

knative-prow bot assigned dprotaso, ikavgo, julz, nader-ziada, rhuss, vagababov and vaikas Apr 26, 2022

dprotaso reviewed Apr 26, 2022

View reviewed changes

evankanderson reviewed May 2, 2022

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2022

dprotaso closed this Jul 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎁 Deterministic exit code for zap's Fatal for `sharedmain` #2496

🎁 Deterministic exit code for zap's Fatal for `sharedmain` #2496

cardil commented Apr 22, 2022

knative-prow bot commented Apr 22, 2022

codecov bot commented Apr 22, 2022 •

edited

cardil commented Apr 25, 2022

cardil commented Apr 26, 2022

cardil commented Apr 26, 2022

dprotaso Apr 26, 2022

dprotaso commented Apr 26, 2022

dprotaso commented Apr 26, 2022 •

edited

evankanderson Apr 26, 2022

evankanderson Apr 26, 2022

evankanderson Apr 26, 2022

evankanderson Apr 26, 2022

evankanderson May 2, 2022

evankanderson May 2, 2022

evankanderson May 2, 2022

evankanderson May 2, 2022

evankanderson May 2, 2022

knative-prow-robot commented Jun 22, 2022

rhuss commented Jun 30, 2022

dprotaso commented Jun 30, 2022 •

edited

dprotaso commented Jul 27, 2022


		import "hash/crc32"

		// Calc will calculate an POSIX retcode from an error.

		assert.Check(t, cmp.Contains(logs, `"message":"foo"`))
		assert.Check(t, cmp.Contains(logs, `"error":"bar"`))

🎁 Deterministic exit code for zap's Fatal for sharedmain #2496

🎁 Deterministic exit code for zap's Fatal for sharedmain #2496

Conversation

cardil commented Apr 22, 2022

Changes

knative-prow bot commented Apr 22, 2022

codecov bot commented Apr 22, 2022 • edited

Codecov Report

cardil commented Apr 25, 2022

cardil commented Apr 26, 2022

cardil commented Apr 26, 2022

Choose a reason for hiding this comment

dprotaso commented Apr 26, 2022

dprotaso commented Apr 26, 2022 • edited

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

knative-prow-robot commented Jun 22, 2022

rhuss commented Jun 30, 2022

dprotaso commented Jun 30, 2022 • edited

dprotaso commented Jul 27, 2022

🎁 Deterministic exit code for zap's Fatal for `sharedmain` #2496

🎁 Deterministic exit code for zap's Fatal for `sharedmain` #2496

codecov bot commented Apr 22, 2022 •

edited

dprotaso commented Apr 26, 2022 •

edited

dprotaso commented Jun 30, 2022 •

edited