Drop lsblk in favour of ghw library #155

Itxaka · 2022-03-20T15:05:38Z

ghw uses udev database and files under /sys and /proc to gather all
info
This uses that exclusively so no more lsblk
Probably means that the partition/disk identification utils can now
without root privs
Adds a new mock to mock a full disk(s) with partition(s) for tests
only
Makes tests use the new mock to mock the lsblk calls
Fixes a couple of leftovers on dir permissions

Unofrtunately this needs my own fork of ghw as upstream is missing a
patch to get the labels from the udev database, but it should be merged
upstream soon.

It also means that when running elemental from docker we need to link
the /run/udev/data volume, but this can be done on RO as we only read,
which would be a bit nicer. Commadn will look uglier thougth, but as
long as we explain why it needs that volume, it should all be good.

Signed-off-by: Itxaka [email protected]

codecov · 2022-03-21T08:51:19Z

Codecov Report

Merging #155 (70bbf9e) into main (1b81b18) will decrease coverage by 0.88%.
The diff coverage is 78.26%.

@@            Coverage Diff             @@
##             main     #155      +/-   ##
==========================================
- Coverage   75.66%   74.77%   -0.89%     
==========================================
  Files          40       40              
  Lines        3291     3271      -20     
==========================================
- Hits         2490     2446      -44     
- Misses        649      667      +18     
- Partials      152      158       +6

Impacted Files	Coverage Δ
pkg/partitioner/disk.go	`56.80% <40.00%> (-0.74%)`	⬇️
pkg/utils/getpartitions.go	`82.05% <82.05%> (ø)`
pkg/utils/common.go	`88.82% <100.00%> (-2.04%)`	⬇️
pkg/utils/runstage.go	`83.52% <100.00%> (ø)`
pkg/action/reset.go	`72.13% <0.00%> (-3.28%)`	⬇️
pkg/elemental/elemental.go	`80.09% <0.00%> (-1.25%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b81b18...70bbf9e. Read the comment docs.

davidcassany

Looks good to me. Just a couple of nits

If we are changing from GetAllPartitions(runner v1.Runner) to GetAllPartitionsV2(), probably without keeping the former method, probably we can simply avoid the V2 into the method name. Looks like there could be an alternate version of the method. Also, the pkg/utils/lsbk.go file I think it should be renamed to something else if it is not wrapping lsblk.

Beyond this two naming details it all looks good to me. 👍 I think this is a good move. And as discussed already, using the installer within a container will always be tricky or require specific uncommon volumes and privilages, it should not be the preferred way to use elemental.

Itxaka · 2022-03-21T09:23:49Z

yup, the naming and old functions are there 'cause...I was testing them side by side :D

In practice this was gonna be a work on top of the existing stuff to be substituted, but I saw it was only called in a few places, tried changing those and fixing the tests and suddenly those functions are not called anymore :o did not expect it to be this straightforward.

Itxaka · 2022-03-21T09:26:04Z

oh yeah what about the ghw Partition type VS our Partition type? We could drop our partition as they are identical but relaying on a external type....not good if they change it, but I would like to avoid doing the absurd transformation from one struct to the other which IMHO is wasted cycles.

Itxaka · 2022-03-21T09:37:01Z

I changed it to return our type, we can work out if we want to drop the transformation afterwards

davidcassany · 2022-03-21T09:38:43Z

oh yeah what about the ghw Partition type VS our Partition type? We could drop our partition as they are identical but relaying on a external type....not good if they change it, but I would like to avoid doing the absurd transformation from one struct to the other which IMHO is wasted cycles.

I'd leave it for another PR and probably after a while. The good thing of this the current approach is that ghw is only imported within the utils wrapper and in tests. v1.Partitions is used in several places, so if we encounter some issue with ghw and consider going back to something else the changes are mostly affecting tests, the elemental logic is pretty isolated from it.

kkaempf · 2022-03-23T08:53:57Z

Not sure about the long-term implications of this:

lsblk is part of the core operating system
there's someone to blame if lsblk has problems 😉
ghw increases our external dependencies
problems in ghw are on us to fix

mudler · 2022-03-23T09:03:16Z

Not sure about the long-term implications of this:

* `lsblk` is part of the core operating system

* there's someone to blame if lsblk has problems wink

* `ghw` increases our external dependencies

* problems in `ghw` are _on us_ to fix

Let's not confuse blkid lsblk and ghw. ghw is a library for hardware detection, and for partitions and such it uses also blkid behind the scene, among other tools (@Itxaka also improved it to use udev!).

This is about avoiding us to re-invent the wheel and write from scratch a detecting library, with all the issues that it might bring.. it makes sense instead to contribute to an open source one that we just adopt.

Itxaka · 2022-03-23T13:06:44Z

lsblk is part of the core operating system

Not on cos, only 'cause we need it for elemental :P

there's someone to blame if lsblk has problems wink

well...same? we blame ghw :P

ghw increases our external dependencies

Not really, its compiled into a final binary babyyyyy. ghw is really ligth and we only use a small part of it, not sure why the problem with this and not the other 300 dep that we have 🤣

problems in ghw are on us to fix

Good. I like that, cause I already fixed issues on it and expanded. Good luck getting me to fix something on lsblk 🤣

Itxaka · 2022-03-30T08:30:21Z

upstream PR missing jaypipes/ghw#310

- ghw uses udev database and files under /sys and /proc to gather all info - This uses that exclusively so no more lsblk - Probably means that the partition/disk identification utils can now without root privs - Adds a new mock to mock a full disk(s) with partition(s) for tests only - Makes tests use the new mock to mock the lsblk calls - Fixes a couple of leftovers on dir permissions Unofrtunately this needs my own fork of ghw as upstream is missing a patch to get the labels from the udev database, but it should be merged upstream soon. It also means that when running elemental from docker we need to link the /run/udev/data volume, but this can be done on RO as we only read, which would be a bit nicer. Commadn will look uglier thougth, but as long as we explain why it needs that volume, it should all be good. Signed-off-by: Itxaka <[email protected]>

Also drop a method not used anywhere, return a v1.Partition by default Signed-off-by: Itxaka <[email protected]>

Signed-off-by: Itxaka <[email protected]>

Itxaka · 2022-04-04T15:21:36Z

upstream patch merged, this should be ready tomorrow!

Signed-off-by: Itxaka <[email protected]>

davidcassany

LGTM 👍

mudler · 2022-04-05T09:34:07Z

pkg/action/action_test.go

+					},
+				},
+			}
+			ghwTest = v1mock.GhwMock{}


mudler

Looks good!

Itxaka requested review from davidcassany and mudler March 21, 2022 09:02

davidcassany approved these changes Mar 21, 2022

View reviewed changes

Itxaka added 3 commits March 30, 2022 10:31

Rename lsblk to getpartitions and fix func naming

5f7a844

Also drop a method not used anywhere, return a v1.Partition by default Signed-off-by: Itxaka <[email protected]>

Adapt partitioner test

66b6b1f

Signed-off-by: Itxaka <[email protected]>

Itxaka self-assigned this Mar 30, 2022

Bump to use upstream ghw

70bbf9e

Signed-off-by: Itxaka <[email protected]>

Itxaka marked this pull request as ready for review April 4, 2022 19:16

davidcassany approved these changes Apr 5, 2022

View reviewed changes

mudler reviewed Apr 5, 2022

View reviewed changes

pkg/action/action_test.go

},

},

}

ghwTest = v1mock.GhwMock{}

Copy link

Contributor

mudler Apr 5, 2022

Choose a reason for hiding this comment

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

❤️

mudler approved these changes Apr 5, 2022

View reviewed changes

Itxaka merged commit 89d323a into rancher:main Apr 5, 2022

Itxaka deleted the use_ghw branch April 5, 2022 10:13

davidcassany mentioned this pull request Jul 21, 2022

Create a device parsing library/package #58

Closed

mjura pushed a commit to mjura/elemental-cli that referenced this pull request Oct 18, 2022

Set proper container repo for testing (rancher#155)

b7263a9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop lsblk in favour of ghw library #155

Drop lsblk in favour of ghw library #155

Itxaka commented Mar 20, 2022

codecov bot commented Mar 21, 2022 •

edited

Loading

davidcassany left a comment

Itxaka commented Mar 21, 2022

Itxaka commented Mar 21, 2022

Itxaka commented Mar 21, 2022

davidcassany commented Mar 21, 2022

kkaempf commented Mar 23, 2022

mudler commented Mar 23, 2022 •

edited

Loading

Itxaka commented Mar 23, 2022

Itxaka commented Mar 30, 2022

Itxaka commented Apr 4, 2022

davidcassany left a comment

mudler Apr 5, 2022

mudler left a comment

Drop lsblk in favour of ghw library #155

Drop lsblk in favour of ghw library #155

Conversation

Itxaka commented Mar 20, 2022

codecov bot commented Mar 21, 2022 • edited Loading

Codecov Report

davidcassany left a comment

Choose a reason for hiding this comment

Itxaka commented Mar 21, 2022

Itxaka commented Mar 21, 2022

Itxaka commented Mar 21, 2022

davidcassany commented Mar 21, 2022

kkaempf commented Mar 23, 2022

mudler commented Mar 23, 2022 • edited Loading

Itxaka commented Mar 23, 2022

Itxaka commented Mar 30, 2022

Itxaka commented Apr 4, 2022

davidcassany left a comment

Choose a reason for hiding this comment

mudler Apr 5, 2022

Choose a reason for hiding this comment

mudler left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 21, 2022 •

edited

Loading

mudler commented Mar 23, 2022 •

edited

Loading