Split horizon pilot tests#13131
Conversation
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the ℹ️ Googlers: Go here for more info. |
Running the regular pilot tests on two clusters, with Split Horizon EDS. The apps are deployed in the both clusters identically.
prow/e2e-split-horizon-eds.sh
Outdated
| CLUSTERREG_DIR="${CLUSTERREG_DIR:-$(mktemp -d /tmp/clusterregXXX)}" | ||
| export CLUSTERREG_DIR | ||
|
|
||
| ./prow/e2e-suite.sh --timeout 50 --cluster_registry_dir="$CLUSTERREG_DIR" --single_test e2e_multicluster_split_horizon "$@" |
There was a problem hiding this comment.
Does your test take anywhere close to 50 minutes to run? If not might use a lower value to fail quicker?
There was a problem hiding this comment.
No, the longest run was 9 minutes and something, reduced timeout to 15 in cbe0c44
prow/istio-pilot-multicluster-e2e.sh
Outdated
|
|
||
| #echo 'Running pilot multi-cluster e2e tests (v1alpha1, noauth)' | ||
| ./prow/e2e-suite.sh --timeout 50 --cluster_registry_dir="$CLUSTERREG_DIR" --single_test e2e_pilotv2_v1alpha3 "$@" | ||
| #echo 'Running pilot multi-cluster e2e tests with split horizon (v1alpha1, auth)' |
There was a problem hiding this comment.
I think you meant to remove the changes to this file before submitting
| export CLUSTERREG_DIR | ||
|
|
||
| #echo 'Running pilot multi-cluster e2e tests with split horizon (v1alpha1, auth)' | ||
| ./prow/e2e-suite.sh --timeout 50 --cluster_registry_dir="$CLUSTERREG_DIR" --split_horizon --single_test e2e_pilotv2_v1alpha3_auth "$@" |
There was a problem hiding this comment.
So it looks like your plan is to create 2 additional prow jobs. One for running the new split Horizon test, while the other runs a pilot multicluster test with a split Horizon deployment?
There was a problem hiding this comment.
right, I removed the changes in prow/istio-pilot-multicluster-e2e.sh
| if !*authEnable { | ||
| return errors.New("the split horizon test can only work with auth enabled") | ||
| } | ||
| istioYaml = mcSplitHorizonInstallFile |
There was a problem hiding this comment.
You will need changes in the teardown function around line 523 to get the proper yaml file if clusterwide is enabled. Which is think it typically is enabled. you can look at my 13246 to see a model.
tests/e2e/framework/kubernetes.go
Outdated
|
|
||
| if *splitHorizon { | ||
| registryName := filepath.Base(k.RemoteKubeConfig) | ||
| content = replacePattern(content, "N2_REGISTRY_TOKEN", registryName) |
There was a problem hiding this comment.
might consider declaring N2_REGISTRY_TOKEN a constant
|
@vadimeisenbergibm: The following test failed, say
DetailsInstructions 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. I understand the commands that are listed here. |
| // for the deployments because they won't be able to connect to Pilot | ||
| // until their service is added by watching the remote k8s api. Adding | ||
| // the remote k8s api watcher is done later on in the test. | ||
| aRemote = NewAppManager(tmpDir, *namespace, remoteI, remoteKubeConfig, false) |
There was a problem hiding this comment.
I understand why you initially can't check the remote deployment, but it might be good to check the remote deployment add some point. If it doesn't come up the test will fail either way so it isn't critical but it would both provide better failure insight and also likely fail sooner.
john-a-joyce
left a comment
There was a problem hiding this comment.
My final comment can be addressed in another PR if you prefer
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: john-a-joyce, vadimeisenbergibm If they are not already assigned, you can assign the PR to them by writing 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 |
|
@vadimeisenbergibm: PR needs rebase. DetailsInstructions 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. |
|
Closed to be refactored into the new integration test framework. |
Continuation of #12709, must not be merged before that PR.
Running the regular pilot tests on two clusters, with Split Horizon EDS. The apps are deployed in the both clusters identically.