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.

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

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

Merged
merged 3 commits into from
Apr 12, 2024
Merged

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

merged 3 commits into from
Apr 12, 2024

Conversation

Revolyssup
Copy link
Contributor

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

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

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

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

@Revolyssup @AlinsRan
Update pkg/providers/apisix/apisix_upstream.go
66c34f2 
Co-authored-by: AlinsRan <alinsran333@gmail.com>
@Revolyssup
Copy link
Contributor Author

@AlinsRan committed your suggestion

@Revolyssup Revolyssup merged commit 42b9e4d into apache:master Apr 12, 2024
79 checks passed
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 <alinsran333@gmail.com>
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 <ashishjaitiwari15112000@gmail.com>

* remove route.yaml

Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>

* fix e2e test

Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>

* update gomod gosum

* fix e2e test

Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>

* fix e2e test

Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>

* Update pkg/providers/apisix/apisix_route.go

Co-authored-by: Gallardot <gallardot@apache.org>

* create namespace

* refactor test

* refactor test

* fix e2e

* fix e2e

* update crd

* Add EOL

Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>

---------

Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>
Co-authored-by: Gallardot <gallardot@apache.org>

* 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] <support@github.com>
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] <support@github.com>
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] <support@github.com>
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] <support@github.com>
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] <support@github.com>
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] <support@github.com>
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] <support@github.com>
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] <support@github.com>
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 <ashishjaitiwari15112000@gmail.com>

* 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 <alinsran333@gmail.com>


---------

Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Gallardot <gallardot@apache.org>
Co-authored-by: Leigang Zhang <71714656+zll600@users.noreply.github.com>
Co-authored-by: Aurelia <aurelia@acuteaura.net>
Co-authored-by: AlinsRan <alinsran333@gmail.com>
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
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlinsRan AlinsRan AlinsRan approved these changes

@shreemaan-abhishek shreemaan-abhishek shreemaan-abhishek 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