Skip to content

chore: remove redundant logs and improve logs for users #2206

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.

Sign up for GitHub

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

Jump to bottom
Merged
Revolyssup merged 3 commits into from
Apr 12, 2024
Merged

chore: remove redundant logs and improve logs for users #2206

Revolyssup merged 3 commits into from
Apr 12, 2024

Conversation

@Revolyssup
Copy link
Contributor

@Revolyssup Revolyssup commented Apr 9, 2024

Type of change:

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches
  • Documentation
  • Refactor
  • Chore
  • CI/CD or Tests

What this PR does / why we need it:

Pre-submission checklist:

  • 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 first

@Revolyssup Revolyssup changed the title chore: remove redundant logs and improve error when upstream is created chore: remove redundant logs and improve logs for users Apr 9, 2024
AlinsRan
AlinsRan reviewed Apr 12, 2024
View reviewed changes
pkg/providers/apisix/apisix_upstream.go Outdated
)
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())
Copy link
Contributor

@AlinsRan AlinsRan Apr 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ResourceSynced : When do you think the au resource is considered to be successfully synchronized? Isn't it strange to retry even after success

Copy link
Contributor Author

@Revolyssup Revolyssup Apr 12, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@AlinsRan AlinsRan Apr 12, 2024

Choose a reason for hiding this comment

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

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.

image

Copy link
Contributor Author

@Revolyssup Revolyssup Apr 12, 2024

Choose a reason for hiding this comment

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

okay so I can remove the recordStatus in this case and keep the recordEvent with the new error. Right? @AlinsRan

AlinsRan
AlinsRan reviewed Apr 12, 2024
View reviewed changes
@Revolyssup
Copy link
Contributor Author

Revolyssup commented Apr 12, 2024

@AlinsRan committed your suggestion

@Revolyssup Revolyssup merged commit 42b9e4d into apache:master Apr 12, 2024
Revolyssup added a commit to Revolyssup/apisix-ingress-controller that referenced this pull request Apr 12, 2024
@Revolyssup @AlinsRan
chore: remove redundant logs and improve logs for users (apache#2206)
6f1a775 
* chore: remove redundant logs and improve error when upstream is created

Co-authored-by: AlinsRan <[email protected]>
Revolyssup added a commit that referenced this pull request Apr 12, 2024
@Revolyssup @Gallardot
@zll600 @acuteaura @AlinsRan @dependabot
release: add release notes for 1.8.1 and cherry pick commits (#2211)
4d42c9a 
* docs: clarify usage of external service discovery (#2124)

* feat: add plugin_config_namespace parameter to ApisixRoute (#2137)

* feat: add plugin_config_namespace parameter to ApisixRoute

Add plugin_config_namespace parameter to ApisixRoute resource to allow cross namespace discovery.

* fix indentation

Signed-off-by: Ashish Tiwari <[email protected]>

* remove route.yaml

Signed-off-by: Ashish Tiwari <[email protected]>

* fix e2e test

Signed-off-by: Ashish Tiwari <[email protected]>

* update gomod gosum

* fix e2e test

Signed-off-by: Ashish Tiwari <[email protected]>

* fix e2e test

Signed-off-by: Ashish Tiwari <[email protected]>

* Update pkg/providers/apisix/apisix_route.go

Co-authored-by: Gallardot <[email protected]>

* create namespace

* refactor test

* refactor test

* fix e2e

* fix e2e

* update crd

* Add EOL

Signed-off-by: Ashish Tiwari <[email protected]>

---------

Signed-off-by: Ashish Tiwari <[email protected]>
Co-authored-by: Gallardot <[email protected]>

* fix: remove path validation (#2140)

* docs: update NOTICE (#2149)

* refactor(cmd/ingress): invert signal ctx logic (#2139)

* refactor(cmd/ingress): invert signal ctx logic

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)

* test: failing flaky unit test (#2151)

* fix: failing flaky unit test

* chore(ci): remove tao12345666333 and lingsamuel in reviewers (#2150)

* chore(deps): bump github.com/onsi/ginkgo/v2 in /test/e2e (#2177)

Bumps [github.com/onsi/ginkgo/v2](https://github.com/onsi/ginkgo) from 2.13.2 to 2.16.0.
- [Release notes](https://github.com/onsi/ginkgo/releases)
- [Changelog](https://github.com/onsi/ginkgo/blob/master/CHANGELOG.md)
- [Commits](onsi/ginkgo@v2.13.2...v2.16.0)

---
updated-dependencies:
- dependency-name: github.com/onsi/ginkgo/v2
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(deps): bump k8s.io/apimachinery from 0.29.0 to 0.29.2 in /test/e2e (#2161)

Bumps [k8s.io/apimachinery](https://github.com/kubernetes/apimachinery) from 0.29.0 to 0.29.2.
- [Commits](kubernetes/apimachinery@v0.29.0...v0.29.2)

---
updated-dependencies:
- dependency-name: k8s.io/apimachinery
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(deps): bump k8s.io/api from 0.29.0 to 0.29.2 in /test/e2e (#2163)

Bumps [k8s.io/api](https://github.com/kubernetes/api) from 0.29.0 to 0.29.2.
- [Commits](kubernetes/api@v0.29.0...v0.29.2)

---
updated-dependencies:
- dependency-name: k8s.io/api
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(deps): bump go.uber.org/zap from 1.26.0 to 1.27.0 in /test/e2e (#2172)

Bumps [go.uber.org/zap](https://github.com/uber-go/zap) from 1.26.0 to 1.27.0.
- [Release notes](https://github.com/uber-go/zap/releases)
- [Changelog](https://github.com/uber-go/zap/blob/master/CHANGELOG.md)
- [Commits](uber-go/zap@v1.26.0...v1.27.0)

---
updated-dependencies:
- dependency-name: go.uber.org/zap
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(deps): bump k8s.io/client-go from 0.29.0 to 0.29.2 in /test/e2e (#2162)

Bumps [k8s.io/client-go](https://github.com/kubernetes/client-go) from 0.29.0 to 0.29.2.
- [Changelog](https://github.com/kubernetes/client-go/blob/master/CHANGELOG.md)
- [Commits](kubernetes/client-go@v0.29.0...v0.29.2)

---
updated-dependencies:
- dependency-name: k8s.io/client-go
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(deps): bump k8s.io/apimachinery from 0.29.2 to 0.29.3 in /test/e2e (#2185)

Bumps [k8s.io/apimachinery](https://github.com/kubernetes/apimachinery) from 0.29.2 to 0.29.3.
- [Commits](kubernetes/apimachinery@v0.29.2...v0.29.3)

---
updated-dependencies:
- dependency-name: k8s.io/apimachinery
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore: upgrade etcd-adapter to latest version (#2205)

* chore(deps): bump github.com/spf13/cobra from 1.7.0 to 1.8.0 (#2196)

Bumps [github.com/spf13/cobra](https://github.com/spf13/cobra) from 1.7.0 to 1.8.0.
- [Release notes](https://github.com/spf13/cobra/releases)
- [Commits](spf13/cobra@v1.7.0...v1.8.0)

---
updated-dependencies:
- dependency-name: github.com/spf13/cobra
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(deps): bump github.com/onsi/ginkgo/v2 in /test/e2e (#2195)

Bumps [github.com/onsi/ginkgo/v2](https://github.com/onsi/ginkgo) from 2.16.0 to 2.17.1.
- [Release notes](https://github.com/onsi/ginkgo/releases)
- [Changelog](https://github.com/onsi/ginkgo/blob/master/CHANGELOG.md)
- [Commits](onsi/ginkgo@v2.16.0...v2.17.1)

---
updated-dependencies:
- dependency-name: github.com/onsi/ginkgo/v2
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix: use force=true to hard delete the apisix resource (#2210)

Signed-off-by: Ashish Tiwari <[email protected]>

* chore: remove redundant logs and improve logs for users (#2206)

* chore: remove redundant logs and improve error when upstream is created

Co-authored-by: AlinsRan <[email protected]>


---------

Signed-off-by: Ashish Tiwari <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Gallardot <[email protected]>
Co-authored-by: Leigang Zhang <[email protected]>
Co-authored-by: Aurelia <[email protected]>
Co-authored-by: AlinsRan <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@Revolyssup Revolyssup deleted the revolyssup/removewarn branch April 14, 2024 19:34
This was referenced Jul 9, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shreemaan-abhishek shreemaan-abhishek shreemaan-abhishek approved these changes

@AlinsRan AlinsRan AlinsRan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Revolyssup @shreemaan-abhishek @AlinsRan