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

Ci busy no more #143

Merged
merged 24 commits into from
Jun 28, 2019
Merged

Ci busy no more #143

merged 24 commits into from
Jun 28, 2019

Conversation

kevindweb
Copy link
Contributor

@kevindweb kevindweb commented Jun 8, 2019

A huge improvement to testing pull requests throughout the day!

Summary:

Previously, CI would not run a request if another was already running. This is a bug annoyance (especially for @koolzz ) when trying to merge things and get the 'go ahead' from CI. Now, CI will send a message that if we are busy, it will run right after the last CI. This only happens if a duplicate request is not already there. For example, while CI is busy, we make a 2 requests in another PR, only one of them will be put in the list.

Usage:
Regular with CI comment arguments

This PR includes
Resolves issues
Breaking API changes
Internal API changes
Usability improvements
Bug fixes
New functionality 👍
New NF/onvm_mgr args
Changes to starting NFs
Dependency updates
Web stats updates

Merging notes:

TODO before merging :

  • PR is ready for review

Test Plan:

Review:

@koolzz again please lmk if anything doesn't work

@onvm
Copy link

onvm commented Jun 8, 2019

In response to PR creation

CI Message

Your results will arrive shortly

@koolzz
Copy link
Member

koolzz commented Jun 8, 2019

Exiting stuff, will review soon!

Copy link

@onvm onvm left a comment

Choose a reason for hiding this comment

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

In response to PR creation

CI Message

Run successful see results:
✔️ PR submitted to develop branch
✔️ Speed tester performance check passed
✔️ Linter passed

[Results from nimbnode30]

  • Median TX pps for Speed Tester: 38624205
  • Performance rating - 110.35% (compared to 35000000 average)

@koolzz koolzz added the CI/Testing 🤖 Continuous Integration and Testing related label Jun 8, 2019
Copy link
Member

@koolzz koolzz left a comment

Choose a reason for hiding this comment

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

Not a fan of the main CI pipeline call both adding events to the queue and running all of them in a while loop, would be better if a dedicated thread was specifically waiting for those events to be added to the list, and then processing those.

Also the name ci_list isn't very descriptive, we should do better. Something like ci_job_list or ci_requests_list?

Overall cool stuff though looking forward to not waiting 10 minutes for ci requests 👍

duplicate_req = False
busy_msg = "Duplicate request already waiting, ignoring message"
with queue_lock:
# make sure we don't bypass thread safety
Copy link
Member

Choose a reason for hiding this comment

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

Should this comment be above one line? Before you grab the queue_lock

ci/webhook-receiver.py Show resolved Hide resolved
run_manager(request_ctx)
while len(ci_list) > 0:
# new requests were made since our last call, run them in order
run_manager(ci_list.pop(0))
Copy link
Member

Choose a reason for hiding this comment

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

Even though I think this is safe (pop is atomic right?) the design isn't very elegant. Would it be possible for us to have a different thread poll the queue and run things when items appear there?

@kevindweb
Copy link
Contributor Author

@onvm test thread

@kevindweb
Copy link
Contributor Author

@onvm test me again

@kevindweb
Copy link
Contributor Author

@onvm testing again

@onvm
Copy link

onvm commented Jun 10, 2019

@onvm testing again

CI Message

Your results will arrive shortly

run_manager(ci_request_list.pop(0))
else:
# no requests, sleep
time.sleep(1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figured we could just sleep for a second, but not sure how long is best

Copy link
Member

Choose a reason for hiding this comment

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

Might want a sleeping queue sort of implementation in the future, but good enough for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean?

@kevindweb
Copy link
Contributor Author

@onvm authorized stick in queue

@kevindweb
Copy link
Contributor Author

@onvm test please run

@koolzz
Copy link
Member

koolzz commented Jun 14, 2019

Discussing these in slack

@kevindweb
Copy link
Contributor Author

@onvm test

1 similar comment
@kevindweb
Copy link
Contributor Author

@onvm test

@kevindweb
Copy link
Contributor Author

@onvm test 1

@onvm
Copy link

onvm commented Jun 15, 2019

New message here

3 similar comments
@onvm
Copy link

onvm commented Jun 15, 2019

New message here

@onvm
Copy link

onvm commented Jun 15, 2019

New message here

@onvm
Copy link

onvm commented Jun 15, 2019

New message here

@kevindweb
Copy link
Contributor Author

@onvm first one

@onvm
Copy link

onvm commented Jun 17, 2019

@onvm first one

CI Message

Your results will arrive shortly

@kevindweb
Copy link
Contributor Author

@onvm run me

@kevindweb
Copy link
Contributor Author

@onvm test

@kevindweb
Copy link
Contributor Author

onvmstats "commited" the changes because on nn44, cronjob for stats is running

@onvm
Copy link

onvm commented Jun 18, 2019

qqq

CI Message

Error: ERROR: Failed to copy ONVM files to nimbnode17

Copy link
Member

@koolzz koolzz left a comment

Choose a reason for hiding this comment

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

Commented on the event flag, lmk what you think


def add_request(request_ctx):
ci_request_list.append(request_ctx)
if not request_event.isSet():
Copy link
Member

Choose a reason for hiding this comment

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

Why not just set request_event.set() and then do equest_event.clear() in the request_handler?

manager_running = False
if not ci_request_list:
# list empty, go to sleep until signaled
request_event.wait()
Copy link
Member

Choose a reason for hiding this comment

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

request_event.wait()
request_event.clear()

@kevindweb
Copy link
Contributor Author

I liked that idea, makes it look cleaner in add_request

@koolzz
Copy link
Member

koolzz commented Jun 20, 2019

onvmstats is now sentient and pushing commits? I'm impressed with our development program.
@onvm

@onvm
Copy link

onvm commented Jun 23, 2019

qqq

CI Message

Error: ERROR: Failed to copy ONVM files to nimbnode17

2 similar comments
@onvm
Copy link

onvm commented Jun 23, 2019

qqq

CI Message

Error: ERROR: Failed to copy ONVM files to nimbnode17

@onvm
Copy link

onvm commented Jun 23, 2019

qqq

CI Message

Error: ERROR: Failed to copy ONVM files to nimbnode17

@kevindweb kevindweb mentioned this pull request Jun 24, 2019
1 task
@koolzz
Copy link
Member

koolzz commented Jun 28, 2019

@onvm do your thing

@onvm
Copy link

onvm commented Jun 28, 2019

@onvm do your thing

CI Message

Your results will arrive shortly

Copy link

@onvm onvm left a comment

Choose a reason for hiding this comment

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

@onvm do your thing

CI Message

Run successful see results:
✔️ PR submitted to develop branch
✔️ Pktgen performance check passed
✔️ Speed Test performance check passed
✔️ Linter passed

[Results from nimbnode17]

  • Median TX pps for Pktgen: 6325632

  • Performance rating - 105.43% (compared to 6000000 average)

  • Median TX pps for Speed Tester: 38346945

  • Performance rating - 109.56% (compared to 35000000 average)

Copy link
Member

@koolzz koolzz left a comment

Choose a reason for hiding this comment

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

Changes from previous review implemented, approving

@koolzz koolzz merged commit 5a40435 into sdnfv:develop Jun 28, 2019
@kevindweb kevindweb deleted the ci_queue branch June 29, 2019 21:21
@koolzz koolzz added this to the ONVM v19.07 milestone Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/Testing 🤖 Continuous Integration and Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants