GoogleCloudPlatform / k8s-cloud-provider

Support code for implementing a Kubernetes cloud provider for Google Cloud Platform
Apache License 2.0
37 stars 46 forks source link

implement Parallel executor #186

Closed kl52752 closed 7 months ago

kl52752 commented 7 months ago

Implement parallel executor based on parallel queue

This change is implemented on top of refactor executor PR

In this change other changes were introduced:

kl52752 commented 7 months ago

/cc @mag-kol /cc @AwesomePatrol /cc @akwi-github

bowei commented 7 months ago

Please make sure your commit messages are the right format and are descriptive.

kl52752 commented 7 months ago

Please make sure your commit messages are the right format and are descriptive.

This PR is created based on refactor executor. I will fix the message once we decide on refactor strategy :)

kl52752 commented 7 months ago

@bowei I added changes we discussed offline PTAL /assign @bowei

kl52752 commented 7 months ago

Have we run this with the thread sanitizer?

I run parallel_executor ut tests with go test -race options and I run I e2e test which I modify locally to use parallel_executor

go test -race -timeout 30s -run ^TestParallelExecutor* github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/rgraph/exec -v --- PASS: TestParallelExecutor (0.01s) --- PASS: TestParallelExecutor/empty_graph (0.00s) --- PASS: TestParallelExecutor/one_action (0.00s) --- PASS: TestParallelExecutor/action_and_dependency (0.00s) --- PASS: TestParallelExecutor/chain_of_3_actions (0.00s) --- PASS: TestParallelExecutor/two_chains_with_common_root (0.00s) --- PASS: TestParallelExecutor/two_node_cycle (0.00s) --- PASS: TestParallelExecutor/lot_of_children (0.00s) --- PASS: TestParallelExecutor/complex_fan_in (0.00s) --- PASS: TestParallelExecutor/cycle_in_larger_graph (0.00s) --- PASS: TestParallelExecutor/error_in_action (0.00s) --- PASS: TestParallelExecutorErrorStrategy (0.00s) --- PASS: TestParallelExecutorErrorStrategy/linear_graph_StopOnError (0.00s) --- PASS: TestParallelExecutorErrorStrategy/linear_graph_ContinueOnError (0.00s) --- PASS: TestParallelExecutorErrorStrategy/branched_graph_StopOnError (0.00s) --- PASS: TestParallelExecutorErrorStrategy/branched_graph_ContinueOnError (0.00s) PASS ok github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/rgraph/exec 1.254s

go[101/328]bin/go test -race -timeout 30m -run ^TestRgraphTCPRouteAddBackends$ github.com/GoogleCloudPlatform/k8s-cloud-provider/e2e -v -project katarzynalach-gke-dev -resourcePrefix rgraph-tcp
=== RUN TestRgraphTCPRouteAddBackends --- PASS: TestRgraphTCPRouteAddBackends (212.86s) PASS ok github.com/GoogleCloudPlatform/k8s-cloud-provider/e2e 214.140s

aojea commented 7 months ago

if you add this commit then the CI will do it for us anytime :)

diff --git a/Makefile b/Makefile
index b91b4ee..fa8e51a 100644
--- a/Makefile
+++ b/Makefile
@@ -30,7 +30,7 @@ build: gen
 test: gen
        # Test only the library. e2e must be run in a special environment,
        # so is skipped.
-       go test ./pkg/...
+       go test -race ./pkg/...
        # We cannot use golint currently due to errors in the GCP API naming.
        # golint ./...
        go vet ./...
kl52752 commented 7 months ago

Thanks @aojea I will do it in separate PR

bowei commented 7 months ago

Mostly LGTM, some minor comments.

kl52752 commented 7 months ago

Mostly LGTM, some minor comments.

resolved

bowei commented 7 months ago

Please do the Makefile change in a separate PR.

bowei commented 7 months ago

I would like to see some more adversarial testing. This can be a follow up.

a -> b
a -> c

set timeout
c blocks; b returns error

test both modes

Really try to break it and find race conditions.

kl52752 commented 7 months ago

I would like to see some more adversarial testing. This can be a follow up.

a -> b
a -> c

set timeout
c blocks; b returns error

test both modes

Really try to break it and find race conditions.

hi I tested both scenarios (timeouting executor with TimeoutOptions and using context with canceled passed by user). Please see tests scenarios and results in this PR

kl52752 commented 7 months ago

Minor comments. Otherwise LGTM

fixed, PTAL

bowei commented 7 months ago

There are still changes to be made, but let's put them in follow on PRs:

bowei commented 7 months ago

/lgtm /approve

google-oss-prow[bot] commented 7 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, kl52752

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: - ~~[OWNERS](https://github.com/GoogleCloudPlatform/k8s-cloud-provider/blob/master/OWNERS)~~ [bowei] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment