Page MenuHomePhabricator

Drydock blueprint for preallocated remote hosts
ClosedPublic

Authored by hach-que on Nov 16 2013, 12:28 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 19, 8:40 AM
Unknown Object (File)
Sun, Nov 17, 6:04 PM
Unknown Object (File)
Sat, Nov 16, 8:42 PM
Unknown Object (File)
Sat, Nov 16, 8:42 PM
Unknown Object (File)
Sat, Nov 16, 8:42 PM
Unknown Object (File)
Sat, Nov 16, 8:42 PM
Unknown Object (File)
Sat, Nov 16, 8:42 PM
Unknown Object (File)
Fri, Nov 15, 1:03 AM
Subscribers
Unknown Object (User)

Details

Summary

This adds a Drydock blueprint for preallocated, remote hosts. This will be used by the Harbormaster interface to allow users to specify remote hosts that builds can be run on.

This adds a canAllocateResource method to Drydock blueprints; it is used to detect whether a blueprint can allocate a resource for the given type and attributes.

Test Plan

Ran:

bin/drydock lease --type host --attributes remote=true,preallocated=true,host=192.168.56.101,port=22,user=james,keyfile=,path=C:\\Build\\,platform=windows

and saw the "C:\Build\<id>" folder appear on the remote Windows machine. Viewed the lease and resource in Drydock as well.

Diff Detail

Branch
d1
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

The filter API is an abomination and should be avoided on principle. This is a purely ideological objection to how dumb the language design is in this arena, not a functional issue. For more, see D6672 / D6680. The whole thing is a bad implementation of a bad idea and nothing good can come of its use.

The rest of the design here is a little bit backwards from the intended approach. For static hosts, Drydock should never handle the allocation itself. Instead, some side-channel mechanism should just create the hosts. For example:

phabricator/ $ ./bin/drydock create-resource --type host ...

The localhost blueprint fakes its way through this, but only because it was easier. If Drydock isn't truly allocating resources, they shouldn't go through its allocation pathways.

Similarly, leases generally should not contain specific details about a host (like port, credentials, hostname). They may contain general specifications, like platform (eventually, it will probably be possible to be highly specific, but in the broad case we shouldn't be).

We should probably wait for T4122 for credential management.

We should probably get rid of the path stuff soonish too.

If the interface is gathered based on the blueprint, and we're creating resources without an associated blueprint, how do we provide interfaces to consumers of the preallocated resource?

For the sake of IRC being a madhouse right now -- the manually created resources would still have the right blueprint. The command would probably look more like --type host --blueprint BlahBlah -- or maybe just --blueprint, since I think the blueprint implies a type -- than the junk I typed.

hach-que updated this revision to Unknown Object (????).Nov 18 2013, 9:34 PM

Remove filter_var calls

hach-que updated this revision to Unknown Object (????).Nov 18 2013, 9:42 PM

Added create-resource workflow; tested with:

bin/drydock create-resource --type host --blueprint DrydockPreallocatedHostBlueprint --name "My Host" --attributes remote=true,preallocated=true,host=127.0.0.1,port=22,user=james,keyfile=,path=/var/drydock/,platform=linux
hach-que updated this revision to Unknown Object (????).Nov 18 2013, 9:53 PM

Prevent DrydockPreallocatedHostBlueprint from allocating resources

src/applications/drydock/blueprint/DrydockBlueprint.php
276–279 ↗(On Diff #17154)

This doesn't do what it say it does. If this is false; the blueprint can't even provide existing resources for leases.

hach-que updated this revision to Unknown Object (????).Nov 19 2013, 7:30 AM

Remove canAllocateResource as it's no longer needed

Some fuzzy stuff here, nothing too concrete. Push back on whatever's not such a great idea.

src/applications/drydock/blueprint/DrydockLocalHostBlueprint.php
7

Let's just make this return false unconditionally from canAllocateMoreResources(), and remove the remote/preallocated stuff. The false will make sure this never allocates naturally. In development, you can manually allocate it (in the future, we can add a big warning or something).

src/applications/drydock/blueprint/DrydockPreallocatedHostBlueprint.php
21–30

I don't think the blueprint should care about any of this except platform and maybe host if it's set on the lease. That should also probably be handled by Drydock itself, but just doing it here is fine for now.

The idea being that the RemoteCommandBuildStep shouldn't care which port it's connecting on or need to have any credentials. It just says "give me any Windows machine", and Drydock picks one from the pool and hands it over.

56–58

wut

o windows yuo silly

src/applications/drydock/interface/command/DrydockSSHCommandInterface.php
3

Maybe we should split the windows version into a DrydockWindowsSSHCommandInterface or something. Probably fine for now, but yuck.

25–26

Having no SSH key seems really weird/bad/insecure. Is there a valid reason for this beyond, like, intense laziness?

I think without "-i", SSH will use the default key, which means it will break unexpectedly and for no apparent reason if run as a different user or on a different machine, etc. Do we need to support this?

src/applications/drydock/management/DrydockManagementCreateResourceWorkflow.php
37–42

$blueprint->getType() should imply this.

Unknown Object (User) added inline comments.Nov 19 2013, 11:25 PM
src/applications/drydock/blueprint/DrydockPreallocatedHostBlueprint.php
21–30

Ah yeah, this is all left over stuff from when Drydock was used to create the resource (and the Harbormaster UI would pass the details via the lease request).

56–58

Seriously I wasted like 2 hours of my life figuring this out

src/applications/drydock/interface/command/DrydockSSHCommandInterface.php
25–26

Basically just to infer the SSH key as id_rsa in the user's home directory. This will change when Passphrase arrives since there'll no longer be any need to hide the SSH key path from the public (at the moment you can view the SSH key path in Drydock, which may or may not be a problem).

src/applications/drydock/management/DrydockManagementCreateResourceWorkflow.php
37–42

Will fix.

hach-que updated this revision to Unknown Object (????).Nov 20 2013, 7:17 AM

Made changes requested in code review.

Also tested;

  • Attempting lease with no resources (it fails)
  • Creating a Linux preallocated host resource
  • Leasing a Linux host
  • Attempting to lease a Windows host, with only Linux hosts available (it fails)
  • Creating a Windows preallocated host resource
  • Leasing a Windows host

All of the above scenarios behaved as expected.

Cool, this all seems pretty reasonable to me. Thanks!