You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "Revolyssup (via GitHub)" <gi...@apache.org> on 2024/04/09 18:23:58 UTC
[PR] chore: remove redundant logs and improve error when upstream is created [apisix-ingress-controller]
Revolyssup opened a new pull request, #2206:
URL: https://github.com/apache/apisix-ingress-controller/pull/2206
<!-- Please answer these questions before submitting a pull request -->
### Type of change:
<!-- Please delete options that are not relevant. -->
<!-- Select all the options from below that matches the type your PR best -->
- [ ] Bugfix
- [ ] New feature provided
- [ ] Improve performance
- [ ] Backport patches
- [ ] Documentation
- [ ] Refactor
- [ ] Chore
- [ ] CI/CD or Tests
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
### Pre-submission checklist:
<!--
Please follow the requirements:
1. Use Draft if the PR is not ready to be reviewed
2. Test is required for the feat/fix PR, unless you have a good reason
3. Doc is required for the feat PR
4. Use a new commit to resolve review instead of `push -f`
5. Use "request review" to notify the reviewer once you have resolved the review
-->
- [ ] Did you explain what problem does this PR solve? Or what new features have been added?
- [ ] Have you added corresponding test cases?
- [ ] Have you modified the corresponding document?
- [ ] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix-ingress-controller#community) 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
Re: [PR] chore: remove redundant logs and improve logs for users [apisix-ingress-controller]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #2206:
URL: https://github.com/apache/apisix-ingress-controller/pull/2206#discussion_r1562195114
##########
pkg/providers/apisix/apisix_upstream.go:
##########
@@ -373,9 +383,19 @@ func (c *apisixUpstreamController) updateExternalNodes(ctx context.Context, au *
upsName := apisixv1.ComposeExternalUpstreamName(ns, name)
ups, err := c.APISIX.Cluster(clusterName).Upstream().Get(ctx, upsName)
if err != nil {
- log.Errorf("failed to get upstream %s: %s", upsName, err)
- c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSyncAborted, err)
- c.recordStatus(au, utils.ResourceSyncAborted, err, metav1.ConditionFalse, au.GetGeneration())
+ if err == apisix.ErrNotFound {
+ log.Debugw("upstream is not referenced",
+ zap.String("cluster", clusterName),
+ zap.String("upstream", upsName),
+ )
+ err = fmt.Errorf("%s", "upstream doesn't exist. It will be created after ApisixRoute is created referencing it.")
+ c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSynced, err)
+ c.recordStatus(au, utils.ResourceSynced, err, metav1.ConditionFalse, au.GetGeneration())
Review Comment:
If the synchronization is successful, there should be no error content. At the same time, the incident was reported as a failure, don't you think it's contradictory? You only need to add more error messages without modifying the status.
--
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] chore: remove redundant logs and improve logs for users [apisix-ingress-controller]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #2206:
URL: https://github.com/apache/apisix-ingress-controller/pull/2206#discussion_r1562102310
##########
pkg/providers/apisix/apisix_upstream.go:
##########
@@ -373,9 +383,19 @@ func (c *apisixUpstreamController) updateExternalNodes(ctx context.Context, au *
upsName := apisixv1.ComposeExternalUpstreamName(ns, name)
ups, err := c.APISIX.Cluster(clusterName).Upstream().Get(ctx, upsName)
if err != nil {
- log.Errorf("failed to get upstream %s: %s", upsName, err)
- c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSyncAborted, err)
- c.recordStatus(au, utils.ResourceSyncAborted, err, metav1.ConditionFalse, au.GetGeneration())
+ if err == apisix.ErrNotFound {
+ log.Debugw("upstream is not referenced",
+ zap.String("cluster", clusterName),
+ zap.String("upstream", upsName),
+ )
+ err = fmt.Errorf("%s", "upstream doesn't exist. It will be created after ApisixRoute is created referencing it.")
+ c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSynced, err)
+ c.recordStatus(au, utils.ResourceSynced, err, metav1.ConditionFalse, au.GetGeneration())
Review Comment:
`ResourceSynced` : When do you think the `au` resource is considered to be successfully synchronized? Isn't it strange to retry even after success
--
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] chore: remove redundant logs and improve logs for users [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #2206:
URL: https://github.com/apache/apisix-ingress-controller/pull/2206#issuecomment-2051314752
@AlinsRan committed your suggestion
--
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] chore: remove redundant logs and improve logs for users [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup merged PR #2206:
URL: https://github.com/apache/apisix-ingress-controller/pull/2206
--
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] chore: remove redundant logs and improve logs for users [apisix-ingress-controller]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #2206:
URL: https://github.com/apache/apisix-ingress-controller/pull/2206#discussion_r1562222532
##########
pkg/providers/apisix/apisix_upstream.go:
##########
@@ -373,9 +383,19 @@ func (c *apisixUpstreamController) updateExternalNodes(ctx context.Context, au *
upsName := apisixv1.ComposeExternalUpstreamName(ns, name)
ups, err := c.APISIX.Cluster(clusterName).Upstream().Get(ctx, upsName)
if err != nil {
- log.Errorf("failed to get upstream %s: %s", upsName, err)
- c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSyncAborted, err)
- c.recordStatus(au, utils.ResourceSyncAborted, err, metav1.ConditionFalse, au.GetGeneration())
+ if err == apisix.ErrNotFound {
+ log.Debugw("upstream is not referenced",
+ zap.String("cluster", clusterName),
+ zap.String("upstream", upsName),
+ )
+ err = fmt.Errorf("%s", "upstream doesn't exist. It will be created after ApisixRoute is created referencing it.")
+ c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSynced, err)
+ c.recordStatus(au, utils.ResourceSynced, err, metav1.ConditionFalse, au.GetGeneration())
+ } else {
+ c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSyncAborted, err)
+ c.recordStatus(au, utils.ResourceSyncAborted, err, metav1.ConditionFalse, au.GetGeneration())
+ log.Errorf("failed to get upstream %s: %s", upsName, err)
+ }
Review Comment:
```suggestion
if err == apisix.ErrNotFound {
log.Debugw("upstream is not referenced",
zap.String("cluster", clusterName),
zap.String("upstream", upsName),
)
err = fmt.Errorf("%s", "upstream doesn't exist. It will be created after ApisixRoute is created referencing it.")
c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSyncAborted, err)
c.recordStatus(au, utils.ResourceSyncAborted, err, metav1.ConditionFalse, au.GetGeneration())
} else {
c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSyncAborted, err)
c.recordStatus(au, utils.ResourceSyncAborted, err, metav1.ConditionFalse, au.GetGeneration())
log.Errorf("failed to get upstream %s: %s", upsName, err)
}
```
--
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] chore: remove redundant logs and improve logs for users [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #2206:
URL: https://github.com/apache/apisix-ingress-controller/pull/2206#discussion_r1562136260
##########
pkg/providers/apisix/apisix_upstream.go:
##########
@@ -373,9 +383,19 @@ func (c *apisixUpstreamController) updateExternalNodes(ctx context.Context, au *
upsName := apisixv1.ComposeExternalUpstreamName(ns, name)
ups, err := c.APISIX.Cluster(clusterName).Upstream().Get(ctx, upsName)
if err != nil {
- log.Errorf("failed to get upstream %s: %s", upsName, err)
- c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSyncAborted, err)
- c.recordStatus(au, utils.ResourceSyncAborted, err, metav1.ConditionFalse, au.GetGeneration())
+ if err == apisix.ErrNotFound {
+ log.Debugw("upstream is not referenced",
+ zap.String("cluster", clusterName),
+ zap.String("upstream", upsName),
+ )
+ err = fmt.Errorf("%s", "upstream doesn't exist. It will be created after ApisixRoute is created referencing it.")
+ c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSynced, err)
+ c.recordStatus(au, utils.ResourceSynced, err, metav1.ConditionFalse, au.GetGeneration())
Review Comment:
Here I didn't do `ResourceSyncAborted` because that generally means some problem. Here it will be eventually synced when ApisixRoute is created. What do you think is better in this case where we want to convey to the user that this will be eventually synced but currently not created because apisixroute isn't referencing it?
ResourceSyncAborted or ResourceSyced ? @AlinsRan
--
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] chore: remove redundant logs and improve logs for users [apisix-ingress-controller]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #2206:
URL: https://github.com/apache/apisix-ingress-controller/pull/2206#discussion_r1562222532
##########
pkg/providers/apisix/apisix_upstream.go:
##########
@@ -373,9 +383,19 @@ func (c *apisixUpstreamController) updateExternalNodes(ctx context.Context, au *
upsName := apisixv1.ComposeExternalUpstreamName(ns, name)
ups, err := c.APISIX.Cluster(clusterName).Upstream().Get(ctx, upsName)
if err != nil {
- log.Errorf("failed to get upstream %s: %s", upsName, err)
- c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSyncAborted, err)
- c.recordStatus(au, utils.ResourceSyncAborted, err, metav1.ConditionFalse, au.GetGeneration())
+ if err == apisix.ErrNotFound {
+ log.Debugw("upstream is not referenced",
+ zap.String("cluster", clusterName),
+ zap.String("upstream", upsName),
+ )
+ err = fmt.Errorf("%s", "upstream doesn't exist. It will be created after ApisixRoute is created referencing it.")
+ c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSynced, err)
+ c.recordStatus(au, utils.ResourceSynced, err, metav1.ConditionFalse, au.GetGeneration())
+ } else {
+ c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSyncAborted, err)
+ c.recordStatus(au, utils.ResourceSyncAborted, err, metav1.ConditionFalse, au.GetGeneration())
+ log.Errorf("failed to get upstream %s: %s", upsName, err)
+ }
Review Comment:
```suggestion
if err == apisix.ErrNotFound {
log.Debugw("upstream is not referenced",
zap.String("cluster", clusterName),
zap.String("upstream", upsName),
)
err = fmt.Errorf("%s", "upstream doesn't exist. It will be created after ApisixRoute is created referencing it.")
c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSynced, err)
c.recordStatus(au, utils.ResourceSyncAborted, err, metav1.ConditionFalse, au.GetGeneration())
} else {
c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSyncAborted, err)
c.recordStatus(au, utils.ResourceSyncAborted, err, metav1.ConditionFalse, au.GetGeneration())
log.Errorf("failed to get upstream %s: %s", upsName, err)
}
```
--
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] chore: remove redundant logs and improve logs for users [apisix-ingress-controller]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #2206:
URL: https://github.com/apache/apisix-ingress-controller/pull/2206#discussion_r1562102310
##########
pkg/providers/apisix/apisix_upstream.go:
##########
@@ -373,9 +383,19 @@ func (c *apisixUpstreamController) updateExternalNodes(ctx context.Context, au *
upsName := apisixv1.ComposeExternalUpstreamName(ns, name)
ups, err := c.APISIX.Cluster(clusterName).Upstream().Get(ctx, upsName)
if err != nil {
- log.Errorf("failed to get upstream %s: %s", upsName, err)
- c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSyncAborted, err)
- c.recordStatus(au, utils.ResourceSyncAborted, err, metav1.ConditionFalse, au.GetGeneration())
+ if err == apisix.ErrNotFound {
+ log.Debugw("upstream is not referenced",
+ zap.String("cluster", clusterName),
+ zap.String("upstream", upsName),
+ )
+ err = fmt.Errorf("%s", "upstream doesn't exist. It will be created after ApisixRoute is created referencing it.")
+ c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSynced, err)
+ c.recordStatus(au, utils.ResourceSynced, err, metav1.ConditionFalse, au.GetGeneration())
Review Comment:
`ResourceSynced` : When do you think the 'au' resource is considered to be successfully synchronized? Isn't it strange to retry even after success
--
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] chore: remove redundant logs and improve logs for users [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #2206:
URL: https://github.com/apache/apisix-ingress-controller/pull/2206#discussion_r1562206775
##########
pkg/providers/apisix/apisix_upstream.go:
##########
@@ -373,9 +383,19 @@ func (c *apisixUpstreamController) updateExternalNodes(ctx context.Context, au *
upsName := apisixv1.ComposeExternalUpstreamName(ns, name)
ups, err := c.APISIX.Cluster(clusterName).Upstream().Get(ctx, upsName)
if err != nil {
- log.Errorf("failed to get upstream %s: %s", upsName, err)
- c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSyncAborted, err)
- c.recordStatus(au, utils.ResourceSyncAborted, err, metav1.ConditionFalse, au.GetGeneration())
+ if err == apisix.ErrNotFound {
+ log.Debugw("upstream is not referenced",
+ zap.String("cluster", clusterName),
+ zap.String("upstream", upsName),
+ )
+ err = fmt.Errorf("%s", "upstream doesn't exist. It will be created after ApisixRoute is created referencing it.")
+ c.RecordEvent(au, corev1.EventTypeWarning, utils.ResourceSynced, err)
+ c.recordStatus(au, utils.ResourceSynced, err, metav1.ConditionFalse, au.GetGeneration())
Review Comment:
okay so I can remove the recordStatus in this case and keep the recordEvent with the new error. Right? @AlinsRan
--
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