Add shared e2e.sh script #115
Conversation
|
🤖 Created branch: z_pr115/mkolesnik/e2e_script |
|
🤖 Updated branch: z_pr115/mkolesnik/e2e_script |
|
🤖 Updated branch: z_pr115/mkolesnik/e2e_script |
scripts/shared/e2e.sh
Outdated
| $ kubectl config use-context cluster1 # or cluster2, cluster3.. | ||
|
|
||
| To clean evertyhing up, just run: make cleanup | ||
| EOM |
There was a problem hiding this comment.
I'm not sure of the general usefulness of having this in shipyard. It seems this would only be useful for submariner. Lighthouse has to do additional core-dns setup in its e2e.sh. Admiral and shipyard tests (in progress) don't deploy submariner. Perhaps it would be better to make more utility functions, eg test_with_e2e_tests. I already added utility to print the clusters message.
There was a problem hiding this comment.
Actually lighthouse runs E2E tests so it makes sense to have this shared. The additional setup will be done in an intermediary step.
As for Admiral and shipyard, we'll have to see exactly what we run in e2e, but at least in shipyard we do deploy to test the deployment itself is working so it's not like we're going to remove it. In Admiral I see that you are adding some deployment capability. For that I suggest we can use cluster_settings to skip deployments on certain clusters as it's already possible, instead of running your own deployment procedure. Alternatively you can define your own deploy target there and still use the E2E code here.
If we need to make adjustments we can always tweak what we need of course, but we should make the typical use case shared IMHO and not just some functions which are scattered.
There was a problem hiding this comment.
Admiral E2E deploys 2 clusters w/o submariner and deploys custom CRDs and sets up BROKER env vars for the tests. Currently I'm doing that in an admiral e2e.sh.
Shipyard E2E deploys 1 cluster w/o submariner and runs the existing example E2E and a simple tcp connectivity test between 2 pods in the same cluster.
There was a problem hiding this comment.
After reviewing the scripts at submariner-io/admiral#49 it seems indeed admiral can benefit from this change (well if we make sure it doesn't try to find subm :)) and just employ it's own deploy target.
There was a problem hiding this comment.
Let's try to converge into a single set of e2e scripts, if we need to readjust later... we can figure out.
|
🤖 Updated branch: z_pr115/mkolesnik/e2e_script |
|
Had to force push due to conflicts with the master, only thing that was changed was removal of |
| if with_context cluster3 kubectl wait --for=condition=Ready pods -l app=submariner-engine -n "${SUBM_NS}" --timeout=3s > /dev/null 2>&1; then | ||
| echo "Submariner already deployed, skipping deployment..." | ||
| return | ||
| fi |
There was a problem hiding this comment.
Why is this necessary since the e2e make target depends/should depend on the deploy target? Also this assumes that submariner is actually deployed by the project invoking it (admiral does not).
There was a problem hiding this comment.
Yeah it will mostly be true.. It was part of the "lazy deploy" change on submariner submariner-io/submariner#512
However I can add a flag to toggle this, if that makes sense let me know and I'll add one
There was a problem hiding this comment.
But why have this here at all? How does it differ from or is better than a make target dependency?
There was a problem hiding this comment.
This was already discussed on submariner-io/submariner#488 (which you approved) and superseded by submariner-io/submariner#510 (which was itself superseded by submariner-io/submariner#512).
The gist is: if it's a make dependency, then running make e2e would cause a whole chain of actions (compile, docker image, clusters) to occur even if you only intend to run only the e2e tests since you only changed the files there.
Hence this seemed like a nice way to avoid a hard dependency of e2e on deploy and allow the requested behavior of running just the e2e tests.
I can leave this code solely in submariner, but I think most projects could benefit from it.
If we have e2e depend on delpoy in the Makefile, it'll be very hard not to rerun deploy each time you run make e2e since the deploy target itself isnt a real file so make can't know if it should or shouldn't re run it.
Hence I proposed this can be toggle-able via a flag to this script.
There was a problem hiding this comment.
Yeah, we need to optimize that make e2e so the workflow of developing a new e2e test is not a PITA for the developer, or involves logging into the dapper shell to run the e2e tests manually or other kind of setup.
Also having a common "e2e" that will let you re-run your e2e tests in a simple way is better because we can have a single development document then.
I see it's not ideal. May be there's a more appropriate name than "lazy" ? :)
| if [[ "${cluster_subm[$cluster]}" = "true" ]]; then | ||
| printf " -dp-context $cluster" | ||
| fi | ||
| done |
There was a problem hiding this comment.
I don't think this will generally work as expected for all use cases. First cluster_subm iteration order is random - submariner, eg, expects a specific order: cluster2 cluster3 cluster1. Perhaps we can utilize the ordering of the clusters array being introduced by #113. Second, I don't think we can assume that if cluster_subm$cluster] is false then the cluster shouldn't be passed in.
There was a problem hiding this comment.
Yeah if it makes sense I don't mind passing all the clusters.
However I thought it is expecting only ones that have subm deployed?
As for the order I'm not quite sure why that matters to the e2e tests, shouldn't they work with any order? If we need specific cluster to identify a specific condition (eg gateway failover) then we should probably have an extra flag?
I could change this to receive the clusters order as parameters instead of trying to guess it, in which case the caller would be responsible (and would know it for sure since he's defining the deployment anyhow).
WDYT?
BTW it seems that associative arrays (such as the one used) have consistent order (but not easily predictable)
There was a problem hiding this comment.
They're passed in a particular order so the code can distinguish them as they may be used for a certain purpose or the code may assume a certain configuration. For submariner, ClusterC in the code is used specifically for the add cluster test which assumes no gateway is configured. Lighthouse passes the 3 clusters in numeric order, not sure what assumptions it makes.
I think it's reasonable to use the ordering of the new clusters array as the order they will be passed to the test code. So submariner would specify clusters=(cluster2, cluster3, cluster1). Of course then we'll have to reconcile positional assumptions in the deploy scripts, eg where it uses ${clusters[1]} and ${clusters[2]} for the connectivity test. I assume this is b/c "cluster1" doesn't have submariner running. Actually I'm not clear why we even run the connectivity test on deploy as the E2E tests do that anyway. It seems the scripts should just install/ deploy the clusters and components and let E2E handle the testing. Removing it would certainly simplify the generic scripting.
There was a problem hiding this comment.
I think the safest is to receive them in the arguments then each project can define it's own order (since they define their own potential cluster settings so they know best what order to send).
There was a problem hiding this comment.
I agree argument would make sense. Otherwise we need to handle the logic of which one is configured as cluster etc...
e2e_cluster_contexts=(cluster2, cluster3, cluster1) ?
something like that
| if with_context cluster3 kubectl wait --for=condition=Ready pods -l app=submariner-engine -n "${SUBM_NS}" --timeout=3s > /dev/null 2>&1; then | ||
| echo "Submariner already deployed, skipping deployment..." | ||
| return | ||
| fi |
There was a problem hiding this comment.
Yeah it will mostly be true.. It was part of the "lazy deploy" change on submariner submariner-io/submariner#512
However I can add a flag to toggle this, if that makes sense let me know and I'll add one
| if [[ "${cluster_subm[$cluster]}" = "true" ]]; then | ||
| printf " -dp-context $cluster" | ||
| fi | ||
| done |
There was a problem hiding this comment.
Yeah if it makes sense I don't mind passing all the clusters.
However I thought it is expecting only ones that have subm deployed?
As for the order I'm not quite sure why that matters to the e2e tests, shouldn't they work with any order? If we need specific cluster to identify a specific condition (eg gateway failover) then we should probably have an extra flag?
I could change this to receive the clusters order as parameters instead of trying to guess it, in which case the caller would be responsible (and would know it for sure since he's defining the deployment anyhow).
WDYT?
BTW it seems that associative arrays (such as the one used) have consistent order (but not easily predictable)
scripts/shared/e2e.sh
Outdated
| $ kubectl config use-context cluster1 # or cluster2, cluster3.. | ||
|
|
||
| To clean evertyhing up, just run: make cleanup | ||
| EOM |
There was a problem hiding this comment.
After reviewing the scripts at submariner-io/admiral#49 it seems indeed admiral can benefit from this change (well if we make sure it doesn't try to find subm :)) and just employ it's own deploy target.
|
🤖 Updated branch: z_pr115/mkolesnik/e2e_script |
2 similar comments
|
🤖 Updated branch: z_pr115/mkolesnik/e2e_script |
|
🤖 Updated branch: z_pr115/mkolesnik/e2e_script |
scripts/shared/e2e.sh
Outdated
| test_with_e2e_tests | ||
|
|
||
| cat << EOM | ||
| Your 3 virtual clusters are deployed and working properly with your local source code, and can be accessed with: |
There was a problem hiding this comment.
I'd move this to a function we can also call at the end of deploy. (It'd be useful for the "quickstart on kind" guide). Although it's tangential to this patch purpose.
|
🤖 Updated branch: z_pr115/mkolesnik/e2e_script |
1 similar comment
|
🤖 Updated branch: z_pr115/mkolesnik/e2e_script |
Seems this script (the basic part to run the tests) is pretty much the same across projects, hence adding a shared version that acknowledges cluster_settings to generate the correct test contexts. Also changed the CI config to run the e2e, and use the proper make variables.
Following discussion on the PR #115, adding flag to control lazy deploy behavior, and receive clusters from arguments
In case we're deploying we need to reset KUBECONFIG variable.
|
🤖 Updated branch: z_pr115/mkolesnik/e2e_script |
|
🤖 Closed branches: [z_pr115/mkolesnik/e2e_script] |
Seems this script (the basic part to run the tests) is pretty much the
same across projects, hence adding a shared version that acknowledges
cluster_settings to generate the correct test contexts.
Also changed the CI config to run the e2e, and use the proper make
variables.