fine-grained control over canary: add canary-weight-total to configure the total weight #6338

cofyc · 2020-10-16T11:00:28Z

What this PR does / why we need it:

Try to fine-grained control over canary weight.

Alternatively, we can allow the fraction part in the canary-weight, but how many fraction digits should we allow?

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

fixes #6337

How Has This Been Tested?

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.

cofyc · 2020-10-16T11:02:19Z

internal/ingress/annotations/canary/main.go

+	config.WeightTotal, err = parser.GetIntAnnotation("canary-weight-total", ing)
+	if err != nil {
+		config.WeightTotal = 100
+	}


in Lua script, < 100 value is ignored and the value defaults to 100, should we add this logic here too?

aledbf · 2020-10-16T11:03:20Z

/assign @ElvinEfendi

codecov-io · 2020-10-16T11:11:29Z

Codecov Report

Merging #6338 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6338      +/-   ##
==========================================
+ Coverage   55.81%   55.87%   +0.05%     
==========================================
  Files          89       89              
  Lines        6358     6362       +4     
==========================================
+ Hits         3549     3555       +6     
+ Misses       2369     2368       -1     
+ Partials      440      439       -1

Impacted Files	Coverage Δ
internal/ingress/controller/nginx.go	`28.91% <ø> (ø)`
internal/ingress/types.go	`0.00% <ø> (ø)`
internal/ingress/annotations/canary/main.go	`100.00% <100.00%> (ø)`
internal/ingress/controller/controller.go	`46.99% <100.00%> (+0.06%)`	⬆️
internal/ingress/metric/collectors/process.go	`90.47% <0.00%> (+2.38%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb6a03f...bd9424e. Read the comment docs.

fejta-bot · 2021-01-17T02:21:02Z

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-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

cofyc · 2021-01-17T04:37:16Z

/remove-lifecycle stale

cofyc · 2021-02-09T22:43:43Z

@ElvinEfendi what do you think?

fejta-bot · 2021-05-10T23:17:55Z

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

rikatz · 2021-05-30T19:16:35Z

/kind feature

cofyc · 2021-05-31T07:50:02Z

/remove-lifecycle stale

k8s-triage-robot · 2021-10-28T05:02:53Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

iamNoah1 · 2021-11-15T14:45:57Z

@cofyc can you please rebase, and then we should follow up @rikatz

iamNoah1 · 2021-11-16T10:27:49Z

@cofyc build is failing

cofyc · 2021-11-16T10:49:15Z

working on it

cofyc · 2021-11-17T06:09:06Z

@cofyc build is failing

fixed

iamNoah1

/lgtm

cofyc · 2021-11-17T10:29:08Z

/remove-lifecycle stale

larivierec · 2021-11-17T12:23:18Z

Maybe I'm missing context but it seems that weight is the same thing as weight total except for the fact that we can go to 1000.

Shouldn't we simply reuse the weight variable?
What happens if we put both Weight and Weight Total?

Update: after reading the docs, I see that both are needed in order to be used one allows you to increase the other.
Shouldn't the original variable Weight be converted to a float instead?

iamNoah1 · 2021-11-19T09:51:37Z

@cofyc friendly ping to consider the comment of @larivierec

cofyc · 2021-11-22T05:38:53Z

I think it's a good idea if it's ok to convert nginx.ingress.kubernetes.io/canary-weight to float.

iamNoah1 · 2021-11-22T11:11:48Z

@cofyc @larivierec my opinion on this is, even though it may make sense, let's just get this PR merged if everything is fine. Having the possibility to be more precise on the weight using float can be a follow-up issue.

larivierec · 2021-11-22T20:17:45Z

doesn't bother me, we should create an issue to follow up though

iamNoah1 · 2021-11-24T16:07:58Z

created a follow up issue: #7971 @cofyc @larivierec FYI

iamNoah1 · 2021-11-24T16:32:25Z

/triage accepted
/priority backlog

strongjz · 2021-12-07T16:33:32Z

/lgtm

k8s-ci-robot · 2021-12-07T16:33:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cofyc, iamNoah1, strongjz

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~~ [strongjz]

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 added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 16, 2020

k8s-ci-robot requested review from aledbf and ElvinEfendi October 16, 2020 11:00

cofyc commented Oct 16, 2020

View reviewed changes

k8s-ci-robot assigned ElvinEfendi Oct 16, 2020

cofyc force-pushed the fix6337 branch 2 times, most recently from 78831be to 518ae57 Compare October 16, 2020 11:06

cofyc force-pushed the fix6337 branch from 518ae57 to bd9424e Compare October 19, 2020 02:13

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

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

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

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label May 30, 2021

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 31, 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

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2021

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

cofyc force-pushed the fix6337 branch from bd9424e to 7818d6f Compare November 16, 2021 10:07

k8s-ci-robot added the area/docs label Nov 16, 2021

cofyc force-pushed the fix6337 branch from d9f4cc3 to 265189f Compare November 16, 2021 10:17

add canary-weight-total annotation

3c46262

cofyc force-pushed the fix6337 branch from 265189f to 3c46262 Compare November 17, 2021 04:13

iamNoah1 reviewed Nov 17, 2021

View reviewed changes

k8s-ci-robot assigned iamNoah1 Nov 17, 2021

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

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

iamNoah1 mentioned this pull request Nov 24, 2021

Improving Canary Weight Handling #7971

Closed

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/backlog Higher priority than priority/awaiting-more-evidence. labels Nov 24, 2021

k8s-ci-robot assigned strongjz Dec 7, 2021

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

k8s-ci-robot merged commit 5cff197 into kubernetes:main Dec 7, 2021

strongjz mentioned this pull request Dec 7, 2021

canary consistency #6890

Closed

rikatz added this to the v1.1.1 milestone Jan 9, 2022

antoinedeschenes mentioned this pull request Mar 16, 2023

Fix canary-weight-total annotation ignored in rule backends #9729

Merged

11 tasks

rchshld pushed a commit to joomcode/ingress-nginx that referenced this pull request May 19, 2023

add canary-weight-total annotation (kubernetes#6338)

dadcb65

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fine-grained control over canary: add canary-weight-total to configure the total weight #6338

fine-grained control over canary: add canary-weight-total to configure the total weight #6338

cofyc commented Oct 16, 2020 •

edited

Loading

cofyc Oct 16, 2020

aledbf commented Oct 16, 2020

codecov-io commented Oct 16, 2020 •

edited

Loading

fejta-bot commented Jan 17, 2021

cofyc commented Jan 17, 2021

cofyc commented Feb 9, 2021

fejta-bot commented May 10, 2021

rikatz commented May 30, 2021

cofyc commented May 31, 2021

k8s-triage-robot commented Oct 28, 2021

iamNoah1 commented Nov 15, 2021

iamNoah1 commented Nov 16, 2021

cofyc commented Nov 16, 2021 •

edited

Loading

cofyc commented Nov 17, 2021

iamNoah1 left a comment

cofyc commented Nov 17, 2021

larivierec commented Nov 17, 2021 •

edited

Loading

iamNoah1 commented Nov 19, 2021

cofyc commented Nov 22, 2021

iamNoah1 commented Nov 22, 2021

larivierec commented Nov 22, 2021

iamNoah1 commented Nov 24, 2021

iamNoah1 commented Nov 24, 2021

strongjz commented Dec 7, 2021

k8s-ci-robot commented Dec 7, 2021

fine-grained control over canary: add canary-weight-total to configure the total weight #6338

fine-grained control over canary: add canary-weight-total to configure the total weight #6338

Conversation

cofyc commented Oct 16, 2020 • 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:

cofyc Oct 16, 2020

Choose a reason for hiding this comment

aledbf commented Oct 16, 2020

codecov-io commented Oct 16, 2020 • edited Loading

Codecov Report

fejta-bot commented Jan 17, 2021

cofyc commented Jan 17, 2021

cofyc commented Feb 9, 2021

fejta-bot commented May 10, 2021

rikatz commented May 30, 2021

cofyc commented May 31, 2021

k8s-triage-robot commented Oct 28, 2021

iamNoah1 commented Nov 15, 2021

iamNoah1 commented Nov 16, 2021

cofyc commented Nov 16, 2021 • edited Loading

cofyc commented Nov 17, 2021

iamNoah1 left a comment

Choose a reason for hiding this comment

cofyc commented Nov 17, 2021

larivierec commented Nov 17, 2021 • edited Loading

iamNoah1 commented Nov 19, 2021

cofyc commented Nov 22, 2021

iamNoah1 commented Nov 22, 2021

larivierec commented Nov 22, 2021

iamNoah1 commented Nov 24, 2021

iamNoah1 commented Nov 24, 2021

strongjz commented Dec 7, 2021

k8s-ci-robot commented Dec 7, 2021

cofyc commented Oct 16, 2020 •

edited

Loading

codecov-io commented Oct 16, 2020 •

edited

Loading

cofyc commented Nov 16, 2021 •

edited

Loading

larivierec commented Nov 17, 2021 •

edited

Loading