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.

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

use patch instead of replace to test the dry-run option #95831

Merged
merged 1 commit into from Nov 3, 2020

Conversation

cpanato
Copy link
Member

@cpanato cpanato commented Oct 23, 2020

What type of PR is this?

/kind failing-test

What this PR does / why we need it:

Was observing some jobs for other fixes we have been changing and notice this test is failing a lot

can see in this job, for example, https://testgrid.k8s.io/sig-release-master-informing#capg-conformance-v1alpha3-k8s-master and https://testgrid.k8s.io/sig-release-master-informing#capa-conformance-v1alpha3-k8s-master&width=30

The test creates a pod and get the JSON spec right away and then replaces the image to perform the actual test.
However, sometimes might have other reconciliation in the cluster that changes the resource version and when the test try to apply the change this the spec is old and the test fail.
Waiting for the pod to be running maybe is a good option and then get the spec and perform the test.
Test locally and did not reproduce the issue with this change.

error in the job:


Kubernetes e2e suite: [sig-cli] Kubectl client Kubectl server-side dry-run should check if kubectl can dry-run update Pods [Conformance] expand_less | 2s
-- | --
/workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/framework/framework.go:629 Oct 22 21:18:04.003: Unexpected error:     
<exec.CodeExitError>: {       
  Err: {          
   s: "error running /workspace/kubernetes/_output/bin/kubectl --server=https://localhost:6443 --kubeconfig=/workspace/.kube/config --namespace=kubectl-4231 replace -f - --dry-run=server:\nCommand stdout:\n\nstderr:\nError from server (Conflict): error when replacing \"STDIN\": Operation cannot be fulfilled on pods \"e2e-test-httpd-pod\": the object has been modified; please apply your changes to the latest version and try again\n\nerror:\nexit status 1",        
 },         
Code: 1,    
 }     

error running /workspace/kubernetes/_output/bin/kubectl --server=https://localhost:6443 --kubeconfig=/workspace/.kube/config --namespace=kubectl-4231 replace -f - --dry-run=server:     Command stdout:          stderr:     Error from server (Conflict): error when replacing "STDIN": Operation cannot be fulfilled on pods "e2e-test-httpd-pod": the object has been modified; please apply your changes to the latest version and try again          error:     exit status 1 occurred /workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/framework/util.go:592


Which issue(s) this PR fixes:

Fixes: #95832

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 23, 2020
@k8s-ci-robot k8s-ci-robot added area/test sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 23, 2020
@cpanato
Copy link
Member Author

cpanato commented Oct 23, 2020

/assign @soltysh

@mkumatag
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2020
@mkumatag
Copy link
Member

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 26, 2020
@dims
Copy link
Member

dims commented Oct 27, 2020

/assign @deads2k

@soltysh @deads2k one line PR, so should be quick :)

@mkumatag
Copy link
Member

/assign @deads2k

@soltysh @deads2k one line PR, so should be quick :)

We need this badly as we are hitting with this issue for every run, it will be great if someone can help us merging this asap

@deads2k
Copy link
Contributor

deads2k commented Nov 2, 2020

I'd like to know why pods were chosen. A conflict error looks to be correct in cases where there is a conflict. It seems more correct to me to use an unconditional update if that's what you want or to choose a resource with less contention.

/assign @apelisse

@deads2k
Copy link
Contributor

deads2k commented Nov 2, 2020

I'd like to know why pods were chosen. A conflict error looks to be correct in cases where there is a conflict. It seems more correct to me to use an unconditional update if that's what you want or to choose a resource with less contention.

I'd suggest going with an unconditional update to avoid the conflict error, but keep the spirit of the test.

@apelisse
Copy link
Member

apelisse commented Nov 2, 2020

@cpanato Would you mind updating with a patch rather than a replace so that you avoid the conflicts? Thanks.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 3, 2020
@cpanato cpanato changed the title wait for the pod be running to get the spec to apply the changes use patch instead of replace to test the dry-run option Nov 3, 2020
@cpanato
Copy link
Member Author

cpanato commented Nov 3, 2020

@apelisse make the change, thanks for your review PTAL

@k8s-ci-robot k8s-ci-robot added area/conformance Issues or PRs related to kubernetes conformance tests sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. labels Nov 3, 2020
@apelisse
Copy link
Member

apelisse commented Nov 3, 2020

Yeah, I think that's good and shouldn't fail like it did before.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2020
@dims
Copy link
Member

dims commented Nov 3, 2020

/assign @johnbelamaric

(for test/conformance/testdata/OWNERS)

Copy link
Member

@spiffxp spiffxp left a comment

Choose a reason for hiding this comment

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

/triage accepted
/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 3, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, spiffxp

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 Nov 3, 2020
@hh
Copy link
Member

hh commented Nov 3, 2020

/test pull-kubernetes-node-e2e

@hh
Copy link
Member

hh commented Nov 3, 2020

Keeping an eye on this as a dependency of #95832

@Riaankl
Copy link
Contributor

Riaankl commented Nov 3, 2020

/test pull-kubernetes-bazel-test

@k8s-ci-robot k8s-ci-robot merged commit 7e1f4a9 into kubernetes:master Nov 3, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Nov 3, 2020
@cpanato cpanato deleted the fix-test-flaky branch November 4, 2020 12:02
k8s-ci-robot added a commit that referenced this pull request Nov 30, 2020
…f-#94609-#94789-#95594-#95831-origin-release-1.19

Automated cherry pick of #94609: Use namespace flag passed to RunKubectl* methods #94789: Clean up remaining ns flag #95594: Fix --dry-run invocation in kubectl e2e #95831: use patch instead of replace to test the dry-run option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance Issues or PRs related to kubernetes conformance tests area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Failing Test] Kubectl client Kubectl server-side dry-run should check if kubectl can dry-run update Pods