/juju

api;apiserver: AllAddresses() added to SSHClient facade (v2) #6468

Merged
merged 2 commits into from Oct 19, 2016

Conversation

Reviewers

@frobware frobware

@voidspace voidspace

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@dimitern @jujubot @voidspace @frobware
@dimitern
Contributor

dimitern commented Oct 18, 2016 •
Edited 1 time

The SSHClient API facade only has methods getting the preferred
public or private addresses of a machine/unit entity. This PR
adds a new AllAddresses() method to the facade, and bumps the
version to 2.

QA Steps (only unit tests):

  1. Run go test -check.v -cover in the api/sshclient/ and then
    in the apiserver/ package directories.
  2. Ensure tests pass and there is no reduction of coverage before
    and after this PR.

Requires (and includes!) PR #6467 - please review the state/ changes there.
Prerequisite (3/3) to fix bug http://pad.lv/1616098.
@frobware

frobware reviewed Oct 18, 2016
View changes

Somebody else should take a look through this as well. It was awkward to separate out code that I had already reviewed - let's keep the PR's distinct. Less is more. 👍

apiserver/sshclient/facade.go
-func (facade *Facade) getAddresses(args params.Entities, getter func(SSHMachine) (network.Address, error)) (
- params.SSHAddressResults, error,
+// AllAddresses reports all addresses known to Juju for each given entity in
+// args. Machines and units are suppored as entity types. Since the returned
@frobware

frobware Oct 18, 2016

Contributor

typo: "supported"

@dimitern

dimitern Oct 18, 2016

Contributor

👍 Done

apiserver/sshclient/facade.go
- params.SSHAddressResults, error,
+// AllAddresses reports all addresses known to Juju for each given entity in
+// args. Machines and units are suppored as entity types. Since the returned
+// addresses are gathered from multiple sources, results can include duplicates.
@frobware

frobware Oct 18, 2016

Contributor

s/can/may

@dimitern

dimitern Oct 18, 2016

Contributor

👍 Done

apiserver/sshclient/facade.go
+ Results: make([]params.SSHAddressResult, len(args.Entities)),
+ }
+
+ getterWrapper := func(m SSHMachine) ([]network.Address, error) {
@frobware

frobware Oct 18, 2016

Contributor

In one of the previous functions this was called "getter", make them the same for consistency?

@dimitern

dimitern Oct 18, 2016

Contributor

👍 It is wrapping a getter, but ok

@dimitern
Contributor

dimitern commented Oct 19, 2016

!!build!!
@dimitern
Contributor

dimitern commented Oct 19, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Oct 19, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju
@jujubot
Contributor

jujubot commented Oct 19, 2016

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/9532
@dimitern
Contributor

dimitern commented Oct 19, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Oct 19, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit ab705af into juju:develop Oct 19, 2016
1 check passed

1 check passed

github-check-merge-juju Built PR, ran unit tests, and tested LXD deploy.
Details

@dimitern dimitern deleted the dimitern:api-sshclient-alladdresses branch Oct 19, 2016

@dimitern dimitern referenced this pull request Oct 20, 2016

Merged

lp:1616098: ssh,scp,dh pick reachable address among all #6481

jujubot added a commit that referenced this pull request Oct 21, 2016

@jujubot
Merge pull request #6481 from dimitern/lp-1616098-ssh+scp-alladdresses 
lp:1616098: ssh,scp,dh pick reachable address among all

Uses the new SSHClient API facade v2 (from #6468), if
available, and network.ReachableHostPort() (#6454) in
code shared between the ssh, scp, and debug-hooks CLI
commands, to select a reachable address from all known
addresses of the target (machine/unit/host).

Fixes http://pad.lv/1616098.

QA steps (live tested on both MAAS 1.9 with multiple-
NIC nodes and on lxd - see below):

  1. Using the tip of `develop`, bootstrap a lxd xenial controller.
  2. juju switch controller
  3. juju status 0    # note of the IP address (IP1)
  4. lxc exec \<juju-instance-id\> -- ip addr add \<IP2\>/24 dev eth0
(add an extra IP on machine-0, from the same subnet)
  5. lxc exec \<juju-instance-id\> -- systemctl restart jujud-machine-0.service
(bounce the controller agent to pick up the new IP)
  6. juju ssh 0             # should work
  7. sudo ip route add blackhole \<IP1\>/32
(make the preferred public IP unreachable from your laptop)
  8. juju --debug ssh 0     # should fail with 'Invalid argument'
  9. git checkout dimitern/lp-1616098-ssh+scp-alladdresses
(switch to this PR's branch and rebuild juju & jujud)
  10. juju ssh 0                    # should work now, using IP2
  11. juju scp 0:/etc/issue .  # as above
  12. juju deploy ubuntu --to 0
  13. juju debug-hooks ubuntu/0 # should work as well
(small delay ~10s observed on lxd, because of 'sudo')
dd11dad
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment