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

Rendering for Director Operator docs variant Fixes #780

Merged

Conversation

pinikomarov
Copy link
Contributor

@pinikomarov pinikomarov commented Jan 10, 2025

This addresses the code change requests from :
Jira: https://issues.redhat.com/browse/OSPRH-12836

Fixes docs generation for upstream and downstream odpso added targets

and removes misc. abnormalities from the text.

pinikomarov added a commit to pinikomarov/data-plane-adoption_pkomarov that referenced this pull request Jan 10, 2025
Copy link

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/b38bc9e471a04adcbba962ec3108a99f

✔️ noop SUCCESS in 0s
✔️ adoption-standalone-to-crc-ceph SUCCESS in 2h 57m 04s
adoption-standalone-to-crc-no-ceph RETRY_LIMIT in 47m 32s
adoption-docs-preview FAILURE in 1m 13s

Copy link

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/9c461deb26514fb4a0079b8cbc835b01

✔️ noop SUCCESS in 0s
✔️ adoption-standalone-to-crc-ceph SUCCESS in 3h 04m 27s
adoption-standalone-to-crc-no-ceph RETRY_LIMIT in 49m 01s
adoption-docs-preview FAILURE in 1m 27s

@pinikomarov
Copy link
Contributor Author

recheck

Copy link

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/4e129364dab543e98f3553594276ca46

✔️ noop SUCCESS in 0s
✔️ adoption-standalone-to-crc-ceph SUCCESS in 3h 00m 10s
adoption-standalone-to-crc-no-ceph RETRY_LIMIT in 46m 40s
adoption-docs-preview FAILURE in 1m 24s

Copy link

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/2821a3a6bbb44ecfb7131d7d9687ba0c

✔️ noop SUCCESS in 0s
✔️ adoption-standalone-to-crc-ceph SUCCESS in 3h 05m 35s
✔️ adoption-standalone-to-crc-no-ceph SUCCESS in 3h 00m 15s
adoption-docs-preview FAILURE in 1m 12s

@pinikomarov pinikomarov changed the title Rendering for Director Operator docs variant debug Rendering for Director Operator docs variant Fixes Jan 15, 2025
@pinikomarov
Copy link
Contributor Author

recheck

@pinikomarov
Copy link
Contributor Author

@jistr @klgill review please?

Copy link
Contributor

@klgill klgill left a comment

Choose a reason for hiding this comment

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

@pinikomarov I have some questions and suggestions.

@@ -23,6 +23,8 @@ $ oc project openstack
----

. Define the common environment variables:
+
+
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
+

There only needs to be one plus sign above the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

ifeval::["{OpenStackPreviousInstaller}" == "director_operator"]
include::assemblies/assembly_ospdo-scale-down-pre-database-adoption.adoc[leveloffset=+1]
endif::[]

Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this assembly removed from main.adoc and deleted from the repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably my mistake , returning it

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the ifeval be ifeval::["{build_variant}" == "ospdo"] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

include::assemblies/assembly_prepare_director_operator_for_adoption_process.adoc[leveloffset=+1]
ifeval::["{build_variant}" == "ospdo"]
include::assemblies/assembly_prepare_director_operator_and_rhoso_for_adoption_process.adoc
[leveloffset=+1]
Copy link
Contributor

Choose a reason for hiding this comment

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

This assembly is not rendering in the docs because the "assembly_prepare_director_operator_and_rhoso_for_adoption_process.adoc" is not in the assemblies folder, but it should be. Can you please move the "assembly_prepare_director_operator_and_rhoso_for_adoption_process.adoc" to the assemblies folder? After you do that, this include statement should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on my patch I do see it on that location : ```

k@k:/my_workspace/data-plane-adoption/docs_user$ pwd
/home/k/my_workspace/data-plane-adoption/docs_user
k@k:
/my_workspace/data-plane-adoption/docs_user$ ls assemblies/assembly_prepare_director_operator_and_rhoso_for_adoption_process.adoc
assemblies/assembly_prepare_director_operator_and_rhoso_for_adoption_process.adoc

Copy link
Contributor

Choose a reason for hiding this comment

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

The file still isn't rendering, but I think I know why. The file name should be formatted as follows:

assemblies/assembly_prepare-director-operator-and-rhoso-for-adoption-process.adoc

If you rename the file with the name I provided here, and then update the file name in the include statement in main.adoc, I'm hoping that fixes the problem.

@@ -126,7 +126,7 @@ endif::[]
ifeval::["{build}" == "downstream"]
$(cat <path_to_SSH_key> | base64 | sed \'s/^/ /')
endif::[]
ifeval::["{OpenStackPreviousInstaller}" == "director_operator"]
ifeval::["{build_variant}" == "ospdo"]
$(oc exec -n $<ospdo_namespace> -t openstackclient openstackclient -- cat /home/cloud-admin/.ssh/id_rsa | base64 | sed 's/^/ /')
Copy link
Contributor

Choose a reason for hiding this comment

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

The s/^/ / ` is rendering as italics in the doc, and the single quotes do not display. However, the first instance of that code above it looks okay. Is the code on line 130 missing a back slash? See the following:

  • line 127: sed \'s/^/ /')
  • line 130: sed 's/^/ /')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -113,7 +113,7 @@ make input
----
endif::[]

ifeval::["{OpenStackPreviousInstaller}" == "director_operator"]
ifeval::["{build_variant}" == "ospdo"]
----
$ oc get secret tripleo-passwords -o jsonpath='{.data.*}' | base64 -d>~/tripleo-standalone-passwords.yaml
----
Copy link
Contributor

Choose a reason for hiding this comment

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

In the rendered OSPdO doc, both the $ oc project openstack command and the $ oc get secret tripleo-passwords command are rendering. Was that intended?

If not, we might need another conditional statement on the $ oc project openstack command that ensures that the command only renders in the "regular" adoption guide. Following the syntax of the downstream/upstream conditionals, you could try something like this:

ifeval::["{build}" != "ospdo"]
----
$ oc project openstack
----
endif::[]

This might require a change to the MakeFile as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

both : oc project openstack command and the oc get secret tripleo-passwords command commands are needed in the ospdo scenario, the first is needed for the second to get the right pass from the right namespace.
and in a downstream scenarion only the first is needed. so that makes sense as is now

To get the value to set `SOURCE_MARIADB_IP`, query the puppet-generated configurations in a Controller node:
+
Copy link
Contributor

Choose a reason for hiding this comment

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

You need this plus sign.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

Comment on lines 18 to 19
CONTROLLER1_SSH="ssh -i ~/install_yamls/out/edpm/ansibleee-ssh-key-id_rsa [email protected]"
MARIADB_IMAGE=quay.io/podified-antelope-centos9/openstack-mariadb:current-podified
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you only want lines 18-19 to appear in the OSPdO docs? The following code block is also appearing:

CONTROLLER1_SSH="ssh -i *<path to SSH key>* root@*<node IP>*"
MARIADB_IMAGE=registry.redhat.io/rhosp-dev-preview/openstack-mariadb-rhel9:18.0
SOURCE_MARIADB_IP=172.17.0.2
SOURCE_DB_ROOT_PASSWORD=$(cat ~/overcloud-deploy/overcloud/overcloud-passwords.yaml | grep ' MysqlRootPassword:' | awk -F ': ' '{ print $2; }')
MARIADB_CLIENT_ANNOTATIONS='--annotations=k8s.v1.cni.cncf.io/networks=internalapi'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes that's ok

Comment on lines +49 to 57
ifeval::["{build_variant}" == "ospdo"]
export PULL_OPENSTACK_CONFIGURATION_DATABASES=$(oc run mariadb-client --overrides="$RUN_OVERRIDES" -n $OSPDO_NAMESPACE -q --image ${MARIADB_IMAGE} -i --rm --restart=Never -- \
endif::[]
mysql -rsh "$SOURCE_MARIADB_IP" -uroot -p"$SOURCE_DB_ROOT_PASSWORD" -e 'SHOW databases;')
echo "$PULL_OPENSTACK_CONFIGURATION_DATABASES"
----
ifeval::["{OpenStackPreviousInstaller}" == "director_operator"]
ifeval::["{build_variant}" == "ospdo"]
----
export CONTROLLER1_SSH="oc -n $OSPDO_NAMESPACE rsh -c openstackclient openstackclient ssh controller-0.ctlplane"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should both export commands appear in the OSPdO docs? Lines 50-57.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes that's ok

export PULL_OPENSTACK_CONFIGURATION_DATABASES=$(oc run mariadb-client --overrides="$RUN_OVERRIDES" -n $OSPDO_NAMESPACE -q --image ${MARIADB_IMAGE} -i --rm --restart=Never -- \
endif::[]
mysql -rsh "$SOURCE_MARIADB_IP" -uroot -p"$SOURCE_DB_ROOT_PASSWORD" -e 'SHOW databases;')
echo "$PULL_OPENSTACK_CONFIGURATION_DATABASES"
----
ifeval::["{OpenStackPreviousInstaller}" == "director_operator"]
ifeval::["{build_variant}" == "ospdo"]
----
export CONTROLLER1_SSH="oc -n $OSPDO_NAMESPACE rsh -c openstackclient openstackclient ssh controller-0.ctlplane"
----
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
----
----
+

Copy link
Contributor

Choose a reason for hiding this comment

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

You also need a plus sign between lines 59 and 60. (Between the step and the four dashes).

@@ -149,6 +149,11 @@ ifeval::["{build}" == "downstream"]
:telemetry: Telemetry service
endif::[]

ifeval::["{build}-{build_variant}" == "downstream-ospdo"]
:OpenStackPreviousInstaller: director_operator
Copy link
Contributor

Choose a reason for hiding this comment

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

There are several instances of "director_operator" in the text in the rendered OSPdO doc. It should be "director Operator".

In the OSPdO guide, should “director Operator” replace the word “director” in all cases? For example, in the “Identity service authentication” module, this sentence now reads: “After you adopt a director_operator-based OpenStack deployment to a Red Hat OpenStack Services on OpenShift deployment, the Identity service performs user authentication and authorization by using Secure RBAC (SRBAC).”
Is this what was intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

@pinikomarov
Copy link
Contributor Author

@klgill updated according to your notes thanks, please tell me if there are any more issues thanks

Copy link
Contributor

@klgill klgill left a comment

Choose a reason for hiding this comment

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

I have a couple more comments, mainly about the "preparing" assembly still not rendering. But I suggested a solution. I also noticed that the build variant is incorrect for the scale down procedure. Please see my comments.

Side note: I noticed minor formatting issues, but I would like to fix this in a separate docs PR myself, since I need to split the "preparing" assembly anyway. These formatting issues will also not delay QE review.

include::assemblies/assembly_prepare_director_operator_for_adoption_process.adoc[leveloffset=+1]
ifeval::["{build_variant}" == "ospdo"]
include::assemblies/assembly_prepare_director_operator_and_rhoso_for_adoption_process.adoc
[leveloffset=+1]
Copy link
Contributor

Choose a reason for hiding this comment

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

The file still isn't rendering, but I think I know why. The file name should be formatted as follows:

assemblies/assembly_prepare-director-operator-and-rhoso-for-adoption-process.adoc

If you rename the file with the name I provided here, and then update the file name in the include statement in main.adoc, I'm hoping that fixes the problem.

Comment on lines 117 to 118
----
$ oc get secret tripleo-passwords -o jsonpath='{.data.*}' | base64 -d>~/tripleo-standalone-passwords.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
----
$ oc get secret tripleo-passwords -o jsonpath='{.data.*}' | base64 -d>~/tripleo-standalone-passwords.yaml
+
----
$ oc get secret tripleo-passwords -o jsonpath='{.data.*}' | base64 -d>~/tripleo-standalone-passwords.yaml

Copy link
Contributor

Choose a reason for hiding this comment

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

I added a plus sign so that the second command aligns with the command above it.

@@ -39,20 +39,20 @@ endif::[]

. To locate the CA certificate and key, list all the certificates inside your NSSDB:
Copy link
Contributor

Choose a reason for hiding this comment

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

If both lines 40 and 51 need to appear in the OSPdO docs, then you don't need to have the following line twice:
"To locate the CA certificate and key, list all the certificates inside your NSSDB."

I would remove that line from between the OSPdO ifeval statements. So it just starts with "If you installed OSPdO..."

To locate the CA certificate and key, list all the certificates inside your NSSDB. If you installed OSPdO by using director-dev-tools, the server host runs the freeipa server as a container:
+
----
$ IPA_SSH="podman exec -ti freeipa-server"
----
endif::[]
+
Copy link
Contributor

Choose a reason for hiding this comment

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

The alignment is still off, but I think I'd like to fix this in another PR.

ifeval::["{OpenStackPreviousInstaller}" == "director_operator"]
include::assemblies/assembly_ospdo-scale-down-pre-database-adoption.adoc[leveloffset=+1]
endif::[]

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the ifeval be ifeval::["{build_variant}" == "ospdo"] ?

@pinikomarov
Copy link
Contributor Author

@klgill ok updated my patch

Copy link

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/3d8b5e1fe81a423f9dffea5302e2093b

✔️ noop SUCCESS in 0s
✔️ adoption-standalone-to-crc-ceph SUCCESS in 3h 05m 45s
✔️ adoption-standalone-to-crc-no-ceph SUCCESS in 2h 55m 21s
adoption-docs-preview FAILURE in 1m 12s

@klgill
Copy link
Contributor

klgill commented Jan 17, 2025

recheck

@klgill
Copy link
Contributor

klgill commented Jan 17, 2025

@klgill ok updated my patch

@pinikomarov Unfortunately the doc build is failing. I tried to do a recheck, but rdoproject.org/github-check has stayed yellow for hours. The log says the following:

if ! type bundle; then \
	echo "Bundler not found. On Linux run 'sudo dnf install /usr/bin/bundle' to install it."; \
	exit 1; \

Can you try running sudo dnf install /usr/bin/bundle and see if that fixes it?

Copy link

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/de9d7d0614e943d3a11b222298505575

✔️ noop SUCCESS in 0s
✔️ adoption-standalone-to-crc-ceph SUCCESS in 3h 02m 11s
✔️ adoption-standalone-to-crc-no-ceph SUCCESS in 2h 59m 06s
adoption-docs-preview FAILURE in 1m 26s

@klgill
Copy link
Contributor

klgill commented Jan 17, 2025

recheck

Comment on lines 117 to 119
++
+----
+$ oc get secret tripleo-passwords -o jsonpath='{.data.*}' | base64 -d>~/tripleo-standalone-passwords.yaml
Copy link
Contributor

@klgill klgill Jan 17, 2025

Choose a reason for hiding this comment

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

Suggested change
++
+----
+$ oc get secret tripleo-passwords -o jsonpath='{.data.*}' | base64 -d>~/tripleo-standalone-passwords.yaml
+
----
$ oc get secret tripleo-passwords -o jsonpath='{.data.*}' | base64 -d>~/tripleo-standalone-passwords.yaml

Copy link
Contributor

Choose a reason for hiding this comment

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

@pinikomarov This seems to be where the problem starts. There are extra plus signs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right , removed those

@@ -231,16 +232,16 @@ spec:
service:
metadata:
annotations:
ifeval::["{OpenStackPreviousInstaller}" != "director_operator"]
ifeval::["{OpenStackPreviousInstaller}" != "director Operator"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This ifeval needs to be ["{build_variant}" == "ospdo"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

@@ -312,7 +313,7 @@ endif::[]
ovn:
enabled: false
template:
ifeval::["{OpenStackPreviousInstaller}" != "director_operator"]
ifeval::["{OpenStackPreviousInstaller}" != "director Operator"]
Copy link
Contributor

Choose a reason for hiding this comment

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

["{build_variant}" == "ospdo"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

@@ -362,13 +363,13 @@ endif::[]
service:
metadata:
annotations:
ifeval::["{OpenStackPreviousInstaller}" != "director_operator"]
ifeval::["{OpenStackPreviousInstaller}" != "director Operator"]
Copy link
Contributor

Choose a reason for hiding this comment

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

["{build_variant}" == "ospdo"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

@pinikomarov
Copy link
Contributor Author

updated the ospdo ifeval , fixed the extra + signs and sorted the numbering

@pinikomarov
Copy link
Contributor Author

recheck

Copy link
Contributor

@klgill klgill left a comment

Choose a reason for hiding this comment

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

1 comment to help troubleshoot the downstream build.

Comment on lines 216 to 219
storageClass: <storage_class> <1>
endif::[]
barbican:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
storageClass: <storage_class> <1>
endif::[]
barbican:
storageClass: <storage_class> <1>
endif::[]
barbican:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@klgill
Copy link
Contributor

klgill commented Jan 20, 2025

recheck

Copy link

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/3d512f5e8e2b48369085ccdc2b539b21

✔️ noop SUCCESS in 0s
✔️ adoption-standalone-to-crc-ceph SUCCESS in 3h 03m 10s
adoption-standalone-to-crc-no-ceph FAILURE in 47m 38s
✔️ adoption-docs-preview SUCCESS in 1m 15s

@klgill
Copy link
Contributor

klgill commented Jan 20, 2025

recheck

Copy link

openshift-ci bot commented Jan 20, 2025

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 10c42a7 into openstack-k8s-operators:main Jan 20, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants