Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

Expose --conntrack-max-per-core kube-proxy flag #1187

Merged
merged 3 commits into from
Dec 1, 2020
Merged

Conversation

invidian
Copy link
Member

This PR exposes --conntrack-max-per-core kube-proxy flag in
kubernetes Helm chart and adds required plumbing to expose it to the
user using HCL.

It also adds sample usage to CI configuration and e2e tests to verify
that settings are properly applied.

Closes #1081

Signed-off-by: Mateusz Gozdek mateusz@kinvolk.io

@invidian invidian force-pushed the invidian/conntrack branch 3 times, most recently from 68aa5ca to b24a57c Compare November 16, 2020 16:42
@invidian invidian added the priority/P3 Low priority label Nov 17, 2020
@@ -42,6 +42,9 @@ spec:
- --proxy-mode=iptables
- --metrics-bind-address=$(HOST_IP)
- --healthz-bind-address=$(HOST_IP)
{{- if not (eq (int .Values.kubeProxy.conntrackMaxPerCore) 32768) }}
Copy link
Member

Choose a reason for hiding this comment

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

We can get rid of this if block if we provide the same default value as whatever is set in the node right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

This line allows kube-proxy to steer the default value, allowing us to update it asynchronously if it ever changes.

It also reduces final manifest size, I don't see a value in specifying the flag which is exactly the same as default value. I'd prefer to keep it.

assets/terraform-modules/bootkube/assets.tf Show resolved Hide resolved
assets/terraform-modules/bootkube/variables.tf Outdated Show resolved Hide resolved
pkg/platform/baremetal/template.go Outdated Show resolved Hide resolved
pkg/platform/packet/template.go Outdated Show resolved Hide resolved
ci/aws/aws-cluster.lokocfg.envsubst Show resolved Hide resolved
@invidian invidian requested a review from surajssd November 19, 2020 14:04
@johananl johananl removed their request for review November 19, 2020 17:35
@johananl
Copy link
Member

@invidian I can't review this right now due to load, sorry. Please assign someone else.

@invidian invidian force-pushed the invidian/conntrack branch 2 times, most recently from 62f1d0a to 03cfea2 Compare November 23, 2020 12:04
@invidian invidian requested review from knrt10 and iaguis November 23, 2020 12:34
Copy link
Member

@knrt10 knrt10 left a comment

Choose a reason for hiding this comment

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

Nice work, some minor suggestions

test/system/conntrack_clc_test.go Outdated Show resolved Hide resolved
test/system/conntrack_clc_test.go Outdated Show resolved Hide resolved
test/system/conntrack_clc_test.go Outdated Show resolved Hide resolved
test/system/conntrack_clc_test.go Outdated Show resolved Hide resolved
test/system/conntrack_global_test.go Outdated Show resolved Hide resolved
test/system/conntrack_global_test.go Outdated Show resolved Hide resolved
surajssd
surajssd previously approved these changes Nov 24, 2020
@invidian
Copy link
Member Author

@knrt10 addressed your (good) review comments. @surajssd mind approving again please? :)

knrt10
knrt10 previously approved these changes Nov 24, 2020
examples/aws-testing/cluster.lokocfg Outdated Show resolved Hide resolved
pkg/platform/platform.go Outdated Show resolved Hide resolved
test/system/conntrack_clc_test.go Show resolved Hide resolved
@invidian
Copy link
Member Author

Also added configuration validation rules and unit tests for that.

@invidian invidian requested review from knrt10 and iaguis November 25, 2020 17:52
@knrt10
Copy link
Member

knrt10 commented Nov 26, 2020

@invidian can you please check the CI errors?

@invidian
Copy link
Member Author

@knrt10 they were caused by Docker Hub limits, which should be now resolves, so I restarted them.

knrt10
knrt10 previously approved these changes Nov 27, 2020
iaguis
iaguis previously approved these changes Nov 27, 2020
Copy link
Contributor

@iaguis iaguis left a comment

Choose a reason for hiding this comment

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

lgtm

surajssd
surajssd previously approved these changes Nov 27, 2020
@invidian invidian dismissed stale reviews from surajssd and iaguis via cbd94cf November 30, 2020 21:11
surajssd
surajssd previously approved these changes Dec 1, 2020
@knrt10
Copy link
Member

knrt10 commented Dec 1, 2020

Have to re-run the tests again after rebasing for bare metal to pass 😢

invidian and others added 3 commits December 1, 2020 14:03
Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
Signed-off-by: knrt10 <kautilya@kinvolk.io>
This commit exposes --conntrack-max-per-core kube-proxy flag in
kubernetes Helm chart and adds required plumbing to expose it to the
user using HCL.

It also adds sample usage to CI configuration and e2e tests to verify
that settings are properly applied.

Closes #1081

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian invidian merged commit 86c67e6 into master Dec 1, 2020
@invidian invidian deleted the invidian/conntrack branch December 1, 2020 15:13
@invidian invidian mentioned this pull request Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority/P3 Low priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose --conntrack-max-per-core kube-proxy flag
5 participants