- Notifications
You must be signed in to change notification settings - Fork1k
feat: Add provisionerd service#127
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedFeb 1, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #127 +/- ##==========================================- Coverage 72.09% 71.92% -0.17%========================================== Files 91 92 +1 Lines 3773 4168 +395 Branches 59 59 ==========================================+ Hits 2720 2998 +278- Misses 832 927 +95- Partials 221 243 +22
Continue to review full report at Codecov.
|
fa0df57 to0886db4CompareThis brings an async service that parses andprovisions to life! It's separated from coderdintentionally to allow for simpler testing.Integration with coderd will come in another PR!
| completeChan := make(chan struct{}) | ||
| closer := createProvisionerd(t, func(ctx context.Context) (proto.DRPCProvisionerDaemonClient, error) { | ||
| acquireJobAttempt := 0 | ||
| return createProvisionerDaemonClient(t, provisionerDaemonTestServer{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I like that it is so easy to create testing dialers to control the acquire/create/update/cancel job behavior 👍
| var closer io.Closer | ||
| var closerMutex sync.Mutex | ||
| closerMutex.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
One thing that isn't obvious to me - why does this test require a mutex to protect theio.Closer?
It's not clear to me why we need to callcloser.Close in both the provisioner and at the end of the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The go data race was triggering on this, because technically in a perfect scenario it could callcloser before it completed being defined.
Obviously the scheduler would make this essentially impossible, but the race detector is strict!
| acquireJob: func(ctx context.Context, _ *proto.Empty) (*proto.AcquiredJob, error) { | ||
| return &proto.AcquiredJob{ | ||
| JobId: "test", | ||
| Provisioner: "someprovisioner", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Cool that we can test actually sending stuff to a testing provisioner 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just a small question about one of the tests but otherwise looks good to me! Another big step forward for black triangle
Upgrades yamux to 0.1.2, which includes a couple bug fixes.> Significant Changes> * Fixed a case where Streams may continue to exist and block operations even after their Session has been closed.#127 ensures when a Session closes that blocking Stream operations exit as well.> * Allow Reads on locally closed streams. Prior to#131 calling Close() and then Read() on a Stream would fail. Close should only indicate the Stream is done writing. The peer must call Close before Read considers the stream closed. See#131 for details.> * Tests have been improved significantly. See below for details.https://github.com/hashicorp/yamux/releases/tag/v0.1.2
This brings an async service that parses and
provisions to life! It's separated from coderd
intentionally to allow for simpler testing.
Integration with coderd will come in another PR!