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

Test selecting disk by uuid+label #2877

Merged
merged 13 commits into from
Sep 25, 2024
Merged

Test selecting disk by uuid+label #2877

merged 13 commits into from
Sep 25, 2024

Conversation

Itxaka
Copy link
Member

@Itxaka Itxaka commented Sep 23, 2024

What this PR does / why we need it:
Adds tests for the new functionality at kairos-io/kairos-agent#552

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1879

Will get green once the PR is merged, new version released and new framework created and added as part of this PR

@Itxaka Itxaka changed the title [WIP] Test selecting disk by uuid [WIP] Test selecting disk by uuid+label Sep 23, 2024
@Itxaka
Copy link
Member Author

Itxaka commented Sep 23, 2024

It passed when building with the new branch that brings this change, now that its removed, the test should fail

Signed-off-by: Itxaka <[email protected]>
Signed-off-by: Itxaka <[email protected]>
@Itxaka Itxaka requested a review from a team September 23, 2024 15:11
@Itxaka Itxaka changed the title [WIP] Test selecting disk by uuid+label Test selecting disk by uuid+label Sep 23, 2024
@Itxaka Itxaka marked this pull request as ready for review September 23, 2024 15:12
@Itxaka
Copy link
Member Author

Itxaka commented Sep 24, 2024

now its pointing to master framework, so it should pass

EDIT: Now it has the final fix! should pass?

stateContains(vm, "system.os.name", "alpine", "opensuse", "ubuntu", "debian")
stateContains(vm, "kairos.flavor", "alpine", "opensuse", "ubuntu", "debian")
})
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any check that the correct disk was actually used. We only check that the command didn't fail and that installation occurred. I guess, since we only have one disk, it was indeed selected but would be better if we had 2 disks and we made sure the correct one was selected, to avoid for example the flag being completely ignored and the default disk (bigger one?) gets selected instead.

But it can be tricky:

It("installs on the pre-configured disks", func() {
Expect(installError).ToNot(HaveOccurred(), installationOutput)
By("Rebooting into live CD again")
// In qemu it's tricky to boot the second disk. In multiple disk scenarios,
// setting "-boot=cd" will make qemu try to boot from the first disk and
// then from the cdrom.
// We want to make sure that kairos-agent selected the second disk so we
// simply let it boot from the cdrom again. Hopefully if the installation
// failed, we would see the error from the manual-install command.

I don't have a strong opinion, I'll let you decide.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes indeed, that's a good point. I am letting it work as is because if you pass a wrong or nonexistent disk, the install fails but I'll have a look at peg to see if we can pass multiple disks to the VM, that would make it more robust

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I introduced it for this exact reason. It's the boot order that is hard to do in qemu I think (see the link above).

Copy link
Member Author

Choose a reason for hiding this comment

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

but boot order should not matter no? It will try first all the disks and then livecd? Or it just tries the first disk only?

Copy link
Contributor

@jimmykarily jimmykarily Sep 25, 2024

Choose a reason for hiding this comment

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

I don't recall to be honest. Maybe back then I was no aware of how to setup boot order using bootindex in qemu. We need to look at it again.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are rigth, it doesnt try to boot from the second one somehow 😭

Copy link
Member Author

Choose a reason for hiding this comment

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

I could not make it work, maybe we should introduce a config in peg to choose the boot device or something, would be difficult to have it working for all, but at least we could make it work for qemu

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can add a default bootindex to all disks in qemu in peg, as in: first check disks (/dev/vda, /dev/vdb, etc) then cdrom.
This should allow us to try to target the second disk and see if it worked (since vda will be skipped as not being bootable).

@Itxaka Itxaka requested review from jimmykarily and a team September 25, 2024 14:30
@Itxaka
Copy link
Member Author

Itxaka commented Sep 25, 2024

Now passing :D

Expect(err).ToNot(HaveOccurred())

vm.EventuallyConnects(1200)
// Format the second disk so it gets an uuid and label
Copy link
Contributor

Choose a reason for hiding this comment

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

/dev/vda would be the "first" disk no?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed!

Signed-off-by: Itxaka <[email protected]>
@Itxaka
Copy link
Member Author

Itxaka commented Sep 25, 2024

rebase and merge once #2891 gets in with the new framework already

@Itxaka Itxaka merged commit c4e8b16 into master Sep 25, 2024
8 of 16 checks passed
@Itxaka Itxaka deleted the test_uuid_disk branch September 25, 2024 18:46
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.

Failure to install to /dev/disk/by-id/<id>
2 participants