Skip to content

TELCODOCS#2230: Coordinating reboots for configuration changes #91723

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

Merged
merged 1 commit into from
Apr 30, 2025

Conversation

sr1kar99
Copy link
Contributor

@sr1kar99 sr1kar99 commented Apr 7, 2025

Version(s):
4.19

Issue:
TELCODOCS-2230

Link to docs preview:

QE review:

  • QE has approved this change.

@openshift-ci openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 7, 2025
Copy link
Contributor

@xenolinux xenolinux left a comment

Choose a reason for hiding this comment

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

Suggested conditionals for trial as per #91723 (comment)

@sr1kar99 sr1kar99 force-pushed the 2230-controlled-reboots branch 2 times, most recently from 00e1010 to d439288 Compare April 8, 2025 05:08
@sr1kar99 sr1kar99 force-pushed the 2230-controlled-reboots branch from d439288 to 735afd2 Compare April 8, 2025 12:13
@sr1kar99 sr1kar99 force-pushed the 2230-controlled-reboots branch from 735afd2 to 5112fd9 Compare April 9, 2025 07:43
@sr1kar99 sr1kar99 force-pushed the 2230-controlled-reboots branch from 5112fd9 to 8563776 Compare April 9, 2025 08:21
@sr1kar99 sr1kar99 force-pushed the 2230-controlled-reboots branch from 8563776 to 3cb3e72 Compare April 21, 2025 14:49
@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 21, 2025
@sr1kar99 sr1kar99 force-pushed the 2230-controlled-reboots branch from 3cb3e72 to bca86ff Compare April 21, 2025 15:29
@sr1kar99
Copy link
Contributor Author

/label peer-review-needed

@openshift-ci openshift-ci bot added the peer-review-needed Signifies that the peer review team needs to review this PR label Apr 26, 2025
@GroceryBoyJr
Copy link
Contributor

/label peer-review-in-progress
/remove-label peer-review-needed
/assign GroceryBoyJr

@openshift-ci openshift-ci bot added peer-review-in-progress Signifies that the peer review team is reviewing this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Apr 26, 2025
Copy link
Contributor

@GroceryBoyJr GroceryBoyJr left a comment

Choose a reason for hiding this comment

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

LGTM

@GroceryBoyJr
Copy link
Contributor

/label peer-review-done
/remove-label peer-review-in-progress
/unassign GroceryBoyJr

@openshift-ci openshift-ci bot added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-in-progress Signifies that the peer review team is reviewing this PR labels Apr 26, 2025
@sr1kar99 sr1kar99 force-pushed the 2230-controlled-reboots branch from b49772f to 1ff4b94 Compare April 27, 2025 13:05
Copy link

openshift-ci bot commented Apr 27, 2025

@sr1kar99: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@sudomakeinstall2
Copy link

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2025
@sr1kar99
Copy link
Contributor Author

/label merge-review-needed

@openshift-ci openshift-ci bot added the merge-review-needed Signifies that the merge review team needs to review this PR label Apr 30, 2025
@michaelryanpeter
Copy link
Contributor

/label merge-review-in-progress

@openshift-ci openshift-ci bot added the merge-review-in-progress Signifies that the merge review team is reviewing this PR label Apr 30, 2025
Copy link
Contributor

@michaelryanpeter michaelryanpeter left a comment

Choose a reason for hiding this comment

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

I left a few style and minimalism nits that do not block merging. Please consider addressing these comments in a follow up PR. Great work.

<2> Add all required configuration policies before the reboot policy. {cgu-operator} applies the configuration changes as specified in the policies, in the order they are listed.
<3> Specify the timeout in seconds for the entire upgrade across all selected clusters. Set this field by considering the worst-case scenario.

. After you apply the CGU custom resource, {cgu-operator} rolls out the configuration policies in order. Once all policies are compliant, it applies the reboot policy and triggers a reboot of all nodes in the specified `MachineConfigPool`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't really a step that the customer performs. This is a consequence of applying the CR.

This is not a merge blocker, but in a follow up PR, I recommend explicitly adding the oc apply -f command and then you can include this information below the procedure step, similar to the following example:

Suggested change
. After you apply the CGU custom resource, {cgu-operator} rolls out the configuration policies in order. Once all policies are compliant, it applies the reboot policy and triggers a reboot of all nodes in the specified `MachineConfigPool`.
. Apply the CGU CR by entering the following command:
+
[source,terminal]
----
$ oc apply -f <cgu_cr>.yaml
----
+
After you apply the CGU custom resource, {cgu-operator} rolls out the configuration policies in order. Once all policies are compliant, it applies the reboot policy and triggers a reboot of all nodes in the specified machine config pool.

Also, when you use a generic reference to an API object, as you do in the previous sentence, the guidance is to drop the camel case and backticks, hence "machine config pool". If you are referring to the specific API object, which you can do but I don't think is necessary, then MachineConfigPool should be followed by a noun such as object or resource. See the repo guidelines for more information: https://github.com/openshift/openshift-docs/blob/main/contributing_to_docs/doc_guidelines.adoc#api-object-formatting

Comment on lines +62 to +64
. Monitor the CGU rollout status.
+
You can monitor the rollout of the CGU custom resource on the hub by checking the status. Verify the successful rollout of the reboot by running the following command:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker, but the first part of the step is fluff and can be cut:

Suggested change
. Monitor the CGU rollout status.
+
You can monitor the rollout of the CGU custom resource on the hub by checking the status. Verify the successful rollout of the reboot by running the following command:
. You can monitor the rollout of the CGU custom resource on the hub by checking the status. Verify the successful rollout of the reboot by running the following command:

Comment on lines +78 to +79
. Verify successful reboot on a specific node.
+
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above, this section is fluff and can be deleted. Not a merge blocker.


. Verify successful reboot on a specific node.
+
To confirm that the reboot was successful on a specific node, check the status of the `MachineConfigPool` (MCP) for the node by running the following command:
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the comment above, to conform with the rules around API objects, the correct usage is "MachineConfigPool " or simply "machine config pool". Not a merge blocker.

@michaelryanpeter michaelryanpeter added this to the Planned for 4.19 GA milestone Apr 30, 2025
@michaelryanpeter michaelryanpeter merged commit 6af204c into openshift:main Apr 30, 2025
2 checks passed
@michaelryanpeter
Copy link
Contributor

/cherrypick enterprise-4.19

@openshift-cherrypick-robot

@michaelryanpeter: new pull request created: #92789

In response to this:

/cherrypick enterprise-4.19

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.19 lgtm Indicates that a PR is ready to be merged. merge-review-in-progress Signifies that the merge review team is reviewing this PR merge-review-needed Signifies that the merge review team needs to review this PR peer-review-done Signifies that the peer review team has reviewed this PR size/L Denotes a PR that changes 100-499 lines, ignoring generated files. telco Label for all Telco PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants