allow kb granularity for lua shared dicts #6750

timmysilv · 2021-01-13T15:36:58Z

What this PR does / why we need it:

Lua shared dicts can be configured via Configmaps, but it enforces a megabyte granularity. This PR adds regex parsing to allow an appended "k" or "m" (case-insensitive) to specify the size unit of the shared dict. If no unit is specified, it will default to the old behaviour of using megabytes. If for example a user wanted to add many shared dicts of only 16k size, this PR would make sure they don't all use the previous minimum size of 1MB, which could become a considerable memory optimization.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Which issue/s this PR fixes

How Has This Been Tested?

I updated old tests for this feature to conform to the new unit formatting I set
I wrote new unit tests for the utility functions I introduced

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have added tests to cover my changes.
All new and existing tests passed.

PS: I changed TemplateWriter to Writer because my go-lint plugin told me to.

k8s-ci-robot · 2021-01-13T15:37:06Z

Hi @timmysilv. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

aledbf · 2021-01-13T15:50:21Z

/ok-to-test

aledbf · 2021-01-13T15:50:29Z

/assign @ElvinEfendi

timmysilv · 2021-01-13T18:12:36Z

internal/ingress/controller/template/configmap.go

+func dictStrToKb(sizeStr string) int {
+	sizeMatch := dictSizeRegex.FindStringSubmatch(sizeStr)
+	if sizeMatch == nil {
+		return -1


I use the convention of a negative return value implying an error here. If there is a preferred format, let me know. It just seemed most minimal and intuitive to me when making this.

fejta-bot · 2021-04-13T18:56:48Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

timmysilv · 2021-04-13T19:26:07Z

/remove-lifecycle stale

rikatz · 2021-05-30T19:24:01Z

/assing @moonming

If you can please take a look, will appreciate that

rikatz

Left some minor/nit things.

I'm curious about why aren't we converting Kb to Mb as *1024 and instead using 1000, but maybe I'm missing something.

I'll leave the approval here, and ask someone else just to make a final review.

Thanks!

internal/ingress/controller/template/configmap.go

internal/ingress/controller/template/template_test.go

rikatz · 2021-07-27T15:44:17Z

/cc @tao12345666333 @strongjz @cpanato

tao12345666333 · 2021-07-27T15:47:59Z

I will review this tomorrow (here is 11:50 pm 😄

tao12345666333 · 2021-07-27T15:48:11Z

/assign

k8s-ci-robot · 2021-07-27T17:32:05Z

@timmysilv: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
pull-ingress-nginx-e2e-helm-chart	0bc069c1af6c5fa45138c171b3645e41d1f32a9a	link	`/test pull-ingress-nginx-e2e-helm-chart`
pull-ingress-nginx-chart-lint	0bc069c1af6c5fa45138c171b3645e41d1f32a9a	link	`/test pull-ingress-nginx-chart-lint`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Update internal/ingress/controller/template/configmap.go Co-authored-by: Ricardo Katz <[email protected]>

timmysilv · 2021-07-27T17:57:05Z

totally should have been 1024 and not 1000, thanks for pointing it out!

timmysilv · 2021-08-03T15:04:19Z

/retest

rikatz · 2021-08-12T18:04:59Z

/lgtm
Thanks!

k8s-ci-robot · 2021-08-12T18:05:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rikatz, timmysilv

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:

~~OWNERS~~ [rikatz]

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

Update internal/ingress/controller/template/configmap.go Co-authored-by: Ricardo Katz <[email protected]> Co-authored-by: Ricardo Katz <[email protected]>

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 13, 2021

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 13, 2021

k8s-ci-robot requested review from cmluciano and ElvinEfendi January 13, 2021 15:37

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 13, 2021

k8s-ci-robot assigned ElvinEfendi Jan 13, 2021

timmysilv commented Jan 13, 2021

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 13, 2021

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 13, 2021

rikatz changed the base branch from master to dev-v1 July 16, 2021 12:44

rikatz changed the base branch from dev-v1 to main July 27, 2021 15:31

rikatz approved these changes Jul 27, 2021

View reviewed changes

internal/ingress/controller/template/configmap.go Outdated Show resolved Hide resolved

internal/ingress/controller/template/template_test.go Show resolved Hide resolved

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2021

k8s-ci-robot requested review from cpanato, strongjz and tao12345666333 July 27, 2021 15:44

k8s-ci-robot assigned tao12345666333 Jul 27, 2021

k8s-ci-robot added the area/docs label Jul 27, 2021

timmysilv force-pushed the kb-dicts branch from 3fe5cf9 to 0bc069c Compare July 27, 2021 17:31

allow kb granularity for lua shared dicts

d0e644a

Update internal/ingress/controller/template/configmap.go Co-authored-by: Ricardo Katz <[email protected]>

timmysilv force-pushed the kb-dicts branch from 0bc069c to d0e644a Compare July 27, 2021 17:54

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 27, 2021

k8s-ci-robot assigned rikatz Aug 12, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 12, 2021

k8s-ci-robot merged commit b591ada into kubernetes:main Aug 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow kb granularity for lua shared dicts #6750

allow kb granularity for lua shared dicts #6750

timmysilv commented Jan 13, 2021 •

edited

Loading

k8s-ci-robot commented Jan 13, 2021

aledbf commented Jan 13, 2021

aledbf commented Jan 13, 2021

timmysilv Jan 13, 2021

fejta-bot commented Apr 13, 2021

timmysilv commented Apr 13, 2021

rikatz commented May 30, 2021

rikatz left a comment

rikatz commented Jul 27, 2021

tao12345666333 commented Jul 27, 2021

tao12345666333 commented Jul 27, 2021

k8s-ci-robot commented Jul 27, 2021 •

edited

Loading

timmysilv commented Jul 27, 2021

timmysilv commented Aug 3, 2021

rikatz commented Aug 12, 2021

k8s-ci-robot commented Aug 12, 2021

allow kb granularity for lua shared dicts #6750

allow kb granularity for lua shared dicts #6750

Conversation

timmysilv commented Jan 13, 2021 • edited Loading

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

k8s-ci-robot commented Jan 13, 2021

aledbf commented Jan 13, 2021

aledbf commented Jan 13, 2021

timmysilv Jan 13, 2021

Choose a reason for hiding this comment

fejta-bot commented Apr 13, 2021

timmysilv commented Apr 13, 2021

rikatz commented May 30, 2021

rikatz left a comment

Choose a reason for hiding this comment

rikatz commented Jul 27, 2021

tao12345666333 commented Jul 27, 2021

tao12345666333 commented Jul 27, 2021

k8s-ci-robot commented Jul 27, 2021 • edited Loading

timmysilv commented Jul 27, 2021

timmysilv commented Aug 3, 2021

rikatz commented Aug 12, 2021

k8s-ci-robot commented Aug 12, 2021

timmysilv commented Jan 13, 2021 •

edited

Loading

k8s-ci-robot commented Jul 27, 2021 •

edited

Loading