-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add an integration test for ambassador addon, fix filename and bump operator version #8372
Conversation
Hi @concaf. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: concaf The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Can one of the admins verify this patch? |
Codecov Report
@@ Coverage Diff @@
## master #8372 +/- ##
=======================================
Coverage 31.88% 31.89%
=======================================
Files 165 165
Lines 10822 10821 -1
=======================================
Hits 3451 3451
+ Misses 6957 6956 -1
Partials 414 414
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please see comments above
/ok-to-test |
kvm2 Driver |
Hey @concaf thank you for opening this PR. Are you still working on this? If so could you please:
|
Hey @priyawadhwa, I'm winding up a few things right now, I'll be fixing this next week. Sorry. |
No worries, totally understand. Just checking in :) |
3d6b3b3
to
4531b40
Compare
Whenever a `kubectl apply` fails while enabling an addon, it is retried with exponential backoff. The command (type `*exec.Cmd`) that this retry function runs in created outside the function - which means that it is reused on every retry. This is a problem because `exec.Cmd` (https://godoc.org/github.com/pkg/exec#Cmd) states that "... Cmd cannot be reused after calling its Run or Start methods." This retry is a common case due to, say, a CRD and its resource being present in the same YAML file of the addon which causes a race condition where the resource is created before its CRD is created in the cluster - this race is fixed by subsequent retries. I've noticed this in the dashboard and the ambassador addon. Due to the above mentioned bug, minikube throws errors like `exec: already started` in every retry and the retry is never successful, manifests are never deployed and addon creation errors out. Fix kubernetes#8138 Fix kubernetes#8119 Fix a few CI errors in kubernetes#8372
Whenever a `kubectl apply` fails while enabling an addon, it is retried with exponential backoff. The command (type `*exec.Cmd`) that this retry function runs in created outside the function - which means that it is reused on every retry. This is a problem because `exec.Cmd` (https://godoc.org/github.com/pkg/exec#Cmd) states that "... Cmd cannot be reused after calling its Run or Start methods." This retry is a common case due to, say, a CRD and its resource being present in the same YAML file of the addon which causes a race condition where the resource is created before its CRD is created in the cluster - this race is fixed by subsequent retries. I've noticed this in the dashboard and the ambassador addon. Due to the above mentioned bug, minikube throws errors like `exec: already started` in every retry and the retry is never successful, manifests are never deployed and addon creation errors out. Related to kubernetes#8138 kubernetes#8119 kubernetes#8372
I've sent #8792 which fixes intermittent failures while deploying ambassador (and dashboard) addon with the |
kvm2 Driver Times for Minikube (PR 8372): [65.38053608800001 62.40060027799999 62.480269337] Averages Time Per Log
docker Driver Times for Minikube (PR 8372): [25.551426993 27.567862065 25.657753418] Averages Time Per Log
|
c3723f2
to
18f3842
Compare
kvm2 Driver Times for Minikube (PR 8372): [64.37840650199999 58.90594394 64.32788087299998] Averages Time Per Log
docker Driver Times for Minikube (PR 8372): [26.628006410999998 25.561492643000005 26.446466915] Averages Time Per Log
|
This commit does the following: - Update Ambassador operator to v1.2.8 - Add integration test for `ambassador` addon - Fix a previously incorrect filename
18f3842
to
6f353ea
Compare
Hey @priyawadhwa @medyagh, I've addressed the review comments and fixed the tests. I don't see any failures on "none_Linux" and "KVM_Linux". Failures on "Docker_Linux" and "VirtualBox_Linux" look unrelated. Waiting on other CI results but I think this is ready for another shot ;) |
Gentle nudge 😉 |
Fixed merge conflict. /ok-to-test |
Travis tests have failedHey @concaf, 1st Buildmake test
TravisBuddy Request Identifier: 88d7aca0-ecab-11ea-831e-55c1b07d6d51 |
kvm2 Driver Times for Minikube (PR 8372): 95.9s 96.4s 92.1s Averages Time Per Log
docker Driver Times for Minikube (PR 8372): 54.4s 52.1s 52.8s Averages Time Per Log
|
@concaf sorry for the long delay in the PR review, do you mind pulling master one more time ? feel free to ping me on slack if got delayed again |
@concaf: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@concaf would you mind please rebasing the PR, otherwise it looks good |
@concaf do u mind please to solve merge conflicts and try one more time? |
Please re-open once merge conflicts are addressed. Thanks! |
This PR adds an integration test for the ambassador addon as discussed in #8161. Also bumps the operator version.
CC: @priyawadhwa @medyagh