Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

test(*): add retry policy e2e #4600

Merged
merged 7 commits into from
May 31, 2022
Merged

Conversation

shalier
Copy link
Contributor

@shalier shalier commented Mar 20, 2022

Description:
Adds E2E testing for retry policy

Testing done:

Affected area:

Functional Area
Tests [x ]

Please answer the following questions with yes/no.

  1. Does this change contain code from or inspired by another project?

    • Did you notify the maintainers and provide attribution?
  2. Is this a breaking change?

  3. Has documentation corresponding to this change been updated in the osm-docs repo (if applicable)?

@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2022

Codecov Report

Merging #4600 (9b8c6f7) into main (53a2238) will decrease coverage by 0.48%.
The diff coverage is n/a.

❗ Current head 9b8c6f7 differs from pull request most recent head e9b3636. Consider uploading reports for the commit e9b3636 to get more accurate results

@@            Coverage Diff             @@
##             main    #4600      +/-   ##
==========================================
- Coverage   69.55%   69.06%   -0.49%     
==========================================
  Files         220      225       +5     
  Lines       15800    16365     +565     
==========================================
+ Hits        10990    11303     +313     
- Misses       4760     5010     +250     
- Partials       50       52       +2     
Flag Coverage Δ
unittests 69.06% <ø> (-0.49%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/cli/verifier/sidecar.go 65.95% <0.00%> (-18.42%) ⬇️
pkg/cli/verifier/envoy_config.go 65.56% <0.00%> (-6.48%) ⬇️
pkg/envoy/registry/registry.go 87.50% <0.00%> (-5.10%) ⬇️
pkg/service/types.go 66.66% <0.00%> (-4.77%) ⬇️
pkg/envoy/ads/server.go 50.00% <0.00%> (-1.79%) ⬇️
pkg/configurator/client.go 88.97% <0.00%> (-1.14%) ⬇️
pkg/envoy/proxy.go 85.48% <0.00%> (-1.08%) ⬇️
cmd/osm-controller/osm-controller.go 13.93% <0.00%> (-0.92%) ⬇️
pkg/catalog/outbound_traffic_policies.go 94.47% <0.00%> (-0.20%) ⬇️
pkg/k8s/util.go 96.03% <0.00%> (-0.20%) ⬇️
... and 58 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 53a2238...e9b3636. Read the comment docs.

Signed-off-by: Shalier Xia <shalierxia@microsoft.com>
Signed-off-by: Shalier Xia <shalierxia@microsoft.com>
Comment on lines 124 to 125
// wait for server
time.Sleep(3 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not something to change in this PR, but if this is to wait for the retry policy to have taken effect, a better solution might be to add a status to retries like we have for IngressBackends which can be queried whereas a sleep like this is a roll of the dice to a degree.

// IngressBackendStatus is the type used to represent the status of an IngressBackend resource.
type IngressBackendStatus struct {
// CurrentStatus defines the current status of an IngressBackend resource.
// +optional
CurrentStatus string `json:"currentStatus,omitempty"`
// Reason defines the reason for the current status of an IngressBackend resource.
// +optional
Reason string `json:"reason,omitempty"`
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

demo/cmd/retry/retry.go Outdated Show resolved Hide resolved
Signed-off-by: Shalier Xia <shalierxia@microsoft.com>
Signed-off-by: Shalier Xia <shalierxia@microsoft.com>
}

By("A request that will be retried NumRetries times then succeed")
time.Sleep(3 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my own understanding, why are we sleeping 3 seconds here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sleep was to wait for the retry policy to be applied, there's an issue to add a Status field for Retry in the future so that this won't be needed. #4600 (comment)

Signed-off-by: Shalier Xia <shalierxia@microsoft.com>
Signed-off-by: Shalier Xia <shalierxia@microsoft.com>
var _ = OSMDescribe("Test Retry Policy",
OSMDescribeInfo{
Tier: 2,
Bucket: 8,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: move to bucket 4 which typically completes sooner than other buckets. This helps in keeping the bucket times more balanced.

Signed-off-by: Shalier Xia <shalierxia@microsoft.com>
@shashankram shashankram merged commit 28ed531 into openservicemesh:main May 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants