vc: fix netns leak by Ace-Tang · Pull Request #1641 · kata-containers/runtime

Ace-Tang · 2019-05-08T14:32:02Z

when kata container start fails, netns will leak, add a defer func
to fix it

Signed-off-by: Ace-Tang aceapril@126.com

devimc · 2019-05-08T15:17:43Z

pkg/katautils/create.go

+		ns := sandboxConfig.NetworkConfig
+		if err != nil && ns.NetNsCreated {
+			if ex := cleanupNetNS(ns.NetNSPath); ex != nil {
+				kataUtilsLogger.Warnf("Failed to cleanup netns %s", ns.NetNSPath)


please use WithField in all the logs to avoid using format specifiers. for example

kataUtilsLogger.WithField("path", ns.NetNSPath).Warn("Failed to cleanup netns")

amshinde

Just a minor suggestion. lgtm otherwise. Thanks for fixing this @Ace-Tang

amshinde · 2019-05-08T16:48:27Z

pkg/katautils/network.go

+
+	err = n.Close()
+	if err != nil {
+		return err


Can you return a more verbose message here : "Failed to close netns: %s"

lifupan · 2019-05-09T02:39:12Z

/test

grahamwhaley

lgtm

jodh-intel · 2019-05-09T08:29:25Z

pkg/katautils/create.go

+		ns := sandboxConfig.NetworkConfig
+		if err != nil && ns.NetNsCreated {
+			if ex := cleanupNetNS(ns.NetNSPath); ex != nil {
+				kataUtilsLogger.WithField("path", ns.NetNSPath).Warn("Failed to cleanup netns")


Could you tweak this slightly to also include the error:

kataUtilsLogger.WithField("path", ns.NetNSPath).WithError(ex).Warn("Failed to cleanup netns")

Oh, yes , forget to log error

grahamwhaley · 2019-05-09T08:32:07Z

I've nudged a few CI rebuilds.
The metrics one just looked like something fell over.
The fedora and vsocks ones (which is also fedora) both had what looked like the same fail - I suspect it is not related to this PR, so respan. Here is the error for ref, in case it does re-occur:

• Failure [22.752 seconds]
state
/tmp/jenkins/workspace/kata-containers-runtime-fedora-PR/go/src/github.com/kata-containers/tests/functional/state_test.go:26
  container
  /tmp/jenkins/workspace/kata-containers-runtime-fedora-PR/go/src/github.com/kata-containers/tests/vendor/github.com/onsi/ginkgo/extensions/table/table.go:92
    with workload [true], timeWait 5 [It]
    /tmp/jenkins/workspace/kata-containers-runtime-fedora-PR/go/src/github.com/kata-containers/tests/vendor/github.com/onsi/ginkgo/extensions/table/table_entry.go:46

    Expected
        <int>: 1
    to equal
        <int>: 0

    /tmp/jenkins/workspace/kata-containers-runtime-fedora-PR/go/src/github.com/kata-containers/tests/functional/state_test.go:45
------------------------------
SS

Summarizing 1 Failure:

[Fail] state container [It] with workload [true], timeWait 5

jodh-intel

Thanks @Ace-Tang.

lgtm

jodh-intel · 2019-05-09T10:13:11Z

pkg/katautils/create.go

+		ns := sandboxConfig.NetworkConfig
+		if err != nil && ns.NetNsCreated {
+			if ex := cleanupNetNS(ns.NetNSPath); ex != nil {
+				kataUtilsLogger.WithField("path", ns.NetNSPath).WithError(ex).Warn("Failed to cleanup netns")


You might find that the CI linters complain as errors should start with a lower-case letter (same comment for errors below...)

I was vary confuse about this, if my memory is right, golang advised log should be begin with lower-case. But kata use them both, that confused me, so I followed the upper-case way.
Make all log begin with lower-case?

Hi @Ace-Tang - sorry! I meant to put this comment on the first of your new fmt.Errorf() calls as this clearly isn't an error, but a log call that's logging an error ;)

But yes, I think you are correct - we need to be more consistent and make all errors start with a lower-case letter at some point (not this PR).

But, for now, if the CI is happy, I'm happy 😄

Hi, @jodh-intel ,do you mean

n, err := ns.GetNS(netNSPath) 187 if err != nil { 188 return fmt.Errorf("Failed to get netns %s: %v", netNSPath, err) --> change Fail to fail, is it ? 189 }

Yes. As I say, if the CI checks are happy, no need to change strictly. But I think this should probably be:

return fmt.Errorf("failed to get netns %s: %v", netNSPath, err)

Update, @jodh-intel

when kata container start fails, netns will leak, add a defer func to fix it Fixes: #1640 Signed-off-by: Ace-Tang <aceapril@126.com>

jodh-intel · 2019-05-09T15:16:18Z

Thanks @Ace-Tang.

/retest

lifupan · 2019-05-10T01:42:09Z

Since the failed case "TestHostNetworkingRequested" is not related with the PR, so merged this PR.

Ace-Tang · 2019-05-10T01:44:32Z

I will try to see if TestHostNetworkingRequested fail can be fixed

devimc reviewed May 8, 2019

View reviewed changes

amshinde reviewed May 8, 2019

View reviewed changes

grahamwhaley approved these changes May 9, 2019

View reviewed changes

jodh-intel reviewed May 9, 2019

View reviewed changes

jodh-intel approved these changes May 9, 2019

View reviewed changes

vc: fix netns leak

a5bc98c

when kata container start fails, netns will leak, add a defer func to fix it Fixes: #1640 Signed-off-by: Ace-Tang <aceapril@126.com>

devimc approved these changes May 9, 2019

View reviewed changes

lifupan approved these changes May 10, 2019

View reviewed changes

lifupan merged commit 8386610 into kata-containers:master May 10, 2019

Ace-Tang deleted the leak-ns branch May 10, 2019 01:44

ganeshmaharaj added the stable-candidate label May 14, 2019

ganeshmaharaj mentioned this pull request May 14, 2019

backports: Bp 1.6.4 rel #1675

Merged

Conversation

Ace-Tang commented May 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amshinde left a comment

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!

lifupan commented May 9, 2019

Uh oh!

grahamwhaley left a comment

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!

grahamwhaley commented May 9, 2019

Uh oh!

jodh-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ace-Tang May 9, 2019 • 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!

jodh-intel commented May 9, 2019

Uh oh!

lifupan commented May 10, 2019

Uh oh!

Ace-Tang commented May 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Ace-Tang May 9, 2019 •

edited

Loading