Control plane verification fixes #866

jistr · 2025-03-21T16:39:15Z

check/amend tests

Resolves: https://issues.redhat.com/browse/OSPRH-14744

openshift-ci · 2025-03-21T16:39:19Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

docs_user/modules/proc_adopting-the-identity-service.adoc

docs_user/modules/proc_deploying-backend-services.adoc

klgill · 2025-03-21T17:29:29Z

docs_user/modules/proc_deploying-backend-services.adoc

Is the dollar sign omitted on lines 411 and 412 because these lines are indented?

Yes lines 410-413 are to be copy pasted as a single command essentially. I saw in some other places we use the > character at the beginning of the line to signify this is a continuation line, but it prevents copy-pasting the multi-line command.

In general i think achieving consistency on this would be nice :)

We usually use forward slashes in the docs to signify a continuation line, like this example in the 17.1 FFU guide:
export STACK=
$ sudo awk '/tripleo_role_name/ {print "--role " $2}'
sudo awk '/tripleo_role_name/ {print "--role " $2}'
/var/lib/mistral/${STACK}/tripleo-ansible-inventory.yaml
| grep -vi compute

We should be using them upstream as well, instead of the > character.

The example you pasted didn't render well in the comment, but i guess you meant backward slashes? I can see those in FFU guide.

Here it's different though, i misspoke when i mentioned it's a continuation line, it's actually an open for command but not really a continuation line. There are multiple lines and it's in a single control loop, but it's not a single command. If we put there the backslashes, it wouldn't work correctly, unless we also use semicolons after the commands. So it's unclear to me what the best convention for these multi-line control structures should be...

I'll ask the docs team for advice.

I tried the copy button and i saw the copied text does not include the $ and > characters that are used to signal start of the command and multi-line commands. I don't know what's the mechanism behind that but i was pleasantly surprised :). So if we'd like to use that solution globally in the adoption docs, i'd be +1.

For now i updated the PR to use the > prefix for the multi-line command.

Let's wait to add the solution globally. In the past week, I've noticed that the copy icon only copies the first line in many of the code blocks in the adoption guide. I solved some of them by adding the [source,yaml] tag. I need to test whether adding the [source,yaml] tag works on code blocks without the > . If it does work, then we don't need the > and the docs would look cleaner.

klgill · 2025-03-21T17:32:16Z

docs_user/modules/proc_deploying-backend-services.adoc

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.

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.

could you preserve the docs link?

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 :).

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

Relevant PR that originally moved the content: #722

docs_user/modules/proc_deploying-backend-services.adoc

ciecierski · 2025-03-25T11:22:58Z

Can you link jira bug in description on this pull request? There is automation which adds linking in jira to pull request. Thank you

jistr · 2025-03-25T11:51:11Z

Linked the bug, i think perhaps we should also put this on hold until #517 lands.

/hold

softwarefactory-project-zuul · 2025-03-25T12:32:59Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/1d0d032d6c7541d4ab771221e662f78b

✔️ noop SUCCESS in 0s
❌ adoption-standalone-to-crc-ceph FAILURE in 1h 34m 53s
❌ adoption-standalone-to-crc-no-ceph FAILURE in 2h 01m 28s
✔️ adoption-docs-preview SUCCESS in 1m 27s

jistr · 2025-03-27T15:10:31Z

/unhold

jistr · 2025-04-02T16:10:03Z

/hold
Holding this until it's clear whether we will revert #517 or not.

jistr · 2025-04-09T15:49:47Z

/unhold

docs_user/modules/proc_adopting-the-identity-service.adoc

docs_user/modules/proc_deploying-backend-services.adoc

softwarefactory-project-zuul · 2025-04-10T16:16:24Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/32e39da6056848c58a9f82f42668b615

✔️ noop SUCCESS in 0s
❌ adoption-standalone-to-crc-ceph RETRY_LIMIT in 12m 08s
❌ adoption-standalone-to-crc-no-ceph FAILURE in 1h 13m 16s
✔️ adoption-docs-preview SUCCESS in 1m 15s

jistr · 2025-04-11T07:10:18Z

recheck image pull error

docs_user/modules/proc_adopting-the-identity-service.adoc

The previous version with '| grep Running' may be useful in tests to force a non-zero return code and abort a block of code, but in docs the grepping will hide anything that is not "Running", so we actually do not see if there are any issues. Therefore in the docs the 'grep' is now removed and the admin is told to check if all status commands print "Running".

jistr · 2025-04-14T15:47:02Z

Updated according to the comment and rebased.

odyssey4me · 2025-04-15T07:16:35Z

docs_user/modules/proc_deploying-backend-services.adoc

 +
+[source,yaml]
 ----
 $ RENAMED_CELLS="cell1 cell2 cell3"


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?

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.

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

odyssey4me · 2025-04-15T07:17:02Z

docs_user/modules/proc_deploying-backend-services.adoc

-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


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

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.

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
`

jistr · 2025-04-15T08:30:00Z

@odyssey4me Thanks for the review, but given that this patch has been on review for 3 weeks and the sprint is coming to a close, i think i'd focus any edits here on things that are clear breakages.

jistr · 2025-04-15T08:36:06Z

I'd like to avoid respinning the CI on tangential issues.

odyssey4me · 2025-04-15T10:29:59Z

@jistr Yes, fair enough - my suggestions are docs-only and could follow up in a PR that's docs-only to avoid respinning the integration testing in CI.

holser

/lgtm

@jistr

@jistr implemented requested changes in next PR updates. In case of any requests they can be processed as a new bug

bogdando

lgtm

bogdando · 2025-04-15T11:57:35Z

docs_user/modules/proc_deploying-backend-services.adoc

 +
+[source,yaml]
 ----
 $ RENAMED_CELLS="cell1 cell2 cell3"


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

bogdando · 2025-04-15T11:59:06Z

docs_user/modules/proc_deploying-backend-services.adoc

could you preserve the docs link?

tests/roles/keystone_adoption/tasks/main.yaml

jistr · 2025-04-15T16:58:15Z

2x lgtm, comments addressed.
/approve

openshift-ci · 2025-04-15T16:58:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jistr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [jistr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jistr · 2025-04-15T17:00:25Z

For some reason my reply to one of the comments doesn't show in the discussion UI but it does show in the code UI:

https://github.com/openstack-k8s-operators/data-plane-adoption/pull/866/files#r2044430583

ciecierski · 2025-04-15T17:21:32Z

/lgtm

jistr · 2025-04-16T06:02:39Z

/cherry-pick 18.0-fr2

openshift-cherrypick-robot · 2025-04-16T06:03:15Z

@jistr: #866 failed to apply on top of branch "18.0-fr2":

Applying: Add verification steps that look at OpenStackControlPlane CR status
Applying: Use MariaDB/Rabbit verification commands that will make issues visible
Using index info to reconstruct a base tree...
M	docs_user/modules/proc_deploying-backend-services.adoc
Falling back to patching base and 3-way merge...
Auto-merging docs_user/modules/proc_deploying-backend-services.adoc
CONFLICT (content): Merge conflict in docs_user/modules/proc_deploying-backend-services.adoc
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0002 Use MariaDB/Rabbit verification commands that will make issues visible

Details

In response to this:

/cherry-pick 18.0-fr2

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

jistr · 2025-04-17T13:36:04Z

@odyssey4me Follow ups:
#932
https://issues.redhat.com/browse/OSPRH-15896

openshift-ci bot added the do-not-merge/work-in-progress label Mar 21, 2025

jistr requested a review from klgill March 21, 2025 16:45

klgill previously requested changes Mar 21, 2025

View reviewed changes

jistr force-pushed the b/control-plane-verification-14744 branch from 30fdad0 to cc732ad Compare March 24, 2025 10:50

jistr marked this pull request as ready for review March 24, 2025 16:02

openshift-ci bot removed the do-not-merge/work-in-progress label Mar 24, 2025

klgill reviewed Mar 24, 2025

View reviewed changes

docs_user/modules/proc_deploying-backend-services.adoc Outdated Show resolved Hide resolved

jistr force-pushed the b/control-plane-verification-14744 branch from 17f4436 to f8e3c8b Compare March 25, 2025 10:30

openshift-ci bot added the do-not-merge/hold label Mar 25, 2025

openshift-ci bot removed the do-not-merge/hold label Mar 27, 2025

openshift-merge-robot added the needs-rebase label Mar 27, 2025

openshift-ci bot added the do-not-merge/hold label Apr 2, 2025

jistr force-pushed the b/control-plane-verification-14744 branch from f8e3c8b to 9f28ad9 Compare April 9, 2025 15:49

openshift-merge-robot removed the needs-rebase label Apr 9, 2025

openshift-ci bot removed the do-not-merge/hold label Apr 9, 2025

ciecierski reviewed Apr 9, 2025

View reviewed changes

docs_user/modules/proc_adopting-the-identity-service.adoc Outdated Show resolved Hide resolved

ciecierski reviewed Apr 9, 2025

View reviewed changes

docs_user/modules/proc_deploying-backend-services.adoc Outdated Show resolved Hide resolved

jistr force-pushed the b/control-plane-verification-14744 branch from 9f28ad9 to 355b3a2 Compare April 10, 2025 15:02

ciecierski reviewed Apr 14, 2025

View reviewed changes

docs_user/modules/proc_adopting-the-identity-service.adoc Outdated Show resolved Hide resolved

jistr added 2 commits April 14, 2025 17:46

Add verification steps that look at OpenStackControlPlane CR status

bd082ac

jistr added 2 commits April 14, 2025 17:46

Use oc wait for checking that OpenStackControlPlane is Ready

0f95f56

Use Galera service name in 'deploying backend services' doc

7d96934

jistr force-pushed the b/control-plane-verification-14744 branch from 355b3a2 to 7d96934 Compare April 14, 2025 15:46

odyssey4me reviewed Apr 15, 2025

View reviewed changes

holser requested review from bogdando and holser April 15, 2025 11:11

holser reviewed Apr 15, 2025

View reviewed changes

openshift-ci bot assigned holser Apr 15, 2025

openshift-ci bot added the lgtm label Apr 15, 2025

holser approved these changes Apr 15, 2025

View reviewed changes

holser removed their assignment Apr 15, 2025

bogdando approved these changes Apr 15, 2025

View reviewed changes

ciecierski reviewed Apr 15, 2025

View reviewed changes

tests/roles/keystone_adoption/tasks/main.yaml Show resolved Hide resolved

openshift-ci bot added the approved label Apr 15, 2025

openshift-ci bot assigned ciecierski Apr 15, 2025

jparoly approved these changes Apr 15, 2025

View reviewed changes

openshift-merge-bot bot merged commit 20c3126 into openstack-k8s-operators:main Apr 15, 2025
6 checks passed

Control plane verification fixes #866

Control plane verification fixes #866

Uh oh!

Conversation

jistr commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Mar 21, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jistr Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ciecierski commented Mar 25, 2025

Uh oh!

jistr commented Mar 25, 2025

Uh oh!

softwarefactory-project-zuul bot commented Mar 25, 2025

Uh oh!

jistr commented Mar 27, 2025

Uh oh!

jistr commented Apr 2, 2025

Uh oh!

jistr commented Apr 9, 2025

Uh oh!

Uh oh!

Uh oh!

softwarefactory-project-zuul bot commented Apr 10, 2025

Uh oh!

jistr commented Apr 11, 2025

Uh oh!

Uh oh!

jistr commented Apr 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ciecierski Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jistr commented Apr 15, 2025

jistr commented Mar 21, 2025 •

edited

Loading

jistr Mar 24, 2025 •

edited

Loading

ciecierski Apr 15, 2025 •

edited

Loading