search

lxc / go-lxc

Go bindings for liblxc
https://linuxcontainers.org/lxc
Other
431 stars 76 forks source link

Fix the locking logic #88

Closed caglar10ur closed 7 years ago

caglar10ur commented 7 years ago

The original code where we were doing following

func X()  {
    call makesure
        makesure locks
        ...work
        makesure unlocks
    lock
    ...work
    unlock
}

was potentially racy as someone else could obtain the lock in between makesure and mutex.Lock call.

Now all exported functions are holding the lock throughout their scope.

stgraber commented 7 years ago

Travis looks very unhappy with this branch, showing Go panics in the locking/futex code.

caglar10ur commented 7 years ago

Hmm, will take a look and update the PR

caglar10ur commented 7 years ago

Looks like I missed the SaveConfigPath in Execute. Fixed now, waiting travis :)

caglar10ur commented 7 years ago

@stgraber privileged tests are now happy but unprivileged ones are failing. Looks like they never passed in travis before. I assumed print_top_failing_dir is the culprit and added ~/.local/share to the .travis.yml but they are still failing. Any idea why?

stgraber commented 7 years ago

I'm not sure, I tried adding more debugging before but it didn't really help... A simple lxc-create + lxc-start did seem to work though, so I'm confused as to what's going on.

caglar10ur commented 7 years ago

Hmm, interesting. Will play with it to see whether there is something we can do to fix that.

stgraber commented 7 years ago

Hmm, we're getting all our tests stuck on locks in LXD ever since this landed. Running a local test with only this commit applied to confirm that's the culprit, if it is, I'll revert it.

stgraber commented 7 years ago

The revert fixed the issue, not sure exactly what's going on with the locking change, but it looks like a lock is being kept when it shouldn't somewhere.

caglar10ur commented 7 years ago

Bummer, I'll run the LXD tests tomorrow to see what's going

caglar10ur commented 7 years ago

I believe RunningConfigItem was the culprit. I'm in the middle of running LXD test suite and will open another PR (soon) if it completes successfully.