Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

factory: add SetLogger API#521

Merged
bergwolf merged 1 commit intokata-containers:masterfrom
bergwolf:log
Jul 27, 2018
Merged

factory: add SetLogger API#521
bergwolf merged 1 commit intokata-containers:masterfrom
bergwolf:log

Conversation

@bergwolf
Copy link
Member

So that we actually use the same logger as other packages when being
invoked by CLI.

Fixes: #520

@bergwolf bergwolf mentioned this pull request Jul 26, 2018
4 tasks
Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice :-)
lgtm

func SetLogger(logger logrus.FieldLogger) {
fields := logrus.Fields{
"source": "virtcontainers",
"arch": runtime.GOARCH,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this is basically doing the same as virtcontainers/api.go:SetLogger(). But I'm wondering why we don't just add the arch field here:

But also, you might want to add a "subsystem": "factory" field as we've done that in various parts of vc (like network.go).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point about moving arch to main.go.

As for subsystem, it is set in factory.log() function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. PTAL @jodh-intel .

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 170939 KB
Proxy: 5922 KB
Shim: 8796 KB

Memory inside container:
Total Memory: 2043480 KB
Free Memory: 2003852 KB

So that we actually use the same logger as other packages when being
invoked by CLI.

Fixes: kata-containers#520

Signed-off-by: Peng Tao <bergwolf@gmail.com>
@jodh-intel
Copy link

jodh-intel commented Jul 26, 2018

lgtm

Approved with PullApprove

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 164953 KB
Proxy: 5790 KB
Shim: 8836 KB

Memory inside container:
Total Memory: 2043480 KB
Free Memory: 2003976 KB

@opendev-zuul
Copy link

opendev-zuul bot commented Jul 26, 2018

Build succeeded (third-party-check pipeline).

@devimc
Copy link

devimc commented Jul 26, 2018

lgtm

Approved with PullApprove

@bergwolf
Copy link
Member Author

fedora CI failed on #525. restarting.

@codecov
Copy link

codecov bot commented Jul 27, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@daa65a5). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #521   +/-   ##
=========================================
  Coverage          ?   66.67%           
=========================================
  Files             ?       93           
  Lines             ?     9580           
  Branches          ?        0           
=========================================
  Hits              ?     6387           
  Misses            ?     2506           
  Partials          ?      687
Impacted Files Coverage Δ
virtcontainers/factory/factory.go 71.59% <100%> (ø)
cli/main.go 88.37% <100%> (ø)

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 daa65a5...9a497fe. Read the comment docs.

@bergwolf bergwolf merged commit cfbc974 into kata-containers:master Jul 27, 2018
jodh-intel pushed a commit to jodh-intel/runtimes that referenced this pull request Jul 27, 2018
As of kata-containers#521, the runtime now adds the `arch` log field so
`virtcontainers` doesn't need to set it too.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
jodh-intel pushed a commit to jodh-intel/runtimes that referenced this pull request Jul 30, 2018
As of kata-containers#521, the runtime now adds the `arch` log field so
`virtcontainers` doesn't need to set it too.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
jodh-intel pushed a commit to jodh-intel/runtimes that referenced this pull request Jul 30, 2018
As of kata-containers#521, the runtime now adds the `arch` log field so
`virtcontainers` doesn't need to set it too.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
@egernst egernst mentioned this pull request Aug 22, 2018
@sboeuf sboeuf added the enhancement Improvement to an existing feature label Sep 12, 2018
@bergwolf bergwolf deleted the log branch September 13, 2018 03:26
zklei pushed a commit to zklei/runtime that referenced this pull request Jun 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement Improvement to an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants