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

Revert "conntrack e2e test debug connections" #94800

Merged
merged 1 commit into from Sep 16, 2020
Merged

Revert "conntrack e2e test debug connections" #94800

merged 1 commit into from Sep 16, 2020

Conversation

mborsz
Copy link
Member

@mborsz mborsz commented Sep 15, 2020 •
edited

Reverts #92643

After this PR has been merged, gce-master-scale-correctness started OOMing: https://screenshot.googleplex.com/773vvp4MTsfMxv6

I'm not 100% it's caused by this change, but it seems to be likely given that this PR adds dumping iptables-save for each node pair + submission time correlates with the time when OOMs has started.

NONE

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/test sig/network Categorizes an issue or PR as relevant to SIG Network. 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 Sep 15, 2020
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 15, 2020
@mborsz
Copy link
Member Author

mborsz commented Sep 15, 2020

/assign @wojtek-t

wojtek-t
wojtek-t reviewed Sep 15, 2020
View reviewed changes
test/e2e/network/conntrack.go
@@ -328,25 +297,3 @@ func dumpConntrack(cs clientset.Interface) {
}
}
}

func dumpIptables(cs clientset.Interface) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we revert only the iptables part?

This is problematic because we log it for all nodes (5k), but logging conntrack should be fine as we log that for two nodes...

@aojea - FYI

Copy link
Member

Choose a reason for hiding this comment

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

yeah sure, , sorry,

Copy link
Member Author

Choose a reason for hiding this comment

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

@wojtek-t, I'm not sure if I understand your comment: I think both dumpIptables and dumpConntrack iterates over all nodes.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry - I think this has changed in the meantime, I think we were dumping only particular nodes in the past.

@wojtek-t
Copy link
Member

@aojea - I'm going to approve it to fix scalability tests; we should re-revert by dumping iptables/conntrack only for nodes that are interested for us.

/lgtm
/approve
/retest

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mborsz, wojtek-t

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 Sep 15, 2020
@aojea
Copy link
Member

aojea commented Sep 15, 2020

/lgtm

@wojtek-t
Copy link
Member

/retest

@aojea
Copy link
Member

aojea commented Sep 15, 2020

@danehans this is breaking scsalability tests, we need milestone here please

@wojtek-t wojtek-t added this to the v1.20 milestone Sep 15, 2020
@wojtek-t
Copy link
Member

Added milestone.

@wojtek-t wojtek-t removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 16, 2020
@wojtek-t wojtek-t added kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Sep 16, 2020
@mborsz mborsz mentioned this pull request Sep 16, 2020
Closed
@wojtek-t
Copy link
Member

/retest

1 similar comment
@wojtek-t
Copy link
Member

/retest

@k8s-ci-robot k8s-ci-robot merged commit fcba5fe into kubernetes:master Sep 16, 2020
mborsz added a commit to mborsz/test-infra that referenced this pull request Sep 17, 2020
@mborsz
Revert memory bump in ci-kubernetes-e2e-gce-scale-correctness
b45605b 
After revert kubernetes/kubernetes#94800, the test takes ~12-15GiB now, so 48GiB should be really safe.
@mborsz mborsz mentioned this pull request Sep 17, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aojea aojea aojea left review comments

@wojtek-t wojtek-t wojtek-t left review comments

@cmluciano cmluciano Awaiting requested review from cmluciano

@freehan freehan Awaiting requested review from freehan

Assignees

@aojea aojea

@wojtek-t wojtek-t

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/network Categorizes an issue or PR as relevant to SIG Network. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.20
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mborsz @wojtek-t @k8s-ci-robot @aojea