Add operator support to e2e tests #17864

sdake · 2019-10-14T19:36:47Z

This adds a --use_operator flag to the e2e tests. The e2e tests
expect a different component to store a manifest for deployment
in the $ISTIO/install/kubernetes directory.

One aspect of this test case is incorrect. Deletion of the operator
appears not to work within the test framework. The namespace locks
up with the namespace in the terminating state.

Please provide a description for what this PR is for.

And to help us figure out who should review this PR, please
put an X in all the areas that this PR affects.

[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[ ] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure

This adds a --use_operator flag to the e2e tests. The e2e tests expect a different component to store a manifest for deployment in the $ISTIO/install/kubernetes directory. One aspect of this test case is incorrect. Deletion of the operator appears not to work within the test framework. The namespace locks up with the namespace in the terminating state.

This work runs e2e testing using end to end testing against the istio/istio repository. To run - use `make e2e`. Depends-On: istio/istio#17864

tests/e2e/framework/kubernetes.go

howardjohn · 2019-10-14T20:04:52Z

tests/e2e/framework/kubernetes.go

+				log.Errorf("Failed to delete istio-operator namespace.")
+				return err
+			}
+			if _, err := util.Shell("kubectl delete ns %s --grace-period=0 --force --kubeconfig=%s",


not really sure - operations determined by others. Will dig into that.

grace period and force have been removed.

howardjohn · 2019-10-14T20:21:30Z

I don't know the context behind the finalizer change, but something seems off about the solution. We should make sure to revisit that (unless this was already well thought through by someone, in which case we should document the rationale)

…

On Mon, Oct 14, 2019 at 1:11 PM Istio Automation ***@***.***> wrote: @sdake <https://github.com/sdake>: The following tests *failed*, say /retest to rerun them all: Test name Commit Details Rerun command integ-istioio-k8s-tests_istio 6b22e31 <6b22e31> link <https://prow.istio.io/view/gcs/istio-prow/pr-logs/pull/istio_istio/17864/integ-istioio-k8s-tests_istio/13> /test integ-istioio-k8s-tests_istio lint_istio 6b22e31 <6b22e31> link <https://prow.istio.io/view/gcs/istio-prow/pr-logs/pull/istio_istio/17864/lint_istio/2097> /test lint_istio integ-new-install-k8s-tests_istio 6b22e31 <6b22e31> link <https://prow.istio.io/view/gcs/istio-prow/pr-logs/pull/istio_istio/17864/integ-new-install-k8s-tests_istio/1603> /test integ-new-install-k8s-tests_istio Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/guide/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://go.k8s.io/bot-commands>. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#17864?email_source=notifications&email_token=AAEYGXPEN3RB434OLYP3DB3QOTHAXA5CNFSM4JATGBD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBGKN3I#issuecomment-541894381>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXNU5TR7WIXW5YCBJPDQOTHAXANCNFSM4JATGBDQ> .

tests/e2e/framework/kubernetes.go

richardwxn · 2019-10-14T21:40:46Z

tests/e2e/framework/kubernetes.go

@@ -605,7 +628,16 @@ func (k *KubeInfo) Teardown() error {
 	validatingWebhookConfigurationExists := false
 	log.Infof("Deleting namespace %v", k.Namespace)
 	for attempts := 1; attempts <= maxAttempts; attempts++ {
-		namespaceDeleted, _ = util.NamespaceDeleted(k.Namespace, k.KubeConfig)
+		if *useOperator {
+			if _, err := util.Shell("kubectl delete ns istio-operator --grace-period=0 --force",


k.namespace and k.kubeconfig not passed in

k.namesapace is in this case istio-system. k.kubeconfig in this case maps to k.namespace. There is no context for these, hen ce why they are not passed into the shell util command since they are not absolutely needed.

but you are deleting istio-operator ns here? also why we don't remove the extra args if we don't pass in.

why force and no grace period?

In the non operator code this is checking if the namespace was deleted, but here we are deleting it. Don't we already delete it above?

I have removed force and grace period. The logic sets namespaceDeleted=true only if the namespace was deleted. The same logic checks that the namespace is deleted:

if namespaceDeleted && !validatingWebhookConfigurationExists { break }

This basically just adds logic for the case the --use_operator is set.

In the non operator code this is checking if the namespace was deleted, but here we are deleting it. Don't we already delete it above?

I see, I misunderstood the purpose of the function call. I will check the logic again and submit a followup.

sdake · 2019-10-14T23:30:52Z

I don't know the context behind the finalizer change, but something seems off about the solution. We should make sure to revisit that (unless this was already well thought through by someone, in which case we should document the rationale)

The finalizer hacking doesn't work. If I run it on the shell, all is grand. if run from the test cases, it has no effect - possibly because of parsing of the text string in the shell command.

More importantly we need a better way of handling deletion then no way at all (which is what we currently have). I am simply attempting to make an e2e test that tests the system. I could remove the deletion if you prefer.

Cheers
-steve

sdake · 2019-10-15T19:56:05Z

Regarding the discussion around deletion in this PR, the deletion has been a sticking point since Friday afternoon after the first standalone operator was birthed. I'd recommend we merge this PR and i will follow up with a proper delete that matches how we think the operator should delete Istio.

FWIW, We believe we have the delete working properly and none of this complex deletion procedure is needed, but we want to get the gate moving.

Cheers
-steve

howardjohn

Nice, a few comments

howardjohn · 2019-10-15T22:49:40Z

tests/e2e/framework/kubernetes.go

@@ -605,7 +628,16 @@ func (k *KubeInfo) Teardown() error {
 	validatingWebhookConfigurationExists := false
 	log.Infof("Deleting namespace %v", k.Namespace)
 	for attempts := 1; attempts <= maxAttempts; attempts++ {
-		namespaceDeleted, _ = util.NamespaceDeleted(k.Namespace, k.KubeConfig)
+		if *useOperator {
+			if _, err := util.Shell("kubectl delete ns istio-operator --grace-period=0 --force",


why force and no grace period?

howardjohn · 2019-10-15T22:50:50Z

tests/e2e/framework/kubernetes.go

@@ -605,7 +628,16 @@ func (k *KubeInfo) Teardown() error {
 	validatingWebhookConfigurationExists := false
 	log.Infof("Deleting namespace %v", k.Namespace)
 	for attempts := 1; attempts <= maxAttempts; attempts++ {
-		namespaceDeleted, _ = util.NamespaceDeleted(k.Namespace, k.KubeConfig)
+		if *useOperator {
+			if _, err := util.Shell("kubectl delete ns istio-operator --grace-period=0 --force",


In the non operator code this is checking if the namespace was deleted, but here we are deleting it. Don't we already delete it above?

tests/e2e/framework/kubernetes.go

howardjohn · 2019-10-15T22:52:07Z

tests/e2e/framework/kubernetes.go

@@ -754,12 +809,6 @@ func (k *KubeInfo) deployIstio() error {
 		}
 	}

-	// Create istio-system namespace


This may break the CNI stuff below?

there are two branches. --use_operator and not. I have not implemented for the scenario where cni is set and operator is set. IMO if we we with to test this scenario, that is something we could tackle as follow-on work. I will post results of e2e.sh tested against this PR for make e2e with the cni repo though.

Cheers
-steve

Thats not what I mean, for the CNI test before we create the namespace, then install CNI in the namespace. Now we install cni in the namespace, then create the namespace. This may not work, unless CNI is also creating the namespace?

i am running e2e cni now. Actually I am building a buildtools image because e2e for cni repo is broken as a result of a recent build tools change around the /home dir. (the cni tests create /home/.helm). I will report back the results as well as my diff.

e2e cni image regression fix: istio/tools#441

my cni diff tested with:

sdake@beast-01:~/go/src/istio.io/cni$ git diff diff --git a/Makefile b/Makefile index cadc120..6da2153 100644 --- a/Makefile +++ b/Makefile @@ -56,7 +56,7 @@ ifeq ($(BUILD_WITH_CONTAINER),1) export TARGET_OUT = /work/out/$(TARGET_ARCH)_$(TARGET_OS) CONTAINER_CLI ?= docker DOCKER_SOCKET_MOUNT ?= -v /var/run/docker.sock:/var/run/docker.sock -IMG ?= gcr.io/istio-testing/build-tools:2019-10-11T13-37-52 +IMG ?= gcr.io/istio-testing/build-tools:2019-10-15T17-10-27 UID = $(shell id -u) PWD = $(shell pwd) diff --git a/test/e2e.sh b/test/e2e.sh index 8b43027..835830b 100755 --- a/test/e2e.sh +++ b/test/e2e.sh @@ -59,7 +59,13 @@ kubectl get pods --all-namespaces -o wide ISTIO_DIR="${GOPATH}/src/istio.io/istio" if [[ ! -d "${ISTIO_DIR}" ]] then - git clone https://github.com/istio/istio.git "${ISTIO_DIR}" + mkdir -p ${GOPATH}/src/istio.io + cd ${GOPATH}/src/istio.io + git clone https://github.com/sdake/istio.git "${ISTIO_DIR}" + pushd "${ISTIO_DIR}" + git fetch origin + git checkout operator_e2e + popd fi pushd "${ISTIO_DIR}" || exit make istioctl

I just recognized the e2e tests in BUILD_WTIH_CONTAINER in the istio/cni repo use cached repos. I am in the process of a reboot so I may docker volume prune.

Seems like cni tests pass. Makes me question the results of the cni e2e test cases, although I have not investigated in much detail what the local test does. What would you recommend next?

howardjohn · 2019-10-15T22:52:56Z

tests/e2e/framework/kubernetes.go

@@ -55,6 +56,7 @@ const (
 	mcAuthInstallFileNamespace     = "istio-auth-multicluster.yaml"
 	mcRemoteInstallFile            = "istio-remote.yaml"
 	mcSplitHorizonInstallFile      = "istio-multicluster-split-horizon.yaml"
+	authOperatorInstallFile        = "istio-operator.yaml"


Where is this file?

this file is created by the e2e.sh script in istio/operator#379

Cheers
-steve

This work runs e2e testing using end to end testing against the istio/istio repository. To run - use `make e2e`. Depends-On: istio/istio#17864

* cross-repo e2e gating for the operator This work runs e2e testing using end to end testing against the istio/istio repository. To run - use `make e2e`. Depends-On: istio/istio#17864 * Address reviewer comments. * Revert all gomod changes

sdake requested a review from a team as a code owner October 14, 2019 19:36

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Oct 14, 2019

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 14, 2019

sdake pushed a commit to sdake/operator that referenced this pull request Oct 14, 2019

cross-repo e2e gating for the operator

b117baf

This work runs e2e testing using end to end testing against the istio/istio repository. To run - use `make e2e`. Depends-On: istio/istio#17864

sdake mentioned this pull request Oct 14, 2019

cross-repo e2e gating for the operator istio/operator#379

Merged

howardjohn reviewed Oct 14, 2019

View reviewed changes

richardwxn reviewed Oct 14, 2019

View reviewed changes

run through go format

ea99241

howardjohn reviewed Oct 15, 2019

View reviewed changes

Steven Dake added 2 commits October 15, 2019 16:57

Address reviewer comments.

e09ec57

Fix unused module error

708e49f

howardjohn approved these changes Oct 16, 2019

View reviewed changes

istio-testing merged commit 88439bb into istio:master Oct 16, 2019

sdake pushed a commit to sdake/operator that referenced this pull request Oct 17, 2019

cross-repo e2e gating for the operator

9ee515d

This work runs e2e testing using end to end testing against the istio/istio repository. To run - use `make e2e`. Depends-On: istio/istio#17864

Add operator support to e2e tests #17864

Add operator support to e2e tests #17864

Uh oh!

Conversation

sdake commented Oct 14, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

howardjohn commented Oct 14, 2019 via email

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdake commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdake commented Oct 15, 2019

Uh oh!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdake Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdake Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sdake commented Oct 14, 2019 •

edited

Loading

sdake Oct 16, 2019 •

edited

Loading

sdake Oct 16, 2019 •

edited

Loading