logging: Add sandbox CLI option by jodh-intel · Pull Request #90 · kata-containers/proxy

jodh-intel · 2018-07-04T10:48:25Z

Added a new -sandbox command-line option. This is not required by the
proxy itself, but is used to add a sandbox log field to make log
analysis easier.

Also changed the initial log message to show the standard "announce" message tag along with fields for each of the command-line options.

Fixes #87.

Signed-off-by: James O. D. Hunt [email protected]

jodh-intel · 2018-07-04T10:50:05Z

Note: the -sandbox option is not mandatory (yet). The plan is:

land this PR.
update the runtime/vc to launch the proxy with the new -sandbox option.
raise a new proxy PR to make the option mandatory.

Alternatively, I could raise a runtime PR to pass the option now and we could land them together.

I don't mind either way - thoughts @sboeuf, @bergwolf, @grahamwhaley?

grahamwhaley · 2018-07-04T10:53:41Z

I thought we might add it as an optinal (that is, not mandatory) in order to not break backwards compatibility. We could then make it mandatory later once all versions support it and have filtered through and/or been deprecated.

gnawux · 2018-07-04T10:55:47Z

As it is used in logs only, I think optional is enough.

jodh-intel · 2018-07-04T10:57:01Z

Thanks guys. The option is currently optional so this should work for us ;) I'll raise a runtime PR to add the option when starting the kata proxy and block on this PR landing...

grahamwhaley

lgtm

opendev-zuul · 2018-07-04T11:34:45Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : FAILURE in 43m 50s
kata-runsh-fedora-28 : RETRY_LIMIT in 10m 21s

codecov · 2018-07-04T13:20:08Z

Codecov Report

Merging #90 into master will decrease coverage by 0.46%.
The diff coverage is 28.57%.

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
- Coverage   34.63%   34.16%   -0.47%     
==========================================
  Files           2        2              
  Lines         231      240       +9     
==========================================
+ Hits           80       82       +2     
- Misses        141      148       +7     
  Partials       10       10

Impacted Files	Coverage Δ
proxy.go	`29.12% <28.57%> (-0.32%)`	⬇️

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 749b317...1672418. Read the comment docs.

devimc

one minor nit comment

devimc · 2018-07-04T18:53:51Z

proxy.go

+		"name":    proxyName,
+		"pid":     os.Getpid(),
+		"source":  "proxy",
+		"sandbox": sandboxID,


since sandboxID is optional, I think you should add it only if it's not empty, right?

I guess that's fair :) Branch updated.

opendev-zuul · 2018-07-05T05:54:21Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : RETRY_LIMIT in 2m 07s
kata-runsh-fedora-28 : RETRY_LIMIT in 1m 39s

jodh-intel · 2018-07-05T06:18:58Z

Hi @raravena80 - amusing problem - Travis is failing over here, but only for osx:

$ .ci/static-checks.sh
/Users/travis/gopath/src/github.com/kata-containers/tests/.ci/static-checks.sh: line 24: typeset: -A: invalid option
typeset: usage: typeset [-afFirtx] [-p] name[=value] ...

The static-checks.sh script is now failing as I recently updated it on kata-containers/tests#471. The problem here appears to be due to osx not providing a new enough version of bash that supports associative arrays. Any idea how to fix this?

devimc · 2018-07-05T12:14:32Z

lgtm

The version of bash provided by OSX under Travis is too old to support `typeset -A`, so install a newer version using homebrew. Signed-off-by: James O. D. Hunt <[email protected]>

For parity with other system components, Change the initial log message to show the standard "announce" message tag along with fields for each of the command-line options. Signed-off-by: James O. D. Hunt <[email protected]>

jodh-intel · 2018-07-05T12:44:04Z

@raravena80 - I think I've fixed it by installing bash using homebrew.

opendev-zuul · 2018-07-05T13:49:08Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : FAILURE in 44m 02s
kata-runsh-fedora-28 : FAILURE in 26m 05s

sboeuf · 2018-07-05T14:24:18Z

@jodh-intel are you planning to add some unit tests here ? If not, let me know and I'll merge it!

jodh-intel · 2018-07-05T14:52:49Z

Tests added! ;)

Added a new `-sandbox` command-line option. This is not required by the proxy itself, but is used to add a `sandbox` log field to make log analysis easier. Fixes kata-containers#87. Signed-off-by: James O. D. Hunt <[email protected]>

opendev-zuul · 2018-07-05T16:16:13Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : FAILURE in 45m 02s
kata-runsh-fedora-28 : TIMED_OUT in 1h 18m 22s

raravena80 · 2018-07-05T18:20:30Z

@jodh-intel good to hear. It would be interesting to find out what version of bash travis is using. This is what's on my laptop for High Sierra:

$ bash --version
GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin17)
Copyright (C) 2007 Free Software Foundation, Inc.

jodh-intel · 2018-07-06T06:58:39Z

@raravena80 - I don't think the version was logged but bash 4 added associative arrays (typeset -A) so it is likely to be version 3.*.

grahamwhaley approved these changes Jul 4, 2018

View reviewed changes

jodh-intel mentioned this pull request Jul 4, 2018

proxy: Pass sandbox to proxy kata-containers/runtime#464

Merged

jodh-intel mentioned this pull request Jul 4, 2018

log-parser: Add sandboxID to list of standard fields kata-containers/tests#474

Closed

devimc reviewed Jul 4, 2018

View reviewed changes

jodh-intel force-pushed the add-sandbox-to-logs branch from 990aa7a to 6de5cad Compare July 5, 2018 05:44

James O. D. Hunt added 2 commits July 5, 2018 13:43

CI: Update bash on Travis OSX for hashes

246f707

The version of bash provided by OSX under Travis is too old to support `typeset -A`, so install a newer version using homebrew. Signed-off-by: James O. D. Hunt <[email protected]>

logging: Display standard announce message

9b6bb05

For parity with other system components, Change the initial log message to show the standard "announce" message tag along with fields for each of the command-line options. Signed-off-by: James O. D. Hunt <[email protected]>

jodh-intel force-pushed the add-sandbox-to-logs branch from 6de5cad to fc4c79e Compare July 5, 2018 12:43

jodh-intel mentioned this pull request Jul 5, 2018

Update bash on Travis osx for hash support kata-containers/shim#98

Closed

sboeuf approved these changes Jul 5, 2018

View reviewed changes

jodh-intel force-pushed the add-sandbox-to-logs branch from fc4c79e to e5f334c Compare July 5, 2018 14:52

logging: Add sandbox CLI option

1672418

Added a new `-sandbox` command-line option. This is not required by the proxy itself, but is used to add a `sandbox` log field to make log analysis easier. Fixes kata-containers#87. Signed-off-by: James O. D. Hunt <[email protected]>

jodh-intel force-pushed the add-sandbox-to-logs branch from e5f334c to 1672418 Compare July 5, 2018 14:55

jodh-intel mentioned this pull request Jul 5, 2018

logging: Redirect yamux warnings/errors to logger #89

Merged

sboeuf merged commit 7e2a93d into kata-containers:master Jul 5, 2018

jodh-intel mentioned this pull request Aug 7, 2018

OSX CI failure: missing /Users/travis/gopath/bin/yq kata-containers/tests#558

Closed

Conversation

jodh-intel commented Jul 4, 2018

Uh oh!

jodh-intel commented Jul 4, 2018

Uh oh!

grahamwhaley commented Jul 4, 2018

Uh oh!

gnawux commented Jul 4, 2018

Uh oh!

jodh-intel commented Jul 4, 2018

Uh oh!

grahamwhaley left a comment

Choose a reason for hiding this comment

Uh oh!

opendev-zuul bot commented Jul 4, 2018

Uh oh!

codecov bot commented Jul 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

devimc left a comment

Choose a reason for hiding this comment

Uh oh!

devimc Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

jodh-intel Jul 5, 2018

Choose a reason for hiding this comment

Uh oh!

opendev-zuul bot commented Jul 5, 2018

Uh oh!

jodh-intel commented Jul 5, 2018

Uh oh!

devimc commented Jul 5, 2018

Uh oh!

jodh-intel commented Jul 5, 2018

Uh oh!

opendev-zuul bot commented Jul 5, 2018

Uh oh!

sboeuf commented Jul 5, 2018

Uh oh!

jodh-intel commented Jul 5, 2018

Uh oh!

opendev-zuul bot commented Jul 5, 2018

Uh oh!

raravena80 commented Jul 5, 2018

Uh oh!

jodh-intel commented Jul 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Jul 4, 2018 •

edited

Loading