Collect disk information along with other info by parthaa · Pull Request #3621 · candlepin/subscription-manager

parthaa · 2025-10-29T16:04:54Z

adds disk..size_bytes fact along with other facts. This is useful for customers.

# subscription-manager facts | grep disk
disk.vda.size_bytes: 1500000
disks.sda.size_bytes: 1700000

sourcery-ai · 2025-10-29T16:05:00Z

Reviewer's Guide

This PR introduces a new DiskCollector that scans /sys/block for real block devices, reads and converts their sector counts into size_bytes facts, integrates this collector into the main facts pipeline, and adds comprehensive unit tests to validate its behavior.

Entity relationship diagram for disk facts

erDiagram
    DISK {
        string device_name
        int size_bytes
    }
    DISK ||--o| FACTS : provides
    FACTS {
        string fact_name
        int fact_value
    }

Class diagram for the new DiskCollector and its integration

classDiagram
    class collector.FactsCollector {
        <<abstract>>
        +arch: str
        +prefix: str
        +testing: bool
        +collected_hw_info: Dict
    }
    class DiskCollector {
        +hardware_methods: List[Callable]
        +_get_block_devices() List[str]
        +_get_device_size_bytes(device: str) int
        +get_disk_size_info() Dict[str, Union[str, int]]
    }
    DiskCollector --|> collector.FactsCollector
    class all.AllFactsCollector {
        +collectors: List[collector.FactsCollector]
    }
    all.AllFactsCollector o-- DiskCollector

File-Level Changes

Change	Details	Files
Register the new DiskCollector in the facts engine	Imported DiskCollector in all.py Added DiskCollector to the host facts collector list	`src/rhsmlib/facts/all.py`
Implement DiskCollector to retrieve disk size facts	Defined DiskCollector class extending FactsCollector Enumerates block devices with regex filtering in _get_block_devices Reads sector counts and converts to bytes in _get_device_size_bytes Aggregates non-zero sizes into facts in get_disk_size_info	`src/rhsmlib/facts/disk.py`
Add unit tests covering DiskCollector functionality	Test block device discovery for missing, empty, and mixed /sys/block contents Validate size reading for missing, invalid, and valid files Verify get_disk_size_info filtering and key formatting Ensure get_all and collect methods integrate correctly	`b/test/rhsmlib/facts/test_disk.py`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

chris1984

Tested and works ok:

[root@cappreview facts]# subscription-manager facts | grep disk
disk.vda.size_bytes: 107374182400

sourcery-ai

Hey there - I've reviewed your changes - here's some feedback:

Use os.path.join for constructing filesystem paths rather than string concatenation to improve portability and readability.
The block device regex currently only matches sd*/vd*/nvme*/hd*/xvd*; consider extending it (and adding tests) to include other valid device types like mmcblk*.
Defaulting prefix to None can lead to errors when concatenating paths; consider defaulting prefix to an empty string or validating it before use.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Use os.path.join for constructing filesystem paths rather than string concatenation to improve portability and readability.
- The block device regex currently only matches sd*/vd*/nvme*/hd*/xvd*; consider extending it (and adding tests) to include other valid device types like mmcblk*.
- Defaulting prefix to None can lead to errors when concatenating paths; consider defaulting prefix to an empty string or validating it before use.

## Individual Comments

### Comment 1
<location> `test/rhsmlib/facts/test_disk.py:70-73` </location>
<code_context>
+        expected = ['hda', 'nvme0n1', 'sda', 'vda', 'xvda']  # sorted
+        self.assertEqual(result, expected)
+
+    def test_get_device_size_bytes_missing_file(self):
+        """Test _get_device_size_bytes with missing size file"""
+        result = self.collector._get_device_size_bytes('nonexistent')
+        self.assertEqual(result, 0)
+
+    def test_get_device_size_bytes_invalid_content(self):
</code_context>

<issue_to_address>
**suggestion (testing):** Consider adding a test for permission errors when reading the size file.

Testing permission errors would help ensure _get_device_size_bytes handles all relevant file access issues robustly.
</issue_to_address>

### Comment 2
<location> `test/rhsmlib/facts/test_disk.py:148-157` </location>
<code_context>
+        self.assertEqual(result, {'disk.sda.size_bytes': 1000000000000})
+        mock_method.assert_called_once()
+
+    def test_collect(self):
+        """Test collect method returns FactsCollection"""
+        with patch.object(self.collector, 'get_all') as mock_get_all:
+            mock_get_all.return_value = {'disk.sda.size_bytes': 1000000000000}
+
+            result = self.collector.collect()
+
+            # Check that it returns a FactsCollection object
+            from rhsmlib.facts.collection import FactsCollection
+            self.assertIsInstance(result, FactsCollection)
+
+
</code_context>

<issue_to_address>
**suggestion (testing):** Consider testing collect() with an empty result.

Add a test where get_all returns an empty dictionary to verify that collect() correctly returns a FactsCollection and handles empty input.

```suggestion
    def test_collect(self):
        """Test collect method returns FactsCollection"""
        with patch.object(self.collector, 'get_all') as mock_get_all:
            mock_get_all.return_value = {'disk.sda.size_bytes': 1000000000000}

            result = self.collector.collect()

            # Check that it returns a FactsCollection object
            from rhsmlib.facts.collection import FactsCollection
            self.assertIsInstance(result, FactsCollection)

    def test_collect_empty(self):
        """Test collect method returns FactsCollection for empty input"""
        with patch.object(self.collector, 'get_all') as mock_get_all:
            mock_get_all.return_value = {}

            result = self.collector.collect()

            from rhsmlib.facts.collection import FactsCollection
            self.assertIsInstance(result, FactsCollection)
            # Optionally, check that the FactsCollection is empty
            self.assertEqual(len(result), 0)
```
</issue_to_address>

### Comment 3
<location> `src/rhsmlib/facts/disk.py:40` </location>
<code_context>
    def _get_block_devices(self) -> List[str]:
        """Get list of block devices from /sys/block/"""
        block_devices: List[str] = []
        sys_block_path: str = self.prefix + "/sys/block"

        try:
            if os.path.exists(sys_block_path):
                for device in os.listdir(sys_block_path):
                    # Skip loop devices, ram devices, and other virtual devices
                    # Focus on actual disk devices (sd*, vd*, nvme*, hd*, xvd*)
                    if re.match(r'^(sd[a-z]+|vd[a-z]+|nvme[0-9]+n[0-9]+|hd[a-z]+|xvd[a-z]+)$', device):
                        block_devices.append(device)
        except OSError as e:
            log.debug(f"Could not read /sys/block directory: {e}")

        return sorted(block_devices)

</code_context>

<issue_to_address>
**suggestion (code-quality):** Use f-string instead of string concatenation ([`use-fstring-for-concatenation`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-concatenation/))

```suggestion
        sys_block_path: str = f"{self.prefix}/sys/block"
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

test/rhsmlib/facts/test_disk.py

src/rhsmlib/facts/disk.py

chris1984 · 2025-10-29T16:06:12Z

@Aftermath Can this get a review or merge?

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

Aftermath · 2025-10-29T16:46:50Z

@Aftermath Can this get a review or merge?

Developer should review, I will leave to @jirihnidek.
Also this will need a RHEL card, or its not getting considered. This should instead go to future tooling, if possible.
If there is urgent need to add facts to sub-man, it should follow RHEL process.

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

m-horky

I have some requests. Please note that addressing them doesn't mean this PR will get merged, we need to pass this through product. For transparency, this PR seems to address RHEL-124924/SAT-27982.

Trees under /sys/block/ seem to have SELinux type of sysfs_t, so we should be good with existing SELinux policy. This will definitely make it easier to get merged and released.

m-horky · 2025-10-30T08:39:18Z

src/rhsmlib/facts/disk.py

@@ -0,0 +1,80 @@
+# Copyright (c) 2023 Red Hat, Inc.


m-horky · 2025-10-30T08:46:17Z

src/rhsmlib/facts/disk.py

+            if size_bytes > 0:
+                result[f"disk.{device}.size_bytes"] = size_bytes
+
+        return result


Missing newline.

m-horky · 2025-10-30T08:46:34Z

test/rhsmlib/facts/test_disk.py

@@ -0,0 +1,173 @@
+# Copyright (c) 2023 Red Hat, Inc.


m-horky · 2025-10-30T08:48:22Z

test/rhsmlib/facts/test_disk.py

+
+
+if __name__ == '__main__':
+    unittest.main()


Missing newline.

m-horky · 2025-10-30T08:51:09Z

src/rhsmlib/facts/disk.py

+                # The size file contains the number of 512-byte sectors
+                sectors = int(f.read().strip())
+                return sectors * 512


512 might be wrong. You should check the content of /sys/block/*/queue/logical_block_size instead.

Collect disk information along with other info

6fdcc1e

chris1984 approved these changes Oct 29, 2025

View reviewed changes

sourcery-ai bot reviewed Oct 29, 2025

View reviewed changes

test/rhsmlib/facts/test_disk.py Show resolved Hide resolved

src/rhsmlib/facts/disk.py Outdated Show resolved Hide resolved

Update src/rhsmlib/facts/disk.py

25e1ea1

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

Update test/rhsmlib/facts/test_disk.py

31d8099

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

m-horky requested changes Oct 30, 2025

View reviewed changes



		if __name__ == '__main__':
		unittest.main() No newline at end of file

Conversation

parthaa commented Oct 29, 2025

Uh oh!

sourcery-ai bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Guide

Entity relationship diagram for disk facts

Class diagram for the new DiskCollector and its integration

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

chris1984 left a comment

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chris1984 commented Oct 29, 2025

Uh oh!

Aftermath commented Oct 29, 2025

Uh oh!

m-horky left a comment

Choose a reason for hiding this comment

Uh oh!

m-horky Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

m-horky Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

m-horky Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

m-horky Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

m-horky Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sourcery-ai bot commented Oct 29, 2025 •

edited

Loading