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.

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

test: Enable embedded Hubble globally #11378

Merged
merged 1 commit into from May 8, 2020
Merged

test: Enable embedded Hubble globally #11378

merged 1 commit into from May 8, 2020

Conversation

michi-covalent
Copy link
Contributor

@michi-covalent michi-covalent commented May 6, 2020 •
edited

test: Enable embedded Hubble globally

  • Enable embedded Hubble in the end-to-end testing by default.
  • Set global.registry properly by splitting the image into 2 parts.

Signed-off-by: Michi Mutsuzaki michi@isovalent.com

@michi-covalent michi-covalent added area/CI-improvement Topic or proposal to improve the Continuous Integration workflow release-note/misc This PR makes changes that have no direct user impact. sig/hubble Impacts hubble server or relay labels May 6, 2020
@michi-covalent michi-covalent requested a review from a team as a code owner May 6, 2020 21:26
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 May 6, 2020
@michi-covalent michi-covalent requested a review from a team May 6, 2020 21:28
@coveralls
Copy link

coveralls commented May 6, 2020 •
edited

Coverage Status

Coverage increased (+0.008%) to 37.869% when pulling e9c03df on pr/michi/test into bad730c on master.

glibsm
glibsm reviewed May 6, 2020
View reviewed changes
test/helpers/kubectl.go
@@ -184,7 +190,7 @@ func init() {
defaultHelmOptions["global.tag"] = version
// This always works because SplitContainerURL would not return
// isFullyQualified == true otherwise
parts := strings.SplitN(image, "/", 1)
parts := strings.SplitN(image, "/", 2)
Copy link
Member

Choose a reason for hiding this comment

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

i'm not sure i get this change, what exactly is going on 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.

ok yeah sorry i should have added more description in the commit message. basically this chunk of code is for extracting docker registry (e.g. quay.io/cilium) from CILIUM_IMAGE env variable. say CILIUM_IMAGE is set to quay.io/cilium/cilium:latest. then:

  • registry, image, version, isFullyQualified := SplitContainerURL(v) splits it into quay.io, cilium/cilium, and latest.
  • get the "organization" part from cilium/cilium by splitting it by slashes.
  • set global.registry to quay.io/cilium.

the issue is that SplitNreturns the string itself if the third argument is set to one.

gandro
gandro requested changes May 7, 2020
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

This will be a great step forward. 🚀

On the approach: I'm not entirely sure if we really want to deploy the CLI pods unconditionally in each test. It is unquestionably useful, but it introduces another moving part. It might be something we only want to once a test failed. Or maybe we could add the Hubble CLI to the log-gatherer pods instead, which are already used for the purpose of fetching the logs when tests fail.

Secondly, on a technical note, I believe this will deploy the Hubble CLI and relay pods for all tests using DeployCiliumOptionsAndDNS, but will likely not remove them at the end of each test, since most tests seem to use kubectl.DeleteCiliumDS(), which only removes the cilium-agent. This has already bitten us in the Hubble testsuite (#11141)

This is not the fault of this PR per se, but unfortunately I think we first have to review all uses of DeleteCiliumDS() in all tests. The new deployment manager (#11170) would solve that, but as far as I can tell, tests are not using it yet.

@michi-covalent
Copy link
Contributor Author

@gandro how about just doing kubectl delete -f on the yaml file used to deploy cilium?

kubectl.Delete(ciliumFilename)

@gandro
Copy link
Member

gandro commented May 7, 2020 •
edited

@gandro how about just doing kubectl delete -f on the yaml file used to deploy cilium?

kubectl.Delete(ciliumFilename)

Yes, that's what we currently do in K8sHubble. The deployment manager that I linked to also does the same, but many tests are not using it yet. One example usage is this one:

cilium/test/k8sT/DatapathConfiguration.go

Line 60 in e0fbe8a

deploymentManager.DeleteCilium()

But most other testsuite are not using it yet, since it is a recent addition.

@michi-covalent
Copy link
Contributor Author

got it that makes sense. in that case i guess i should migrate existing tests to use deployment manager first.

@michi-covalent michi-covalent added the dont-merge/blocked Another PR must be merged before this one. label May 7, 2020
@michi-covalent
Copy link
Contributor Author

blocked on #11330 as well

@michi-covalent
Copy link
Contributor Author

had an offline conversation with @gandro and others. we'll enable embedded hubble to start with, and then follow up with cli/relay later.

@michi-covalent michi-covalent changed the title test: Go all in on Hubble test: Enable embedded Hubble globally May 7, 2020
@maintainer-s-little-helper
Copy link

Commit fd3c71aa9dd0f0cccbed1b3deeda4cfe90d199b4 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 8, 2020
@michi-covalent
test: Enable embedded Hubble globally
e9c03df 
- Enable embedded Hubble in the end-to-end testing by default.
- Set `global.registry` properly by splitting the image into 2 parts.

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 8, 2020
@gandro
Copy link
Member

gandro commented May 8, 2020

test-me-please

@michi-covalent michi-covalent removed the dont-merge/blocked Another PR must be merged before this one. label May 8, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 8, 2020
@rolinh rolinh merged commit 3a4c386 into master May 8, 2020
1.8.0 automation moved this from In progress to Merged May 8, 2020
@rolinh rolinh deleted the pr/michi/test branch May 8, 2020 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glibsm glibsm glibsm left review comments

@gandro gandro gandro approved these changes

@nebril nebril nebril approved these changes

Assignees
No one assigned
Labels
area/CI-improvement Topic or proposal to improve the Continuous Integration workflow ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/hubble Impacts hubble server or relay
Projects
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@michi-covalent @coveralls @gandro @glibsm @nebril @rolinh