You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/03/04 10:19:50 UTC
[GitHub] [apisix-ingress-controller] chzhuo opened a new issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
chzhuo opened a new issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806
### Issue description
The code show that synced failure Event will be requeue to the `workQueue`. But the Event may be expired while resync
### Environment
- your apisix-ingress-controller version (output of apisix-ingress-controller version --long):
- your Kubernetes cluster version (output of kubectl version):
- if you run apisix-ingress-controller in Bare-metal environment, also show your OS version (uname -a):
### Minimal test code / Steps to reproduce
to reproduce this bug by adding two Endpoints to one Deployment
1. Adding Endpoint Event1 arrived
2. Adding Endpoint Event2 arrived
3. The `workQueue` has two events
4. The Event1 sync failure, duo to the network error. and The even1 be requeue
5. The Event2 sync successfully. There are two Nodes in the APISIX Upstream
6. The Event1 sync successfully. There is only one Node in the APISIX Upstream
### Actual result
only one Node in the APISIX Upstream
### Error log
no error
### Expected result
two Nodes in the APISIX Upstream
--
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
[GitHub] [apisix-ingress-controller] gxthrj edited a comment on issue #806: bug: Special circumstance my cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
gxthrj edited a comment on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-997825977
Good catch, thanks.
I think we should compare `resourceVersion` before adding or re-adding `event` into `workqueue`.
--
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
[GitHub] [apisix-ingress-controller] tao12345666333 commented on issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
tao12345666333 commented on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-1059030712
@cmssczy Can you help with confirmation?
--
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
[GitHub] [apisix-ingress-controller] tokers commented on issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
tokers commented on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-1060084375
> > why using `==` here when compare `resourceVersion`? It should be `>=`.
>
> Should we submit a PR to fix this first?
Sure, PR's welcome!
--
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
[GitHub] [apisix-ingress-controller] lingsamuel edited a comment on issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
lingsamuel edited a comment on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-1059953610
IIRC what we enqueued is the meta key, is this really happened?
(edit: maybe some types of resources are not the meta key)
--
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
[GitHub] [apisix-ingress-controller] tokers commented on issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
tokers commented on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-1059950303
> After checking the code in `endpointController`, [here](https://github.com/apache/apisix-ingress-controller/blob/master/pkg/ingress/endpoint.go#L136) is strange ，why using `==` here when compare `resourceVersion`? It should be `>=`.
Yes.
--
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
[GitHub] [apisix-ingress-controller] tao12345666333 commented on issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
tao12345666333 commented on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-1059256024
yep, thanks!
--
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
[GitHub] [apisix-ingress-controller] cmssczy commented on issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
cmssczy commented on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-1059743332
I think the resources (located in `/pkg/ingress/xxx.go`.) that follow the logic below will be affected.
```go
func (c *xxxController) runWorker(ctx context.Context) {
for {
obj, quit := c.workqueue.Get()
if quit {
return
}
err := c.sync(ctx, obj.(*types.Event))
c.workqueue.Done(obj)
// The obj may be expired at this time,
// so it maybe lead to unexpected result after we requeue obj into workqueue.
c.handleSyncErr(obj, err)
}
}
func (c *apisixClusterConfigController) sync(ctx context.Context, ev *types.Event) error {
// ...
// do something using `ev` , eg.
nodes, err := c.translator.TranslateUpstreamNodes(ep, port.Port, subset.Labels)
// ...
}
func (c *apisixUpstreamController) handleSyncErr(obj interface{}, err error) {
// ...
// add obj but not the latest obj to workquque again.
c.workqueue.AddRateLimited(obj)
// ...
}
```
These resources include `ApisixClusterConfig`/`ApisixConsumer`/`ApisixPluginConfig`/`ApisixRoute`/`ApisixTls`/`ApisixUpstream`/`endpoints`/`Gateway`/`Ingress`/`Namespace`/`Secret`.
`endpointSlice` resource is not affected because we get the latest ep everytime instead of using the object poped from the workquque(`*types.Event`)
https://github.com/apache/apisix-ingress-controller/blob/b5448c37e7b043b4d2a5b5d35b115429a20760a0/pkg/ingress/endpointslice.go#L100-L106
`pod` resource is aslo not affected because we did not use workquque at all.
Correct me if I'm wrong. @tao12345666333
--
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
[GitHub] [apisix-ingress-controller] tao12345666333 closed issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
tao12345666333 closed issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806
--
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
[GitHub] [apisix-ingress-controller] tao12345666333 closed issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
tao12345666333 closed issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806
--
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
[GitHub] [apisix-ingress-controller] tao12345666333 commented on issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
tao12345666333 commented on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-1059970471
> After checking the code in `endpointController`, [here](https://github.com/apache/apisix-ingress-controller/blob/master/pkg/ingress/endpoint.go#L136) is strange ，why using `==` here when compare `resourceVersion`? It should be `>=`.
yes.
Stability is one of the roadmaps of APISIX Ingress, I think we need to check the existing code and fix some bugs.
The processing logic of resources can be unified or optimized. As mentioned in the previous issue, the handling of events can also be optimized, like #893
--
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
[GitHub] [apisix-ingress-controller] gxthrj edited a comment on issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
gxthrj edited a comment on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-1059945155
After checking the code in `endpointController`, [here](https://github.com/apache/apisix-ingress-controller/blob/master/pkg/ingress/endpoint.go#L136) is strange ，why using `==` here when compare `resourceVersion`? It should be `>=`.
--
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
[GitHub] [apisix-ingress-controller] tokers commented on issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
tokers commented on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-1059887182
If I am correct, we don't save the updated object when we get the update event, instead, we save the key and fetch the object when we handle it, so even we handle an old event, we still get the newest object and we can compare the `ResourceVersion` so that stale update can be avoid.
--
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
[GitHub] [apisix-ingress-controller] tao12345666333 commented on issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
tao12345666333 commented on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-1059029650
> I think not only `endpoint` resource has this problem, other resources such as `apiroute`/`ingress`/`namespce` also have this problem.
@cmssczy Thanks, I will reopen this issue.
Let's check it out in more detail.
--
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
[GitHub] [apisix-ingress-controller] gxthrj commented on issue #806: bug: Special circumstance my cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
gxthrj commented on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-997825977
Good catch, thanks.
I think we should compare `resourceVersion` before synchronization, when `event` is taken out from `workqueue`.
--
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
[GitHub] [apisix-ingress-controller] tokers commented on issue #806: bug: Special circumstance my cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
tokers commented on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-998399482
We can just can change the objects entering the queue, just use the namespace/name is enough (instead of the pointer to the endpoint object), just like other controllers.
This is a good way to write a controller, see https://github.com/kubernetes/kubernetes/blob/da9a46bac2f0b389405efad28b165543d94a3f73/pkg/controller/endpoint/endpoints_controller.go#L378-L404.
--
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
[GitHub] [apisix-ingress-controller] tao12345666333 commented on issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
tao12345666333 commented on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-1059766558
This situation can happen. Adding double-checking might be more safer, what do you think? @lingsamuel @gxthrj @tokers
--
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
[GitHub] [apisix-ingress-controller] gxthrj commented on issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
gxthrj commented on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-1059945155
After checking the code in `endpointController`, [here](https://github.com/apache/apisix-ingress-controller/blob/master/pkg/ingress/endpoint.go#L136) is strange ，why using `==` here? It should be `>=`.
--
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
[GitHub] [apisix-ingress-controller] cmssczy commented on issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
cmssczy commented on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-1058880470
I think not only `endpoint` resource has this problem, other resources such as `apiroute`/`ingress`/`namespce` also have this problem.
--
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
[GitHub] [apisix-ingress-controller] gxthrj edited a comment on issue #806: bug: Special circumstance my cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
gxthrj edited a comment on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-997825977
Good catch, thanks.
I think we should compare `resourceVersion` before adding or re-adding `event` into `workqueue`.
And compare `resourceVersion` when taking `event` from `workqueue`.
--
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
[GitHub] [apisix-ingress-controller] cmssczy commented on issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
cmssczy commented on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-1059984506
> why using `==` here when compare `resourceVersion`? It should be `>=`.
Should we submit a PR to fix this first?
--
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
[GitHub] [apisix-ingress-controller] lingsamuel commented on issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
lingsamuel commented on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-1059953610
IIRC what we enqueued is the meta key, is this really happened?
--
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
[GitHub] [apisix-ingress-controller] cmssczy commented on issue #806: bug: Special circumstance may cause expired configuration be PUT to APISIX
Posted by GitBox <gi...@apache.org>.
cmssczy commented on issue #806:
URL: https://github.com/apache/apisix-ingress-controller/issues/806#issuecomment-1059138727
> @cmssczy Can you help with confirmation?
Confirm what resources are related to this issue? @tao12345666333
I will take a look this weekend.
--
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