Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

conformance: use Patch to make ObservedGeneration tests robust against conflicts #1760

Merged
merged 1 commit into from
Mar 8, 2023
Merged

conformance: use Patch to make ObservedGeneration tests robust against conflicts #1760

merged 1 commit into from
Mar 8, 2023

Conversation

michaelbeaumont
Copy link
Contributor

What type of PR is this?
/kind feature
/area conformance

What this PR does / why we need it:
Using Update can lead to Conflict errors that fail the test:

the object has been modified; please apply your changes to the latest version and try again

Another option would be to use something like https://pkg.go.dev/k8s.io/client-go/util/retry#RetryOnConflict but just using Patch seems a simpler option for this case.

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 27, 2023
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 27, 2023
@michaelbeaumont michaelbeaumont force-pushed the fix/conformance_observed_gen branch from a748a7f to ed1fee8 Compare February 27, 2023 16:25
@michaelbeaumont michaelbeaumont mentioned this pull request Feb 28, 2023
Merged
6 tasks
@arkodg
Copy link
Contributor

arkodg commented Mar 1, 2023

we in Envoy Gateway might have this same issue, our tests are flaking as well envoyproxy/gateway#1016. Will test this PR out and report back

@sunjayBhatia
Copy link
Member

would have to run this in CI for a little while, but Contour generally runs into this type of issue as well

@arkodg
Copy link
Contributor

arkodg commented Mar 6, 2023

didnt seem to help in EG, here's the draft PR I tested with envoyproxy/gateway#1106

@michaelbeaumont
Copy link
Contributor Author

didnt seem to help in EG, here's the draft PR I tested with envoyproxy/gateway#1106

I may be missing it in the output but it looks like the *ObservedGenerationBump tests are passing in the checks.

mlavacca
mlavacca approved these changes Mar 7, 2023
View reviewed changes
Copy link
Member

@mlavacca mlavacca left a comment

Choose a reason for hiding this comment

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

The Kong Ingress controller did not experience this flake, but the change looks legit and did not trigger any error in our CI.

@arkodg
Copy link
Contributor

arkodg commented Mar 7, 2023

didnt seem to help in EG, here's the draft PR I tested with envoyproxy/gateway#1106

I may be missing it in the output but it looks like the *ObservedGenerationBump tests are passing in the checks.

@michaelbeaumont you're right, the ObservedGenerationBump tests pass, but some newer changes that were pulled in with your fork cause another unrelated test to fail, my bad

@dprotaso
Copy link
Contributor

dprotaso commented Mar 7, 2023

Changes looks good - but a question since I'm not familiar so much with the controller-runtime client.

When calling Patch if the supplied resource includes a resourceVersion in the metadata is that included in the patch request?

It's not really documented but if the resourceVersion is present in a PATCH it will return a 409 conflict if it doesn't match the resourceVersion on the server - knative/serving#1992

@michaelbeaumont
Copy link
Contributor Author

Changes looks good - but a question since I'm not familiar so much with the controller-runtime client.

When calling Patch if the supplied resource includes a resourceVersion in the metadata is that included in the patch request?

It's not really documented but if the resourceVersion is present in a PATCH it will return a 409 conflict if it doesn't match the resourceVersion on the server - knative/serving#1992

Looking at controller-runtime, the resourceVersion isn't included by default but can be by passing in a client.MergeFromWithOptimisticLock{} to client.MergeFromWithOptions instead of client.MergeFrom as called in the PR.

@shaneutt shaneutt added this to the v0.6.2 milestone Mar 8, 2023
@robscott
Copy link
Member

robscott commented Mar 8, 2023

Thanks @michaelbeaumont!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkodg, michaelbeaumont, mlavacca, robscott

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2023
@k8s-ci-robot k8s-ci-robot merged commit 96a786d into kubernetes-sigs:main Mar 8, 2023
@michaelbeaumont michaelbeaumont deleted the fix/conformance_observed_gen branch March 8, 2023 21:32
shaneutt pushed a commit to shaneutt/gateway-api that referenced this pull request Mar 14, 2023
@k8s-ci-robot @shaneutt
Merge pull request kubernetes-sigs#1760 from michaelbeaumont/fix/conf…
09a11ae 
…ormance_observed_gen

conformance: use Patch to make ObservedGeneration tests robust against conflicts
@arkodg arkodg mentioned this pull request Mar 21, 2023
Merged
arkodg added a commit to envoyproxy/gateway that referenced this pull request Mar 22, 2023
@arkodg
Bump to Gateway API v0.6.2 (#1153)
6936ac7 
* Bumps gateway-api version to `v0.6.2`
* Ensure the status or code do not refer to any resource as `Ready`, because we cannot guarantee that today, raised  #1171 to fix it
* Refactor `gateway-api` translator to ensure we always compute `ResolvedRefs` condition which is checked by the newer conformance tests
* Brings in kubernetes-sigs/gateway-api#1760 which removes some test flakes
  * enable`GatewayObservedGenerationBump` test
    * Fixes #1016
  * enable `HTTPRouteObservedGenerationBump` test
    * Fixes #1016
* Disable `GatewayWithAttachedRoutes` test which should be fixed with #1201

Fixes: #1152
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mlavacca mlavacca mlavacca approved these changes

@arkodg arkodg arkodg approved these changes

@mikemorris mikemorris Awaiting requested review from mikemorris

@youngnick youngnick Awaiting requested review from youngnick

@sunjayBhatia sunjayBhatia Awaiting requested review from sunjayBhatia

Assignees

@robscott robscott

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
No open projects
Gateway API: The Road to GA
Status: Done
Milestone
v0.6.2
Development

Successfully merging this pull request may close these issues.
8 participants
@michaelbeaumont @arkodg @sunjayBhatia @dprotaso @robscott @k8s-ci-robot @mlavacca @shaneutt