Operator e2e Fix by irisdingbj · Pull Request #456 · istio/operator

irisdingbj · 2019-10-24T10:25:53Z

bases on #404

@sdake I am trying to fix the e2e failure for operator controller here based on your above PR.

sdake

looks good - minus the loss of metallb functionality. If removal of metallb gets things moving for e2e, can you follow up this work with a metallb implementation for folks running on Linux systems? After that, a further generalization of this script so that it works correctly for cni is in order - perhaps you can take that work on as a followup.

Cheers
-steve

tests/e2e/e2e.sh

sdake · 2019-10-24T13:00:56Z

tests/e2e/e2e.sh

-      - 172.17.255.1-172.17.255.250
-EOF
-
-  kubectl apply -f https://raw.githubusercontent.com/google/metallb/v0.8.1/manifests/metallb.yaml


this part is suspect for prow. I'd suggest taking a look at removal of this for the prow case (but keep for the e2e case). Additionally I have read metallb does nt work on Mac,but does work on Linux, so we may need some host checking before using metallb and then use -use-local or whatever the test flag is for the actual e2e run based upon that conditional.

Please be aware - make e2e should work locally on your system (if your on Linux)l. I didn't test mac local e2e, but that must work, although Linux dev environments take priority. make e2e was working locally and in prow for me prior to this submission of this PR, however, I noticed I was using master of sdake/istio - and may have had some minor changes in istio/istio that were not pushed (and are now sady lost as I have force pushed to sdake/istio)...

THis can work on my mac locally

if metallb works locally on your mac without ``--use-local-cluster=true` that would be ideal. Can you clarify your statement?

tests/e2e/e2e.sh

sdake · 2019-10-24T13:07:52Z

tests/e2e/e2e.sh

+#kind cluster setup
+pushd "${ISTIO_DIR}"
+# shellcheck disable=SC1091
+source "./prow/lib.sh"


not sure what this is or does. Did you add prow/lib.sh to the repo? If its already in repo - make sure it does what you expect it to. @howardjohn and I were speaking of the need to generalize this work so it can be reused around all satellite repos (cni and operator to be specific at first) - although that generalization work can follow on making it work for the specific case of operator - which is essential for the release. If your struggling, I should be available for a conversation tonight on webex.

It is part of istio repo. So we can resue all the functions there.

disagree - operator should stand alone in its dependencies on building the repository. If we need a common files prow.ib, we can do that, but it should be merged as a separate PR in common-files repo (see link above).

see review for more information about why using a prow library is an undesirable approach. As painful as it may be, please copy the parts you need into this e2e.sh script. As followup work a refactor of the e2e for the various repos (istio/operator, istio/cni, istio/istio) can be done in https://github.com/istio/common-files.

sdake

looks pretty good, some more comments inline.

sdake · 2019-10-25T05:59:28Z

tests/e2e/e2e.sh

-  kubectl apply -f https://raw.githubusercontent.com/google/metallb/v0.8.1/manifests/metallb.yaml
-  kubectl apply -f ./metallb-config.yaml
-}
+GOPATH=$(cd ../../../; pwd)


you shouldn't need to specify GOPATH. This should come from either the docker container in a local env or the docker container in make e2e local case. See : https://github.com/istio/tools/blob/master/docker/build-tools/Dockerfile#L59

rename it as ROOT to prevent confusion

sdake · 2019-10-25T06:00:17Z

tests/e2e/e2e.sh

-
-setup_kind_cluster
-setup_docker
+  HUB=istio-testing TAG=1.5-dev make -f Makefile.core.mk controller docker


this should just be make. Make determines if it is running in a container environment or not, and this model would break container builds.

Changed to make controller docker as we only need these two targets.

make docker controller are or should be dependencies of the top level Makefile.core.mk file so they build whenever e2e runs. You may need to specify docker and docker.save as PHONY target if not already done.

tests/e2e/e2e.sh

sdake · 2019-10-25T06:03:18Z

tests/e2e/e2e.sh

+#kind cluster setup
+pushd "${ISTIO_DIR}"
+# shellcheck disable=SC1091
+source "./prow/lib.sh"


disagree - operator should stand alone in its dependencies on building the repository. If we need a common files prow.ib, we can do that, but it should be merged as a separate PR in common-files repo (see link above).

tests/e2e/e2e.sh

irisdingbj · 2019-10-25T12:19:43Z

/retest

sdake · 2019-10-25T13:27:20Z

tests/e2e/e2e.sh


 export ARTIFACTS="${ARTIFACTS:-$(mktemp -d)}"
 ROOT=$(cd ../../../; pwd)
+HUB=istio-testing


it is better to accept these from the Makeile.core.mk file directly rather than have to maintain them in two places. On the call to e2e.sh i'd recommend HUB=$(HUB) TAG=$(TAG) tests/e2e/e2e.sh such that user overrides of HUB or TAG are used during the build process.

sdake · 2019-10-25T13:27:44Z

tests/e2e/e2e.sh

+TAG=istio-testing

 function setup_docker() {
  HUB=istio-testing TAG=1.5-dev make controller docker


try with `HUB=$(HUB)

making TAG=1.5-dev here and not istio-testing is confusing. This is NOT pulling from a real repo, it is building the images locally and using local images.

ok thats fair.

will address in a followon PR if we can get this PR merged today.

tests/e2e/e2e.sh

sdake · 2019-10-25T13:29:43Z

tests/e2e/e2e.sh

  kind --loglevel debug --name istio-testing load docker-image istio-testing/operator:1.5-dev
  }

+function build_kind_images() {


rename function to build_istio_images()

tests/e2e/e2e.sh

sdake

implementation looks ok. There are some problems around HUB,TAG variable usage and the git cloing specifically. If you tell me it wont work with a git clone in a temp dir, we can merge without that.

I'd really strongly like to avoid the usage of the prow library - but instead copy the relevant bits into this script so we can make a unified e2e script in common-tools as a followup PR.

sdake · 2019-10-25T14:02:30Z

tests/e2e/e2e.sh

 set -eux

 export ARTIFACTS="${ARTIFACTS:-$(mktemp -d)}"
+ROOT=$(cd ../../../; pwd)


I continue not to like this use of (cd ../../../; pwd). There is an environment variable (IIRC) that contains PWD in the top level Makefile. Can you see if that can be used? If its not being passed to Makefile.core.mk - it can be easily added.

tests/e2e/e2e.sh

sdake · 2019-10-25T14:09:20Z

tests/e2e/e2e.sh

-      - 172.17.255.1-172.17.255.250
-EOF
-
-  kubectl apply -f https://raw.githubusercontent.com/google/metallb/v0.8.1/manifests/metallb.yaml


if metallb works locally on your mac without ``--use-local-cluster=true` that would be ideal. Can you clarify your statement?

tests/e2e/e2e.sh

sdake · 2019-10-25T14:14:43Z

tests/e2e/e2e.sh

+#kind cluster setup
+pushd "${ISTIO_DIR}"
+# shellcheck disable=SC1091
+source "./prow/lib.sh"


see review for more information about why using a prow library is an undesirable approach. As painful as it may be, please copy the parts you need into this e2e.sh script. As followup work a refactor of the e2e for the various repos (istio/operator, istio/cni, istio/istio) can be done in https://github.com/istio/common-files.

tests/e2e/e2e.sh

sdake · 2019-10-25T14:19:07Z

@howardjohn would you mind reviewing?

howardjohn · 2019-10-25T15:19:04Z

tests/e2e/e2e.sh

 mkdir -p "${ARTIFACTS}/out"

-ISTIO_DIR="${GOPATH}/src/istio.io/istio"
+ISTIO_DIR="${ROOT}/src/istio.io/istio"


It needs to be src/istio.io/istio, since prow ~~will~~ should check out istio/istio for us instead of doing the clone here.
And to your other concern, I think the clone only happens if the folder doesn't exist

howardjohn · 2019-10-25T15:19:53Z

tests/e2e/e2e.sh

+pushd "${ISTIO_DIR}"
+# shellcheck disable=SC1091
+source "./prow/lib.sh"
+setup_kind_cluster kindest/node:v1.15.3


I don't think we should explicitly set the node here, or this will break when we update kind. Really, any of this can break if we update ./prow/lib.sh so its already sort of at risk

right, my suggestion was to keep this per repo - and then follow this work up with a common-files replication. agree re node, we should bake node into the buildtools image imo.

removed the node images here

howardjohn

This is pretty misleading, you are not actually testing the PR

You set the HUB and TAG, build docker images, but they are not used.

When you pass HUB and TAG to the test, it is not used because the operator part of the test doesn't support it.

When you render the operator install yaml, it has the hardcoded hub and tag: https://storage.googleapis.com/istio-prow/pr-logs/pull/istio_operator/456/e2e_operator/115/artifacts/simple-auth-test-a9a5b04cf9db4c37a5fe/yaml/istio-operator.yaml

So anything you are doing with docker and hubs and tags is entirely unused as far as I can tell

howardjohn · 2019-10-25T19:43:01Z

+1 We should bake the default node image into the build. I tried unsuccessfully

…

On Fri, Oct 25, 2019 at 11:27 AM Steven Dake ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tests/e2e/e2e.sh <#456 (comment)>: > done -kubectl get pods --all-namespaces -o wide + +#kind cluster setup +pushd "${ISTIO_DIR}" +# shellcheck disable=SC1091 +source "./prow/lib.sh" +setup_kind_cluster kindest/node:v1.15.3 right, my suggestion was to keep this per repo - and then follow this work up with a common-files replication. agree re node, we should bake node into the buildtools image imo. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#456?email_source=notifications&email_token=AAEYGXOBHBWCZ7ACQHP7UC3QQM3C5A5CNFSM4JESAGR2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJJDV4I#discussion_r339183837>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXIPQLCYCGGEGALZYJTQQM3C5ANCNFSM4JESAGRQ> .

irisdingbj · 2019-10-28T01:19:12Z

This is pretty misleading, you are not actually testing the PR

You set the HUB and TAG, build docker images, but they are not used.

When you pass HUB and TAG to the test, it is not used because the operator part of the test doesn't support it.

When you render the operator install yaml, it has the hardcoded hub and tag: https://storage.googleapis.com/istio-prow/pr-logs/pull/istio_operator/456/e2e_operator/115/artifacts/simple-auth-test-a9a5b04cf9db4c37a5fe/yaml/istio-operator.yaml

So anything you are doing with docker and hubs and tags is entirely unused as far as I can tell

good catch! Thanks. I will fix this.

howardjohn

looks good, just a couple minor comments. I think this is in a good state though

tests/e2e/e2e.sh

irisdingbj · 2019-10-28T02:28:57Z

looks good, just a couple minor comments. I think this is in a good state though

Thanks @howardjohn seems it fails for istioctl kube-inject this time.. not sure the reason yet

howardjohn · 2019-10-28T02:34:58Z

the istioctl inject is not related to this pr, I have a PR on istio/istio to fix it

irisdingbj · 2019-10-28T04:49:33Z

istio/istio#18352

irisdingbj · 2019-10-28T14:18:50Z

/retest

sdake · 2019-10-28T14:38:35Z

/test e2e_operator

sdake

This is pretty close and I feel comfortable merging assuming the unresolved issues are addressed in a followup iterative PR. Assuming this PR can pass the checks, we can merge it now and follow up with fixes for the outstanding issues. If this PR still blocks in the checks, @irisdingbj can you get it passing as soon as viable?

sdake · 2019-10-28T14:40:19Z

deploy/operator.yaml

          - istio-operator
          - server
-          imagePullPolicy: Always
+          imagePullPolicy: IfNotPresent


@ostromart I believe I like Always as a pull policy better. I understand why this PR changes the default pull policy. If Always is specified, docker will pull the image in all cases, even though it may not be rermotely pushed.

A sed operation on this line may make sense. Do you have any thoughts on what is bettter?

sdake · 2019-10-28T14:41:32Z

tests/e2e/e2e.sh

+TAG=istio-testing

 function setup_docker() {
  HUB=istio-testing TAG=1.5-dev make controller docker


will address in a followon PR if we can get this PR merged today.

sdake · 2019-10-28T14:42:07Z

tests/e2e/e2e.sh

 mkdir -p "${ARTIFACTS}/out"

-ISTIO_DIR="${GOPATH}/src/istio.io/istio"
+ISTIO_DIR="${ROOT}/src/istio.io/istio"


@howardjohn can you clarify your suggestion here?

sdake · 2019-10-28T14:43:59Z

tests/e2e/e2e.sh

  make istioctl
-
-  HUB=gcr.io/istio-testing TAG=latest E2E_ARGS="--use_operator --test_logs_path=${ARTIFACTS}" make e2e_simple_run
+  HUB="${HUB}" TAG="${TAG}" E2E_ARGS="--use_operator --use_local_cluster=true --test_logs_path=${ARTIFACTS}" make e2e_simple_noauth_run


this HUB in use doesn't match the HUB at line 26.

sdake · 2019-10-28T14:45:12Z

tests/e2e/testdata/operator.yaml

@@ -0,0 +1,44 @@
+apiVersion: apps/v1


why not use sed to generate this file dynamically to avoid duplication of the manifests which may become inconsistent over time?

istio-testing · 2019-10-28T14:53:20Z

@irisdingbj: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
e2e_operator	`7675f39`	link	`/test e2e_operator`

Details

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. I understand the commands that are listed here.

sdake · 2019-10-28T15:00:15Z

groan - I had forgot the e2e tests were non-voting. I'll take a look at fixing this up today - if not @irisdingbj - can you take a look at fixing when you start your day.

Cheers
-steve

irisdingbj requested a review from a team as a code owner October 24, 2019 10:25

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 24, 2019

istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 24, 2019

irisdingbj changed the title ~~Operator e2e Fix~~ [WIP] Operator e2e Fix Oct 24, 2019

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Oct 24, 2019

sdake reviewed Oct 24, 2019

View reviewed changes

sdake reviewed Oct 25, 2019

View reviewed changes

irisdingbj force-pushed the e2e branch from d885cd9 to 565efea Compare October 25, 2019 06:15

irisdingbj changed the title ~~[WIP] Operator e2e Fix~~ Operator e2e Fix Oct 25, 2019

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Oct 25, 2019

Support Operator e2e

f0afbc5

irisdingbj force-pushed the e2e branch from 8e6ae89 to f0afbc5 Compare October 25, 2019 09:26

Support Operator e2e

3e05e4a

irisdingbj changed the title ~~Operator e2e Fix~~ [WIP]Operator e2e Fix Oct 25, 2019

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Oct 25, 2019

irisdingbj added 3 commits October 25, 2019 18:12

Load Images for kind nodes

ca343ae

Load Images for kind nodes

7e1763b

Load Images for kind nodes

897eb28

irisdingbj changed the title ~~[WIP]Operator e2e Fix~~ Operator e2e Fix Oct 25, 2019

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Oct 25, 2019

Load Images for kind nodes

0e9dfbd

sdake reviewed Oct 25, 2019

View reviewed changes

howardjohn reviewed Oct 25, 2019

View reviewed changes

Review Comments

da31cea

howardjohn reviewed Oct 28, 2019

View reviewed changes

tests/e2e/e2e.sh Outdated Show resolved Hide resolved

tests/e2e/e2e.sh Outdated Show resolved Hide resolved

tests/e2e/e2e.sh Show resolved Hide resolved

Review Comments

7675f39

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 28, 2019

sdake approved these changes Oct 28, 2019

View reviewed changes

istio-testing merged commit 3610717 into istio:master Oct 28, 2019

irisdingbj mentioned this pull request Oct 29, 2019

Refine e2e implementation #477

Merged

sdake mentioned this pull request Apr 29, 2020

istioctl: allow setting remote istiod port istio/istio#23357

Closed

Conversation

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

Uh oh!

Uh oh!

sdake left a comment

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!

Uh oh!

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!

sdake 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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

irisdingbj commented Oct 25, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sdake left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sdake commented Oct 25, 2019

Uh oh!

irisdingbj commented Oct 24, 2019 •

edited

Loading