Use github.com/gofrs/flock to lock handler #731
Conversation
|
/hold |
|
/retest |
kubevirt-bot
added
do-not-merge/release-note-label-needed
qinqon
force-pushed
the
test-handler-lock
branch
from
7c4edad to
47f3bcc
Compare
kubevirt-bot
added
dco-signoff: yes
The nmstate-handler has a lock that make it wait if another handler is already running in the system, but the library used for that does not work well with containers. This change replace that library with different one that uses golang syscall.Flock behind the scine [1] and that works fine with containers sharing node volume. [1] github.com/nightlyone/lockfile. Signed-off-by: Quique Llorente <[email protected]>
The nmstate handler is marked as ready even if it was not able to take ownership of the lock, this change runs nmstatectl and touchs a file after gaining lock ownership so the readiness probe can check both things. Signed-off-by: Quique Llorente <[email protected]>
qinqon
force-pushed
the
test-handler-lock
branch
from
47f3bcc to
da5f392
Compare
qinqon
changed the title
kubevirt-bot
added
release-note
The nmstate handler don't get ready if another handler is running at the same node. This change add an e2e tests to check that this is working. Signed-off-by: Quique Llorente <[email protected]>
qinqon
force-pushed
the
test-handler-lock
branch
from
da5f392 to
5caab4c
Compare
| // point (we have the handler lock and nmstatectl show is working) a | ||
| // file is touched and the file is checked at readinessProbe field. | ||
| healthyFile := "/tmp/healthy" | ||
| setupLog.Info("Marking handler as healthy touching healthy file", "healthyFile", healthyFile) |
There was a problem hiding this comment.
This message sounds odd, we don't mark handler healthy here, do we? That's what the probe will do once it tries to cat that file
There was a problem hiding this comment.
well we mark the container by touching the file
| AfterEach(func() { | ||
| uninstallNMState(defaultNMState) | ||
| uninstallOperator(operatorNamespace) | ||
| installOperator("nmstate") |
There was a problem hiding this comment.
Why is the operator installed in "nmstate" namespace at the end?
If it's done to test that operator can be installed and uninstalled at one namespace and then installed in another, wouldn't it be better to have another spec for it?
There was a problem hiding this comment.
The test-e2e-operator test suite spec the operator to be installed, but the test we are adding here remove the nmstate operator to check that the handler installed from different namespace is blocked until the "nmstate" one is delete.
The install here ensure that after that test the nmstate operator is there as expected at the other test cases.
|
/retest |
1 similar comment
|
/retest |
|
@qinqon: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Thank you |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhrazdil The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubevirt-bot
added
the
approved
Is this a BUG FIX or a FEATURE ?:
/kind bug
Closes #729
What this PR does / why we need it:
The nmstate-handler has a lock that make it wait if another handler is
already running in the system, but the library used for that does not
work well with containers. This change replace that library with
different one that uses golang syscall.Flock behind the scine [1] and
that works fine with containers sharing node volume.
[1] github.com/nightlyone/lockfile.
Special notes for your reviewer:
Release note: