Skip to content

implement e2e tests for FPGA plugin #359

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 14, 2020
Merged

implement e2e tests for FPGA plugin #359

merged 2 commits into from
Apr 14, 2020

Conversation

bart0sh
Copy link
Member

@bart0sh bart0sh commented Apr 7, 2020

implemented e2e tests for preprogrammed(af) and
orchestrated(region) plugin modes.

Fixes: #350

@bart0sh bart0sh requested review from kad and mythi as code owners April 7, 2020 14:37
@bart0sh bart0sh requested a review from rojkov April 7, 2020 14:37
mythi
mythi reviewed Apr 8, 2020
View reviewed changes
rojkov
rojkov reviewed Apr 8, 2020
View reviewed changes
@bart0sh bart0sh force-pushed the PR0080-fpga-e2e-tests branch 2 times, most recently from 7d8661c to 6ebe24d Compare April 9, 2020 12:14
rojkov
rojkov reviewed Apr 9, 2020
View reviewed changes
Copy link
Contributor

@rojkov rojkov left a comment

Choose a reason for hiding this comment

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

Looks good to me given the commented out leftovers are removed.

@bart0sh
Copy link
Member Author

bart0sh commented Apr 9, 2020

Looks good to me given the commented out leftovers are removed.

If you mean these 2 lines:

+       //fmw.PodClient().WaitForFinish(pod.ObjectMeta.Name, 60*time.Second)
+       //framework.RunKubectlOrDie("--namespace", fmw.Namespace.Name, "logs", pod.ObjectMeta.Name)

I left them on purpose. Sometimes it's useful to see the logs of failing container. LogFailedContainers doesn't do it for some reason.

@rojkov
Copy link
Contributor

rojkov commented Apr 9, 2020

Yes, these two lines. Could you please add comment there about the purpose? To discourage others from dropping it later. :)

@bart0sh bart0sh force-pushed the PR0080-fpga-e2e-tests branch from 6ebe24d to 23e636d Compare April 9, 2020 13:59
@bart0sh
Copy link
Member Author

bart0sh commented Apr 9, 2020

@rojkov done

@bart0sh bart0sh force-pushed the PR0080-fpga-e2e-tests branch from 23e636d to 3cb9bd2 Compare April 9, 2020 14:01
bart0sh added 2 commits April 9, 2020 17:01
@bart0sh
run fpga plugin in arbitrary namespace
182601b
@bart0sh
implement e2e tests for FPGA plugin
3cb9bd2 
implemented e2e tests for preprogrammed(af) and
orchestrated(region) plugin modes.
mythi
mythi reviewed Apr 10, 2020
View reviewed changes
test/e2e/fpga/fpga.go
Comment on lines +66 to +74
ginkgo.By(fmt.Sprintf("deploying webhook in %s mode", webhookMode))
_, _, err := framework.RunCmd(webhookDeployScriptPath, "--mode", webhookMode, "--namespace", fmw.Namespace.Name)
framework.ExpectNoError(err)

waitForPod(fmw, "intel-fpga-webhook")

ginkgo.By(fmt.Sprintf("deploying FPGA plugin in %s mode", pluginMode))
_, _, err = framework.RunCmd(pluginDeployScriptPath, "--mode", pluginMode, "--namespace", fmw.Namespace.Name)
framework.ExpectNoError(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I've not played with FPGA deployments enough to have strong opinion here but I wonder if we should:

  1. deploy a "global webhook" that gets shared between the tests (AFAIK that's the goal with fpga: make admission webhook mode-less #358)
  2. have a custom namespace for the webhook serviceaccount
  3. deploy plugin using the default serviceaccount
  4. the key test case is "Note that the mappings are scoped to the namespaces they were created in and they are applicable to pods created in the corresponding namespaces." isn't that possible in each custom test run where the namespace comes from gingko?

Copy link
Member Author

@bart0sh bart0sh Apr 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Currently webhook and plugin mode must match. The test uses both modes, that's why I implemented it this way. Another point of doing this is that it's a cleaner approach to use ginkgo test namespace for as many components as possible because ginkgo automatically cleans its namespace after the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bart0sh I don't like the new script and that's why I was pondering options to drop 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.

It would be a trivial change to get rid of this script when plugin kustomization works correctly, i.e. when #318 is done. So, I'd propose to merge this PR now. Otherwise we'll have to wait until #318 is done.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, that's probably fine. Do you have input to #318 to ensure "kustomization works correctly"?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mythi Yes, I've added 2 items to the #318 last week.

@bart0sh
Copy link
Member Author

bart0sh commented Apr 14, 2020

@mythi @kad should we remove travis-ci/pr tests or should we fix them?

@mythi
Copy link
Contributor

mythi commented Apr 14, 2020

@bart0sh I'll submit a PR to drop travis (I have other housekeeping items in the pipeline too)

@bart0sh
Copy link
Member Author

bart0sh commented Apr 14, 2020

@rojkov can you merge this PR please?

@rojkov rojkov merged commit bb03a89 into intel:master Apr 14, 2020
@bart0sh bart0sh deleted the PR0080-fpga-e2e-tests branch June 13, 2021 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rojkov rojkov rojkov approved these changes

@mythi mythi mythi approved these changes

@kad kad Awaiting requested review from kad

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement FPGA e2e tests
3 participants
@bart0sh @rojkov @mythi