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

feat: Fix lb ingress status #740

Merged
merged 10 commits into from
Dec 2, 2021
Merged

feat: Fix lb ingress status #740

merged 10 commits into from
Dec 2, 2021

Conversation

tao12345666333
Copy link
Member

Please answer these questions before submitting a pull request

Bugfix

  • Description

  • How to fix?

New feature or improvement

  • Describe the details and related test reports.

Backport patches

  • Why need to backport?

  • Source branch

  • Related commits and pull requests

  • Target branch

@tao12345666333
feat: add configuration for ingress status
64c5e66 
Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>
@tao12345666333
chore: record status
021ec93 
Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2021 •
edited
Loading

Codecov Report

Merging #740 (a46283a) into master (f470867) will decrease coverage by 0.20%.
The diff coverage is 11.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #740      +/-   ##
==========================================
- Coverage   32.28%   32.08%   -0.21%     
==========================================
  Files          66       66              
  Lines        6808     6944     +136     
==========================================
+ Hits         2198     2228      +30     
- Misses       4353     4466     +113     
+ Partials      257      250       -7
Impacted Files Coverage Δ
pkg/ingress/apisix_route.go 0.00% <ø> (ø)
pkg/ingress/ingress.go 6.89% <0.00%> (-1.22%) ⬇️
pkg/ingress/status.go 0.00% <0.00%> (ø)
cmd/ingress/ingress.go 77.77% <100.00%> (+1.95%) ⬆️
pkg/config/config.go 65.68% <100.00%> (+0.68%) ⬆️
pkg/apisix/route.go 38.92% <0.00%> (+2.01%) ⬆️
pkg/apisix/cluster.go 33.33% <0.00%> (+3.30%) ⬆️
pkg/apisix/plugin.go 100.00% <0.00%> (+20.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f470867...a46283a. Read the comment docs.

@tao12345666333 tao12345666333 marked this pull request as ready for review November 14, 2021 15:22
@tao12345666333 tao12345666333 changed the title Fix lb ingress status feat: Fix lb ingress status Nov 14, 2021
@tao12345666333 tao12345666333 added this to In progress in v1.4 Planning via automation Nov 14, 2021
@tao12345666333 tao12345666333 added this to the 1.4.0 milestone Nov 14, 2021
@tao12345666333
test: add e2e test case
01d7079 
Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>
gxthrj
gxthrj reviewed Nov 15, 2021
View reviewed changes
pkg/ingress/ingress.go Outdated Show resolved Hide resolved
tokers
tokers reviewed Nov 15, 2021
View reviewed changes
pkg/ingress/status.go
return addrs, nil
}

namespace, name, err := cache.SplitMetaNamespaceKey(ingressPublishService)
Copy link
Contributor

Choose a reason for hiding this comment

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

The separation can be handled in advance.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't understand what you mean.

Copy link
Member Author

Choose a reason for hiding this comment

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

This specified service is only needed here, I don’t think it needs to be checked or processed in advance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the namespace is passed from users, we can split it and check the validity when the program is starting, so we can avoid such an error check here and we can abort the program in time (if the format is incorrect).

Copy link
Member Author

Choose a reason for hiding this comment

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

But users may not configure Ingress resources.

If the user configures this option, but he only uses custom resources, do we also need to report an error? And if there is any error, it will not affect the actual use of the user.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it is not a parameter required by the real runtime, I don’t want to refuse to provide services to users in advance.

pkg/ingress/status.go Show resolved Hide resolved
pkg/ingress/status.go Show resolved Hide resolved
@tao12345666333
fix: e2e test case
8e29e5f 
Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>
@tao12345666333
fix: comments
554c677 
Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>
@tao12345666333 tao12345666333 mentioned this pull request Nov 18, 2021
Closed
@tao12345666333 tao12345666333 moved this from In progress to Review in progress in v1.4 Planning Nov 26, 2021
@tao12345666333
docs: add description
11b68ec 
Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>
@tao12345666333
Copy link
Member Author

I will fix CI ASAP

@tao12345666333
Merge branch 'master' of https://github.com/apache/apisix-ingress-con…
868a15f 
…troller into fix-lb-ingress-status
@tao12345666333
fix: config unit test
a46283a 
Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>
v1.4 Planning automation moved this from Review in progress to Reviewer approved Dec 2, 2021
@tao12345666333 tao12345666333 merged commit 0c6de2d into apache:master Dec 2, 2021
v1.4 Planning automation moved this from Reviewer approved to Done Dec 2, 2021
@tao12345666333 tao12345666333 deleted the fix-lb-ingress-status branch December 2, 2021 08:53
@tao12345666333 tao12345666333 mentioned this pull request Dec 6, 2021
Closed
Sindweller pushed a commit to Sindweller/apisix-ingress-controller that referenced this pull request Dec 18, 2021
@tao12345666333 @Sindweller
feat: update Ingress LB status (apache#740)
9eda6f0
@tao12345666333 tao12345666333 linked an issue Dec 29, 2021 that may be closed by this pull request
bug: Ingress Status Load Balancer IPs Not Being Updated #690
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gxthrj gxthrj gxthrj approved these changes

@tokers tokers tokers approved these changes

@lingsamuel lingsamuel Awaiting requested review from lingsamuel

Assignees
No one assigned
Labels
area/controller
Projects
No open projects
v1.4 Planning
Done
Milestone
1.4.0
Development

Successfully merging this pull request may close these issues.

bug: Ingress Status Load Balancer IPs Not Being Updated
4 participants
@tao12345666333 @codecov-commenter @gxthrj @tokers