Skip to content
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

(FACT-3160) Fix size calculations for AIX blockdevice and partitions #2528

Conversation

loopway
Copy link
Contributor

@loopway loopway commented Oct 16, 2022

  • disks.rb: TOTAL PPs = USED PPs + FREE PPs. Therefor adding FREE PPs to TOTAL PPs produces incorrect disk sizes.

  • partitions.rb: Using physical_partitions (PPs) to calculate LV sizes are not suitable if there is more than one mirror (can be two or three). LPs * nr_of_mirrors = PPs.

  • info_extractor.rb: Adjusted regexp because using property LPs also matched MAX LPs.

@loopway loopway requested a review from a team as a code owner October 16, 2022 00:38
@puppetlabs-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@CLAassistant
Copy link

CLAassistant commented Oct 16, 2022

CLA assistant check
All committers have signed the CLA.

@smortex
Copy link
Contributor

smortex commented Oct 21, 2022

I guess you'll have to update the tests accordingly in spec/facter/facts/aix/disks_spec.rb and spec/facter/facts/aix/partitions_spec.rb?

@loopway
Copy link
Contributor Author

loopway commented Oct 23, 2022

@smortex I don't think it's necessary, since the sizes in the specs are specified without using nr. of LP's and PP's. Tests would have only failed in previous versions if mirrored LV's have been used and/or there was free space (PPs) in the VGs.

@joshcooper
Copy link
Contributor

@loopway thank you for your pull request. Could you rebase your changes on top of main?

* disks.rb: TOTAL PPs = USED PPs + FREE PPs. Therefor adding FREE PPs to TOTAL PPs produces incorrect disk sizes.

* partitions.rb: Using physical_partitions (PPs) to calculate LV sizes are not suitable if there is more than one mirror (can be two or three). LPs * nr_of_mirrors = PPs.

* info_extractor.rb: Adjusted regexp because using property LPs also matched MAX LPs.
@loopway loopway force-pushed the facter-aix-sizes-blockdevice-partitions-fix branch from a3a7885 to dbc3ebe Compare April 30, 2023 22:12
@joshcooper
Copy link
Contributor

retriggering actions

@joshcooper joshcooper closed this Aug 30, 2023
@joshcooper joshcooper reopened this Aug 30, 2023
@joshcooper
Copy link
Contributor

@loopway did facter 3.x (in puppet-agent 5.x) correctly report blockdevice_hdisk*_size and partitions./dev/*.size facts?

The reason I ask is because facter 3.x on AIX called odm to collect this information:

disk_resolver::data disk_resolver::collect_data(collection& facts)
{
data result;
vector<string> disk_types;
auto pd_dv_query = odm_class<PdDv>::open("PdDv").query("class=disk");
for (auto& pd_dv : pd_dv_query) {
LOG_DEBUG("got a disk type: {1}", pd_dv.uniquetype);
disk_types.push_back(pd_dv.uniquetype);
}
auto cu_dv = odm_class<CuDv>::open("CuDv");
for (string& type : disk_types) {
string query = (boost::format("PdDvLn=%1%") % type).str();
auto cu_dv_query = cu_dv.query(query);
for (auto& cu_dv : cu_dv_query) {
LOG_DEBUG("got a disk: {1}", cu_dv.name);
disk d;
d.name = cu_dv.name;
{
string device = (boost::format("/dev/%1%") % d.name).str();
auto descriptor = open(device.c_str(), O_RDONLY);
if (descriptor < 0) {
LOG_DEBUG("Could not open device %1% for reading: %2% (%3%). Disk facts will not be populated for this device", d.name, strerror(errno), errno);
continue;
}
scoped_descriptor fd(descriptor);
devinfo info;
auto result = ioctl(fd, IOCINFO, &info);
if (result < 0) {
LOG_DEBUG("Ioctl IOCINFO failed for device %1%: %2% (%3%). Disk facts will not be populated for this device", d.name, strerror(errno), errno);
continue;
}
switch (info.devtype) {
case DD_DISK:
case DD_SCDISK:
if (info.flags & DF_LGDSK) {
d.size = (uint32_t)info.un.scdk64.hi_numblks;
d.size <<= 32;
d.size |= (uint32_t)info.un.scdk64.lo_numblks;
d.size *= info.un.scdk64.blksize;
} else {
d.size = (uint32_t)info.un.scdk.numblks;
d.size *= info.un.scdk.blksize;
}
break;
default:
LOG_WARNING("Expected a Disk or SCSI disk device, got device code '{1}'. This is probably a Facter bug. Please report it, and include this error message.", info.devtype);
break;
}
}
result.disks.emplace_back(move(d));
But in my testing facter 3.x incorrectly reported the amount of used disk space, and therefore capacity. For example, given

# df -Im /    
Filesystem    MB blocks      Used      Free %Used Mounted on
/dev/hd4        2048.00    288.61   1759.39   15% /

Facter 3 incorrectly reports used as 2.00 GiB:

mountpoints => {
  / => {
    available => "1.77 GiB",
    available_bytes => 1898340352,
    capacity => "53.08%",
    device => "/dev/hd4",
    filesystem => "jfs2",
    options => [
      "rw",
      "log=/dev/hd8"
    ],
    size => "2.00 GiB",
    size_bytes => 2147483648,
    used => "2.00 GiB",
    used_bytes => 2147483648
  }

While Facter 4 correctly reports 288.59 MiB:

mountpoints => {
  / => {
    available => "1.72 GiB",
    available_bytes => 1844871168,
    capacity => "14.09%",
    device => "/dev/hd4",
    filesystem => "jfs2",
    options => [
      "rw",
      "log=/dev/hd8"
    ],
    size => "2.00 GiB",
    size_bytes => 2147483648,
    used => "288.59 MiB",
    used_bytes => 302612480
  },

@joshcooper
Copy link
Contributor

joshcooper commented Nov 4, 2023

@loopway I submitted a new PR #2633 with your original commit along with a test fix, so I'm going to close this PR. Thank you for your contribution!

@joshcooper joshcooper closed this Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants