Fix getting lvm recovery #70
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Itxaka <[email protected]>
This reverts commit f225315.
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## main #70 +/- ##
==========================================
- Coverage 57.42% 56.48% -0.95%
==========================================
Files 40 40
Lines 4792 4835 +43
==========================================
- Hits 2752 2731 -21
- Misses 1823 1888 +65
+ Partials 217 216 -1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
mudler
approved these changes
Jun 23, 2023
|
Im worried about the disk entry as we cannot infer from udev what disk this comes from and @jimmykarily mentioned that it was important :/ |
|
New fix for it to get the disk, only its missing is the size and mountpoint: with OEM as lvm for testing: |
|
|
jimmykarily
reviewed
Jun 23, 2023
pkg/elemental/elemental.go
Outdated
| @@ -143,7 +143,7 @@ func (e Elemental) UnmountPartitions(parts v1.PartitionList) error { | |||
| if part.MountPoint != "" { | |||
| err = e.UnmountPartition(part) | |||
| if err != nil { | |||
| errMsg += fmt.Sprintf("Failed to unmount %s\n Error: %s\n", part.MountPoint, err.Error()) | |||
| errMsg += fmt.Sprintf("Failed to unmount %s\n", part.MountPoint) | |||
There was a problem hiding this comment.
Let's keep this change
|
Good catch! I'm sorry I missed this, it would have saved us quite some time... |
Oh crap, yes! Reworking it... |
Signed-off-by: Itxaka <[email protected]>
|
now it properly gets the size and parent disk |
Signed-off-by: Itxaka <[email protected]>
|
Now its probably missing the mountpoints 😭 |
jimmykarily
reviewed
Jun 23, 2023
Comment on lines
+167
to
+172
| if strings.HasPrefix(udevLine, "E:") { | ||
| if s := strings.SplitN(udevLine[2:], "=", 2); len(s) == 2 { | ||
| udevInfo[s[0]] = s[1] | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
Are there any docs on what E: prefix is? Not super important but it would make this code a bit less magical.
There was a problem hiding this comment.
device property!
see table 1 on https://man7.org/linux/man-pages/man8/udevadm.8.html
There was a problem hiding this comment.
This is mainly because we are accessing the databsae directly without using udevadm and you can use udevadm to only show some types of values, like only device paths or symlinks. But we dont have udevadm in our systems so we need to filter them ourselves.
jimmykarily
approved these changes
Jun 23, 2023
Signed-off-by: Itxaka <[email protected]>
|
I compiled a kairos-agent binary out of this branch, booted into recovery on a rasberry pi (with lvm partitions), I tried to simply call This is what lsblk shows: For some reason, it doesn't return the lvm partitions |
|
Never mind. |
This was referenced Jun 23, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We already have an function that will use ghw+udev data to identify partition which are under a devicemapper, so we can use that to gather the info about the lvm partitions already without adding more complexity
This also reverts the lsblk introduction