Network integration image test #434

gut · 2018-06-03T01:48:58Z

No description provided.

gut · 2018-06-03T01:53:20Z

Note: since googleapis/google-api-python-client#499, the python test that authenticate are actually broken. I'll fix it afterwards for all image_tests, ok?

gut · 2018-06-04T13:45:47Z

daisy_workflows/image_test/network/network-tester.py

+  # traffic from a different IP even if the router is sending traffic to it.
+  # This was a test to guarantee that behavior.
+  try:
+    TestForwardingRule(MM(compute, testee), testee)


forgot to raise an exception here: the TestForwardingRule should not work on this case. Hang on..

adjackura · 2018-06-05T20:01:53Z

/ok-to-test

codecov · 2018-06-05T20:04:51Z

Codecov Report

❗ No coverage uploaded for pull request base (master@c184a43). Click here to learn what that means.
The diff coverage is 47.46%.

@@            Coverage Diff            @@
##             master     #434   +/-   ##
=========================================
  Coverage          ?   71.76%           
=========================================
  Files             ?       39           
  Lines             ?     3814           
  Branches          ?        0           
=========================================
  Hits              ?     2737           
  Misses            ?      879           
  Partials          ?      198

Flag	Coverage Δ
#go_unittests	`71.76% <47.46%> (?)`

Impacted Files	Coverage Δ
daisy/region.go	`0% <0%> (ø)`
daisy/step.go	`75.37% <0%> (ø)`
daisy/workflow.go	`79.78% <100%> (ø)`
daisy/forwarding_rule.go	`25.49% <25.49%> (ø)`
daisy/target_instance.go	`27.65% <27.65%> (ø)`
daisy/resource.go	`71.25% <31.81%> (ø)`
daisy/compute/test_client.go	`87.77% <54.54%> (ø)`
daisy/compute/compute.go	`67.94% <55.37%> (ø)`
daisy/step_create_forwarding_rule.go	`75% <75%> (ø)`
daisy/step_create_target_instance.go	`75% <75%> (ø)`

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 c184a43...7371fa9. Read the comment docs.

gut · 2018-06-05T21:55:24Z

rebased PR and adjusted content

adjackura · 2018-06-07T19:44:10Z

I feel like a lot of this logic could and should be moved into the daisy workflow. I don't like the idea of maintaining an additional compute library just for tests when daisy can set all that up.

For example if we added steps to add forwarding rules, that way we don't need to add all the logic in for cleanup. This may require a test case for the positive case and the negative but thats ok if it means less code.

gut · 2018-06-07T20:35:37Z

I feel like a lot of this logic could and should be moved into the daisy workflow. I don't like the idea of maintaining an additional compute library just for tests when daisy can set all that up.

That's right. It seems like a bunch of reimplementation, right? There is daisy, the python library, this utils.py... I may reanalize that as you suggested.

For example if we added steps to add forwarding rules, that way we don't need to add all the logic in for cleanup. This may require a test case for the positive case and the negative but thats ok if it means less code.

I can start by implementing this part but I guess we need to reassess what is being done on this python tests.

If that works well, then I'd rather send a new pull request than to update this one, if that's ok for you. If so, please close this without merging.

gut · 2018-06-15T00:59:44Z

@zmarano btw, Is this test wrongly defined?
Ensure routes are added when enabling IP Forwarding

I expect it to be
Ensure routes are added when enabling ForwardingRule

right?

gut · 2018-06-15T01:06:33Z

oh, I just noticed some confusing terms:
https://github.com/GoogleCloudPlatform/compute-image-packages/blob/master/google_compute_engine/networking/network_daemon.py#L116

this forwardedIPs list is actually the list of ForwardingRules IPs and not the canIpForward network parameter as on: https://cloud.google.com/vpc/docs/using-routes#canipforward

gut · 2018-06-15T01:40:50Z

This PR was not closed so I just updated this one with the ForwardingRule and TargetInstances being created on daisy. It's much better this way, thanks for the idea!

gut · 2018-06-15T01:51:17Z

ops, I completely forgot to extend the go tests. Hang on

gut · 2018-06-15T04:35:04Z

that's better :-)

gut · 2018-06-15T11:58:10Z

@adjackura the PR is once again ready to be reviewed

adjackura

Looks good, just a few nits.
Also it looks like our linter isn't working as expected, I'll have to look into that

adjackura · 2018-06-15T16:11:33Z

daisy/compute/compute.go

 	return nil
 }

+func (c *client) CreateForwardingRule(project, region string, ti *compute.ForwardingRule) error {


fr *compute.ForwardingRule

Also docstring

adjackura · 2018-06-15T16:18:42Z

daisy/compute/compute.go

 }

-func (c *client) operationsWait(project, zone, name string) error {
+type OperationGetterFunc func() (*compute.Operation, error)


Does this type need to be public?

No. I'll change it to be unexported.

adjackura · 2018-06-15T16:20:45Z

/lint

daisy-bot

@adjackura: 6 warnings.

Details

In response to this:

/lint

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.

daisy-bot · 2018-06-15T16:20:54Z

daisy/target_instance.go

+//  See the License for the specific language governing permissions and
+//  limitations under the License.
+
+package daisy


Golint comments: should have a package comment, unless it's in another file for this package. More info.

ignore this

daisy-bot · 2018-06-15T16:20:54Z

daisy/forwarding_rule.go

+//  See the License for the specific language governing permissions and
+//  limitations under the License.
+
+package daisy


Golint comments: should have a package comment, unless it's in another file for this package. More info.

ignore this

daisy-bot · 2018-06-15T16:20:54Z

daisy/step_create_target_instance.go

+//  See the License for the specific language governing permissions and
+//  limitations under the License.
+
+package daisy


Golint comments: should have a package comment, unless it's in another file for this package. More info.

ignore this

daisy-bot · 2018-06-15T16:20:55Z

daisy/compute/compute.go

 }

-func (c *client) operationsWait(project, zone, name string) error {
+type OperationGetterFunc func() (*compute.Operation, error)


Golint comments: exported type OperationGetterFunc should have comment or be unexported. More info.

daisy-bot · 2018-06-15T16:20:55Z

daisy/step_create_forwarding_rule.go

+//  See the License for the specific language governing permissions and
+//  limitations under the License.
+
+package daisy


Golint comments: should have a package comment, unless it's in another file for this package. More info.

ignore this

daisy-bot · 2018-06-15T16:20:55Z

daisy/region.go

+//  See the License for the specific language governing permissions and
+//  limitations under the License.
+
+package daisy


Golint comments: should have a package comment, unless it's in another file for this package. More info.

ignore this

daisy-bot · 2018-06-15T20:25:24Z

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

Test name	Commit	Details	Rerun command
prow/presubmit/flake8	`7371fa9`	link	`/flake8`

Full PR test history. Your PR dashboard.

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.

gut · 2018-06-15T20:32:27Z

done. Thanks

…heck Ignore long job name in knative prow config check

daisy-bot added needs-ok-to-test size/L labels Jun 3, 2018

gut mentioned this pull request Jun 3, 2018

Fix oauth2 deprecation #435

Merged

gut commented Jun 4, 2018

View reviewed changes

gut force-pushed the network-integration-test branch 2 times, most recently from 7836abd to 28ba908 Compare June 5, 2018 15:35

daisy-bot removed the needs-ok-to-test label Jun 5, 2018

gut force-pushed the network-integration-test branch from 28ba908 to 647e8f7 Compare June 5, 2018 21:54

gut mentioned this pull request Jun 7, 2018

[question/image_tests]: Create networks through daisy #436

Closed

gut force-pushed the network-integration-test branch from 647e8f7 to a6d7358 Compare June 15, 2018 01:33

daisy-bot added size/XXL and removed size/L labels Jun 15, 2018

gut mentioned this pull request Jun 15, 2018

[question/daisy] Why no region parameter? #445

Closed

gut force-pushed the network-integration-test branch from a6d7358 to 03e321c Compare June 15, 2018 04:06

adjackura requested changes Jun 15, 2018

View reviewed changes

daisy-bot reviewed Jun 15, 2018

View reviewed changes

Add CreateTargetInstances step on daisy

77faca5

Gustavo Serra Scalet added 2 commits June 15, 2018 17:23

Add ForwardingRule step on daisy

8315875

Network test: IP Alias, ForwardingRule and DNS

7371fa9

gut force-pushed the network-integration-test branch from 03e321c to 7371fa9 Compare June 15, 2018 20:24

adjackura merged commit 09972f5 into GoogleCloudPlatform:master Jun 15, 2018

gut deleted the network-integration-test branch June 15, 2018 22:38

gaohannk pushed a commit to gaohannk/compute-image-tools that referenced this pull request May 20, 2021

Merge pull request GoogleCloudPlatform#434 from chaodaiG/fix-config-c…

9eff788

…heck Ignore long job name in knative prow config check

Network integration image test #434

Network integration image test #434

Uh oh!

Conversation

gut commented Jun 3, 2018

Uh oh!

gut commented Jun 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adjackura commented Jun 5, 2018

Uh oh!

codecov bot commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gut commented Jun 5, 2018

Uh oh!

adjackura commented Jun 7, 2018

Uh oh!

gut commented Jun 7, 2018

Uh oh!

gut commented Jun 15, 2018

Uh oh!

gut commented Jun 15, 2018

Uh oh!

gut commented Jun 15, 2018

Uh oh!

gut commented Jun 15, 2018

Uh oh!

gut commented Jun 15, 2018

Uh oh!

gut commented Jun 15, 2018

Uh oh!

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

adjackura commented Jun 15, 2018

Uh oh!

daisy-bot 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!

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!

daisy-bot commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gut commented Jun 15, 2018

Uh oh!

Reviewers

codecov bot commented Jun 5, 2018 •

edited

Loading

daisy-bot commented Jun 15, 2018 •

edited

Loading