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.

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

fix httpserver httppipeline status not show error #441

Merged
merged 4 commits into from
Dec 29, 2021
Merged

fix httpserver httppipeline status not show error #441

merged 4 commits into from
Dec 29, 2021

Conversation

suchen-sci
Copy link
Contributor

@suchen-sci suchen-sci commented Dec 24, 2021

Fix #438

return status is map -> map[member id]status... Since our egctl does not specific namespace now, we return status from all namespace. this part may need change.

@codecov-commenter
Copy link

codecov-commenter commented Dec 24, 2021

Codecov Report

Merging #441 (956d1d7) into main (ffd3bdc) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #441      +/-   ##
==========================================
- Coverage   80.55%   80.52%   -0.03%     
==========================================
  Files          70       70              
  Lines        8094     8154      +60     
==========================================
+ Hits         6520     6566      +46     
- Misses       1218     1229      +11     
- Partials      356      359       +3     
Impacted Files Coverage Δ
pkg/filter/mock/mock.go 90.42% <0.00%> (-4.66%) ⬇️
pkg/filter/proxy/pool.go 80.00% <0.00%> (-2.66%) ⬇️
pkg/cluster/cluster.go 50.68% <0.00%> (-1.03%) ⬇️
pkg/object/mqttproxy/client.go 77.67% <0.00%> (-0.94%) ⬇️
pkg/object/meshcontroller/spec/spec.go 87.20% <0.00%> (+0.07%) ⬆️
pkg/util/urlclusteranalyzer/urlclusteranalyzer.go 96.10% <0.00%> (+0.38%) ⬆️
pkg/util/jmxtool/agent_controller.go 66.66% <0.00%> (+1.96%) ⬆️
pkg/filter/proxy/request.go 95.31% <0.00%> (+9.59%) ⬆️

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 ffd3bdc...956d1d7. Read the comment docs.

@samutamm
Copy link
Contributor

I tested the scenario described in issue #438, using curl command curl localhost:2381/apis/v1/status/objects/server-demo. I got more or less the same output with main branch and this.. Would it be possible to list exact steps to reproduce the problem in main branch and then show that these changes fix it?

@suchen-sci
Copy link
Contributor Author

Hi, @samutamm after start up, you need wait some few seconds before request. easegress will collect status and store it to etcd. We get status from etcd. And if possible, please use egctl object status get server-demo command...

@suchen-sci
Copy link
Contributor Author

➜  easegress git:(fix-object-status) ✗ ./bin/egctl object status get server-demo
eg-default-name: |
  {}
➜  easegress git:(fix-object-status) ✗ ./bin/egctl object status get server-demo
eg-default-name: |
  {}
➜  easegress git:(fix-object-status) ✗ ./bin/egctl object status get server-demo
eg-default-name: |
  default: |
    health: ""
    state: running
    status:
      count: 0
      m1: 0
      m5: 0
      m15: 0
      errCount: 0
      m1Err: 0
      m5Err: 0
      m15Err: 0
      m1ErrPercent: 0
      m5ErrPercent: 0
      m15ErrPercent: 0
      min: 0
      max: 0
      mean: 0
      p25: 9.999999e+06
      p50: 9.999999e+06
      p75: 9.999999e+06
      p95: 9.999999e+06
      p98: 9.999999e+06
      p99: 9.999999e+06
      p999: 9.999999e+06
      reqSize: 0
      respSize: 0
      codes: {}
    topN: []

here are my results after wait about half minute...

@xxx7xxxx
Copy link
Contributor

Actually, the root cause here is that TrafficController took the control of the lifecycle of HTTPServer/HTTPPipeline, so it's intuitive that the statuses of servers/pipelines are reported by the TrafficController, where it will cause the mislocation between config and status of them.

I'm fine with this quick fix without deep and tidy refactoring. But if there brought more kinds of stuff under TrafficController, we should fix them in a whole policy(TrafficController syncing or move the status to the corresponding location).

pkg/api/cluster.go Outdated Show resolved Hide resolved
@suchen-sci
Copy link
Contributor Author

suchen-sci commented Dec 29, 2021

@xxx7xxxx @samutamm @localvar I updated the code, previous egctl object status get will return map[member id]map[namespace]status. now it will show map[member id]status. we only show results from the default namespace when use egctl. Is this ok for you?

@xxx7xxxx
Copy link
Contributor

Returning status only from the default namespace is all right.

Copy link
Contributor

@samutamm samutamm left a comment

Choose a reason for hiding this comment

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

LGTM!

pkg/api/cluster.go Outdated Show resolved Hide resolved
pkg/api/cluster.go Outdated Show resolved Hide resolved
@localvar localvar merged commit 93759ff into easegress-io:main Dec 29, 2021
@suchen-sci suchen-sci deleted the fix-object-status branch January 26, 2022 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

get object status return empty
5 participants