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

Refactor status update #3885

Merged
merged 6 commits into from
Mar 13, 2019
Merged

Refactor status update #3885

merged 6 commits into from
Mar 13, 2019

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Mar 10, 2019

What this PR does / why we need it:

This PR extracts the leader election code from the status package to be able to use such a feature at the controller level for other operations.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged):

fixes #3565
fixes #2773

Special notes for your reviewer:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 10, 2019
@aledbf aledbf changed the title Status Refactor status update Mar 10, 2019
@aledbf
Copy link
Member Author

aledbf commented Mar 10, 2019

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 10, 2019
@aledbf aledbf force-pushed the status branch 2 times, most recently from 8dbe9a9 to 036e355 Compare March 11, 2019 00:49
@aledbf aledbf removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 11, 2019
@ElvinEfendi
Copy link
Member

How is this fixing #2773? https://github.com/kubernetes/ingress-nginx/pull/3885/files#diff-cde3fffe2425ad7efaa8add1d05ae2c0R286 will only clean up hosts, but how are you making sure metric collector will not keep emitting ingress_controller_ssl_expire_time_seconds metrics when the pod it's not leader?

@aledbf
Copy link
Member Author

aledbf commented Mar 11, 2019

How is this fixing #2773?

Last commit

@aledbf aledbf force-pushed the status branch 4 times, most recently from 5fb0a84 to 924d5c9 Compare March 12, 2019 13:13
@aledbf
Copy link
Member Author

aledbf commented Mar 12, 2019

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 12, 2019
@codecov-io
Copy link

Codecov Report

Merging #3885 into master will decrease coverage by 0.72%.
The diff coverage is 17.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3885      +/-   ##
==========================================
- Coverage   55.05%   54.33%   -0.73%     
==========================================
  Files          82       83       +1     
  Lines        6121     6151      +30     
==========================================
- Hits         3370     3342      -28     
- Misses       2362     2427      +65     
+ Partials      389      382       -7
Impacted Files Coverage Δ
internal/ingress/controller/status.go 0% <0%> (ø)
internal/ingress/controller/nginx.go 17.44% <0%> (-0.56%) ⬇️
internal/ingress/controller/controller.go 42.91% <0%> (-0.11%) ⬇️
internal/ingress/status/status.go 76.08% <100%> (+3.54%) ⬆️
internal/ingress/metric/collectors/controller.go 87.12% <56.25%> (-3.56%) ⬇️
internal/watch/file_watcher.go 80.76% <0%> (-3.85%) ⬇️

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 55bda6e...f4e4335. Read the comment docs.

@aledbf aledbf removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 12, 2019
@aledbf
Copy link
Member Author

aledbf commented Mar 12, 2019

@ElvinEfendi ready for another review

@ElvinEfendi
Copy link
Member

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi

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 merged commit e079365 into kubernetes:master Mar 13, 2019
@aledbf aledbf deleted the status branch March 14, 2019 02:48
// if there is no other instances running.
func (s statusSync) Shutdown() {
go s.syncQueue.Shutdown()

// remove IP from Ingress
if s.elector != nil && !s.elector.IsLeader() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@aledbf - do you know why this code was removed. we are running into a regression where during rolling update the ingress status is getting removed by the last pod of old deployment and we are suspecting this is the issue here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@rajatjindal because we now use a callback https://github.com/kubernetes/ingress-nginx/pull/3885/files#diff-7d2578dc5d43f0b395d7cd3312c5c489R63

the ingress status is getting removed by the last pod of old deployment and we are suspecting this is the issue here.

This is expected. If you don't want this behavior you need to set UpdateStatusOnShutdown=false (flag --update-status-on-shutdown)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
5 participants