Use Lockedfile api to acquire lock #1070
Conversation
tamilmani1989
force-pushed
the
tamanoha/lockedfileapi
branch
2 times, most recently
from
dcf13e0 to
22fcd87
Compare
tamilmani1989
force-pushed
the
tamanoha/lockedfileapi
branch
from
22fcd87 to
d883e76
Compare
rbtr
left a comment
rbtr
left a comment
There was a problem hiding this comment.
looks really solid and definitely an improvement 💯
requesting a couple superficial changes
tamilmani1989
force-pushed
the
tamanoha/lockedfileapi
branch
2 times, most recently
from
949b4cb to
f15b07f
Compare
fixed ut
tamilmani1989
force-pushed
the
tamanoha/lockedfileapi
branch
from
f15b07f to
caf38a9
Compare
rbtr
left a comment
rbtr
left a comment
There was a problem hiding this comment.
only requesting that you finish changing the error strings and variable names, everything else lgtm
telemetry/telemetrybuffer.go
Outdated
| b = append(buf, Delimiter) | ||
| //nolint:staticcheck // append says It is therefore necessary to store the | ||
| // result of append, often in the variable holding the slice itself: | ||
| b = append(b, Delimiter) |
There was a problem hiding this comment.
to conform to the standard Go io.Writer interface, Write([]byte) must not modify the slice, even temporarily. Copying b into buf before appending delim was the right thing to do here.
There was a problem hiding this comment.
copy function wasn't working properly in windows atleast in version I tested and thats why changed this. in windows after copy both b and buf contents are just spaces
There was a problem hiding this comment.
these comments are posted in append function and the recommendation is to use same buffer
There was a problem hiding this comment.
using the same buffer breaks the Write([]byte) interface contract. if copy() seems broken on windows I think something else is going on here that needs to be figured out
There was a problem hiding this comment.
hey @tamilmani1989, @ramiro-gamarra just pointed out to me that the signature for copy(dst, src) is...destination first. so the way this was originally written is just backwards. swapping those args in the original code should make everything work
addressed it except one |
tamilmani1989
force-pushed
the
tamanoha/lockedfileapi
branch
from
594fe79 to
33e3105
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…re-container-networking into tamanoha/lockedfileapi # Conflicts: # telemetry/telemetrybuffer.go
| } | ||
| require.NoError(t, err) | ||
| require.Equal(t, tt.want, got) | ||
| require.Equal(t, tt.want, got, "Expected:%d but got:%d", tt.want, got) |
There was a problem hiding this comment.
for future reference, the testify comparison funcs print the diff of the inputs when they fail so this additional logging shouldn't be necessary
* added lockedfileapi support for CNI * fixed interface changes * addressed comments fixed ut * addressed comments * fixed copy to buffer part in writer api * fixed copy to buffer part in writer api * keeping old code not changing it.
Reason for Change:
Reason for Change:
Previously lock obtained by creating file with EXEC permission and on release it has to remove file otherwise other CNI process cannot acquire. If a process exits in middle without releasing, lock file exists preventing other CNI to acquire lock. This required CNI to have complicated logic of checking if process holding lock exists and make sure its CNI process.
Changes:
This PR uses go internal package lockedfile api to implement lock across process. This API internally handles process exit/crash after acquiring lock. Even if exited process didnt release lock explicitly, the new cni process could acquire lock and continue its operation. This is blocking api and added go routine to make it non-blocking as like previous behaviour.
NOTE: This locking behavior changes for CNS also
Issue Fixed:
Requirements:
Notes: