Skip to content

Conversation

@Prashanth684
Copy link
Contributor

This adds support for running pxe-install test on aarch64. In the process of doing this , I discovered
that boot once is not supported and the boot order had to be changed after installation (unlike s390x where
the disk is always checked first). This got me back to using QMP again and it works pretty well and is pretty
convenient to change the boot order of a running system

One follow up would be to try to use QMP in general for injecting bootindex for pxe and iso test cases and do
away with adding the primary disk for testiso altogether.

This adds support for running pxe-install test on aarch64. In the process of doing this , I discovered
that boot once is not supported and the boot order had to be changed after installation (unlike s390x where
the disk is always checked first). This got me back to using QMP again and it works pretty well and is pretty
convenient to change the boot order of a running system

One follow up would be to try to use QMP in general for injecting bootindex for pxe and iso test cases and do
away with adding the primary disk for testiso altogether.
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Nice, this looks really great! Using QMP will help us with other things in the future too.

return
}
// switch the boot order here, we are well into the installation process. Only used for PXE test now
if line == liveOKSignal {
Copy link
Member

Choose a reason for hiding this comment

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

This is completely fine for now though I think the functionality would better live in metal.go - that's intending to be the abstraction over performing an install, whereas testiso.go is just about testing.

Concretely eventually we want kola run -p pxe-install podman.* and cosa run -p pxe-install which would run tests/give you ssh after doing a PXE install, rather than how testiso.go is hardcoded to just a sanity check.

Fine for now though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i agree. I was going to do some refactoring around this and try to move everything to metal. But - for this particular case, is there a better place in metal.go to figure out that we have started with the installation process and can switch the boot order?

Copy link
Member

Choose a reason for hiding this comment

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

It's totally fine as is, feel free to just add a comment for this and we can do it later. Given the size of change here I'd prefer to get it in than force you into rebasing later etc.

But if you want to take a quick shot at it I think what metal.go should do is inject its own Ignition to write to a separate virtio channel, distinct from what testiso.go is doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah..ok got it ..yes..i'll try to tackle that separately :) i'll stick with these changes for now and probably move some of the QMP bits to its own package.

@cgwalters
Copy link
Member

Since this is a big PR leaving open for a bit for others to review!

@Prashanth684 Prashanth684 changed the title testiso: Support pxe-install test on aarch64 WIP: testiso: Support pxe-install test on aarch64 Sep 4, 2020
Copy link
Contributor

@darkmuggle darkmuggle left a comment

Choose a reason for hiding this comment

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

Other than the current conversation, this looks fine to me.
/approve

Happy to relook after the WIP is lifted.

move the qmp related methods to a separate file to make it cleaner
@Prashanth684 Prashanth684 changed the title WIP: testiso: Support pxe-install test on aarch64 testiso: Support pxe-install test on aarch64 Sep 4, 2020
@Prashanth684
Copy link
Contributor Author

Lifting WIP after some changes and sufficient testing.

@Prashanth684
Copy link
Contributor Author

@darkmuggle , anybody - could i get an lgtm ?

@jlebon
Copy link
Member

jlebon commented Sep 8, 2020

Very cool stuff!
/approve

This had two approvals already, so
/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, darkmuggle, jlebon, Prashanth684

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [cgwalters,darkmuggle,jlebon]

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

@openshift-merge-robot openshift-merge-robot merged commit 0f6f651 into coreos:master Sep 8, 2020
Prashanth684 added a commit to Prashanth684/coreos-assembler that referenced this pull request Sep 9, 2020
As a follow up to coreos#1700 also support iso tests on aarch64
Prashanth684 added a commit to Prashanth684/coreos-assembler that referenced this pull request Sep 9, 2020
As a follow up to coreos#1700 also support iso tests on aarch64
openshift-merge-robot pushed a commit that referenced this pull request Sep 9, 2020
As a follow up to #1700 also support iso tests on aarch64
Prashanth684 added a commit to Prashanth684/coreos-assembler that referenced this pull request Oct 8, 2020
Following up on comment in coreos#1700 to split the functionality to trigger the boot order
switch to metal.go making it generic.
openshift-merge-robot pushed a commit that referenced this pull request Nov 10, 2020
Following up on comment in #1700 to split the functionality to trigger the boot order
switch to metal.go making it generic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants