Skip to content

Conversation

kasturinarra
Copy link

@kasturinarra kasturinarra commented Aug 7, 2025

Which issue(s) this PR addresses: Verify a Pod can request multiple instances of a device when count is specified in the GDP configuration.

Closes #

@openshift-ci openshift-ci bot requested review from dhensel-rh and pacevedom August 7, 2025 18:46
@kasturinarra kasturinarra changed the title Test hot plugging of devices [WIP] - Test hot plugging of devices Aug 7, 2025
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 7, 2025
@kasturinarra kasturinarra changed the title [WIP] - Test hot plugging of devices Add test to verify count in GDP config Aug 11, 2025
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2025
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2025
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2025
@kasturinarra
Copy link
Author

@pmtk could you please help review ? thanks !!

Verify FUSE device allocation and accessibility
[Documentation] Verifies FUSE device configuration, allocation, and accessibility in pods
[Tags] fuse-device
Enable And Configure GDP ${GDP_CONFIG_FUSE_COUNT}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Enable And Configure GDP ${GDP_CONFIG_FUSE_COUNT}
[Setup] Enable And Configure GDP ${GDP_CONFIG_FUSE_COUNT}

Copy link
Author

Choose a reason for hiding this comment

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

Done.

Enable And Configure GDP ${GDP_CONFIG_FUSE_COUNT}
Wait Until Device Is Allocatable 10 fuse
# Create and deploy pod requesting 4 FUSE devices
${path}= Create Random Temp File ${POD_FUSE_DEVICE}
Copy link
Member

Choose a reason for hiding this comment

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

How about moving the string into a dedicated file in test/assets/generic-device-plugin/?

Copy link
Author

Choose a reason for hiding this comment

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

okay, i can do that.

Oc Wait -n ${NAMESPACE} pod/fuse-test-pod --for=condition=Ready --timeout=120s

# Verify /dev/fuse is accessible in the pod
${fuse_device}= Run With Kubeconfig oc exec -n ${NAMESPACE} fuse-test-pod -- ls -l /dev/fuse
Copy link
Member

Choose a reason for hiding this comment

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

We have Oc Exec keyword, can we use it instead?

Copy link
Author

Choose a reason for hiding this comment

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

Done.

${describe_output}= Run With Kubeconfig oc describe node ${node_name}
Should Contain ${describe_output} device.microshift.io/fuse: 10
Should Contain ${describe_output} device.microshift.io/fuse 4 4

Copy link
Member

Choose a reason for hiding this comment

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

Missing [Teardown]

Copy link
Author

Choose a reason for hiding this comment

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

Based on the test case i see i do not need individual teardown, since Teardown Suite With GDP Cleanup would be sufficient. Do i still need the Missing [Teardown] ?

Copy link
Member

Choose a reason for hiding this comment

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

True :D

Comment on lines 72 to 73
Should Contain ${describe_output} device.microshift.io/fuse: 10
Should Contain ${describe_output} device.microshift.io/fuse 4 4
Copy link
Member

Choose a reason for hiding this comment

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

There are 2+ spaces after fuse and numbers, this means they are separate arguments to the keyword

Should Contain    ${describe_output}    device.microshift.io/fuse    4    4
                  --- arg 1 -------    --- arg 2 -----------       arg3  arg4

You can see it in the execution log:

Arguments: [ ... | 'device.microshift.io/fuse' | '4' | '4' ]

I think it skips the fours and just matches the first string. I think we need to create string with 4 spaces inside in another way

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@kasturinarra kasturinarra changed the title Add test to verify count in GDP config USHIFT-5723: Add test to verify count in GDP config Aug 20, 2025
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 20, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 20, 2025

@kasturinarra: This pull request references USHIFT-5723 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set.

In response to this:

Which issue(s) this PR addresses: Verify a Pod can request multiple instances of a device when count is specified in the GDP configuration.

Closes #

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 openshift-eng/jira-lifecycle-plugin repository.

@kasturinarra
Copy link
Author

@pmtk addressed all your comments, could you please help review again ?

VAR ${path}= ./assets/generic-device-plugin/fuse-test-pod.yaml
Oc Create -f ${path} -n ${NAMESPACE}

# Wait for pod to be ready and verify device accessibility
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this comment is redundant. "verify device accessibility" applies more to next block which has its own comment

Copy link
Author

Choose a reason for hiding this comment

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

removed.

[Tags] fuse-device
[Setup] Enable And Configure GDP ${GDP_CONFIG_FUSE_COUNT}
Wait Until Device Is Allocatable 10 fuse
# Create and deploy pod requesting 4 FUSE devices
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this comment is redundant

Copy link
Author

Choose a reason for hiding this comment

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

Removed

Comment on lines 58 to 59
VAR ${path}= ./assets/generic-device-plugin/fuse-test-pod.yaml
Oc Create -f ${path} -n ${NAMESPACE}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
VAR ${path}= ./assets/generic-device-plugin/fuse-test-pod.yaml
Oc Create -f ${path} -n ${NAMESPACE}
Oc Create -f ./assets/generic-device-plugin/fuse-test-pod.yaml -n ${NAMESPACE}

Copy link
Author

Choose a reason for hiding this comment

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

Addressed.

Comment on lines 73 to 78
${allocation_matches}= Get Regexp Matches
... ${allocated_line}
... device\\.microshift\\.io/fuse\\s+(\\d+)\\s+(\\d+)
... 1 2
Should Be Equal As Integers ${allocation_matches}[0] 4
Should Be Equal As Integers ${allocation_matches}[1] 4
Copy link
Member

Choose a reason for hiding this comment

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

This failed for me:
image

Copy link
Author

@kasturinarra kasturinarra Aug 20, 2025

Choose a reason for hiding this comment

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

can you try again please ? I just updated it to the right way.

Verify FUSE device allocation and accessibility :: Verifies FUSE d... | PASS |

Copy link
Author

@kasturinarra kasturinarra Aug 20, 2025

Choose a reason for hiding this comment

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

Screenshot From 2025-08-20 15-22-44

@pmtk
Copy link
Member

pmtk commented Aug 20, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 20, 2025
Copy link
Contributor

openshift-ci bot commented Aug 20, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kasturinarra, pmtk

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2025
Copy link
Contributor

openshift-ci bot commented Aug 20, 2025

@kasturinarra: all tests passed!

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit 4ae6183 into openshift:main Aug 20, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants