Add NetMode, UtsMode and IPCMode Namespaces support #64

rhatdan · 2017-11-22T19:08:10Z

No description provided.

TomSweeneyRedHat · 2017-11-22T19:15:41Z

cmd/kpod/create.go

+	ip6Address         string                //ipv6
+	ipAddress          string                //ip
+	labels             map[string]string     //label
+	linkLocalIP        []string              // link-local-ip


nit of a nit, gotta space after // in the comment for these 3, but not in the others. Yes my OCD is kicking in.

I actually think we should just remove these comments, since they were just a helper to @baude to remember the link between the CLI option and the internal storage

TomSweeneyRedHat · 2017-11-22T20:00:11Z

cmd/kpod/spec.go

+	return nil
+}
+
+func addUTSNS(config *createConfig, g *generate.Generator) error {


will you be adding "IsContainer" functionality to this function too? If not, should this be renamed to remove rather than add?

IsContainer is not valid for UTS, it only has host and Private (Default). UTS almost always follows with --net.

TomSweeneyRedHat · 2017-11-22T20:01:27Z

lots of unhappy tests atm.

rhatdan · 2017-11-22T23:35:36Z

Yup, I have to make shm work properly. Taking all day to get close.

rhatdan · 2017-11-28T14:38:24Z

@TomSweeneyRedHat @baude @mheon @umohnani8 Tests finally pass, Please review this. I can break this up into a series of PRs if that is easier.

umohnani8 · 2017-11-28T14:52:45Z

libpod/container.go

@@ -384,7 +403,7 @@ func (c *Container) Init() (err error) {

 	c.state.State = ContainerStateCreated

-	if err := c.runtime.state.SaveContainer(c); err != nil {
+	if err := c.Save(); err != nil {
 		return errors.Wrapf(err, "error saving container %s state", c.ID())


Just return err, the same error message here is being returned in Save() as well. This will just print out the same error message twice.

umohnani8 · 2017-11-28T14:52:54Z

libpod/container.go

@@ -416,7 +435,7 @@ func (c *Container) Start() error {
 		return err
 	}

-	if err := c.runtime.state.SaveContainer(c); err != nil {
+	if err := c.Save(); err != nil {
 		return errors.Wrapf(err, "error saving container %s state", c.ID())


Just return err

umohnani8 · 2017-11-28T14:53:04Z

libpod/container.go

@@ -501,7 +520,7 @@ func (c *Container) Mount(label string) (string, error) {
 	c.state.Mounted = true
 	c.config.MountLabel = mountLabel

-	if err := c.runtime.state.SaveContainer(c); err != nil {
+	if err := c.Save(); err != nil {
 		return "", errors.Wrapf(err, "error saving container %s state", c.ID())


just return err

rh-atomic-bot · 2017-11-28T15:14:59Z

☔ The latest upstream changes (presumably ab62fe1) made this pull request unmergeable. Please resolve the merge conflicts.

mheon · 2017-11-28T15:20:02Z

libpod/container.go

@@ -672,3 +691,11 @@ func (c *Container) isStopped() (bool, error) {
 	}
 	return c.state.State == ContainerStateStopped, nil
 }
+
+// Save container state to the database
+func (c *Container) Save() error {


Don't think this needs to be exposed. We should never need to call this from outside libpod.

mheon · 2017-11-28T15:23:51Z

libpod/runtime_ctr.go

@@ -53,6 +60,27 @@ func (r *Runtime) NewContainer(spec *spec.Spec, options ...CtrCreateOption) (c *
 		}
 	}()

+	shmdir := ctr.ShmDir()
+	if shmdir == "" {


Can you set ctr.config.ShmDir here to the new SHM you create? That way we preserve the path in the DB

I am setting it about 5 rows below. ctr.config.ShmDir() return ctr.config.ShmDir,

mheon · 2017-11-28T15:25:56Z

libpod/runtime_ctr.go

+	}
+	g := generate.NewFromSpec(spec)
+	g.AddBindMount(shmdir, "/dev/shm", []string{"rw"})
+	ctr.replaceSpec(g.Spec())


Instead of using replaceSpec, can you do this in https://github.com/projectatomic/libpod/blob/master/libpod/container.go#L426-L431 - when we generate the runtime spec we're going to use? That will remove the need for replaceSpec and ensure that ctr.config.Spec is always the spec the user gave to us

mheon · 2017-11-28T15:50:59Z

cmd/kpod/create.go

+	if config.pidMode.IsHost() {
+		labelOpts = append(labelOpts, label.DisableSecOpt()...)
+	}
+	if config.pidMode.IsContainer() {


Can this be else-if? I think pid=host and pid= are exclusive

mheon · 2017-11-28T15:51:15Z

cmd/kpod/create.go

+	if config.ipcMode.IsHost() {
+		labelOpts = append(labelOpts, label.DisableSecOpt()...)
+	}
+	if config.ipcMode.IsContainer() {


Can this be else-if? I think host and container are exclusive

mheon · 2017-11-28T15:51:47Z

cmd/kpod/create.go

+	if ipcMode.IsHost() {
+		return "/dev/shm", nil
+	}
+	if ipcMode.IsContainer() {


Can this be else-if

No need other calls are returning.

mheon · 2017-11-28T15:53:17Z

cmd/kpod/spec.go

+	if netMode.IsNone() {
+		return libpod.ErrNotImplemented
+	}
+	if netMode.IsBridge() {


Can this be else-if

no need other calls are returning.

mheon · 2017-11-28T15:53:24Z

cmd/kpod/spec.go

+	if netMode.IsBridge() {
+		return libpod.ErrNotImplemented
+	}
+	if netMode.IsContainer() {


Can this be else-if

No need since others are returning.

rh-atomic-bot · 2017-11-30T02:03:31Z

☔ The latest upstream changes (presumably 1f9c894) made this pull request unmergeable. Please resolve the merge conflicts.

TomSweeneyRedHat · 2017-11-30T15:13:00Z

@rhatdan, Travis isn't happy due to some unwanted whitespace.

rhatdan · 2017-12-01T00:16:31Z

@mheon Now storing the list of mounts in the DB. So code less hacky and it seems to work, PTAL.

mheon · 2017-12-01T00:20:28Z

libpod/container.go

+				}
+			}
+		}
+		c.config.Mounts = []string{}


Since this is in c.config, it won't be saved

mheon · 2017-12-01T00:21:22Z

libpod/container.go

@@ -471,6 +484,19 @@ func (c *Container) Start() error {
 		return err
 	}

+	mounted, err := mount.Mounted(c.config.ShmDir)


I feel like we ought to make a generic Mount() function that ensures the container, SHM, and everything else are mounted and ready to go. But that can wait for another PR.

mheon · 2017-12-01T00:22:32Z

@rhatdan There's a DB schema version in sql_state.go since my DB versioning patch hit. Can you increment that from 1 to 2, to indicate a change has been made in database arrangement?

mheon · 2017-12-01T00:23:43Z

Otherwise, LGTM. I don't know if Mounts should be in Config or State yet, but for now I think we're fine.

rhatdan · 2017-12-01T13:21:20Z

Well mounts should not change after the container has been created.

rhatdan · 2017-12-02T11:38:23Z

@bot retest please

rhatdan · 2017-12-02T11:39:05Z

@mheon updated the schema version.

Allow kpod create/run to create contianers in different network namespaces, uts namespaces and IPC Namespaces. This patch just handles the simple join the host, or another containers namespaces. Lots more work needed to full integrate --net Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

mheon · 2017-12-02T15:13:14Z

bot, retest this please

mheon · 2017-12-02T15:14:49Z

LGTM
@rh-atomic-bot r+

rh-atomic-bot · 2017-12-02T15:14:50Z

📌 Commit 403d5da has been approved by mheon

rh-atomic-bot · 2017-12-02T15:14:53Z

⌛ Testing commit 403d5da with merge e810bf5...

rh-atomic-bot · 2017-12-02T15:30:50Z

💔 Test failed - status-papr

mheon · 2017-12-02T15:32:54Z

@rh-atomic-bot retry

rh-atomic-bot · 2017-12-02T15:32:58Z

⌛ Testing commit 403d5da with merge adf8809...

rh-atomic-bot · 2017-12-02T15:50:20Z

☀️ Test successful - status-papr
Approved by: mheon
Pushing adf8809 to master...

travis: run with ginkgo -p instead of go test

rhatdan force-pushed the namespaces branch 2 times, most recently from 6e7646e to 8740e19 Compare November 22, 2017 19:12

TomSweeneyRedHat reviewed Nov 22, 2017

View reviewed changes

rhatdan force-pushed the namespaces branch from 8740e19 to ef51515 Compare November 22, 2017 19:31

TomSweeneyRedHat reviewed Nov 22, 2017

View reviewed changes

rhatdan force-pushed the namespaces branch 7 times, most recently from a5d4ab3 to f14145d Compare November 28, 2017 14:06

umohnani8 reviewed Nov 28, 2017

View reviewed changes

mheon reviewed Nov 28, 2017

View reviewed changes

rhatdan force-pushed the namespaces branch 3 times, most recently from ccc22a5 to 9d0b0ac Compare November 28, 2017 15:23

mheon reviewed Nov 28, 2017

View reviewed changes

rhatdan force-pushed the namespaces branch 2 times, most recently from caaae06 to ce64196 Compare November 30, 2017 14:38

rhatdan force-pushed the namespaces branch from ce64196 to 9e74428 Compare December 1, 2017 00:15

mheon reviewed Dec 1, 2017

View reviewed changes

libpod/container.go

}

}

}

c.config.Mounts = []string{}

Copy link

Member

mheon Dec 1, 2017

Choose a reason for hiding this comment

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

Since this is in c.config, it won't be saved

mheon reviewed Dec 1, 2017

View reviewed changes

rhatdan force-pushed the namespaces branch from 9e74428 to 26cde29 Compare December 2, 2017 11:03

rhatdan force-pushed the namespaces branch from 26cde29 to 403d5da Compare December 2, 2017 11:40

rh-atomic-bot closed this in adf8809 Dec 2, 2017

baude pushed a commit to baude/podman that referenced this pull request Aug 31, 2019

Merge pull request containers#64 from rosenhouse/travis-ginkgo

556e509

travis: run with ginkgo -p instead of go test

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 28, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 28, 2023

Add NetMode, UtsMode and IPCMode Namespaces support #64

Add NetMode, UtsMode and IPCMode Namespaces support #64

Conversation

rhatdan commented Nov 22, 2017

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

TomSweeneyRedHat commented Nov 22, 2017

rhatdan commented Nov 22, 2017

rhatdan commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rh-atomic-bot commented Nov 28, 2017

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

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

rh-atomic-bot commented Nov 30, 2017

TomSweeneyRedHat commented Nov 30, 2017

rhatdan commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Dec 1, 2017

mheon commented Dec 1, 2017

rhatdan commented Dec 1, 2017

rhatdan commented Dec 2, 2017

rhatdan commented Dec 2, 2017

mheon commented Dec 2, 2017

mheon commented Dec 2, 2017

rh-atomic-bot commented Dec 2, 2017

rh-atomic-bot commented Dec 2, 2017

rh-atomic-bot commented Dec 2, 2017

mheon commented Dec 2, 2017

rh-atomic-bot commented Dec 2, 2017

rh-atomic-bot commented Dec 2, 2017