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

Fix path to indico CLI #234

Merged
merged 2 commits into from
Feb 6, 2023
Merged

Fix path to indico CLI #234

merged 2 commits into from
Feb 6, 2023

Conversation

nrobinaubertin
Copy link
Contributor

@nrobinaubertin nrobinaubertin commented Feb 3, 2023 •
edited
Loading

The PR #220 had an issue with the indico CLI path (that has changed while working on it).
This fixes it.

The k6 load tests don't pass anymore with the p95 threshold of 800ms. But they do for 1500ms.
The PR #220 should have nothing to do with it in principle. I'm not sure why this is happening.
Is it acceptable to increase the p95 duration threshold ?

@github-actions
Copy link
Contributor

github-actions bot commented Feb 4, 2023

Test coverage for 83b9c25

Name           Stmts   Miss Branch BrPart  Cover   Missing
----------------------------------------------------------
src/charm.py     303      6     82     12    95%   124->exit, 140, 144, 497->501, 498->exit, 545->548, 648->687, 765-766, 828->exit, 848->864, 850->859, 859->864, 872-873, 916->exit
----------------------------------------------------------
TOTAL            303      6     82     12    95%

Static code analysis report

Run started:2023-02-04 12:53:14.941496

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 1801
  Total lines skipped (#nosec): 4
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 0

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

@nrobinaubertin nrobinaubertin marked this pull request as ready for review February 5, 2023 16:46
@nrobinaubertin nrobinaubertin requested a review from a team as a code owner February 5, 2023 16:46
@gregory-schiano
Copy link
Contributor

Could be linked to the GH Action instance, do you see a difference when you run k6s with indico locally deployed ?

@jdkandersson
Copy link
Contributor

jdkandersson commented Feb 5, 2023 •
edited by nrobinaubertin
Loading

Given there was a bug, should we now add an e2ea test that would have caught it?

@nrobinaubertin
Copy link
Contributor Author

Given there was a bug, should we now add an e2ea test that would have caught it?

It was actually caught. It was a mistake on my part to merge the previous PR

@jdkandersson
Copy link
Contributor

Given there was a bug, should we now add an e2ea test that would have caught it?

It was actually caught. It was a mistake on my part to merge the previous PR

Yes I spotted that later as well, the PR to operator workflows improving the required status checks should help with this in the future

merkata
merkata approved these changes Feb 6, 2023
View reviewed changes
Copy link
Contributor

@merkata merkata left a comment

Choose a reason for hiding this comment

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

I'm fine with the change, let's see how and why the response time has changed, we can tweak around after the PR.

arturo-seijas
arturo-seijas approved these changes Feb 6, 2023
View reviewed changes
Copy link
Collaborator

@arturo-seijas arturo-seijas left a comment

Choose a reason for hiding this comment

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

LGTM

@nrobinaubertin
Copy link
Contributor Author

Could be linked to the GH Action instance, do you see a difference when you run k6s with indico locally deployed ?

Here is the k6 load test result on my local microk8s on commit 472ddfc:

running (1m00.6s), 00/50 VUs, 2900 complete and 0 interrupted iterations
default ✓ [======================================] 50 VUs  1m0s

     data_received..................: 51 MB  845 kB/s
     data_sent......................: 566 kB 9.3 kB/s
     http_req_blocked...............: avg=15.92µs  min=3.77µs  med=8.66µs   max=3.28ms   p(90)=12.01µs  p(95)=13.82µs
     http_req_connecting............: avg=4.74µs   min=0s      med=0s       max=2.8ms    p(90)=0s       p(95)=0s
   ✓ http_req_duration..............: avg=20.32ms  min=4.81ms  med=16.35ms  max=450.5ms  p(90)=29.45ms  p(95)=35.33ms
       { expected_response:true }...: avg=20.32ms  min=4.81ms  med=16.35ms  max=450.5ms  p(90)=29.45ms  p(95)=35.33ms
   ✓ http_req_failed................: 0.00%  ✓ 0         ✗ 5800
     http_req_receiving.............: avg=115.71µs min=34.85µs med=109.89µs max=697.16µs p(90)=160.35µs p(95)=179.85µs
     http_req_sending...............: avg=26.7µs   min=8.1µs   med=24.3µs   max=835.58µs p(90)=35.27µs  p(95)=40.43µs
     http_req_tls_handshaking.......: avg=0s       min=0s      med=0s       max=0s       p(90)=0s       p(95)=0s
     http_req_waiting...............: avg=20.18ms  min=4.72ms  med=16.21ms  max=450.32ms p(90)=29.27ms  p(95)=35.15ms
     http_reqs......................: 5800   95.778061/s
     iteration_duration.............: avg=1.04s    min=1.01s   med=1.03s    max=1.6s     p(90)=1.05s    p(95)=1.06s
     iterations.....................: 2900   47.889031/s
     vus............................: 50     min=50      max=50
     vus_max........................: 50     min=50      max=50

And here is the one on this commit:

running (1m00.9s), 00/50 VUs, 2901 complete and 0 interrupted iterations
default ✓ [======================================] 50 VUs  1m0s

     data_received..................: 51 MB  840 kB/s
     data_sent......................: 566 kB 9.3 kB/s
     http_req_blocked...............: avg=25.74µs  min=3.77µs  med=8.86µs   max=5.08ms   p(90)=12.36µs  p(95)=14.1µs
     http_req_connecting............: avg=5.34µs   min=0s      med=0s       max=4.91ms   p(90)=0s       p(95)=0s
   ✓ http_req_duration..............: avg=21.09ms  min=5.05ms  med=17.23ms  max=409.05ms p(90)=30.66ms  p(95)=36.63ms
       { expected_response:true }...: avg=21.09ms  min=5.05ms  med=17.23ms  max=409.05ms p(90)=30.66ms  p(95)=36.63ms
   ✓ http_req_failed................: 0.00%  ✓ 0         ✗ 5802
     http_req_receiving.............: avg=118.56µs min=28.56µs med=112.23µs max=702.33µs p(90)=166.36µs p(95)=186.88µs
     http_req_sending...............: avg=27.07µs  min=7.96µs  med=24.86µs  max=496.22µs p(90)=36.45µs  p(95)=41.83µs
     http_req_tls_handshaking.......: avg=0s       min=0s      med=0s       max=0s       p(90)=0s       p(95)=0s
     http_req_waiting...............: avg=20.94ms  min=4.94ms  med=17.08ms  max=408.86ms p(90)=30.55ms  p(95)=36.41ms
     http_reqs......................: 5802   95.247821/s
     iteration_duration.............: avg=1.04s    min=1.01s   med=1.03s    max=1.54s    p(90)=1.05s    p(95)=1.06s
     iterations.....................: 2901   47.62391/s
     vus............................: 50     min=50      max=50
     vus_max........................: 50     min=50      max=50

No difference to be seen locally.
If it's okay for you @gregory-schiano, I'll merge this.

@gregory-schiano
Copy link
Contributor

Could be linked to the GH Action instance, do you see a difference when you run k6s with indico locally deployed ?

Here is the k6 load test result on my local microk8s on commit 472ddfc:

running (1m00.6s), 00/50 VUs, 2900 complete and 0 interrupted iterations
default ✓ [======================================] 50 VUs  1m0s

     data_received..................: 51 MB  845 kB/s
     data_sent......................: 566 kB 9.3 kB/s
     http_req_blocked...............: avg=15.92µs  min=3.77µs  med=8.66µs   max=3.28ms   p(90)=12.01µs  p(95)=13.82µs
     http_req_connecting............: avg=4.74µs   min=0s      med=0s       max=2.8ms    p(90)=0s       p(95)=0s
   ✓ http_req_duration..............: avg=20.32ms  min=4.81ms  med=16.35ms  max=450.5ms  p(90)=29.45ms  p(95)=35.33ms
       { expected_response:true }...: avg=20.32ms  min=4.81ms  med=16.35ms  max=450.5ms  p(90)=29.45ms  p(95)=35.33ms
   ✓ http_req_failed................: 0.00%  ✓ 0         ✗ 5800
     http_req_receiving.............: avg=115.71µs min=34.85µs med=109.89µs max=697.16µs p(90)=160.35µs p(95)=179.85µs
     http_req_sending...............: avg=26.7µs   min=8.1µs   med=24.3µs   max=835.58µs p(90)=35.27µs  p(95)=40.43µs
     http_req_tls_handshaking.......: avg=0s       min=0s      med=0s       max=0s       p(90)=0s       p(95)=0s
     http_req_waiting...............: avg=20.18ms  min=4.72ms  med=16.21ms  max=450.32ms p(90)=29.27ms  p(95)=35.15ms
     http_reqs......................: 5800   95.778061/s
     iteration_duration.............: avg=1.04s    min=1.01s   med=1.03s    max=1.6s     p(90)=1.05s    p(95)=1.06s
     iterations.....................: 2900   47.889031/s
     vus............................: 50     min=50      max=50
     vus_max........................: 50     min=50      max=50

And here is the one on this commit:

running (1m00.9s), 00/50 VUs, 2901 complete and 0 interrupted iterations
default ✓ [======================================] 50 VUs  1m0s

     data_received..................: 51 MB  840 kB/s
     data_sent......................: 566 kB 9.3 kB/s
     http_req_blocked...............: avg=25.74µs  min=3.77µs  med=8.86µs   max=5.08ms   p(90)=12.36µs  p(95)=14.1µs
     http_req_connecting............: avg=5.34µs   min=0s      med=0s       max=4.91ms   p(90)=0s       p(95)=0s
   ✓ http_req_duration..............: avg=21.09ms  min=5.05ms  med=17.23ms  max=409.05ms p(90)=30.66ms  p(95)=36.63ms
       { expected_response:true }...: avg=21.09ms  min=5.05ms  med=17.23ms  max=409.05ms p(90)=30.66ms  p(95)=36.63ms
   ✓ http_req_failed................: 0.00%  ✓ 0         ✗ 5802
     http_req_receiving.............: avg=118.56µs min=28.56µs med=112.23µs max=702.33µs p(90)=166.36µs p(95)=186.88µs
     http_req_sending...............: avg=27.07µs  min=7.96µs  med=24.86µs  max=496.22µs p(90)=36.45µs  p(95)=41.83µs
     http_req_tls_handshaking.......: avg=0s       min=0s      med=0s       max=0s       p(90)=0s       p(95)=0s
     http_req_waiting...............: avg=20.94ms  min=4.94ms  med=17.08ms  max=408.86ms p(90)=30.55ms  p(95)=36.41ms
     http_reqs......................: 5802   95.247821/s
     iteration_duration.............: avg=1.04s    min=1.01s   med=1.03s    max=1.54s    p(90)=1.05s    p(95)=1.06s
     iterations.....................: 2901   47.62391/s
     vus............................: 50     min=50      max=50
     vus_max........................: 50     min=50      max=50

No difference to be seen locally. If it's okay for you @gregory-schiano, I'll merge this.

👍
You can go

@nrobinaubertin nrobinaubertin merged commit 345ae0b into main Feb 6, 2023
@nrobinaubertin nrobinaubertin deleted the fix-integ branch February 6, 2023 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merkata merkata merkata approved these changes

@arturo-seijas arturo-seijas arturo-seijas approved these changes

Assignees
No one assigned
Labels
Libraries: Out of sync
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nrobinaubertin @gregory-schiano @jdkandersson @merkata @arturo-seijas