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

E2E tests for numerous NRI features #98

Closed

Conversation

martinkennelly
Copy link
Member

E2E Tests for features:

  • node selector
  • hugepages (optional)
  • user defined injections
  • resourceName

Co-authored-by: Michal Guzieniuk [email protected]
Co-authored-by: Martin Kennelly [email protected]

Hugepages test is optional and will be skipped if the number of 1Gi / 2Mi hugepages arent available. This allows us to run the same tests in an environment with (self hosted) and without hugepages (GH VM).

This patch adds a self hosted job which requires a GH environment. A GH environment can contain a set of approvers to allow execution. This environment should be created before this patch can be merged and tested.

Example of how we can use GH environments to allow a set of people to approve workflows: https://docs.google.com/document/d/1kWko-Hnne-BcuFvzEZPlmGxFQ9FSkTzPSzxP1m9xUuY/edit

Thanks to @MichalGuzieniuk for creating all the tests.

Sample execution: https://github.com/martinkennelly/network-resources-injector/runs/2504160518?check_suite_focus=true

@martinkennelly
Copy link
Member Author

@MichalGuzieniuk any idea why this failing:

/home/github/actions-runner/_work/network-resources-injector/network-resources-injector/test/e2e/custominection_test.go:123 

@MichalGuzieniuk
Copy link
Contributor

@MichalGuzieniuk any idea why this failing:

/home/github/actions-runner/_work/network-resources-injector/network-resources-injector/test/e2e/custominection_test.go:123 

I have checked this test and there is an issue inside NRI implementation. It does not allows to inject two labels or in other words it injects randomly only one. Some time ago, I have reported this at #94

@martinkennelly What do you think about excluding this test with comment inside issue?

@martinkennelly
Copy link
Member Author

@MichalGuzieniuk any idea why this failing:

/home/github/actions-runner/_work/network-resources-injector/network-resources-injector/test/e2e/custominection_test.go:123 

I have checked this test and there is an issue inside NRI implementation. It does not allows to inject two labels or in other words it injects randomly only one. Some time ago, I have reported this at #94

@martinkennelly What do you think about excluding this test with comment inside issue?

Thanks @MichalGuzieniuk
I think we should leave it until we decide how we will handle it. Last thing I want is to ignore this issue. Do you want to fix it Michal?

@MichalGuzieniuk
Copy link
Contributor

@MichalGuzieniuk any idea why this failing:

/home/github/actions-runner/_work/network-resources-injector/network-resources-injector/test/e2e/custominection_test.go:123 

I have checked this test and there is an issue inside NRI implementation. It does not allows to inject two labels or in other words it injects randomly only one. Some time ago, I have reported this at #94
@martinkennelly What do you think about excluding this test with comment inside issue?

Thanks @MichalGuzieniuk
I think we should leave it until we decide how we will handle it. Last thing I want is to ignore this issue. Do you want to fix it Michal?

@martinkennelly yes, I may fix it when I will finish with tests. Question is if such functionality is needed within NRI?

martinkennelly added a commit to martinkennelly/network-resources-injector that referenced this pull request Jun 1, 2021
Allowing multiple add ops for annotations allows user
not to understand the current limitation of allowing
a single add op.

Allow only "add" Op for annotation user defined
injection.

Fixes k8snetworkplumbingwg#94
Tested with k8snetworkplumbingwg#98
Signed-off-by: Kennelly, Martin <[email protected]>
martinkennelly added a commit to martinkennelly/network-resources-injector that referenced this pull request Jun 1, 2021
Allowing multiple add ops for annotations allows user
not to understand the current limitation of allowing
a single add op.

Allow only "add" Op for annotation user defined
injection.

Fixes k8snetworkplumbingwg#94
Tested with k8snetworkplumbingwg#98
Signed-off-by: Kennelly, Martin <[email protected]>
martinkennelly added a commit that referenced this pull request Jun 10, 2021
Allowing multiple add ops for annotations allows user
not to understand the current limitation of allowing
a single add op.

Allow only "add" Op for annotation user defined
injection.

Fixes #94
Tested with #98
Signed-off-by: Kennelly, Martin <[email protected]>
MichalGuzieniuk and others added 2 commits June 23, 2021 13:21
E2E Tests for features:
  * node selector
  * hugepages (optional)
  * user defined injections
  * resourceName

Co-authored-by: Michal Guzieniuk <[email protected]>
Co-authored-by: Martin Kennelly <[email protected]>
@martinkennelly
Copy link
Member Author

rebased upon current master

@Eoghan1232
Copy link
Collaborator

Closing this PR.
It will be continued in #121.

@Eoghan1232 Eoghan1232 closed this Nov 25, 2021
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.

3 participants