Ironic overcloud hypervisor deployment #1089
Conversation
assumptionsandg
changed the title
assumptionsandg
force-pushed
the
baremetal-hypervisor
branch
from
e2e2213
to
e24e4ff
Compare
assumptionsandg
force-pushed
the
baremetal-hypervisor
branch
from
e24e4ff
to
0493247
Compare
assumptionsandg
force-pushed
the
baremetal-hypervisor
branch
from
c01bda2
to
1218ec8
Compare
assumptionsandg
force-pushed
the
baremetal-hypervisor
branch
2 times, most recently
from
d02d07d
to
7598716
Compare
assumptionsandg
force-pushed
the
baremetal-hypervisor
branch
2 times, most recently
from
ed6155e
to
e850079
Compare
assumptionsandg
force-pushed
the
baremetal-hypervisor
branch
from
e850079
to
d7e920a
Compare
|
@assumptionsandg remember to mark conversations as resolved when you've completed them! Makes it much easier for reviewers to know when to re-review. Is this still a draft? Who do you need a review from to get it merged? |
It's still WIP on the client side of things, hence leaving it as a draft here. We could merge what's here now and interate/fix things as the client work progresses if what's here will be useful to people. |
I'd be happy with it as long as you add a big warning at the top saying the docs are still WIP. Something is better than nothing |
assumptionsandg
force-pushed
the
baremetal-hypervisor
branch
from
72cf9c8
to
eb82723
Compare
assumptionsandg
changed the title
assumptionsandg
force-pushed
the
baremetal-hypervisor
branch
from
c484916
to
7f42dee
Compare
|
@assumptionsandg you need to tag people for re-review when you make changes. This has been sat around for two months and I don't think anyone has looked at it |
There was a problem hiding this comment.
Generally I think the page should be a bit more declarative, at the moment it's written in a future tense. For example, I think that:
Overcloud Ironic will be deployed with a listening TFTP server on the control plane
Would flow better as
Overcloud Ironic is deployed with a listening TFTP server on the control plane
I don't think it would be a lot of effort to update (basically a find & replace: will be -> is) but it's a bit of a nitpick so if you want to leave it as is then that's fine
| It is also required to load the conntrack kernel module ``nf_nat_tftp``, | ||
| ``nf_conntrack`` and ``nf_conntrack_tftp`` on network nodes. You can load these | ||
| modules using modprobe or define these in /etc/module-load. |
There was a problem hiding this comment.
I think the grammar can be improved here a bit. The I think the line lengths are a bit off in my suggestion because I'm doing it in the github comment editor
| It is also required to load the conntrack kernel module ``nf_nat_tftp``, | |
| ``nf_conntrack`` and ``nf_conntrack_tftp`` on network nodes. You can load these | |
| modules using modprobe or define these in /etc/module-load. | |
| The conntrack kernel modules ``nf_nat_tftp``, ``nf_conntrack``, | |
| and ``nf_conntrack_tftp`` are also required on network nodes. You | |
| can load these modules using modprobe or define these in /etc/module-load. |
| Conntrack_helper will be required when UEFI booting on a cloud with ML2/OVS | ||
| and using the iptables firewall_driver, otherwise TFTP traffic is dropped due | ||
| to it being UDP. You will need to define some extension drivers in ``neutron.yml`` | ||
| to ensure conntrack is enabled in neutron server. |
There was a problem hiding this comment.
You've done well to capitalise Nova and Ironic but missed a few instances of OpenStack, Neutron, and Kayobe
This is just one instance, but rather than pointing every one of them out, I'll let you search for them
| to ensure conntrack is enabled in neutron server. | |
| to ensure conntrack is enabled in Neutron server. |
| modules using modprobe or define these in /etc/module-load. | ||
|
|
||
| The Ironic neutron router will also need to be configured to use | ||
| conntrack_helper. |
There was a problem hiding this comment.
Not completely sure on this one, I'll leave it for you to decide
| conntrack_helper. | |
| ``conntrack_helper``. |
| Now the node has no instances allocated to it you can delete the instance using | ||
| the OpenStack CLI and the node will be moved back to ``available`` state. |
There was a problem hiding this comment.
Maybe just clarify here that it's the baremetal instance that you're deleting, not the VMs that you've migrated away
|
|
||
| - name: Set baremetal node JSON variable | ||
| ansible.builtin.set_fact: | ||
| node_show_json: "{{ node_show.stdout | to_json | from_json }}" |
There was a problem hiding this comment.
| to_json | from_json is a bit weird, and the name of the variable doesn't really fit.
You're taking a string, which is JSON, then converting it to a nice ansible dict var, so "node_show_json" isn't actually json
Also, is it actually used? Can it just be deleted?
|
|
||
| - name: Set baremetal node JSON variable | ||
| ansible.builtin.set_fact: | ||
| node_show_json: "{{ node_show.stdout | to_json | from_json }}" |
There was a problem hiding this comment.
see previous comments on to & from json
| - name: Slurp network allocations | ||
| ansible.builtin.slurp: | ||
| path: "{{ network_allocation_path }}" | ||
| register: net_alc | ||
|
|
||
| - name: Read network allocations | ||
| ansible.builtin.set_fact: | ||
| net_alc_yaml: "{{ net_alc['content'] | b64decode | from_yaml }}" | ||
|
|
||
| - name: Write node IP address to allocations | ||
| ansible.builtin.set_fact: | ||
| new_net_alc: "{{ net_alc_yaml | combine(new_ips, recursive=True) }}" | ||
| vars: | ||
| new_ips: "{ '{{ admin_oc_net_name }}_ips': { '{{ inventory_hostname }}': '{{ ansible_host }}' } }" |
There was a problem hiding this comment.
The net IPs are dictionary vars you have access to right? I don't think you need to slurp them in from a file
Co-authored-by: Alex-Welsh <[email protected]>
Alex-Welsh
added
documentation
Deployment playbook for baremetal instances to be used as hypervisors.
TODO: Documentation refactoring (separate everything prior to hypervisors?)