Skip to content

Conversation

@tamilmani1989
Copy link
Member

@tamilmani1989 tamilmani1989 commented Nov 22, 2019

What this PR does / why we need it:
This PR make sure that CNI process is not blocked bacause of orphaned lock file. The cni will remove lock file if the process that is holding lockfile exited

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

@tamilmani1989 tamilmani1989 changed the title Remove lock file Remove Orphan lock file Nov 22, 2019
@codecov
Copy link

codecov bot commented Nov 22, 2019

Codecov Report

Merging #445 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #445      +/-   ##
==========================================
- Coverage   53.32%   53.29%   -0.03%     
==========================================
  Files          28       28              
  Lines        4032     4034       +2     
==========================================
  Hits         2150     2150              
- Misses       1610     1612       +2     
  Partials      272      272
Impacted Files Coverage Δ
store/json.go 45.61% <0%> (-0.82%) ⬇️

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 534e646...eca0cb6. Read the comment docs.

jaer-tsun
jaer-tsun previously approved these changes Nov 23, 2019
Copy link
Contributor

@jaer-tsun jaer-tsun left a comment

Choose a reason for hiding this comment

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

/lgtm

cni/plugin.go Outdated
log.Printf("[CNI] Process name for pid %s is %s", pid, pName)

if pName != processName {
return true
Copy link
Member

Choose a reason for hiding this comment

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

can you add a log here? This will help us debug where log can show that we successfully removed the lock

Copy link
Member Author

Choose a reason for hiding this comment

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

i have already added log in other place before removing lock file

fixed trim line ending
…working into removeLockFile

# Conflicts:
#	cni/ipam/plugin/main.go
fmt.Printf("Failed to initialize key-value store of ipam plugin, err:%v.\n", err)

if isSafe, err := ipamPlugin.Plugin.IsSafeToRemoveLock(ipamPlugin.Plugin.Name); isSafe {
log.Printf("[IPAM] Removing lock file as process holding lock exited. Error:%v", err)
Copy link
Member

Choose a reason for hiding this comment

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

You are not logging the err when isSafe is false - which is where we get non-nil err.
You are always logging nil error.

// check if that matches with our expected process
pName, err := platform.GetProcessNameByID(string(content))
if err != nil {
return true, nil
Copy link
Member

Choose a reason for hiding this comment

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

if GetProcessNameByID returned err due to some internal err, we would end up deleting the lock file even if the process which acquired the lock is running.
I think we should return false here. Let's discuss this if you want.

@tamilmani1989 tamilmani1989 merged commit 6ca873c into Azure:master Dec 4, 2019
ashutoshishere04 pushed a commit to ashutoshishere04/azure-container-networking that referenced this pull request Jan 23, 2020
* added removeorphanlockfile function

* remove lock file if process holding that exited

* addressed comments

* addressed comments and added a condition to check get process cmd is supported

* Addressed comments
fixed trim line ending

* updated log
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants