Pilot unit and integration test fixes by seflerZ · Pull Request #19790 · istio/istio

seflerZ · 2019-12-25T17:04:10Z

This PR resolves two issues in Pilot component:
~~✔ Fixed the corrupted docker building and unit testing process.~~(Moved here)
✔ Fixed some tests not fully applied to namespace isolation in integration tests.

I'll explain those in details:
📃 The Istio integration test framework supports namespace customizing while deploying Istio despite the default "istio-system". But not all tests obeyed this instruction and even some of the configurations were hard coded in source. I fixed them up and tested.

⚠ Other Istio components may also have the same issues and need fix up but I focused on Pilot only this time.

This reverts commit bd6d803a9ed81869e27ebff96f9c9d66678b9c56.

…ful"

This reverts commit ae9b612b2416d640e58afb4850d109eb11e5e1f9.

…l be added in 'stats.go' automatically

istio-testing · 2019-12-25T17:04:25Z

Hi @seflerZ. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

seflerZ · 2019-12-25T17:05:10Z

Makefile

 endif

-RUN = $(CONTAINER_CLI) run -t -i --sig-proxy=true -u $(UID):$(GID) --rm \
+RUN = $(CONTAINER_CLI) run --sig-proxy=true -u $(UID):$(GID) --rm \


This is no need to start the container in interactive and terminal mode because this is a automation process. And it will cause some trouble if the caller process didn't get assigned to a tty, so I just removed them.

I think there are commands that run in a shell? I think there's a make shell that drops you into a shell in the docker container. Maybe there is a way to automatically detect this?

I didn't find the "make shell" command and if I execute "make shell" in Istio src root. I got "make: *** No rule to make target 'shell'. Stop." error.

The "-t -i" options are used for user inputting from TTY. This is a automation process, no inputs needed from user. Removing this option doesn't affect shell script executing within docker container.

In some cases, if the caller doesn't get assigned to a shell (for example, a process started the building process), it will fail with error "input device is not a TTY". Therefore, I think it's better to remove this option.

Its in another repo, there is a pr out to add the Target to all repos. I don't think there is any reason we need to limit this to not user input

Okay, this patch can be ignored. I'll remove it later.

seflerZ · 2019-12-25T17:08:35Z

pilot/cmd/pilot-agent/status/ready/probe_test.go


 	server := createAndStartServer(liveServerStats)
 	defer server.Close()
-	probe := Probe{LocalHostAddr: "localhost", AdminPort: 1234}


Request HTTP without host is not allowed in HTTP1.1. I added them in some test cases, but encountered more empty host requests in test cases with updates from master branch. So I wrote a check in 'stats.go'(in the next diff window) and removed the redundant configurations.

seflerZ · 2019-12-25T17:09:41Z

@howardjohn Would you please take a look?

hzxuzhonghu · 2019-12-26T01:54:10Z

/ok-to-test

Just one question: Isn't this run in the current CI? Why doesn't it fail?

seflerZ · 2019-12-26T02:19:22Z

/ok-to-test

Just one question: Isn't this run in the current CI? Why doesn't it fail?

The Istio CI runs tests and CI in local building tool chain, not docker and applied the default namespace "istio-system", so we wouldn't encounter those issues.

howardjohn

test changes lgtm

howardjohn · 2019-12-26T01:55:29Z

Makefile

 endif

-RUN = $(CONTAINER_CLI) run -t -i --sig-proxy=true -u $(UID):$(GID) --rm \
+RUN = $(CONTAINER_CLI) run --sig-proxy=true -u $(UID):$(GID) --rm \


I think there are commands that run in a shell? I think there's a make shell that drops you into a shell in the docker container. Maybe there is a way to automatically detect this?

howardjohn · 2019-12-26T02:23:21Z

Makefile

 	--mount type=volume,source=gocache,destination="/gocache" \
 	$(CONDITIONAL_HOST_MOUNTS) \
-	-w /work $(IMG)
+	-w /work/src/istio.io/istio $(IMG)


a couple notes on this

This file comes from the common-files repo so it shouldn't be modified there.

Moving the src to a path like this seems pretty reasonable, especially if we start mounting other local repos so you can reference them with ../api for example. Would like @sdake opinion here as well.
Note that the build tools support for this repo was merged in a few days ago only and is off by default due to a few minor issues like this one. If we don't decide to change the folder structure like this I have a patch locally I was working on that fixes the same problem

Thanks @howardjohn. If we don't change the path, run unit tests through "make pilot-test" command will fall directly with env BUILD_WITH_CONTAINER=1.

I understand, but there are other options to fix this as well. We should make sure this won't have other negative impact

Okay, what's the next step? Wait for review from @sdake ?

Yes, but it will need to be in the common-files repo. If we remove this part from the pr I'll approve all the other changes

but yes I think we should get Steve to look at this, he put a lot of work into getting this working across all the repos and I am not familiar with why all of it works the way it does

Okay, Thanks. I'll remove this and leave it there.

howardjohn

thanks!

howardjohn · 2019-12-26T05:24:38Z

Makefile

 	$(DOCKER_SOCKET_MOUNT) \
 	$(CONTAINER_OPTIONS) \
-	--mount type=bind,source="$(PWD)",destination="/work" \
+	--mount type=bind,source="$(GOPATH)",destination="/work" \


what's the motivation for this? I think gopath is optional now

oh nevermind we are mouning the entire gopath rather than just the working directory.

howardjohn · 2019-12-26T05:26:12Z

Makefile

 	--mount type=volume,source=gocache,destination="/gocache" \
 	$(CONDITIONAL_HOST_MOUNTS) \
-	-w /work $(IMG)
+	-w /work/src/istio.io/istio $(IMG)


but yes I think we should get Steve to look at this, he put a lot of work into getting this working across all the repos and I am not familiar with why all of it works the way it does

seflerZ and others added 19 commits December 25, 2019 22:35

have a test

d2aa77b

Revert "have a test"

f62e2b7

This reverts commit bd6d803a9ed81869e27ebff96f9c9d66678b9c56.

improved compatability of unit test "TestEnvoyStatsCompleteAndSuccess…

2f0f7ad

…ful"

addressed an issue of namespace configuration in CI tests

40d4e2f

improved robust of integration test

8c7c28d

fixed compatibility issue in 'mirror_test.go'

fe6c509

improved compatibility of CI tests in 'util.go'

331c653

Revert "improved compatibility of CI tests in 'util.go'"

58683d7

This reverts commit ae9b612b2416d640e58afb4850d109eb11e5e1f9.

fixed bugs in building with docker image

c85f276

format Makefile

ddee74d

remove docker run terminal emulator because there is no need

2f7b7d4

remove unnecessary env var

a5df297

fixed format issue

e580d6c

removed 'localhostAddr' configuration in probe because empty host wil…

2afa90a

…l be added in 'stats.go' automatically

add host if it is empty

5eddf22

removed un-used tests compared to the community mater

10fa558

adopt to gobin

f5b4618

fixed corrupted docker building process

294e11f

format fixes

463f39d

seflerZ requested review from a team, costinm, howardjohn, linsun and rshriram as code owners December 25, 2019 17:04

istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test labels Dec 25, 2019

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

seflerZ commented Dec 25, 2019

View reviewed changes

istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Dec 26, 2019

howardjohn reviewed Dec 26, 2019

View reviewed changes

seflerZ added 4 commits December 26, 2019 10:24

fixed format

f90ffe4

remove changes in docker executing command

732e17d

remove commands in docker executing command

d5a2470

rollback make file changes

89bf1f7

seflerZ requested a review from howardjohn December 26, 2019 08:54

howardjohn approved these changes Dec 26, 2019

View reviewed changes

istio-testing merged commit f0218ef into istio:master Dec 26, 2019

seflerZ deleted the integration-unit-test-fixes branch December 27, 2019 02:48

Conversation

seflerZ commented Dec 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-testing commented Dec 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seflerZ Dec 26, 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!

seflerZ Dec 26, 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!

seflerZ commented Dec 25, 2019

Uh oh!

hzxuzhonghu commented Dec 26, 2019

Uh oh!

seflerZ commented Dec 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

seflerZ Dec 26, 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!

seflerZ Dec 26, 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!

seflerZ Dec 26, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

seflerZ commented Dec 25, 2019 •

edited

Loading

seflerZ Dec 26, 2019 •

edited

Loading

seflerZ Dec 26, 2019 •

edited

Loading

seflerZ commented Dec 26, 2019 •

edited

Loading

seflerZ Dec 26, 2019 •

edited

Loading

seflerZ Dec 26, 2019 •

edited

Loading

seflerZ Dec 26, 2019 •

edited

Loading