Implementation of SQL-backed state #26

mheon · 2017-11-07T18:49:06Z

Still not wired into the rest of the project

rhatdan · 2017-11-07T18:59:58Z

libpod/container.go

logrus.Wrapf

I don't think there is such a function?

I was thinking about this.

func (c *Container) Create() (err error) {

But you already have it, so all set.

rhatdan · 2017-11-07T19:33:56Z

libpod/container.go

I belive you need to pass in err

defer func() err error

Not necessary - the error returned by Create() is named err, so this will always be the error returned by the function

rhatdan · 2017-11-07T19:34:17Z

libpod/container.go

Do we need these lines?

This is cleanup - it ensures the state knows the container is unmounted

rhatdan · 2017-11-07T19:36:57Z

libpod/sql_state.go

Put on single line.

if err := s.db.Close()

rhatdan · 2017-11-07T19:42:25Z

@nalind @mitrmac ptal

rhatdan · 2017-11-07T19:43:19Z

@mheon Please cleanup gofmt and lint errors.

mheon · 2017-11-09T18:57:58Z

Wired the SQL backed state into the rest of libpod. Still needs unit tests.

mheon · 2017-11-09T19:43:57Z

Tests are segfaulting. Looks like a combination of errors (initializing the SQL-backed state is erroring, and then the deferred function to clean up the store on an error is segfaulting. I really don't understand how that segfault is happening, but I'll try and dig into this.

mheon · 2017-11-09T21:25:07Z

Tests should be passing now. @rhatdan @baude PTAL. Still needs unit tests to be ready to merge, but otherwise should be fine.

rhatdan · 2017-11-10T11:04:26Z

libpod/container.go

rhatdan · 2017-11-10T11:07:09Z

libpod/options.go

nit: remove line.

rhatdan · 2017-11-10T11:10:03Z

libpod/runtime.go

Why not defer it?

rhatdan · 2017-11-10T11:12:55Z

libpod/sql_state.go

Don't you need to use err error for defer func below?

rhatdan · 2017-11-10T11:15:25Z

libpod/runtime.go

Nit: Can you do this in a single line?

if _, err2 := store.Shutdwon(false); err2 != nil

rhatdan · 2017-11-10T11:17:12Z

libpod/runtime_ctr.go

Should this be c.pod?

c is probably going to be nil when this is called, because we always return nil, err when an error occurs. If I could, I wouldn't have a variable assigned to the container we return at all, only the error, but I think Go won't let me do that.

Ok I guess this works, as long as this function is always called after ctr is created. I don't think go should require you to name the variable, or does it do this, because if you name one return you have to name them all?

TomSweeneyRedHat

LGTM. I would like to see this one go in at least a week or more before then next release. I'd like to see us run with it internally quite a bit before it gets put into an official kit. I'd also like Sebastian to run a perf run before making a kit too.

Nice chunk of work @mheon.

rhatdan · 2017-11-11T11:11:44Z

Well Sebastion has not tested perf on kpod at all yet, and I am pretty sure it would be really bad. :^(
If we wanted to test this for performance we would need to rig it into CRI-O to see if it hurt its performance.

rhatdan · 2017-11-11T12:11:11Z

@baude @nalind PTAL

TomSweeneyRedHat · 2017-11-11T13:45:00Z

@rhatdan, I'd thought kpd was being used in some of the calls that Sebastian was doing. If not, then yeah, no need to tie him in.

rh-atomic-bot · 2017-11-15T22:59:00Z

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

rhatdan · 2017-11-15T23:02:05Z

@mheon Rebase.

Signed-off-by: Matthew Heon <[email protected]>

This one cleans up after container creation fails Signed-off-by: Matthew Heon <[email protected]>

If StopSignal is 0, it is assumed that the default signal will be used. Signed-off-by: Matthew Heon <[email protected]>

Signed-off-by: Matthew Heon <[email protected]>

mheon · 2017-11-20T18:55:09Z

Tests can land in another PR. @rhatdan this should be ready to merge.

rhatdan · 2017-11-20T19:44:57Z

Ok lets merge and we can start testing as we merge components in.

vendor.conf has been pinned at containerd/cgroups@7a5fdd83 (Merge pull request containers#26 from onorua/error-ignore-example, 2017-08-24) since libpod forked from CRI-O with a031b83 (Initial checkin from CRI-O repo, 2017-11-01). The content in vendor/github.com/containerd/cgroups was bumped to containerd/cgroups@77e62851 (Use /proc/diskstats to get device names, 2018-01-31) in ae89dc2 (Update containerd/cgroups repo fix perf issue, 2018-02-01, containers#284), but ae89dc2 forgot to update vendor.conf. With this commit: $ vndr github.com/containerd/cgroups no longer changes anything under vendor/github.com/containerd/cgroups. Signed-off-by: W. Trevor King <[email protected]>

vendor.conf has been pinned at containerd/cgroups@7a5fdd83 (Merge pull request #26 from onorua/error-ignore-example, 2017-08-24) since libpod forked from CRI-O with a031b83 (Initial checkin from CRI-O repo, 2017-11-01). The content in vendor/github.com/containerd/cgroups was bumped to containerd/cgroups@77e62851 (Use /proc/diskstats to get device names, 2018-01-31) in ae89dc2 (Update containerd/cgroups repo fix perf issue, 2018-02-01, #284), but ae89dc2 forgot to update vendor.conf. With this commit: $ vndr github.com/containerd/cgroups no longer changes anything under vendor/github.com/containerd/cgroups. Signed-off-by: W. Trevor King <[email protected]> Closes: #749 Approved by: mheon

linting our content

rhatdan reviewed Nov 7, 2017

View reviewed changes

mheon force-pushed the sql_state branch from 4cf36a0 to fd7594e Compare November 9, 2017 18:55

mheon force-pushed the sql_state branch 3 times, most recently from 854e011 to f676180 Compare November 9, 2017 20:53

rhatdan reviewed Nov 10, 2017

View reviewed changes

TomSweeneyRedHat reviewed Nov 10, 2017

View reviewed changes

rhatdan requested review from baude and nalind November 11, 2017 12:10

mheon added 9 commits November 18, 2017 12:54

Compile-tested implementation of SQL-backed state

c6fe443

Signed-off-by: Matthew Heon <[email protected]>

Address review comments, fix gofmt and lint

cb56716

Signed-off-by: Matthew Heon <[email protected]>

Wire SQL backed state into rest of libpod

763e372

Signed-off-by: Matthew Heon <[email protected]>

Rename Create to Init for containers to be less confusing

4bc3acc

Signed-off-by: Matthew Heon <[email protected]>

Fix segfault cleaning up in after failing to create a Runtime

57300be

Signed-off-by: Matthew Heon <[email protected]>

Fix lingering SQL error

657cb1b

Signed-off-by: Matthew Heon <[email protected]>

Resolve another segfault

12f19ca

This one cleans up after container creation fails Signed-off-by: Matthew Heon <[email protected]>

StopSignal is allowed to be 0

b10fb66

If StopSignal is 0, it is assumed that the default signal will be used. Signed-off-by: Matthew Heon <[email protected]>

Need to provide ID of container being updated

8981384

Signed-off-by: Matthew Heon <[email protected]>

mheon force-pushed the sql_state branch from 824bde9 to 8981384 Compare November 18, 2017 17:58

Fix lint errors

f2894ed

Signed-off-by: Matthew Heon <[email protected]>

mheon changed the title ~~[WIP] Compile-tested implementation of SQL-backed state~~ Implementation of SQL-backed state Nov 20, 2017

rhatdan merged commit 6e0944f into containers:master Nov 20, 2017

rh-atomic-bot mentioned this pull request Nov 20, 2017

Add docker transport to push image before final failure #42

Closed

wking mentioned this pull request May 11, 2018

WIP: vendor: Update with a fresh vndr run #747

Closed

wking mentioned this pull request May 11, 2018

vendor.conf: Bump containerd/cgroups to 77e62851 #749

Closed

openshift-merge-robot pushed a commit that referenced this pull request Nov 26, 2019

Merge pull request #26 from baude/apiv2lint

e6d5e3e

linting our content

juspence mentioned this pull request Sep 17, 2021

setting cgroup config for procHooks process caused: open pids.max: no such file or directory #11632

Closed

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

Implementation of SQL-backed state #26

Implementation of SQL-backed state #26

Uh oh!

Conversation

mheon commented Nov 7, 2017

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!

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!

rhatdan commented Nov 7, 2017

Uh oh!

rhatdan commented Nov 7, 2017

Uh oh!

mheon commented Nov 9, 2017

Uh oh!

mheon commented Nov 9, 2017

Uh oh!

mheon commented Nov 9, 2017

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!

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!

TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Nov 11, 2017

Uh oh!

rhatdan commented Nov 11, 2017

Uh oh!

TomSweeneyRedHat commented Nov 11, 2017

Uh oh!

rh-atomic-bot commented Nov 15, 2017

Uh oh!

rhatdan commented Nov 15, 2017

Uh oh!

mheon commented Nov 20, 2017

Uh oh!

rhatdan commented Nov 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants