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

fix: --update-state=false cause panic #253

Closed
wants to merge 2 commits into from
Closed

fix: --update-state=false cause panic #253

wants to merge 2 commits into from

Conversation

lijiaocn
Copy link
Contributor

Signed-off-by: lijiaocn [email protected]
(cherry picked from commit 8e7e4df6a9d1fedcd69ded6e0ded475f9d8217a7)

What this PR does / why we need it:

Separate "Leader election" from statusSync (internal/ingress/status/status.go).
This imports an independent electoral mechanism and fix bug #232 at the same time.

Which issue this PR fixes *:

fixes #232

Special notes for your reviewer:

Signed-off-by: lijiaocn <[email protected]>
(cherry picked from commit 8e7e4df6a9d1fedcd69ded6e0ded475f9d8217a7)
@CLAassistant
Copy link

CLAassistant commented Mar 22, 2019

CLA assistant check
All committers have signed the CLA.

Signed-off-by: lijiaocn <[email protected]>
@codecov-io
Copy link

codecov-io commented Mar 25, 2019

Codecov Report

❗ No coverage uploaded for pull request base (next@6c54e94). Click here to learn what that means.
The diff coverage is 8%.

Impacted file tree graph

@@           Coverage Diff           @@
##             next     #253   +/-   ##
=======================================
  Coverage        ?   18.89%           
=======================================
  Files           ?       24           
  Lines           ?     2509           
  Branches        ?        0           
=======================================
  Hits            ?      474           
  Misses          ?     1989           
  Partials        ?       46
Impacted Files Coverage Δ
internal/ingress/controller/run.go 0% <0%> (ø)
internal/ingress/controller/controller.go 0% <0%> (ø)
internal/ingress/status/status.go 60.49% <40%> (ø)

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 6c54e94...09859d7. Read the comment docs.

@subnetmarco subnetmarco requested a review from hbagdi April 9, 2019 06:55
@hbagdi hbagdi changed the base branch from master to next April 11, 2019 21:06
@hbagdi
Copy link
Member

hbagdi commented Apr 11, 2019

@lijiaocn
Apologies on the delay on getting back on this.
Would it be possible for you to rebase this on next?

hbagdi pushed a commit that referenced this pull request Apr 12, 2019
Previously, if status need not be synced, then leader election would
fail and ingress controller would fail to start.

This commit separates the logic out in a hacky way.
With DB-less, if status of Ingress resource is not to be updated, then
we don't even need to run leader-election.

In a future commit, leader election logic and sync logic can be
completely separated out, unlike currently, when on shutdown the sync
logic updates the configmap.

Fix #232
From #253
@hbagdi
Copy link
Member

hbagdi commented Apr 12, 2019

@lijiaocn Manually merged with small clean ups in 9eb994e

Thank you for the fix!

@hbagdi hbagdi closed this Apr 12, 2019
@lijiaocn lijiaocn deleted the bugfix-232 branch May 21, 2019 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants