fix: e2e test with kind#6162
Conversation
Pull Request Test Coverage Report for Build 22314723358Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
It's hard to run this tests in bash. Maybe worth to have a python dsl? I'm not sure where it is still green or actually failed |
|
@ivankatliarchuk hard to read you mean? I definitely don’t want a python wrapper, but I agree the output isn’t the best. I think I can make the output better, but running it locally wasn’t an issue for me. Just delete the cluster with kind if you have any, run the script. I do run everything in a vm though, not sure if this complicates people’s local dev, so I’d be eager to hear more from you. |
|
Have a look at the output Bash with log regexes can work for individual tests, but this approach doesn’t scale well if we decide to add more tests later. |
|
Yeah I get that but I took this approach exactly because I didn’t want to write a mega wrapper in go like I did for the end to end tests that I have in my private repo and that for reasons I didn’t want to opensource. Do you think it would make sense to get this in at least to have the tests fixed (even with the crappy output) and then work in another pr to convert them in go? |
|
For sure we should fix the test first. |
|
@ivankatliarchuk I think this works? Is it terrible? |
|
/lgtm |
|
Results, seems like working. I was initially confused, that is trying so many attempts. But this is expected due to DNS propagation |
|
DNS is working, is just takes some time to propagate to a node |
| ATTEMPT=1 | ||
| while [ \$ATTEMPT -le \$MAX_ATTEMPTS ]; do | ||
| echo "Attempt \$ATTEMPT/\$MAX_ATTEMPTS: Querying externaldns-e2e.external.dns A record" | ||
| RESULT=\$(dig @$NODE_IP -p 5353 externaldns-e2e.external.dns A +short +timeout=5 2>/dev/null) |
There was a problem hiding this comment.
maybe something like
dig @coredns.default.svc.cluster.local
If we are using coredns
There was a problem hiding this comment.
you could validate, where the records added to etcd, something like
kubectl exec -it etcd-0 -- etcdctl get /skydns/dns/external --prefix --keys-only
There was a problem hiding this comment.
I am not sure how this would add? Maybe you say it to reduce possible flakyness? I did it just with dig originally to make a test that was as close as possible to users 🤔
There was a problem hiding this comment.
Yeah, not a big difference. Is goot too go as is.
For dig @coredns.default.svc.cluster.local (service DNS) the only difference with NODE_IP
- Tests the actual path -> reflects how real workloads inside the cluster resolve DNS, which is what external-dns is supposed to serve
There was a problem hiding this comment.
Ok cool, I think I understand your point of view now, but the change would also add a bit of complexity. I’ll merge like this, so we have working tests again.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Raffo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What does it do ?
Closes #6149
Looks like the end to end tested that I pushed in #5933 were so so wrong. I am not sure if I screwed up something upon push or if I really tested them wrong, but nonetheless, they didn't work.
This PR should fix them, I tested breaking it and then restoring it a couple of times, both locally and in action. I still would appreciate a careful review to make sure that things make sense.
Aaand of course they don't pass after many successful passes both locally and in CI. I guess they are flaky as hell. Moving this as draft and going to the drawing board again. Le sighI think I fixed it.
More