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

openstack-crowbar: Add pci passthough option #3543

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kwu83tw
Copy link
Contributor

@kwu83tw kwu83tw commented Jul 10, 2019

Add Jenkins parameter 'want_pci_passthrough' to allow running tests to validate pci passthough.

  1. add environment prep and verify function in qa_crowbarsetup.sh
  2. create ci job template

@kwu83tw kwu83tw force-pushed the pci_test branch 10 times, most recently from 988003d to c093bf8 Compare July 18, 2019 17:00
@aspiers aspiers self-requested a review July 19, 2019 11:51
@kwu83tw kwu83tw force-pushed the pci_test branch 2 times, most recently from cff77af to 1c5194a Compare July 20, 2019 00:36
@kwu83tw kwu83tw requested review from toabctl and jdsn July 22, 2019 21:20
@kwu83tw kwu83tw force-pushed the pci_test branch 2 times, most recently from c229220 to f0c0880 Compare July 23, 2019 07:58
@@ -401,6 +401,11 @@
default: '1'
description: set to 0 to not deploy aodh

- string:
name: want_pci_passthrough
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is used seldomly enough, that you can drop this and use custom_settings='export want_pci_passthrough=1' in the template instead.

Copy link
Contributor Author

@kwu83tw kwu83tw Jul 23, 2019

Choose a reason for hiding this comment

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

Sure. I thought it's required to define in here in order to be consumed in this proposed ci job template. Did I miss anything?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to use its own argument for that, like the PR currently does, you need to define it. If you are fine with passing it through the custom_settings argument then you don't.

@kwu83tw kwu83tw force-pushed the pci_test branch 3 times, most recently from 6871efd to 0d51b7b Compare July 23, 2019 20:18
@kwu83tw kwu83tw force-pushed the pci_test branch 5 times, most recently from 94d3422 to 3f2ca8d Compare July 26, 2019 17:34
Copy link
Contributor

@aspiers aspiers left a comment

Choose a reason for hiding this comment

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

Almost perfect now :)

scripts/qa_crowbarsetup.sh Outdated Show resolved Hide resolved
@kwu83tw kwu83tw force-pushed the pci_test branch 2 times, most recently from 642ea13 to 0e28196 Compare August 2, 2019 05:02
@kwu83tw kwu83tw requested a review from aspiers August 2, 2019 05:05
aspiers
aspiers previously approved these changes Aug 2, 2019
return 0
}

function oncontroller_retrieve_image
Copy link
Contributor

Choose a reason for hiding this comment

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

You're duplicating functionality already available in qa_crowbarsetup.sh. Please try to reuse the image used for testvm that is being set up here.

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 was trying to reuse the testvm generated from testsetup step for past few days. However, I run into situation that the testvm won't boot up properly if the device passing from compute is attached (the specific flavor which has pci passthru attribute set). The workaround will be tweaking it to unplug/replug the device during post boot up phase. So I go back to my original formula instead of touching too many parts in oncontroller_testsetup.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand your pain, but we need to think about this from a customer perspective. We should treat this like a bug that needs fixing instead of trying to work around it in our CI by using a different image (Cirros instead of SLES) ? However, I also understand that we don't really test the PCI passthrough feature in a manner that's relevant to our customers, so I'm a bit conflicted about this.

Can you give me more details about why the JeOS image won't boot with a second volume attached ? Is it because it tries to boot from the PCI passthrough device instead of the ephemeral volume set up by OpenStack ? If it's something we can fix easily through cloud-init, we should definitely try that. If not, I agree that we should use your original approach.

fi
}

function oncontroller_upload_image_to_glance
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, please reuse the image upload logic used for testvm available here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

fi
}

function oncontroller_setup_pci_passthru_vm
Copy link
Contributor

Choose a reason for hiding this comment

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

qa_crowbarsetup.sh already has some test automation logic that creates a testvm virtual machine, pings/ssh-es it and so on. You may need to create and use a PCI passthrough enabled flavor, but otherwise please try to reuse the testvm test automation logic here.

Copy link
Contributor Author

@kwu83tw kwu83tw Aug 13, 2019

Choose a reason for hiding this comment

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

Unfortunately, I'm not able to reuse testvm since I need some operations right after booting up VM.

fi
}

function append_pci_passthru_filter
Copy link
Contributor

Choose a reason for hiding this comment

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

This function goes to a lot of trouble to just set one parameter in the nova proposal. Instead, why not add that parameter from the very beginning, when the proposal gets first created here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's part of the acceptance criteria for this Jira which is not introducing any change in Nova proposal for testing pci passthrough but sneak in the configuration when it is needed.

@kwu83tw kwu83tw force-pushed the pci_test branch 11 times, most recently from c6c13f0 to c9c7f4e Compare August 9, 2019 06:35
@kwu83tw kwu83tw force-pushed the pci_test branch 3 times, most recently from 8b5be41 to 3ab9900 Compare August 13, 2019 01:24
@kwu83tw kwu83tw requested a review from guangyee August 14, 2019 16:05
Add Jenkins parameter 'want_pci_passthrough' to allow running tests to validate
pci passthough.

To test pci passthrough locally with mkcloud, simply just export
'want_pci_passthrouh=1' and append 'rebootcloud testpcipassthru' to 'plain'
step, e.g. your_mkcloud.sh plain rebootcloud testpcipassthru

The rebootcloud step will ensure intel_iommu=on kernel param enabled.

The testpcipassthru step will prepare the environment and spin up a nested vm
(L2) on compute (L1) with a virtio disk passing from compute vm (L1). Noted
that this is not passing any device from the host (L0) itself.

There will be another PR to construct job definition and other prep works for
new ci.

1. add environment prep and verify function in qa_crowbarsetup.sh
2. create legacy ci job template
3. introduce testpcipassthru step in mkcloud
@toabctl toabctl removed their request for review July 14, 2020 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants