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

parted: fix the ordering of list command #49804

Merged
merged 2 commits into from Oct 23, 2018

Conversation

@aplanas
Copy link
Contributor

commented Sep 27, 2018

The command parted -m -s {} print do not print the "Type" column,
but after the "File System" column print the name of the partition.

This point that the type of the partition cannot be extracted using
parted with machine parseable output.

@aplanas

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

I submitted a patch to parted to add this field in the --machine format, as is really needed, but for now this change is relevant.

@rallytime rallytime requested a review from garethgreenaway Sep 28, 2018
@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Sep 30, 2018

Hmm. I'd like to know a little more about how this broke. Did the output of parted change at some point? If so, we need to account for both versions. cc: @techhat

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2018

In addition to the question above, this change is breaking some tests: https://jenkinsci.saltstack.com/job/pr-kitchen-centos7-py2/job/PR-49804/2/

The command `parted -m -s {} print` do not print the "Type" column,
but after the "File System" column print the name of the partition.

This point that the type of the partition cannot be extracted using
parted with machine parseable output.
@aplanas aplanas force-pushed the aplanas:fix_parted_list branch from 0fddceb to f23d618 Oct 1, 2018
@aplanas

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2018

@rallytime Thanks! I updated the tests. I run tox locally and I have some errors, but I do not think that are related to the patch.

@aplanas

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2018

@cachedout Looks like that the parted coded didn't change. Looks an honest error in the salt code.

The core of the error is in the interpretation of the parted -m output. The human readable output in parted is different from the machine readable one. In the machine readable parted do not show the type, and optionally shows the name at the end (in the case of a gpt partition). Maybe in the tests is more clear:

f23d618#diff-ecd1221feb34cadb26ee8ac0fc57b66eL210

In the original test ext3 is in the type column. But in parted, type is for the partition type, as primary, extended or logical. Something that is clearly stated in the human readable output:

Model: ATA ST2000DM001-9YN1 (scsi)
Disk /dev/sda: 2000GB
Sector size (logical/physical): 512B/4096B
Partition Table: msdos
Disk Flags: 

Number  Start   End     Size    Type     File system     Flags
 1      1049kB  2155MB  2154MB  primary  linux-swap(v1)  type=82
 2      2155MB  45.1GB  43.0GB  primary  btrfs           boot, type=83
 3      45.1GB  2000GB  1955GB  primary  xfs             type=83

Also in the original test the file system field is empty, where ext3 is naturally expected.

You can inspect the parted code that deal with this logic here:

https://github.com/Distrotech/parted/blob/distrotech-parted/parted/parted.c#L1240

I submitted a patch to parted to add the type column, but I am not sure if it will be reviewed:

https://alioth-lists.debian.net/pipermail/parted-devel/2018-September/005312.html

Note also that I only inspected the code for Debian and openSUSE, I did not check Fedora or Arch. But the tests suggest that there is no any other unkown patch attached to any distribution that add the correct partition type in the parted output.

@aplanas

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2018

Is there anything here that I can do to help the patch?

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2018

Same thing on this one - our test runner set up had some issues, so I restarted some of them here. @garethgreenaway Can you take a look here?

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2018

@aplanas Thank you for the detailed explanation. It resolves my concerns. Once @garethgreenaway reviews this, it should be fine to merge.

@aplanas

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2018

The bug is a bit deeper, as the field list depends on the partition type. For msdos partitions this order is different. I will wait the review of this one (that address the gpt case, as expressed in the tests cases) before submitting the code for the msdos case (and adding tests cases for it)

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2018

@aplanas This has been approved now. I am not sure from your comment - are you planning on making changes to this PR? Or did you want to make those changes in a follow up PR after this is merged?

@aplanas

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2018

@rallytime No this PR, as the error is for a different partition type that the one in the tests.

I have a local branch with the fix, I am writing the tests, but I think that they belong to a different PR.

Thanks!

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

@aplanas Sounds good! I'll get this merged in now. Thank you!

@rallytime rallytime merged commit 2fa64f8 into saltstack:develop Oct 23, 2018
9 of 11 checks passed
9 of 11 checks passed
continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has failed
Details
WIP ready for review
Details
codeclimate All good!
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has passed
Details
@aplanas aplanas deleted the aplanas:fix_parted_list branch Oct 26, 2018
aplanas added a commit to aplanas/salt-1 that referenced this pull request Jan 30, 2019
* blockdev: fix url from comment
saltstack/salt#49668

* Documentation: fix typo in "equivalent"
saltstack/salt#49669

* states_pt3: fix rST link format
saltstack/salt#49670

* parted: fix _validate_partition_boundary
saltstack/salt#49803

* parted: fix the ordering of list command
saltstack/salt#49804

* Fix lowpkg.diff documentation and parameter name
saltstack/salt#50126

* Add root parameter to useradd, shadow and groupadd
saltstack/salt#50175

* cmd: Add root parameter for wait and run states
saltstack/salt#50302

* systemd: add optional root parameter
saltstack/salt#50380

* service: SUSE is not based on sysvinit anymore
saltstack/salt#50396

* Add new chroot module
saltstack/salt#50418

* Add new module freezer
saltstack/salt#50452

* parted: support variable length output for print
saltstack/salt#50473

* btrfs: add all subvolume commands
saltstack/salt#50541

* file: update attributes for lsattr and chattr
saltstack/salt#50607

* btrfs: add new btrfs state
saltstack/salt#50635

* zypper: demote log from error to warning
saltstack/salt#50671

* blkid: add search by token
saltstack/salt#50706

* mount: add fstab_{present,absent} states
saltstack/salt#50725

* btrfs: add option to not set subvolumes as default
saltstack/salt#50801

* Add disk_set and disk_toggle functions, and update valid partition flags
saltstack/salt#50834

* disk: support setting FAT size for format_
saltstack/salt#51074

* cmdmod: add sysfs into the chroot
saltstack/salt#51094

* mount: cache blkid information
saltstack/salt#51135
aplanas added a commit to aplanas/salt-1 that referenced this pull request Feb 4, 2019
* blockdev: fix url from comment
saltstack/salt#49668

* Documentation: fix typo in "equivalent"
saltstack/salt#49669

* states_pt3: fix rST link format
saltstack/salt#49670

* parted: fix _validate_partition_boundary
saltstack/salt#49803

* parted: fix the ordering of list command
saltstack/salt#49804

* Fix lowpkg.diff documentation and parameter name
saltstack/salt#50126

* Add root parameter to useradd, shadow and groupadd
saltstack/salt#50175

* cmd: Add root parameter for wait and run states
saltstack/salt#50302

* systemd: add optional root parameter
saltstack/salt#50380

* service: SUSE is not based on sysvinit anymore
saltstack/salt#50396

* Add new chroot module
saltstack/salt#50418

* Add new module freezer
saltstack/salt#50452

* parted: support variable length output for print
saltstack/salt#50473

* btrfs: add all subvolume commands
saltstack/salt#50541

* file: update attributes for lsattr and chattr
saltstack/salt#50607

* btrfs: add new btrfs state
saltstack/salt#50635

* zypper: demote log from error to warning
saltstack/salt#50671

* blkid: add search by token
saltstack/salt#50706

* mount: add fstab_{present,absent} states
saltstack/salt#50725

* btrfs: add option to not set subvolumes as default
saltstack/salt#50801

* Add disk_set and disk_toggle functions, and update valid partition flags
saltstack/salt#50834

* disk: support setting FAT size for format_
saltstack/salt#51074

* cmdmod: add sysfs into the chroot
saltstack/salt#51094

* mount: cache blkid information
saltstack/salt#51135
aplanas added a commit to aplanas/salt-1 that referenced this pull request Feb 4, 2019
* blockdev: fix url from comment
saltstack/salt#49668

* Documentation: fix typo in "equivalent"
saltstack/salt#49669

* states_pt3: fix rST link format
saltstack/salt#49670

* parted: fix _validate_partition_boundary
saltstack/salt#49803

* parted: fix the ordering of list command
saltstack/salt#49804

* Fix lowpkg.diff documentation and parameter name
saltstack/salt#50126

* Add root parameter to useradd, shadow and groupadd
saltstack/salt#50175

* cmd: Add root parameter for wait and run states
saltstack/salt#50302

* systemd: add optional root parameter
saltstack/salt#50380

* service: SUSE is not based on sysvinit anymore
saltstack/salt#50396

* Add new chroot module
saltstack/salt#50418

* Add new module freezer
saltstack/salt#50452

* parted: support variable length output for print
saltstack/salt#50473

* btrfs: add all subvolume commands
saltstack/salt#50541

* file: update attributes for lsattr and chattr
saltstack/salt#50607

* btrfs: add new btrfs state
saltstack/salt#50635

* zypper: demote log from error to warning
saltstack/salt#50671

* blkid: add search by token
saltstack/salt#50706

* mount: add fstab_{present,absent} states
saltstack/salt#50725

* btrfs: add option to not set subvolumes as default
saltstack/salt#50801

* Add disk_set and disk_toggle functions, and update valid partition flags
saltstack/salt#50834

* disk: support setting FAT size for format_
saltstack/salt#51074

* cmdmod: add sysfs into the chroot
saltstack/salt#51094

* mount: cache blkid information
saltstack/salt#51135
aplanas added a commit to aplanas/salt-1 that referenced this pull request Feb 4, 2019
* blockdev: fix url from comment
saltstack/salt#49668

* Documentation: fix typo in "equivalent"
saltstack/salt#49669

* states_pt3: fix rST link format
saltstack/salt#49670

* parted: fix _validate_partition_boundary
saltstack/salt#49803

* parted: fix the ordering of list command
saltstack/salt#49804

* Fix lowpkg.diff documentation and parameter name
saltstack/salt#50126

* Add root parameter to useradd, shadow and groupadd
saltstack/salt#50175

* cmd: Add root parameter for wait and run states
saltstack/salt#50302

* systemd: add optional root parameter
saltstack/salt#50380

* service: SUSE is not based on sysvinit anymore
saltstack/salt#50396

* Add new chroot module
saltstack/salt#50418

* Add new module freezer
saltstack/salt#50452

* parted: support variable length output for print
saltstack/salt#50473

* btrfs: add all subvolume commands
saltstack/salt#50541

* file: update attributes for lsattr and chattr
saltstack/salt#50607

* btrfs: add new btrfs state
saltstack/salt#50635

* zypper: demote log from error to warning
saltstack/salt#50671

* blkid: add search by token
saltstack/salt#50706

* mount: add fstab_{present,absent} states
saltstack/salt#50725

* btrfs: add option to not set subvolumes as default
saltstack/salt#50801

* Add disk_set and disk_toggle functions, and update valid partition flags
saltstack/salt#50834

* disk: support setting FAT size for format_
saltstack/salt#51074

* cmdmod: add sysfs into the chroot
saltstack/salt#51094

* mount: cache blkid information
saltstack/salt#51135
aplanas added a commit to aplanas/salt-1 that referenced this pull request Feb 26, 2019
* blockdev: fix url from comment
saltstack/salt#49668

* Documentation: fix typo in "equivalent"
saltstack/salt#49669

* states_pt3: fix rST link format
saltstack/salt#49670

* parted: fix _validate_partition_boundary
saltstack/salt#49803

* parted: fix the ordering of list command
saltstack/salt#49804

* Fix lowpkg.diff documentation and parameter name
saltstack/salt#50126

* Add root parameter to useradd, shadow and groupadd
saltstack/salt#50175

* cmd: Add root parameter for wait and run states
saltstack/salt#50302

* systemd: add optional root parameter
saltstack/salt#50380

* service: SUSE is not based on sysvinit anymore
saltstack/salt#50396

* Add new chroot module
saltstack/salt#50418

* Add new module freezer
saltstack/salt#50452

* parted: support variable length output for print
saltstack/salt#50473

* btrfs: add all subvolume commands
saltstack/salt#50541

* file: update attributes for lsattr and chattr
saltstack/salt#50607

* btrfs: add new btrfs state
saltstack/salt#50635

* zypper: demote log from error to warning
saltstack/salt#50671

* blkid: add search by token
saltstack/salt#50706

* mount: add fstab_{present,absent} states
saltstack/salt#50725

* btrfs: add option to not set subvolumes as default
saltstack/salt#50801

* Add disk_set and disk_toggle functions, and update valid partition flags
saltstack/salt#50834

* disk: support setting FAT size for format_
saltstack/salt#51074

* cmdmod: add sysfs into the chroot
saltstack/salt#51094

* mount: cache blkid information
saltstack/salt#51135
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.