This replaces the Drydock preallocated host blueprint with a blueprint that uses Almanac services to source hosts.
Details
Details
- Reviewers
hach-que - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T9253: Implement a Drydock blueprint for leasing hosts from a static, pre-built Almanac pool
Configured an Almanac service pool to use localhost as a binding, then configured a Drydock blueprint to use the service pool. Leased with bin/drydock lease and saw the lease obtained successfully.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- almanac-build-host
- Lint
Lint Passed Severity Location Code Message Advice src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php:81 XHP16 TODO Comment - Unit
Tests Passed - Build Status
Buildable 7559 Build 8994: [Placeholder Plan] Wait for 30 Seconds Build 8158: [Placeholder Plan] Wait for 30 Seconds Build 8157: arc lint + arc unit
Event Timeline
| src/applications/drydock/worker/DrydockAllocatorWorker.php | ||
|---|---|---|
| 165–170 | Pulling this forward from other diffs since it's required to resolve race conditions further down the track, and there's very little value in writing this new blueprint to return a resource when that model won't work in the future. | |
| src/applications/almanac/servicetype/AlmanacBuildPoolServiceType.php | ||
|---|---|---|
| 21–27 | I think I suggested this (putting credentials on the service), but upon consideration I think this shouldn't actually be part of the Service. If it was just an SSH key, it would be reasonable to put it on the Service and create multiple Services if you wanted different users to access the same hardware, but the WinRM stuff means it's really a whole mess of authentication modes and credentials. I think this stuff should live on the Blueprint instead, and Drydock should know how it's supposed to connect to the host. It will generally have to have this logic anyway for other host types, I think. | |
| 28–35 | This kind of thing (x credentials, y credentials) is bad. It's obviously bad if we add a 3rd or 4th auth system, but it's confusing even with just two. | |
| 36–42 | We don't need this yet, and I think platform being this magical string constant is grossly under-engineered for our actual needs when we do. We should leave this off for now. | |
| 43–49 | This should just be part of the storage or working-copy blueprint. | |
| src/applications/almanac/typeahead/AlmanacServiceDatasource.php | ||
| 6 | = null; is redundant / unnecessary. | |
| 8–11 | I'm pretty sure this doesn't actually work? It won't be passed over AJAX to the real datasource. You need to use setParameter() / getParameter() for the data to be included in the datasource URL when the request is made to the server. | |
| src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php | ||
| 7 | Only if Almanac is installed? | |
| 11 | There's no reason these are "Build" hosts. | |
| 17 | There's no reason a Service must represent a "build pool". | |
| 20–22 | This seems like it's either wrong or merits more explanation. I'd expect each host to be allocatable only once. | |
| 28 | Debugging. | |
| 32 | Can we fix this so it isn't in JSON? Why is it in JSON? Why is service a list of PHIDs? I'd expect it to be a single PHID, or be called almanacServicePHIDs or similar. I suspect the reason for both of these is that it's easiest to write the code like this right now, but we should fix things so the code can be written cleanly. | |
| 36 |
| |
| 39–42 | Can you just needBindings(true) on the ServiceQuery? | |
| 40 | Consider pulling $viewer out, even if we aren't pulling out the responsibility completely. | |
| 44–79 | Just get rid of all this, I want the smallest possible amount of platform in v1. We can reasonably assume users won't try to bring up an ARM service pool and then run Amiga builds on it for now. | |
| 88–132 | After earlier changes, I think this should just be storing the binding PHID. Generally, if I make changes in Almanac, existing resources and leases should respect those changes. | |
| 139 | Nuke. | |
| 153–175 | This code shouldn't be in "AlmanacServiceHostBlueprint", it has nothing to do with Almanac. | |
| src/applications/drydock/worker/DrydockAllocatorWorker.php | ||
| 165–170 | I don't understand why this doesn't work today or in the future. These changes should definitely be separated. | |
| src/applications/almanac/servicetype/AlmanacBuildPoolServiceType.php | ||
|---|---|---|
| 28–35 | Yeah I don't know any better way of handling this given that we need different credential types based on the protocol. It doesn't help that we unconditionally need an SSH key when dealing with things like EC2, while the SSH key is optional for other blueprints. | |
| 36–42 | If we don't have the platform constant, there's no way for different blueprints to allocate based on the required platform, nor will the blueprints be able to connect on the correct protocol. I don't know how else we'd engineer this (unless you mean just abstracting it as some constants and a select drop down?) | |
| 43–49 | The storage and working copy blueprints need somewhere to store their data; that location may be different on different hosts (some hosts may store data on additional drives, e.g. hosts with ephemeral storage in EC2). | |
| src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php | ||
| 20–22 | canAllocateMoreResources indicates whether the blueprint can allocate resources as requests for leases come in. Since the Almanac blueprint isn't pre-populated with something like create-resource, it will create a resource based on the available bindings on the Almanac service as needed. | |
| 32 | I believe it's JSON because that's how the tokenizer returns it's value or something weird like that? I'm not really sure, but the result from getDetail is definitely JSON for tokenizer-based field types (and the storage is entirely controlled by the custom field infrastructure). | |
| 36 | All resource allocation failures are permanent. | |
| 40 | There's no viewer context inside Drydock (it all runs from within the daemon task). | |
| 153–175 | We can't make this nicer until we start moving forward with other Drydock changes first (D10304 and friends). I do want to componentize or externalize common functionality between blueprints at some point, but the infrastructure just isn't in place to do so yet. | |
| src/applications/drydock/worker/DrydockAllocatorWorker.php | ||
| 165–170 | These changes are part of D10304; if that lands first, these changes can be removed from this diff. | |