-
Notifications
You must be signed in to change notification settings - Fork 64
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
Adoption of octavia service #728
Adoption of octavia service #728
Conversation
16136da
to
d7bf201
Compare
d61eb34
to
a3291c7
Compare
ebe6304
to
7053d99
Compare
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/41a8e533fe264f5cb491dd92e4ae6a1d ✔️ noop SUCCESS in 0s |
505c13f
to
aaa16da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good. Just a couple of questions. Also, should we include flipping the o-hm0 network configuration scripts on the controller to be ONBOOT=no?
d9fbac4
to
001e057
Compare
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/e9669c0258bc48a19f6171e47963b346 ✔️ noop SUCCESS in 0s |
---- | ||
|
||
Make sure to replace `<path to the ssh key>` with the correct path to the ssh | ||
key for connecting to the controller. | ||
. Run the following set of commands to regenerate the keys and certificates and install the data in {rhocp_long}. These commands convert the existing single certificate authority (CA) configuration into a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
. Run the following set of commands to regenerate the keys and certificates and install the data in {rhocp_long}. These commands convert the existing single certificate authority (CA) configuration into a | |
. Run the following set of commands to convert the existing single certificate authority (CA) configuration into a dual CA configuration. These commands regenerate the keys and certificates and insert them into {rhocp_long}. |
Recommend switching the statements because "Run the following set of commands to..." assumes the user goal, which looks like the second statement. Then the first seems to be saying what the commands are actually doing. Reworded install the data to "insert them into" for clarity, but I'm not sure if this is a technically correct interpretation of what you're saying
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could drop "set of" or maybe even drop "Run the following set of commands to" for fluff. Could change "These commands regenerate the keys and certificates and insert them into {rhocp_long}. " to "You can use these commands to regenerate the keys and certificates and insert them into {rhocp_long}." to make it more user-focused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update this in a separate commit. Your suggestion does not update line 27 so I would need to make a manual update anyway. Thanks for the suggestions!
|
||
. (Optional) Public SSH key of Amphorae | ||
+ | ||
These commands will copy the existing public SSH key that can be used for connecting to the amphorae and installs it in Openshift. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest rewording here. "installs it into OpenShift" has an ambiguous pronoun "it" which can refer to either the SSH key or amphorae. Do we need the "which can be used for connecting..." part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also drop "will". I think we don't really use future tense in our docs. "can be used" is passive, could change to "that you can use". Are "these commands" the previous commands or the next commands? Maybe something like:
"Copy the existing public SSH key that you can use for connecting to the amphorae and install it in Openshift:"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I don't have a strong opinion about the "which can be used for connecting" part. I think it helps the user understand what it is used for. I will leave it in for now.
---- | ||
|
||
These commands convert the existing single CA configuration into a dual CA configuration. | ||
. Add the {loadbalancer_service} interfaces to each `NodeNetworkConfigurationPolicy` custom resource (CR). The following command adds the network interface to use as the VLAN base interface for the management network for network isolation on the {OpenShiftShort} nodes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
. Add the {loadbalancer_service} interfaces to each `NodeNetworkConfigurationPolicy` custom resource (CR). The following command adds the network interface to use as the VLAN base interface for the management network for network isolation on the {OpenShiftShort} nodes: | |
. Add the {loadbalancer_service} interfaces to each `NodeNetworkConfigurationPolicy` custom resource (CR). The following command configures the VLAN base interface for the management network on the {OpenShiftShort} nodes. This is necessary for network isolation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might drop "the following command" to make it more instructional.
Maybe:
"Add the network interface for the VLAN base interface of the management network for network isolation on the {OpenShiftShort} nodes:"
Is the "management network for network isolation" a thing or is "network isolation" like a goal of these steps? If it's the latter, could say: "To isolate the management network, add the network interface for the VLAN base interface:"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure of the wording at the moment but the idea here is that there is a host interface added to the octavia network attachment's bridge. This is there because a bridge style network attachment is needed to be usable with the OVN/OVS (macvlan doesn't work in this case) but bridge NAD's do not provide connectivity between hosts. If that host interface is a VLAN interface, then there is some isolation between the octavia load balancer management network traffic and other networks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we just say "Add the {loadbalancer_service} interfaces to each NodeNetworkConfigurationPolicy
custom resource (CR)." and leave away the rest that is not really relevant to the user?
<1> Replace `<enp6s0>` with the name of the network interface in your {OpenShiftShort} setup. | ||
<2> Replace `<mtu>` with the `mtu` value in your environment. | ||
|
||
. Configure the {loadbalancer_service} network attachment definition to connect pods that manage load balancer virtual machines (amphorae) and the OpenvSwitch pods that are managed by the OVN operator: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I'm unsure if this means that you're providing network connectivity to both the pods that manage load balancer virtual machines and the OpenvSwitch pods that are managed by the OVN operator, or if you are more specifically connecting them to each other. So the following suggestion might not be correct.
. Configure the {loadbalancer_service} network attachment definition to connect pods that manage load balancer virtual machines (amphorae) and the OpenvSwitch pods that are managed by the OVN operator: | |
. Configure the {loadbalancer_service} network attachment definition to connect pods that manage load balancer virtual machines (amphorae) to the OpenvSwitch pods that are managed by the OVN operator: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could put the goal or outcome first:
"To connect pods that manage load balancer virtual machines (amphorae) and the OpenvSwitch pods that are managed by the OVN operator, configure the {loadbalancer_service} network attachment definition:"
I'm not sure if we have style guidelines on this
. Delete the old management network ports: | ||
+ | ||
The first command stores the network ID of the old management network in the | ||
variable `WALLABY_LB_MGMT_NET_ID` for later use. Then, all ports that are used in the network get deleted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it should be two steps
@weinimo There will be two people peer reviewing the docs files in this PR. I gave them both until Monday to complete the review. |
|
||
== Enabling the {loadbalancer_service} in OpenShift | ||
Run the following command in order to enable the {loadbalancer_service} CR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could just say:
"Enable the {loadbalancer_service} CR:"
|
||
.. Connect to each of the Controller nodes on the old control plane, for example, `overcloud-controller-0`. | ||
|
||
.. Open the configuration script for the management interface. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Open the configuration script for the management interface, for example:"
What is the example here? Will customer have a different configuration script? I guess that's what ifcfg-o-hm0 is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see that is mentioned in next line. Could maybe say:
"Open the management interface configuration script in a text editor such as vi
:"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I just noticed it is not actually a script but only a configuration file, so I replaced "script" with "file".
$ sudo vi /etc/sysconfig/network-scripts/ifcfg-o-hm0 | ||
---- | ||
+ | ||
The exact name of the script depends on the previous `OctaviaMgmtPortDevName` setting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe:
"The script name is based on the OctaviaMgmtPortDevName
setting and might differ in your environment. "
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will add "previous" to your suggestion, because this is a setting made in the 17.1 deployment.
+ | ||
The exact name of the script depends on the previous `OctaviaMgmtPortDevName` setting. | ||
|
||
.. Change `ONBOOT=yes` to `ONBOOT=no` so that the interface does not get enabled automatically on reboot: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Change ONBOOT=yes
to ONBOOT=no
to ensure that the interface is disabled on reboot:"
Co-authored-by: Roger Heslop <heslop@outlook.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rheslop @mickogeary Thanks for your reviews. I think I have incorporated all of your suggestions. Please double-check.
---- | ||
|
||
Make sure to replace `<path to the ssh key>` with the correct path to the ssh | ||
key for connecting to the controller. | ||
. Run the following set of commands to regenerate the keys and certificates and install the data in {rhocp_long}. These commands convert the existing single certificate authority (CA) configuration into a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update this in a separate commit. Your suggestion does not update line 27 so I would need to make a manual update anyway. Thanks for the suggestions!
|
||
. (Optional) Public SSH key of Amphorae | ||
+ | ||
These commands will copy the existing public SSH key that can be used for connecting to the amphorae and installs it in Openshift. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I don't have a strong opinion about the "which can be used for connecting" part. I think it helps the user understand what it is used for. I will leave it in for now.
|
||
.. Connect to each of the Controller nodes on the old control plane, for example, `overcloud-controller-0`. | ||
|
||
.. Open the configuration script for the management interface. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I just noticed it is not actually a script but only a configuration file, so I replaced "script" with "file".
$ sudo vi /etc/sysconfig/network-scripts/ifcfg-o-hm0 | ||
---- | ||
+ | ||
The exact name of the script depends on the previous `OctaviaMgmtPortDevName` setting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will add "previous" to your suggestion, because this is a setting made in the 17.1 deployment.
---- | ||
|
||
These commands convert the existing single CA configuration into a dual CA configuration. | ||
. Add the {loadbalancer_service} interfaces to each `NodeNetworkConfigurationPolicy` custom resource (CR). The following command adds the network interface to use as the VLAN base interface for the management network for network isolation on the {OpenShiftShort} nodes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we just say "Add the {loadbalancer_service} interfaces to each NodeNetworkConfigurationPolicy
custom resource (CR)." and leave away the rest that is not really relevant to the user?
Thanks, @weinimo. I'll check the updated docs today. |
@weinimo I made a couple of additional minor edits. Otherwise, LGTM. |
Co-authored-by: Katie Gilligan <kgilliga@redhat.com>
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
0d137d0
into
openstack-k8s-operators:main
Octavia adoption documentation
JIRAs: OSPRH-10750 OSPNET-1078