You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "acuteaura (via GitHub)" <gi...@apache.org> on 2024/01/17 12:26:08 UTC
[PR] refactor(cmd/ingress): invert signal ctx logic [apisix-ingress-controller]
acuteaura opened a new pull request, #2139:
URL: https://github.com/apache/apisix-ingress-controller/pull/2139
### Type of change:
- [x] Bugfix
- [ ] New feature provided
- [ ] Improve performance
- [ ] Backport patches
- [ ] Documentation
- [x] Refactor
- [ ] Chore
- [ ] CI/CD or Tests
### What this PR does / why we need it:
(verbatin commit message)
this commit changes the signal handling in cmd/ingress to be wrapped in a context, and inverts which goroutine runs the controller and which watches for the context to be cancelled, which allows some scaffolding (`sync.WaitGroup`) to be removed and now properly handles the controller exiting with `nil` (as it does when leader election fails)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(cmd/ingress): invert signal ctx logic [apisix-ingress-controller]
Posted by "acuteaura (via GitHub)" <gi...@apache.org>.
acuteaura commented on PR #2139:
URL: https://github.com/apache/apisix-ingress-controller/pull/2139#issuecomment-1910080507
Ah, right, Notify can fire more than once, should be fixed now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(cmd/ingress): invert signal ctx logic [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #2139:
URL: https://github.com/apache/apisix-ingress-controller/pull/2139#issuecomment-1919548693
@acuteaura Thanks for your contribution. You can now create the other PR as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(cmd/ingress): invert signal ctx logic [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #2139:
URL: https://github.com/apache/apisix-ingress-controller/pull/2139#issuecomment-1897832276
@acuteaura Can you fix the failing unit test? Rest, the change LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(cmd/ingress): invert signal ctx logic [apisix-ingress-controller]
Posted by "acuteaura (via GitHub)" <gi...@apache.org>.
acuteaura commented on PR #2139:
URL: https://github.com/apache/apisix-ingress-controller/pull/2139#issuecomment-1897995842
The test failing is code I didn't touch, which is just an unstable test, it seems to be iterating over a map with two elements and not accounting for map iteration order not being stable.
> The iteration order over maps is not specified and is not guaranteed to be the same from one iteration to the next.
https://go.dev/ref/spec#RangeClause
The range over map is here:
https://github.com/acuteaura/apisix-ingress-controller/blob/refactor/root-context-signaling/pkg/types/labels.go#L45
Should go into another PR, and I'd be happy to rebase on that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(cmd/ingress): invert signal ctx logic [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #2139:
URL: https://github.com/apache/apisix-ingress-controller/pull/2139#issuecomment-1898173105
@acuteaura I have rerun the test. I will fix it in another PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(cmd/ingress): invert signal ctx logic [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup merged PR #2139:
URL: https://github.com/apache/apisix-ingress-controller/pull/2139
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(cmd/ingress): invert signal ctx logic [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #2139:
URL: https://github.com/apache/apisix-ingress-controller/pull/2139#issuecomment-1898178499
@acuteaura Actually other than the flaky test, this one is also failing
Can you take a look? https://github.com/apache/apisix-ingress-controller/actions/runs/7555667043/job/20609448277
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org