-
Notifications
You must be signed in to change notification settings - Fork 225
New testcase 'verify_smb_linux' #4184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Depended on PR #4182 |
New testcase 'verify_smb_linux' A comprehensive test to verify CIFS module and SMB share functionality between two Linux VMs.
cfb9c2e to
4ac2dad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a new test case verify_smb_linux to validate CIFS module and SMB share functionality between two Linux VMs. The test creates two VMs in Azure, configures one as an SMB server and the other as a client, then validates SMB mounting and file I/O operations across multiple SMB versions (2.0, 2.1, 3.0, 3.1.1).
Critical Issue: The test cannot execute as written because it depends on SmbClient and SmbServer tools that do not exist in the codebase. These tools must be implemented before the test can be functional.
Key Changes
- Adds comprehensive SMB/CIFS testing between two Linux VMs
- Tests multiple SMB protocol versions (2.0, 2.1, 3.0, 3.1.1)
- Validates bidirectional file I/O through SMB shares
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…com/microsoft/lisa into smyakam/verify_smb_linux/03_01_2025
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| timeout=TIME_OUT, | ||
| requirement=simple_requirement( | ||
| min_count=2, | ||
| unsupported_os=[Redhat, CBLMariner, AlmaLinux, BSD, Windows], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is Redhat, CBLMariner and Alma unsupported?
In the SMBTool, the below line exists
if isinstance(self.node.os, (CBLMariner, Redhat, Fedora, Oracle, Suse)):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SMB "server" packages are not officially supported by Mariner. Redhat family have issues SELinux etc.
Only Ubuntu, Debian and SUSE are supported for this test case now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only Ubuntu, Debian and SUSE are supported for this test case now.
Do you want to rather use supported_os requirement in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redhat has issues because of SELinux, we can't make it work in test code?
Do we have a link for SMB "server" packages are not officially supported by Mariner
| smb_client.unmount_share(mount_point) | ||
| smb_client.cleanup_mount_point(mount_point) | ||
| except Exception as e: | ||
| log.warning( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont use warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to "log.error()"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use log.info or log.debug. If you need to exit, you can raise an Exception
| smb_server.stop() | ||
| smb_server.remove_share(share_path) | ||
| except Exception as e: | ||
| log.warning( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont use warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to "log.error()"
| smb_server = server_node.tools[SmbServer] | ||
| smb_server.stop() | ||
| smb_server.remove_share(share_path) | ||
| except Exception as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If cleanup fails, mark node as dirty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed as suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added comment below
| smb_server.create_share(share_name, share_path) | ||
|
|
||
| # Step 8: Repeat for different SMB versions | ||
| for smb_version in smb_versions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current logic, if one version fails, the case fails and exits.
Is that the expected? or do you want to verify each version independently?
| for role_name, role_node in (("server", server_node), ("client", client_node)): | ||
| if not role_node.tools[KernelConfig].is_enabled("CONFIG_CIFS"): | ||
| raise LisaException( | ||
| f"CIFS module must be present for SMB testing on {role_name} node" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are not printing the node being assigned as server and client, printing the role_name does not add value in the logging.
Do you rather want to print the node name? or I think it's fine to just have it like "CIFS module must be present on the node", as both nodes are identical
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since both the nodes are identical its okay this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case "{role_name}" does not add any value in the logging statement, you can replace it with "CIFS module must be present" ?
|
@SRIKKANTH please make the two PRs into one PR, so that we can validate it. |
SMB server and client tools These tools are useful in validating samba file share with Linux OS and CIFS module in kernel.
Merged both PRs. |
| ) | ||
| bad_cleanup = True | ||
| if bad_cleanup: | ||
| raise BadEnvironmentStateException("SMB test cleanup encountered errors.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raising BadEnvironmentStateException in the case will cause test case failure.
You are running the cleanup in the case itself, hence even if your test logic succeeds, but the cleanup fails, the case will fail.
You can move the cleanup to after case, or ensure that cleanup does not raise an Exception within the case.
If you are moving the cleanup to after case, use self.node.mark_dirty() because BadEnvironmentStateException does not mark the env as dirty in after case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| # Install client utilities | ||
| if isinstance( | ||
| self.node.os, | ||
| (Ubuntu, Debian, CBLMariner, CoreOs, Fedora, Oracle, Redhat, Suse, Alpine), |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _install method for SmbClient includes CBLMariner in the supported distributions list, but the test case excludes CBLMariner in its requirements (line 624 of storage.py). This creates an inconsistency in which distributions are actually supported for SMB client functionality.
| (Ubuntu, Debian, CBLMariner, CoreOs, Fedora, Oracle, Redhat, Suse, Alpine), | |
| (Ubuntu, Debian, CoreOs, Fedora, Oracle, Redhat, Suse, Alpine), |
| mount_options = [ | ||
| f"vers={smb_version}", | ||
| "file_mode=0777", | ||
| "dir_mode=0777", | ||
| "guest", | ||
| ] | ||
|
|
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mount options use hardcoded permissions (file_mode=0777, dir_mode=0777) which create a security risk. Consider making these configurable via the options parameter or using more restrictive defaults like 0755.
| mount_options = [ | |
| f"vers={smb_version}", | |
| "file_mode=0777", | |
| "dir_mode=0777", | |
| "guest", | |
| ] | |
| mount_options: List[str] = [ | |
| f"vers={smb_version}", | |
| "guest", | |
| ] | |
| # Apply default permissions only if not specified by caller | |
| caller_options = options or [] | |
| if not any(opt.startswith("file_mode=") for opt in caller_options): | |
| mount_options.append("file_mode=0755") | |
| if not any(opt.startswith("dir_mode=") for opt in caller_options): | |
| mount_options.append("dir_mode=0755") |
| unsupported_os=[Redhat, CBLMariner, AlmaLinux, BSD, Windows], | ||
| ), |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test excludes Redhat, CBLMariner, and AlmaLinux from the supported_os list, but there's no clear documentation explaining why these distributions are unsupported. Consider adding a comment explaining the technical reason (e.g., package availability issues, known bugs, or version compatibility problems) to help future maintainers understand this decision.
| elif isinstance(self.node.os, (Debian, CoreOs, Fedora, Oracle, Redhat, Suse)): | ||
| self.node.os.install_packages(["samba", "cifs-utils"]) |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _install method includes CBLMariner in the installation logic, but the test case explicitly excludes CBLMariner in its requirements (line 624 of storage.py). Either CBLMariner should be removed from this installation list to match the test requirements, or the test should support CBLMariner. This inconsistency could lead to confusion about which distributions are actually supported.
| # Set permissions for the share directory | ||
| self.node.tools[Chmod].chmod(share_path, "777", sudo=True) |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting directory permissions to 777 creates a security risk by allowing any user to read, write, and execute files in the share directory. While this might be acceptable for testing purposes, consider using more restrictive permissions (e.g., 755 or 775) and document the security implications, especially if this code could be used as a reference for production setups.
| # Set permissions for the share directory | |
| self.node.tools[Chmod].chmod(share_path, "777", sudo=True) | |
| # Set permissions for the share directory. | |
| # Use 775 to avoid world-writable permissions (777) while still | |
| # allowing group members to write to the share directory. | |
| self.node.tools[Chmod].chmod(share_path, "775", sudo=True) |
A comprehensive test to verify CIFS module and SMB share functionality between two Linux VMs.