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.

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

Jump to bottom

RHEL-79554: Consolidate OpenStack and PowerVS hostname handling into mco-hostname script #4866

Merged
merged 1 commit into from
Mar 13, 2025
Merged

RHEL-79554: Consolidate OpenStack and PowerVS hostname handling into mco-hostname script #4866

merged 1 commit into from
Mar 13, 2025

Conversation

EmilienM
Copy link
Member

- What I did

This change simplifies hostname management for OpenStack and PowerVS platforms by
consolidating their handling into the existing mco-hostname script.

  • Introduces set_openstack_hostname and set_powervs_hostname functions in
    mco-hostname which until now was only used by GCP.
  • Removes the standalone openstack-afterburn-hostname script and replaces
    its invocation with mco-hostname --openstack.
  • Updates OpenStack and PowerVS systemd service units to call mco-hostname
    instead of using individual scripts.
  • Renames afterburn-hostname.service.yaml to openstack-hostname.service.yaml
    and powervs-hostname.service.yaml for clarity.
  • Ensures RemainAfterExit=yes is set in relevant services to persist hostname changes.

This refactoring reduces redundancy, improves maintainability, and aligns
hostname configuration across platforms.

- How to verify it

Ensure that afterburn hostname service is not in error state and that the hostname is correctly set.

- Description for the changelog

Fix issue with afterburn hostname retrieval by avoiding /dev/stdout in PowerVS.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 19, 2025
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Feb 19, 2025 •
edited by openshift-ci bot
Loading

@EmilienM: This pull request references RHEL-79554 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.19.0" version, but no target version was set.

In response to this:

- What I did

This change simplifies hostname management for OpenStack and PowerVS platforms by
consolidating their handling into the existing mco-hostname script.

  • Introduces set_openstack_hostname and set_powervs_hostname functions in
    mco-hostname which until now was only used by GCP.
  • Removes the standalone openstack-afterburn-hostname script and replaces
    its invocation with mco-hostname --openstack.
  • Updates OpenStack and PowerVS systemd service units to call mco-hostname
    instead of using individual scripts.
  • Renames afterburn-hostname.service.yaml to openstack-hostname.service.yaml
    and powervs-hostname.service.yaml for clarity.
  • Ensures RemainAfterExit=yes is set in relevant services to persist hostname changes.

This refactoring reduces redundancy, improves maintainability, and aligns
hostname configuration across platforms.

- How to verify it

Ensure that afterburn hostname service is not in error state and that the hostname is correctly set.

- Description for the changelog

Fix issue with afterburn hostname retrieval by avoiding /dev/stdout in PowerVS.

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 openshift-eng/jira-lifecycle-plugin repository.

@EmilienM
Copy link
Member Author

/cc mdbooth mandre

@openshift-ci openshift-ci bot requested review from mandre and mdbooth February 19, 2025 15:04
@EmilienM EmilienM mentioned this pull request Feb 19, 2025
Merged
@EmilienM
Copy link
Member Author

/test unit

@EmilienM EmilienM requested a review from mandre February 20, 2025 00:44
@EmilienM
Copy link
Member Author

/retest

mdbooth
mdbooth reviewed Feb 22, 2025
View reviewed changes
Copy link
Contributor

@mdbooth mdbooth left a comment

Choose a reason for hiding this comment

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

Seems reasonable. Missing systemd bits in powervs.

@EmilienM EmilienM requested review from mdbooth and mandre February 24, 2025 17:42
@mdbooth
Copy link
Contributor

mdbooth commented Feb 25, 2025

/cc @hamzy

@openshift-ci openshift-ci bot requested a review from hamzy February 25, 2025 20:06
@EmilienM
Copy link
Member Author

/test e2e-openstack-singlestackv6

@EmilienM
Copy link
Member Author

/test e2e-openstack-singlestackv6

@mdbooth
Copy link
Contributor

mdbooth commented Feb 28, 2025

Note that depending on the SELinux policy fix, we may also require the following systemd drop-in to make this work at /etc/systemd/system/afterburn.service.d/00-isofs.conf:

[Unit]
[email protected]
[email protected]

This depends on whether we allow afterburn to load kernel modules or not.

@EmilienM
RHEL-79554: Consolidate OpenStack and PowerVS hostname handling into …
be631ac 
…mco-hostname script

This change simplifies hostname management for OpenStack and PowerVS platforms by
consolidating their handling into the existing `mco-hostname` script.

* Introduces `set_openstack_hostname` and `set_powervs_hostname` functions in
  `mco-hostname` which until now was only used by GCP.
* Removes the standalone `openstack-afterburn-hostname` script and replaces
  its invocation with `mco-hostname --openstack`.
* Updates OpenStack and PowerVS systemd service units to call `mco-hostname`
  instead of using individual scripts.
* Renames `afterburn-hostname.service.yaml` to `openstack-hostname.service.yaml`
  and `powervs-hostname.service.yaml` for clarity.
* Ensures `RemainAfterExit=yes` is set in relevant services to persist hostname changes.
* Add `set -euo pipefail` to the `mco-hostname` script so if an error
  happens during the script, it'll stop and return a failure.

This refactoring reduces redundancy, improves maintainability, and aligns
hostname configuration across platforms.
@EmilienM
Copy link
Member Author

EmilienM commented Mar 5, 2025

/test e2e-openstack-singlestackv6

@mjturek
Copy link

mjturek commented Mar 5, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2025
@mdbooth
Copy link
Contributor

mdbooth commented Mar 5, 2025

Note that depending on the SELinux policy fix, we may also require the following systemd drop-in to make this work at /etc/systemd/system/afterburn.service.d/00-isofs.conf:

[Unit]
[email protected]
[email protected]

This depends on whether we allow afterburn to load kernel modules or not.

For info, I think we will allow afterburn to load kernel modules in the RHCOS backport, so this should not be required 🤞

@EmilienM
Copy link
Member Author

EmilienM commented Mar 6, 2025

/retest-required

@EmilienM
Copy link
Member Author

EmilienM commented Mar 6, 2025

/retest

@EmilienM
Copy link
Member Author

EmilienM commented Mar 6, 2025

/test e2e-openstack-singlestackv6

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 6, 2025

@EmilienM: No presubmit jobs available for openshift/machine-config-operator@main

In response to this:

/test e2e-openstack-singlestackv6

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.

@EmilienM
Copy link
Member Author

/retest-required

yuqi-zhang
yuqi-zhang approved these changes Mar 12, 2025
View reviewed changes
Copy link
Contributor

@yuqi-zhang yuqi-zhang left a comment

Choose a reason for hiding this comment

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

I think the consolidation looks good logically. In older versions, the MCO would sometimes wrongly backup removed files/services (in this case, you'd end up with systems that have both afterburn-hostname.service and openstack-hostname.service) but I believe all of those are fixed now

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 12, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: EmilienM, mdbooth, mjturek, yuqi-zhang

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-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2025
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 5dcc44a and 2 for PR HEAD be631ac in total

1 similar comment
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 5dcc44a and 2 for PR HEAD be631ac in total

@mdbooth
Copy link
Contributor

mdbooth commented Mar 12, 2025

/retest-required

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 6e460d1 and 2 for PR HEAD be631ac in total

2 similar comments
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 6e460d1 and 2 for PR HEAD be631ac in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 6e460d1 and 2 for PR HEAD be631ac in total

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 13, 2025

@EmilienM: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-vsphere-ovn-upi-zones be631ac link false /test e2e-vsphere-ovn-upi-zones
ci/prow/e2e-gcp-op-techpreview be631ac link false /test e2e-gcp-op-techpreview
ci/prow/e2e-azure-ovn-upgrade-out-of-change be631ac link false /test e2e-azure-ovn-upgrade-out-of-change
ci/prow/e2e-openstack be631ac link false /test e2e-openstack
ci/prow/e2e-openstack-singlestackv6 be631ac link false /test e2e-openstack-singlestackv6
ci/prow/e2e-gcp-op-ocl be631ac link false /test e2e-gcp-op-ocl
ci/prow/e2e-vsphere-ovn-upi be631ac link false /test e2e-vsphere-ovn-upi
ci/prow/okd-images be631ac link true /test okd-images
ci/prow/cluster-bootimages be631ac link true /test cluster-bootimages
ci/prow/4.12-upgrade-from-stable-4.11-images be631ac link true /test 4.12-upgrade-from-stable-4.11-images

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.

@openshift-merge-bot openshift-merge-bot bot merged commit 8cc645a into openshift:main Mar 13, 2025
18 of 26 checks passed
@mdbooth mdbooth mentioned this pull request Mar 14, 2025
Open
@mdbooth mdbooth deleted the RHEL-79554 branch March 14, 2025 09:07
@Prashanth684
Copy link
Contributor

/cherry-pick release-4.18

@openshift-cherrypick-robot
Copy link

@Prashanth684: #4866 failed to apply on top of branch "release-4.18":

Applying: RHEL-79554: Consolidate OpenStack and PowerVS hostname handling into mco-hostname script
Using index info to reconstruct a base tree...
M	templates/common/openstack/files/usr-local-bin-openstack-afterburn-hostname.yaml
M	templates/common/openstack/units/afterburn-hostname.service.yaml
Falling back to patching base and 3-way merge...
Auto-merging templates/common/openstack/units/openstack-hostname.service.yaml
CONFLICT (content): Merge conflict in templates/common/openstack/units/openstack-hostname.service.yaml
CONFLICT (modify/delete): templates/common/openstack/files/usr-local-bin-openstack-afterburn-hostname.yaml deleted in RHEL-79554: Consolidate OpenStack and PowerVS hostname handling into mco-hostname script and modified in HEAD. Version HEAD of templates/common/openstack/files/usr-local-bin-openstack-afterburn-hostname.yaml left in tree.
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 0001 RHEL-79554: Consolidate OpenStack and PowerVS hostname handling into mco-hostname script

In response to this:

/cherry-pick release-4.18

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.

Prashanth684 added a commit to Prashanth684/machine-config-operator that referenced this pull request Mar 26, 2025
@Prashanth684
Consolidate OpenStack and PowerVS hostname handling into mco-hostname…
19a888b 
… script

manual cherry-pick of openshift#4866 needed for OKD
@Prashanth684 Prashanth684 mentioned this pull request Mar 26, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdbooth mdbooth mdbooth approved these changes

@yuqi-zhang yuqi-zhang yuqi-zhang approved these changes

@cheesesashimi cheesesashimi Awaiting requested review from cheesesashimi

@clnperez clnperez Awaiting requested review from clnperez

@mandre mandre Awaiting requested review from mandre

@hamzy hamzy Awaiting requested review from hamzy

Assignees

@mjturek mjturek

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@EmilienM @openshift-ci-robot @mdbooth @mjturek @Prashanth684 @openshift-cherrypick-robot @mandre @yuqi-zhang