[test-framework] Improve Kubernetes and Mixer support. #8912

ozevren · 2018-09-22T01:16:51Z

Changed istio-system command-line flag and calculation behavior, so that the default system namespace is "istio-system" when nothing is specified. As we make more progress on being able to deploy dynamically to non-"istio-system" namespaces, we can change the behavior.
Added generic DumpState support to environment interface, to allow dumping of state of later investigation. This should be particularly useful in CI environments.
Sprinkled calls to DumpState everywhere. Ideally we should have a centralized way to do this. Unfortunately, I haven't figured out a clean way
to do this holistically yet.
Improved Pod Data dump to include both the pod logs, as well as the current snapshot of the Pod resource in Yaml format. Both get dumped to the work directory, in case of failures.
Fine-tuned and normalized retry timing characteristics for all Kubernetes operations.
Added logic to extract attribute manifest directly from the Helm charts. The attribute manifest needs to be injected during local environment runs, but is not needed in the Kubernetes environment. This approach allows us to have identical configuration for Mixer that can be used in both Local and Kubernetes environments.
Added a retry logic to Mixer bypass adapter's connectivity logic to help issues with initial connectivity after config push.
Introduced framework/repository as a place for common constants for Istio source repository.
Removed the logic that deletes collected reports from the PolicyBackend fake, to reduce state management that needs to be done in the TestFramework.
Updated PolicyBackend's APIs to do a subset query of reports, instead of exact match. This is needed, as unrelated telemetry is also collected (i.e. Mixer's own telemetry) during testing.
Added retry logic in a few places in the Kubernetes code to deal with eventual consistency.
Added template expansion support to Mixer instances.
Fixed the issue with PolicyBackend deployment in CircleCI environment (ImagePullPolicy Always => IfNotPresent)
Fixed basic Mixer tests to work in both environments.

codecov · 2018-09-23T03:44:11Z

Codecov Report

Merging #8912 into master will decrease coverage by 1%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #8912    +/-   ##
=======================================
- Coverage      70%     70%   -<1%     
=======================================
  Files         416     416            
  Lines       37604   37921   +317     
=======================================
+ Hits        26098   26306   +208     
- Misses      10314   10417   +103     
- Partials     1192    1198     +6

Impacted Files	Coverage Δ
mixer/adapter/bypass/bypass.go	`65% <100%> (+3%)`	⬆️
mixer/adapter/servicecontrol/handler.go	`38% <0%> (-16%)`	⬇️
mixer/adapter/solarwinds/log_handler.go	`58% <0%> (-11%)`	⬇️
...ilot/pkg/networking/core/v1alpha3/networkfilter.go	`34% <0%> (-9%)`	⬇️
mixer/adapter/signalfx/tracing.go	`80% <0%> (-2%)`	⬇️
mixer/adapter/servicecontrol/utils.go	`88% <0%> (-1%)`	⬇️
galley/pkg/kube/source/listener.go	`96% <0%> (ø)`	⬇️
pilot/pkg/networking/core/v1alpha3/listener.go	`75% <0%> (ø)`	⬇️
mixer/template/template.gen.go	`0% <0%> (ø)`	⬆️
mixer/pkg/attribute/mutableBag.go	`100% <0%> (ø)`	⬆️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 90ea574...929f4fb. Read the comment docs.

smg223 · 2018-09-23T07:59:10Z

Can u tell me how to use a github repository to detect imsi-catchers?

…

On Sun, Sep 23, 2018 at 12:06 AM istio-bot ***@***.***> wrote: @ozevren <https://github.com/ozevren>: The following test *failed*, say /retest to rerun them all: Test name Commit Details Rerun command prow/e2e-mixer-no_auth-mcp.sh 99936ef <99936ef> link <https://k8s-gubernator.appspot.com/build/istio-prow/pull/istio_istio/8912/e2e-mixer-no_auth-mcp/51/> /test e2e-mixer-no_auth-mcp 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 you are subscribed to this thread. Reply to this email directly, view it on GitHub <#8912 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ApekrfWQkTVv8z-hh-_n88b6D_DEDx92ks5udzMIgaJpZM4W1Dk0> .

+ Changed istio-system command-line flag and calculation behavior, so that the default system namespace is "istio-system" when nothing is specified. As we make more progress on being able to deploy dynamically to non-"istio-system" namespaces, we can change the behavior. + Added generic DumpState support to environment interface, to allow dumping of state of later investigation. This should be particularly useful in CI environments. + Sprinkled calls to DumpState everywhere. Ideally we should have a centralized way to do this. Unfortunately, I haven't figured out a clean way to do this holistically yet. + Improved Pod Data dump to include both the pod logs, as well as the current snapshot of the Pod resource in Yaml format. Both get dumped to the work directory, in case of failures. + Fine-tuned and normalized retry timing characteristics for all Kubernetes operations. + Added logic to extract attribute manifest directly from the Helm charts. The attribute manifest needs to be injected during local environment runs, but is not needed in the Kubernetes environment. This approach allows us to have identical configuration for Mixer that can be used in both Local and Kubernetes environments. + Added a retry logic to Mixer bypass adapter's connectivity logic to help issues with initial connectivity after config push. + Introduced framework/repository as a place for common constants for Istio source repository. + Removed the logic that deletes collected reports from the PolicyBackend fake, to reduce state management that needs to be done in the TestFramework. + Updated PolicyBackend's APIs to do a subset query of reports, instead of exact match. This is needed, as unrelated telemetry is also collected (i.e. Mixer's own telemetry) during testing. + Added retry logic in a few places in the Kubernetes code to deal with eventual consistency. + Added template expansion support to Mixer instances. + Fixed the issue with PolicyBackend deployment in CircleCI environment (ImagePullPolicy Always => IfNotPresent) + Fixed basic Mixer tests to work in both environments.

mixer/adapter/bypass/bypass.go

pkg/test/framework/components/mixer/attrmanifest.go

nmittler · 2018-09-24T15:40:27Z

pkg/test/framework/components/policybackend/policybackend.go

-        image: {{.Hub}}/test_policybackend:{{.Tag}}
-        imagePullPolicy: {{.ImagePullPolicy}}
+        image: "{{.Hub}}/test_policybackend:{{.Tag}}"
+        imagePullPolicy: IfNotPresent


Why hard-coding? For the test framework, I would think we would want Always by defaultt to ensure we're always using the latest image.

This causes problems in CircleCI (Hence the change to IfNotPresent). I had it as Always for precisely the same reason as you mentioned.

We currently do not have a "CI" flag. Perhaps we can add something like that, to alter behavior.

pkg/test/framework/components/policybackend/policybackend.go

nmittler · 2018-09-24T15:47:45Z

pkg/test/framework/environment/environment.go

@@ -110,6 +110,9 @@ type (
 	Implementation interface {
 		// EnvironmentID is the unique ID of the implemented environment.
 		EnvironmentID() settings.EnvironmentID
+
+		// Evaluate the given template with environment specific template variables.
+		Evaluate(tmpl string) (string, error)


Just a note: I suspect we'll have to think a bit about this ... we need a different approach here going forward. The tests shouldn't have environment-specific things in them.

This is somewhat inevitable at this point. For example, we have intrinsic tie to the namespaces. Things like test/deployment/system namespace will frequently be referenced from tests.

nmittler · 2018-09-24T15:51:53Z

pkg/test/framework/repository/paths.go

+
+// Package repository have utility methods related to accessing a local istio code repository. The repository
+// path is deduced from ${GOPATH} environment variable.
+package repository


This file seems somewhat redundant with https://github.com/istio/istio/blob/master/pkg/test/env/istio.go.

I suggest adding the helm-specific variables there.

I aligned the packages, but would it make more sense to try to deduce things from $GO_PATH? or from $ISTIO (given that it is specified in our dev guide). I am not sure where TOP, or other ISTIO_* environment variables get defined.

nmittler · 2018-09-24T15:53:16Z

pkg/test/kube/port_forwarder.go

@@ -60,6 +62,7 @@ type PodSelectOptions struct {
 }

 func (f *defaultPortForwarder) Start() error {
+	scopes.Framework.Debugf("starting port forwarder")


how is this helpful?

It was helpful when debugging an issue. :)

nmittler

/lgtm
/approve

nmittler · 2018-09-24T19:44:08Z

/lgtm
/approve

geeknoid · 2018-09-25T00:14:44Z

/lgtm
/approve

Mixer parts look good.

istio-testing · 2018-09-25T00:14:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geeknoid, nmittler

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~mixer/OWNERS~~ [geeknoid]
~~pkg/OWNERS~~ [geeknoid,nmittler]
~~tests/integration2/OWNERS~~ [geeknoid,nmittler]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ozevren · 2018-09-25T03:10:10Z

/test e2e-mixer-no_auth

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

istio-testing requested review from geeknoid and nmittler September 22, 2018 01:16

googlebot added the cla: yes label Sep 22, 2018

ozevren removed the request for review from geeknoid September 22, 2018 01:17

ozevren changed the title ~~WIP: [test-framework] Cleanup of the Kubernetes environment and Mixer component.~~ [test-framework] Improve Kubernetes and Mixer support. Sep 24, 2018

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

nmittler reviewed Sep 24, 2018

View reviewed changes

ozben added 3 commits September 24, 2018 10:01

Accommodate CR comments.

abcb0ea

Reintroduce the environment variable to fix Istio folder resolution.

eac4ebf

Make linter happy.

929f4fb

istio-testing assigned nmittler Sep 24, 2018

nmittler approved these changes Sep 24, 2018

View reviewed changes

istio-testing added lgtm and removed lgtm labels Sep 24, 2018

istio-testing added the lgtm label Sep 24, 2018

ozevren requested a review from geeknoid September 24, 2018 19:49

istio-testing assigned geeknoid Sep 25, 2018

istio-testing added the approved label Sep 25, 2018

istio-testing merged commit 99294e3 into istio:master Sep 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[test-framework] Improve Kubernetes and Mixer support. #8912

[test-framework] Improve Kubernetes and Mixer support. #8912

ozevren commented Sep 22, 2018 •

edited

Loading

codecov bot commented Sep 23, 2018 •

edited

Loading

smg223 commented Sep 23, 2018 via email

nmittler Sep 24, 2018

ozevren Sep 24, 2018

nmittler Sep 24, 2018

ozevren Sep 24, 2018

nmittler Sep 24, 2018

ozevren Sep 24, 2018

nmittler Sep 24, 2018

ozevren Sep 24, 2018

nmittler left a comment

nmittler commented Sep 24, 2018

geeknoid commented Sep 25, 2018

istio-testing commented Sep 25, 2018

ozevren commented Sep 25, 2018

[test-framework] Improve Kubernetes and Mixer support. #8912

[test-framework] Improve Kubernetes and Mixer support. #8912

Conversation

ozevren commented Sep 22, 2018 • edited Loading

codecov bot commented Sep 23, 2018 • edited Loading

Codecov Report

smg223 commented Sep 23, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmittler left a comment

Choose a reason for hiding this comment

nmittler commented Sep 24, 2018

geeknoid commented Sep 25, 2018

istio-testing commented Sep 25, 2018

ozevren commented Sep 25, 2018

ozevren commented Sep 22, 2018 •

edited

Loading

codecov bot commented Sep 23, 2018 •

edited

Loading