Skip to content

Control plane verification fixes #866

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.

Sign up for GitHub

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

Merged
openshift-merge-bot merged 4 commits into from
Apr 15, 2025
Merged

Control plane verification fixes #866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bd082ac
Add verification steps that look at OpenStackControlPlane CR status
jistr Mar 21, 2025
fbb10ac
Use MariaDB/Rabbit verification commands that will make issues visible
jistr Mar 21, 2025
0f95f56
Use oc wait for checking that OpenStackControlPlane is Ready
jistr Apr 9, 2025
7d96934
Use Galera service name in 'deploying backend services' doc
jistr Apr 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs_user/modules/proc_adopting-the-identity-service.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,3 +79,9 @@ done
----
$ openstack endpoint list | grep keystone
----

. Wait for the `OpenStackControlPlane` resource to become `Ready`:
+
----
$ oc wait --for=condition=Ready --timeout=1m OpenStackControlPlane openstack
----
24 changes: 16 additions & 8 deletions docs_user/modules/proc_deploying-backend-services.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ endif::[]

* If you enabled TLS everywhere (TLS-e) on the {OpenStackShort} environment, you must copy the `tls` root CA from the {OpenStackShort} environment to the `rootca-internal` issuer.

* There are free PVs available for MariaDB and RabbitMQ.
* There are free PVs available for Galera and RabbitMQ.
ifeval::["{build}" != "downstream"]
+
For developer/CI environments driven by install_yamls, make sure
Expand Down Expand Up @@ -415,18 +415,26 @@ spec:

.Verification

* Verify that MariaDB and RabbitMQ are running for all defined cells:
* Verify that the Galera and RabbitMQ status is `Running` for all defined cells:
+
[source,yaml]
----
$ RENAMED_CELLS="cell1 cell2 cell3"
Copy link

@odyssey4me odyssey4me Apr 15, 2025

Choose a reason for hiding this comment

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

This may be slightly tangential, but given that the most common use case is not to have multiple cells don't you think we should have a note here to clarify that this should be modified to suit the environment?

Copy link
Contributor Author

@jistr jistr Apr 15, 2025

Choose a reason for hiding this comment

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

Yes could be done but it's not related to the bug i'm trying to fix. I could piggy-back another commit to the PR if it was brought up earlier in the sprint but now i'd just focus on the must haves.

The RENAMED_CELLS variable is used across the docs in many places, it is described in https://openstack-k8s-operators.github.io/data-plane-adoption/user/downstream.html#adopting-the-compute-service_adopt-control-plane

Now that is searched for it, i see that description is not the first place the variable occurs in the docs, which could be improved upon, but again it's irrelevant to this bug.

Copy link
Contributor

@bogdando bogdando Apr 15, 2025

Choose a reason for hiding this comment

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

In the guide, it is stated as an example we stick with to deploy 3 cells further on.
We could surely improve that in some other commits

$ oc get pod openstack-galera-0 -o jsonpath='{.status.phase}{"\n"}' | grep Running
$ oc get pod rabbitmq-server-0 -o jsonpath='{.status.phase}{"\n"}' | grep Running
$ oc get pod openstack-galera-0 -o jsonpath='{.status.phase}{"\n"}'
$ oc get pod rabbitmq-server-0 -o jsonpath='{.status.phase}{"\n"}'
$ for CELL in $(echo $RENAMED_CELLS); do
oc get pod openstack-$CELL-galera-0 -o jsonpath='{.status.phase}{"\n"}' | grep Running
oc get pod rabbitmq-$CELL-server-0 -o jsonpath='{.status.phase}{"\n"}' | grep Running
done
> oc get pod openstack-$CELL-galera-0 -o jsonpath='{.status.phase}{"\n"}'
> oc get pod rabbitmq-$CELL-server-0 -o jsonpath='{.status.phase}{"\n"}'
> done
Copy link

@odyssey4me odyssey4me Apr 15, 2025

Choose a reason for hiding this comment

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

I think it may make sense to include a sample of expected output here.

Copy link
Contributor Author

@jistr jistr Apr 15, 2025

Choose a reason for hiding this comment

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

The expected output is the word Running per cell, i tried to hint at that in the introductory sentence for the code block. It could be made more clear a the cost of being more verbose, but i'm not sure if that's worth respinning the CI.

Copy link
Contributor

@ciecierski ciecierski Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We can be more verbose in documentation and have grep in CI at the same time.
Here is example output:
`
$ oc get pods -n openstack rabbitmq-server-0

NAME READY STATUS RESTARTS AGE
rabbitmq-server-0 1/1 Running 0 51m
`

and for galera:
`
oc get pods -n openstack openstack-galera-0

NAME READY STATUS RESTARTS AGE
openstack-galera-0 1/1 Running 0 53m
`

----
+
The given cells names are later referred to by using the environment variable `RENAMED_CELLS`.

* Verify that you can access the `OpenStackClient` pod. For more information, see link:{defaultURL}/maintaining_the_red_hat_openstack_services_on_openshift_deployment/assembly_accessing-the-rhoso-cloud#proc_accessing-the-OpenStackClient-pod_cloud-access-admin[Accessing the OpenStackClient pod] in _Maintaining the {rhos_long_noacro} deployment_.
* Verify that the `OpenStackControlPlane` CR is waiting for deployment
of the `openstackclient` pod:
Copy link
Contributor

@klgill klgill Mar 21, 2025

Choose a reason for hiding this comment

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

FYI: We're inconsistent about the use of openstackclientpod vs. OpenStackClientPod in the adoption guide and other 18.0 guides. Another writer on my team is investigating which term is correct. We'll leave this as is until we get an answer.

Copy link
Contributor Author

@jistr jistr Mar 24, 2025

Choose a reason for hiding this comment

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

It's a Pod resource called openstackclient so if we want to be correct we should use all lowercase. I suspect it got camel-cased in docs to become more readable, but it's not what the user will actually see. I'd be happy to use either variant.

Copy link
Contributor

@bogdando bogdando Apr 15, 2025

Choose a reason for hiding this comment

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

could you preserve the docs link?

Copy link
Contributor Author

@jistr jistr Apr 15, 2025

Choose a reason for hiding this comment

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

The content doesn't belong here at all. It already got deleted from here once (moved to adopting identity service where we can actually connect to the pod, here we can't). It got reintroduced as a mistake in a rebase of one of the larger patches, so now i'm just re-deleting it :).

Copy link
Contributor Author

@jistr jistr Apr 15, 2025

Choose a reason for hiding this comment

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

It's here: https://github.com/openstack-k8s-operators/data-plane-adoption/pull/866/files#diff-3a3c5df1a0bf117a4167ea85e9e87727e6ceec6c3a466bc5f60b566cf55f53bfR75

Copy link
Contributor Author

@jistr jistr Apr 15, 2025

Choose a reason for hiding this comment

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

Relevant PR that originally moved the content: #722

+
----
$ oc get OpenStackControlPlane openstack
NAME STATUS MESSAGE
openstack Unknown OpenStackControlPlane Client not started
----
11 changes: 11 additions & 0 deletions tests/roles/backend_services/tasks/main.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,3 +151,14 @@
-p='[{"op": "replace", "path": "/spec/rabbitmq/templates/rabbitmq-cell1/resources/requests/cpu", "value": 500m}]'
oc patch ${crname} --type json \
-p='[{"op": "replace", "path": "/spec/rabbitmq/templates/rabbitmq-cell1/resources/requests/memory", "value": 500Mi}]'

- name: Verify that OpenStackControlPlane is waiting for openstackclient
ansible.builtin.shell: |
{{ shell_header }}
{{ oc_header }}

oc get openstackcontrolplane openstack -o jsonpath='{.status.conditions[?(@.type=="Ready")].message}{"\n"}' | grep 'OpenStackControlPlane Client not started'
register: openstack_control_plane_cr_result
until: openstack_control_plane_cr_result is success
retries: 60
delay: 2
6 changes: 6 additions & 0 deletions tests/roles/keystone_adoption/tasks/main.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@
retries: 60
delay: 2

- name: verify that OpenStackControlPlane setup is complete
ansible.builtin.shell: |
{{ shell_header }}
{{ oc_header }}
oc wait --for=condition=Ready --timeout=1m OpenStackControlPlane openstack
- name: clean up services and endpoints
ansible.builtin.shell: |
{{ shell_header }}
Expand Down