Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2.8] Record hostname when machine agent starts #12754

Merged

Conversation

achilleasa
Copy link
Contributor

This PR provides a solution for scenarios such us the one described in the referenced bug. More specifically, given that (provider) instance IDs are typically not related to the machine's host name and that some providers do not let us query the machine host name, juju show-machine output will include an IP address in the dns-name field.

The PR introduces a consistent cross-provider way of recording and reporting the machine host names. To achieve this, the machine agents will now query the machine host name and report it at the same time that they request from the controller to record the agent start time. While the machiner facade already had a RecordAgentStartTime method (added in 2.8), it accepted a slice of params.Entity values which meant that we could not push additional information to the controller.

To this end, the PR adds a new API call named RecordAgentStartInformation which supersedes RecordAgentStartTime and bumps the machiner facade version to 5. Given that the machiner facade versions have not diverged in the 2.9 branch, this change can easily be forward ported. The machiner API client utilizes facade version checks to select the appropriate method to call.

In a similar fashion to the AgentStartTime field, the Hostname field will always get populated after the agent restart. As a result there is no need to modify the juju/description package or worry about migrations.

To report back the recorded machine hostname, the payload for the client/status (MachineStatus in FullStatus) had to be augmented with an extra field. In this case, the status API has already diverged in 2.9 so I opted to simply add the extra (it's optional anyway) field to the status response without bumping the facade version so we can merge the changes forward.

Finally, the CLI has been tweaked to include the host name (if not empty) in juju show-machine output.

QA steps

# Bootstrap with the current 2.8 juju snap
$ /snap/bin/juju bootstrap lxd test --no-gui

$ juju switch controller
$ juju show-machine 0
model: controller
machines:
  "0":
    juju-status:
      current: started
      since: 11 Mar 2021 12:52:21Z
      version: 2.8.9
    dns-name: 10.232.217.231
    ip-addresses:
    - 10.232.217.231
    instance-id: juju-1ee6f9-0
    machine-status:
      current: running
      message: Running
      since: 11 Mar 2021 12:52:31Z
    modification-status:
      current: idle
      since: 11 Mar 2021 12:52:08Z
    series: bionic
    network-interfaces:
      eth0:
        ip-addresses:
        - 10.232.217.231
        mac-address: 00:16:3e:24:20:9e
        gateway: 10.232.217.1
        is-up: true
    hardware: arch=amd64 cores=0 mem=0M
    controller-member-status: has-vote

# Upgrade
$ juju upgrade-controller --build-agent

# The host name should magically now appear in show-machine output
$ juju show-machine 0
model: controller
machines:
  "0":
    juju-status:
      current: started
      since: 11 Mar 2021 12:53:43Z
      version: 2.8.10.1
    hostname: juju-1ee6f9-0    <----- you are looking for this
    dns-name: 10.232.217.231
    ip-addresses:
    - 10.232.217.231
    instance-id: juju-1ee6f9-0
    machine-status:
      current: running
      message: Running
      since: 11 Mar 2021 12:52:31Z
    modification-status:
      current: idle
      since: 11 Mar 2021 12:52:08Z
    series: bionic
    network-interfaces:
      eth0:
        ip-addresses:
        - 10.232.217.231
        mac-address: 00:16:3e:24:20:9e
        gateway: 10.232.217.1
        is-up: true
    hardware: arch=amd64 cores=0 mem=0M
    controller-member-status: has-vote

Bug reference

https://bugs.launchpad.net/juju/+bug/1918204

This change allows us to update the hostname (and ony other future
details) reported by the agent in addition to the agent start timestamp.
This method supersedes RecordAgentStartTime and allows the calling
machine agent to provide additional information about the machine to the
controller.
The added method utilizes facade version detection to:
- be a no-op if machiner facade version < 2
- use RecordAgentStartTime if facade version < 5
- use RecordAgentStartInformation otherwise
As the client/status API version has diverged in the 2.9 branch, I opted
not to bump the API version given that this is an optional (omitempty)
field.  This will allow us to merge these changes into 2.9 without an
issue.
Copy link
Member

@wallyworld wallyworld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@wallyworld
Copy link
Member

$$merge$$

@jujubot jujubot merged commit 4b11eaa into juju:2.8 Mar 11, 2021
@pengale
Copy link
Contributor

pengale commented Mar 11, 2021

I was doing QA on this. Looks like I didn't finish before the merge, but everything worked as expected. :-)

@achilleasa achilleasa deleted the 2.8-record-hostname-when-machine-agent-starts branch March 12, 2021 09:26
jujubot added a commit that referenced this pull request Mar 12, 2021
…tiwatcher-machine-deltas

#12760

This is a followup PR to #12754 to ensure that the agent-reported machine host name is included in the deltas produced by the multi-watcher/all-watcher API calls so this information can be made available via pylibjuju.

Similar to the client/status facade changes in the linked PR, the watcher facade has not been bumped as hostname is an optional field.

No further QA required; the changes will be tested as part of an upcoming PR against pylibjuju.
@ycliuhw ycliuhw mentioned this pull request Mar 14, 2021
jujubot added a commit that referenced this pull request Mar 15, 2021
#12764

Merge 2.9 with these PRs:

- Record hostname when machine agent starts #12754
- Update github.com/juju/charm/v7 to include v2 block #12756
- Disable tests/run.sh for released bundle test and dump logs and k8s s… #12757

```
Conflicts:
 apiserver/facades/agent/machine/machiner.go
 go.mod
 go.sum
```
@wallyworld wallyworld mentioned this pull request Mar 16, 2021
jujubot added a commit that referenced this pull request Mar 16, 2021
#12770

Merge 2.9

#12735 Silences proxy log messages from klog
#12742 Fixes broken kubeconfig loading tests
#12754 Record hostname when machine agent starts
#12755 Fix broken centos proxy tests part2
#12756 Update github.com/juju/charm/v7 to include v2 block
#12757 Disable tests/run.sh for released bundle test and dump logs
#12739 Scaffold upgrade series logic 
#12753 Disregard refcount for blobstore cleanup
#12758 No panic missing bundle app body
#12759 Fix errant relative timeout for migration minion reports
#12760 Include hostname in multi-watcher machine deltas
#12750 Refactor series support to that juju itself can set its own supported series


```
# Conflicts:
# apiserver/allfacades.go
# apiserver/facades/agent/machine/machiner.go
# apiserver/facades/client/client/client.go
# apiserver/facades/schema.json
# caas/kubernetes/provider/cloud.go
# cloudconfig/userdatacfg_unix.go
# cmd/juju/commands/bootstrap_test.go
# state/migration_internal_test.go
```

## QA steps

See PRs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants