Add new endpoint for joining a cluster by mbarnes · Pull Request #61 · projectatomic/commissaire-mvp

mbarnes · 2016-03-15T18:58:22Z

This was an idea I had to help with the host auto-join feature.

Programmatically determining a host's own IP address to send to the Commissaire server can be non-trivial given the possibility of multiple network devices. And even if you know the right device name, parsing it out from ip address or ifconfig is still a pain.

This helps alleviate the problem by offering a convenient endpoint for hosts to add themselves directly to a cluster.

_Update:_ After further discussion the endpoint was changed to /api/v0/host.

Endpoint: /api/v0/cluster/{NAME}/join

Accepts a PUT request from the host wishing to join cluster {NAME}.

This is equivalent to:

   PUT /api/v0/host/{ADDRESS}

   {
      cluster: {NAME}
      ...
   }

except the host's {ADDRESS} is derived from the request itself.

mbarnes · 2016-03-15T19:01:10Z

@cgwalters-bot r+ e5edbdb

cgwalters-bot · 2016-03-15T19:01:11Z

🙀 e5edbdb is not a valid commit SHA. Please try again with da7a878.

ashcrow · 2016-03-16T13:14:10Z

The idea of using the requesting address makes a lot of sense. Though why not update the current host endpoint instead of creating a new one?

cgwalters · 2016-03-16T13:21:35Z

@cgwalters-bot retry

ashcrow · 2016-03-16T13:29:04Z

@cgwalters-bot r+ da7a878

cgwalters-bot · 2016-03-16T13:29:09Z

⌛ Trying commit da7a878 with merge dfdc590...

Captures most of the host PUT handler logic for host creation. Pull request: #61 Approved by: <try>

Endpoint: /api/v0/cluster/{NAME}/join Accepts a PUT request from the host wishing to join cluster {NAME}. This is equivalent to: PUT /api/v0/host/{ADDRESS} { cluster: {NAME} ... } except the host's {ADDRESS} is derived from the request itself. This is intended to aid host initialization, since determining one's own IP address programmatically can be non-trivial. The PUT response body will include the host's IP address. Pull request: #61 Approved by: <try>

ashcrow · 2016-03-16T13:48:27Z

@cgwalters-bot r- da7a878
@cgwalters-bot r+ da7a878

cgwalters-bot · 2016-03-16T13:48:28Z

💡 This pull request was already approved, no need to approve it again.

This pull request is currently being tested. If there's no response from the continuous integration service, you may use retry to trigger a build again.

ashcrow · 2016-03-16T13:51:26Z

Messing with the bot a bit here to see what happens 😄

@cgwalters-bot clean

mbarnes · 2016-03-16T13:51:58Z

The idea of using the requesting address makes a lot of sense. Though why not update the current host endpoint instead of creating a new one?

Maybe I misunderstand what you mean, but the host endpoint URL embeds the host's IP address, which defeats the purpose here. I'm trying to avoid forcing the host to know its own IP address before automatically registering itself to Commissaire.

Using the endpoint of the cluster it wants to join seemed the most straight-forward.

I'm open to other ideas though.

ashcrow · 2016-03-16T14:02:41Z

@mbarnes I see what you mean. Since the GET will always required a name it would be awkward.

What about having a PUT/POST /api/v0/host/ endpoint that takes the current data /api/v0/host/{IP} takes sans the IP in the URL. The resulting instance would be available at /api/v0/host/{IP} and we can change /api/v0/host/{IP} to be GET/`DELETE`` only.

ashcrow · 2016-03-16T14:04:57Z

@cgwalters-bot clean

cgwalters · 2016-03-16T14:12:37Z

Hmm...I'd have expected "clean" to work. Let me debug.

cgwalters · 2016-03-16T14:13:04Z

Oh although, I bet this needs to be rebased on master so we have the webhook in the PR series.

mbarnes · 2016-03-16T14:25:53Z

@ashcrow: I was with you until that last bit:

and we can change /api/v0/host/{IP} to be GET/DELETE only

Wouldn't that impose a new requirement that a host creation request has to come from the host itself? Unless that was always the intention, I think we'd want to keep both ways for the sake of administrators.

If you still prefer a PUT on /api/v0/host I can rework it.

ashcrow · 2016-03-16T14:28:17Z

@mbarnes good point. Scratch the last bit. Yeah throw it on /api/v0/host/ and I think this will be nice and clean.

ashcrow · 2016-03-16T15:42:12Z

@mbarnes mind rebasing to see if that makes homu happier?

cgwalters · 2016-03-16T18:22:31Z

@ashcrow You'll need to re-r+ with new commit ID of d3fa8b5

ashcrow · 2016-03-16T18:46:25Z

@cgwalters-bot r+ d3fa8b5

Captures most of the host PUT handler logic for host creation. Pull request: #61 Approved by: ashcrow

Endpoint: /api/v0/cluster/{NAME}/join Accepts a PUT request from the host wishing to join cluster {NAME}. This is equivalent to: PUT /api/v0/host/{ADDRESS} { cluster: {NAME} ... } except the host's {ADDRESS} is derived from the request itself. This is intended to aid host initialization, since determining one's own IP address programmatically can be non-trivial. The PUT response body will include the host's IP address. Pull request: #61 Approved by: ashcrow

cgwalters-bot · 2016-03-16T18:46:32Z

⌛ Testing commit d3fa8b5 with merge b657aa6...

cgwalters-bot · 2016-03-16T18:48:44Z

☀️ Test successful - travis

mbarnes · 2016-03-16T18:55:20Z

Why was this PR closed?

cgwalters · 2016-03-16T19:03:36Z

It was closed because it was merged...sorry I know it's a bit confusing right now. See barosl/homu#139

Endpoint: /api/v0/host Accepts a PUT request from the host wishing to add itself. The host IP address is inferred from the request itself, but otherwise works exactly like "PUT /api/v0/host/{ADDRESS}". This is intended to aid host initialization, since determining one's own IP address programmatically can be non-trivial.

mbarnes · 2016-03-16T21:42:33Z

This accidentally got merged to master prematurely, so here's what I did for the rework.

Left the 1st commit (projectatomic/commissaire@4969962) merged since it was just a util function and didn't need changed. If there's corrections to be made there I'll do so in a new commit.
Reverted the 2nd commit on master and rebased this branch on it.
Added ⬆️ for the /api/v0/host endpoint as discussed.

For the endpoint docs I just added a note to the host creation part. This is kind of a special case so it didn't seem worth a whole new section to me.

ashcrow · 2016-03-17T14:25:51Z

@cgwalters-bot try

cgwalters · 2016-03-17T14:30:30Z

@ashcrow I think that may currently not do anything because the PR already passed? Ah yes, look at:

def process_queue(...):
    for repo_label, repo in repos.items():
        for state in repo_states:
            if state.status == '' and state.try_:
                if start_build(state, repo_cfgs, buildbot_slots, logger, db, git_cfg):
                    return

Since status != '' we don't try the merge build. I'd say that's a conceptual bug - Homu should support it.

ashcrow · 2016-03-17T14:30:57Z

@cgwalters ah ok.

@cgwalters-bot r+ 71d9049

cgwalters-bot · 2016-03-17T14:47:35Z

💡 This pull request was already approved, no need to approve it again.

cgwalters-bot · 2016-03-17T14:55:08Z

⌛ Testing commit 71d9049 with merge 8bb60d0...

Endpoint: /api/v0/host Accepts a PUT request from the host wishing to add itself. The host IP address is inferred from the request itself, but otherwise works exactly like "PUT /api/v0/host/{ADDRESS}". This is intended to aid host initialization, since determining one's own IP address programmatically can be non-trivial. Pull request: #61 Approved by: ashcrow

cgwalters-bot · 2016-03-17T14:59:29Z

☀️ Test successful - travis

mbarnes assigned ashcrow Mar 15, 2016

cgwalters-bot pushed a commit that referenced this pull request Mar 16, 2016

Add etcd_host_create() utility function.

e196863

Captures most of the host PUT handler logic for host creation. Pull request: #61 Approved by: <try>

ashcrow closed this Mar 16, 2016

ashcrow reopened this Mar 16, 2016

cgwalters-bot pushed a commit that referenced this pull request Mar 16, 2016

Add etcd_host_create() utility function.

4969962

Captures most of the host PUT handler logic for host creation. Pull request: #61 Approved by: ashcrow

cgwalters-bot closed this Mar 16, 2016

mbarnes reopened this Mar 16, 2016

cgwalters-bot closed this Mar 17, 2016

Conversation

mbarnes commented Mar 15, 2016

Uh oh!

mbarnes commented Mar 15, 2016

Uh oh!

cgwalters-bot commented Mar 15, 2016

Uh oh!

ashcrow commented Mar 16, 2016

Uh oh!

cgwalters commented Mar 16, 2016

Uh oh!

ashcrow commented Mar 16, 2016

Uh oh!

cgwalters-bot commented Mar 16, 2016

Uh oh!

ashcrow commented Mar 16, 2016

Uh oh!

cgwalters-bot commented Mar 16, 2016

Uh oh!

ashcrow commented Mar 16, 2016

Uh oh!

mbarnes commented Mar 16, 2016

Uh oh!

ashcrow commented Mar 16, 2016

Uh oh!

ashcrow commented Mar 16, 2016

Uh oh!

cgwalters commented Mar 16, 2016

Uh oh!

cgwalters commented Mar 16, 2016

Uh oh!

mbarnes commented Mar 16, 2016

Uh oh!

ashcrow commented Mar 16, 2016

Uh oh!

ashcrow commented Mar 16, 2016

Uh oh!

cgwalters commented Mar 16, 2016

Uh oh!

ashcrow commented Mar 16, 2016

Uh oh!

cgwalters-bot commented Mar 16, 2016

Uh oh!

cgwalters-bot commented Mar 16, 2016

Uh oh!

mbarnes commented Mar 16, 2016

Uh oh!

cgwalters commented Mar 16, 2016

Uh oh!

mbarnes commented Mar 16, 2016

Uh oh!

ashcrow commented Mar 17, 2016

Uh oh!

cgwalters commented Mar 17, 2016

Uh oh!

ashcrow commented Mar 17, 2016

Uh oh!

cgwalters-bot commented Mar 17, 2016

Uh oh!

cgwalters-bot commented Mar 17, 2016

Uh oh!

cgwalters-bot commented Mar 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants