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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃尡 Test virtualmedia booting #1520

Merged
merged 1 commit into from Apr 3, 2024

Conversation

lentzi90
Copy link
Member

What this PR does / why we need it:

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 #1512

@metal3-io-bot metal3-io-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 18, 2024
@lentzi90
Copy link
Member Author

/test metal3-bmo-e2e-test-pull

1 similar comment
@lentzi90
Copy link
Member Author

/test metal3-bmo-e2e-test-pull

@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2024
@lentzi90 lentzi90 force-pushed the lentzi90/bmo-e2e-virtualmedia branch from 930a313 to 2bed845 Compare January 19, 2024 10:02
@metal3-io-bot metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2024
@tuminoid
Copy link
Member

/metal3-bmo-e2e-test-pull
I wonder if this works as people are trying to use it this way for sure....

@tuminoid
Copy link
Member

/test metal3-bmo-e2e-test-pull
Now this should work?

@lentzi90
Copy link
Member Author

Prow should also be able to help with commands...
/test ?

@metal3-io-bot
Copy link
Contributor

@lentzi90: The following commands are available to trigger required jobs:

  • /test generate
  • /test gofmt
  • /test golint
  • /test gomod
  • /test gosec
  • /test manifestlint
  • /test markdownlint
  • /test metal3-bmo-e2e-test-pull
  • /test shellcheck
  • /test unit

The following commands are available to trigger optional jobs:

  • /test metal3-bmo-e2e-test-optional-pull

Use /test all to run the following jobs that were automatically triggered:

  • generate
  • gofmt
  • golint
  • gomod
  • gosec
  • manifestlint
  • shellcheck
  • unit

In response to this:

Prow should also be able to help with commands...
/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2024
@lentzi90 lentzi90 force-pushed the lentzi90/bmo-e2e-virtualmedia branch from 2bed845 to df39795 Compare February 1, 2024 13:08
@metal3-io-bot metal3-io-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 1, 2024
@lentzi90
Copy link
Member Author

lentzi90 commented Feb 1, 2024

/test metal3-bmo-e2e-test-pull

2 similar comments
@lentzi90
Copy link
Member Author

lentzi90 commented Feb 1, 2024

/test metal3-bmo-e2e-test-pull

@lentzi90
Copy link
Member Author

lentzi90 commented Feb 1, 2024

/test metal3-bmo-e2e-test-pull

@lentzi90 lentzi90 force-pushed the lentzi90/bmo-e2e-virtualmedia branch from f3d72d8 to 353da27 Compare February 5, 2024 11:13
@lentzi90
Copy link
Member Author

lentzi90 commented Feb 5, 2024

/test metal3-bmo-e2e-test-pull

1 similar comment
@mquhuy
Copy link
Member

mquhuy commented Mar 4, 2024

/test metal3-bmo-e2e-test-pull

@lentzi90 lentzi90 force-pushed the lentzi90/bmo-e2e-virtualmedia branch from 353da27 to 4001d2f Compare March 6, 2024 14:55
@metal3-io-bot metal3-io-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 6, 2024
@lentzi90 lentzi90 force-pushed the lentzi90/bmo-e2e-virtualmedia branch from 4001d2f to 958f216 Compare March 7, 2024 07:24
@lentzi90
Copy link
Member Author

lentzi90 commented Mar 7, 2024

/test metal3-bmo-e2e-test-pull

@lentzi90 lentzi90 force-pushed the lentzi90/bmo-e2e-virtualmedia branch 2 times, most recently from 40b5c90 to 6bf0ac2 Compare March 7, 2024 12:10
@metal3-io-bot metal3-io-bot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 7, 2024
@metal3-io-bot metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 7, 2024
@lentzi90
Copy link
Member Author

lentzi90 commented Mar 7, 2024

/test metal3-bmo-e2e-test-pull

@lentzi90 lentzi90 changed the title WIP 馃尡 Test virtualmedia booting 馃尡 Test virtualmedia booting Mar 7, 2024
@metal3-io-bot metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 7, 2024
// See https://docs.openstack.org/ironic/latest/admin/ramdisk-boot.html
accessDetails, err := metal3bmc.NewAccessDetails(bmc.Address, false)
Expect(err).NotTo(HaveOccurred())
if !accessDetails.SupportsISOPreprovisioningImage() {
Copy link
Member Author

@lentzi90 lentzi90 Mar 7, 2024

Choose a reason for hiding this comment

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

I could use a second opinion here. Does it make sense to skip the test for ipmi and redfish? Is this the right thing to check?
Based on the docs I linked in the comment above, I think it isn't really worth it to test live-ISO in non-virtualmedia cases and the test currently fails for them. This is why I added the skip.

The test as it was before my changes here, booted a qcow2 image and set the disk format to live-ISO. I think that means it did a PXE boot and loaded it to RAM but I'm not completely sure.

Copy link
Member

Choose a reason for hiding this comment

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

Technically, you can boot an ISO via iPXE. We added this to BMO purely accidentally, but when a validation was added that blocked it, some customers complained. Apparently, people use this in production.

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 would you say we need to test it? It seems we would need some special ISO to get something useful with iPXE.

This feature, when utilized with the ipxe boot_interface, will only allow a kernel and ramdisk to be booted from the supplied ISO file. Any additional contents, such as additional ramdisk contents or installer package files will be unavailable after the boot of the Operating System. Operators wishing to leverage this functionality for actions such as OS installation should explore use of the standard ramdisk deploy_interface along with the instance_info/kernel_append_params setting to pass arbitrary settings such as a mirror URL for the initial ramdisk to load data from. This is a limitation of iPXE and the overall boot process of the operating system where memory allocated by iPXE is released.

Copy link
Member

Choose a reason for hiding this comment

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

IMHO it's probably not worth testing it at this stage
we could add a TODO for the future as a reminder in case this become more impactful, but I doubt it

Comment on lines +122 to +132
# Build an ISO image with baked ssh key
# See https://www.system-rescue.org/scripts/sysrescue-customize/
# We use the systemrescue ISO and their script for customizing it.
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'm open to suggestions here. Should we build the ISO in a different way? Use a different image to start from?

Copy link
Member

Choose a reason for hiding this comment

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

In the Ironic CI, we use http://tinycorelinux.net/15.x/x86/release/Core-current.iso, but I don't know how to embed the SSH key there. If sysrescue does not take ages to build, it could be an option.

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 guess we could use that but without the ssh check. I didn't find an easy way to add the ssh key to it.
For now I think system-rescue will do fine and we do get a proper check that the live-ISO booted this way.

@lentzi90
Copy link
Member Author

lentzi90 commented Mar 7, 2024

/cc @dtantsur @Rozzii
I would appreciate your input on this. Especially if you think it makes sense to skip the test for ipmi and redfish as I do now.
My plan for virtualmedia going forward would be that the e2e tests run with redfish and redfish-virtualmedia on all PRs. Periodic tests would also run ipmi with VBMC so we test a different protocol.

@@ -68,8 +80,9 @@ var _ = Describe("Live-ISO", Label("required", "live-iso"), func() {
URL: imageURL,
DiskFormat: pointer.String("live-iso"),
},
BootMode: metal3api.Legacy,
BootMACAddress: bmc.BootMacAddress,
BootMode: metal3api.Legacy,
Copy link
Member

Choose a reason for hiding this comment

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

Sigh.

Copy link
Member Author

Choose a reason for hiding this comment

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

One thing at a time. I have added #1607 for it 馃槈

@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2024
@lentzi90 lentzi90 force-pushed the lentzi90/bmo-e2e-virtualmedia branch from 6bf0ac2 to 7b01349 Compare March 27, 2024 11:32
@metal3-io-bot metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2024
@lentzi90
Copy link
Member Author

/test metal3-bmo-e2e-test-pull

1 similar comment
@lentzi90
Copy link
Member Author

/test metal3-bmo-e2e-test-pull

This makes the live-ISO test use a separate image so we can use an
actual ISO. Also rename the BMCs config files to have the protocol
instead of the emulator in the name and added a third file for
redfish-virtualmedia.

The live-ISO test will be skipped if booting with (i)PXE since it would
require a special image and configuration to verify.

Signed-off-by: Lennart Jern <lennart.jern@est.tech>
@lentzi90 lentzi90 force-pushed the lentzi90/bmo-e2e-virtualmedia branch from 7b01349 to 81a5463 Compare March 28, 2024 08:38
@lentzi90
Copy link
Member Author

/test metal3-bmo-e2e-test-pull

@lentzi90
Copy link
Member Author

/test-centos-e2e-integration-main

@tuminoid
Copy link
Member

Good work! Not having been involved enough in BMO testing, someone else can flag it. LGTM.

@lentzi90 lentzi90 requested a review from dtantsur March 28, 2024 13:46
Copy link
Member

@elfosardo elfosardo left a comment

Choose a reason for hiding this comment

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

/lgtm

# See https://www.system-rescue.org/scripts/sysrescue-customize/
# We use the systemrescue ISO and their script for customizing it.
pushd "${IMAGE_DIR}"
wget -O sysrescue-customize https://gitlab.com/systemrescue/systemrescue-sources/-/raw/main/airootfs/usr/share/sysrescue/bin/sysrescue-customize?inline=false
Copy link
Member

Choose a reason for hiding this comment

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

nit: systemrescue-customize can probably be converted to a variable as it's used in more than a couple of places

// See https://docs.openstack.org/ironic/latest/admin/ramdisk-boot.html
accessDetails, err := metal3bmc.NewAccessDetails(bmc.Address, false)
Expect(err).NotTo(HaveOccurred())
if !accessDetails.SupportsISOPreprovisioningImage() {
Copy link
Member

Choose a reason for hiding this comment

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

IMHO it's probably not worth testing it at this stage
we could add a TODO for the future as a reminder in case this become more impactful, but I doubt it

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2024
@lentzi90
Copy link
Member Author

lentzi90 commented Apr 3, 2024

/cc @kashifest

@kashifest
Copy link
Member

/approve

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kashifest

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 3, 2024
@metal3-io-bot metal3-io-bot merged commit f8a2390 into metal3-io:main Apr 3, 2024
18 checks passed
@metal3-io-bot metal3-io-bot deleted the lentzi90/bmo-e2e-virtualmedia branch April 3, 2024 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E2E: Test virtualmedia boot
7 participants