[1.25] node: device-mgr: Fix recovery flow by ensuring healthy devices exist and pre-allocated devices are healthy #117738
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Start to consolidate the sample device plugin utility and constants in a central place, because we need to use it in different e2e tests. Having a central dependency is better than a maze of entangled e2e tests depending on each other helpers. Signed-off-by: Francesco Romani <fromani@redhat.com>
Update the sample device plugin to enable the e2e node tests (or any other entity with full access to the node filesystem) to control the registration process. We add a new environment variable `REGISTER_CONTROL_FILE`. The value of this variable must be a file which prevents the plugin to register itself while it's present. Once removed, the plugin will go on and complete the registration. The plugin will automatically detect the parent directory on which the file resides and detect deletions, unblocking the registration process. If the file is specified but unaccessible, the plugin will fail. If the file is not specified, the registration process will progress as usual and never pause. The plugin will need read access to the parent directory. This feature is useful because it is not possible to control the order in which the pods are recovered after node reboot/kubelet restart. In this approach, the testing environment will create a directory and then a empty file to pause the registration process of the plugin. Once pointed to that file, the plugin will start and wait for it to be deleted. Only after the directory has been deleted, the plugin would proceed to registration. This feature is used in kubernetes#114640 where e2e test is implemented to simulate scenarios where application pods requesting devices come up before the device plugin pod on node reboot/ kubelet restart. Co-authored-by: Francesco Romani <fromani@redhat.com> Signed-off-by: Swati Sehgal <swsehgal@redhat.com>
Signed-off-by: Swati Sehgal <swsehgal@redhat.com>
The existing path is incorrect (missing `sample-device-plugin`) directory and thus causing test failures. The full path should be `test/e2e/testing-manifests/sample-device-plugin/sample-device-plugin.yaml`. Signed-off-by: David Porter <david@porter.me>
Rather than only returning a string forcing us to log failure with `framework.Fail`, we return a string and error to handle error cases more conventionally. This enables us to use the `parseLog` function inside `Eventually` and `Consistently` blocks, or in general to delegate the error processing and enable better composability. Signed-off-by: Swati Sehgal <swsehgal@redhat.com> Co-authored-by: Francesco Romani <fromani@redhat.com>
We rename to make the intent more explicit; We make it global to be able to reuse the value all across the module (e.g. to check the node readiness) later on. Signed-off-by: Swati Sehgal <swsehgal@redhat.com> Co-authored-by: Francesco Romani <fromani@redhat.com>
With this change the error message are more helpful and easier to troubleshoot in case of test failures. Signed-off-by: Swati Sehgal <swsehgal@redhat.com>
k8s-ci-robot
added
do-not-merge/cherry-pick-not-approved
k8s-ci-robot
added
area/test
sig/node
swatisehgal
force-pushed
the
device-mgr-fix-1.25
branch
from
3ec62b1
to
1fd579b
Compare
swatisehgal
force-pushed
the
device-mgr-fix-1.25
branch
3 times, most recently
from
e25d942
to
16324a1
Compare
|
/test pull-kubernetes-unit |
|
/test pull-kubernetes-e2e-gce-ubuntu-containerd-serial |
k8s-ci-robot
added
release-note
SergeyKanzhelev
moved this from Triage
to PRs - Needs Reviewer
in SIG Node CI/Test Board
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 1b63c67942c091880eca3b0f8b493da67086c6d2
|
|
/lgtm disclosure: I co-authored a large part of the changes The majority of the effort here is spent adding/improving e2e tests to verify this change. While this comes with a cost (larger PR, more code to backport) overall I'm leaning towards preferring a larger backport like this with extensive e2e coverage, because this is a obscure bug in a difficult-to-trigger flow. While I reckon there's probably no universally right answer to the dilemma between larger PR (with more tests) vs targeted fix, IMO the basic principle that hard-to-trigger flows should have more test coverage, not less (exactly because they are hard to trigger!) prevails here. |
|
/assign @klueska |
|
The same patch-set for 1.26 is applied here for 1.25. Also reviewed the diff between this one and the backport for 1.27. /lgtm |
k8s-ci-robot
added
the
approved
SIG Node CI/Test Board
automation
moved this from PRs - Needs Reviewer
to PRs - Needs Approver
May 11, 2023
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dchen1107, swatisehgal, xmudrii 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 |
xmudrii
added
the
cherry-pick-approved
k8s-ci-robot
removed
the
do-not-merge/cherry-pick-not-approved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind bug
What this PR does / why we need it:
Cherry-pick of the following PR (/commits) on release-1.25:
Rationale for the changes
In case of node reboot/kubelet restart, the flow of events involves obtaining the state from the checkpoint file followed by setting the
healthDevices/unhealthyDevicesto its zero value. This is done to allow the device plugin to re-register itself so that capacity can be updated appropriately.During the allocation phase, we need to check if the resources requested by the pod have been registered AND healthy devices are present on the node to be allocated.
Also we need to move this check above
needed==0where needed is required - devices allocated to the container (which is obtained from the checkpoint file) because even in cases where no additional devices have to be allocated (as they were pre-allocated), we still need to make sure that the devices that were previously allocated are healthy.For more details refer to the comment here: #109595 (comment).
Which issue(s) this PR fixes:
Fixes #109595
Does this PR introduce a user-facing change?