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

Add support for root_device_hints #553

Merged
merged 3 commits into from
Feb 11, 2025
Merged

Conversation

thekad
Copy link
Contributor

@thekad thekad commented Feb 3, 2025

SUMMARY

There is no code path that uses installation_disk_path in the OCP agent, adding support for root_device_hints

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
Tests

Test-Hints: no-check

@thekad thekad requested a review from a team as a code owner February 3, 2025 19:57
@thekad thekad changed the title Rework agent-config template Add support for root_device_hints Feb 3, 2025
Copy link

@dcibot
Copy link
Collaborator

dcibot commented Feb 3, 2025

@dcibot
Copy link
Collaborator

dcibot commented Feb 3, 2025

Copy link

@dcibot
Copy link
Collaborator

dcibot commented Feb 3, 2025

@dcibot
Copy link
Collaborator

dcibot commented Feb 3, 2025

Copy link

@dcibot
Copy link
Collaborator

dcibot commented Feb 4, 2025

Copy link

@dcibot
Copy link
Collaborator

dcibot commented Feb 4, 2025

@dcibot
Copy link
Collaborator

dcibot commented Feb 4, 2025

Copy link
Contributor

@tonyskapunk tonyskapunk left a comment

Choose a reason for hiding this comment

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

I'm thinking that it is possible to validate the rootDeviceHints by modifying one of our inventories for testing and then using this change, I'll help with that.

roles/generate_manifests/README.md Outdated Show resolved Hide resolved
| extra_manifest_dir | $MANIFESTS_DIR/openshift | Yes | Path for extra manifests |
| static_network_config | {} | No | Static network config for every node |
| root_device_hints | {} | No | Device Hints per node |
Copy link
Contributor

Choose a reason for hiding this comment

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

could you include the doc for the root device hints you shared in the conversation, I think is quite useful to understand how to use that variable

https://docs.redhat.com/en/documentation/openshift_container_platform/4.17/html-single/installing_an_on-premise_cluster_with_the_agent-based_installer/index#root-device-hints_preparing-to-install-with-agent-based-installer

Copy link

@dcibot
Copy link
Collaborator

dcibot commented Feb 5, 2025

Copy link

Copy link

@dcibot
Copy link
Collaborator

dcibot commented Feb 6, 2025

@dcibot
Copy link
Collaborator

dcibot commented Feb 6, 2025

| static_network_config | {} | No | Static network config for every node |
| installation_disk_path | | No | Disk to use for install if you don't want the first found disk |
| root_device_hints | {} | No | Install device hints per node, in case installation_disk_path is not enough |
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be useful to link to documentation describing what hints are available?

@dcibot
Copy link
Collaborator

dcibot commented Feb 6, 2025

from change #553:

  • no check (not a code change)

1 similar comment
@dcibot
Copy link
Collaborator

dcibot commented Feb 6, 2025

from change #553:

  • no check (not a code change)

Copy link

```

[^1]: As per the [OCP 4.17 docs](https://docs.redhat.com/en/documentation/openshift_container_platform/4.17/html-single/installing_an_on-premise_cluster_with_the_agent-based_installer/index#installing-ocp-agent-inputs_installing-with-agent-based-installer)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
[^1]: As per the [OCP 4.17 docs](https://docs.redhat.com/en/documentation/openshift_container_platform/4.17/html-single/installing_an_on-premise_cluster_with_the_agent-based_installer/index#installing-ocp-agent-inputs_installing-with-agent-based-installer)
[^1]: As per the [OCP 4.17 docs](https://docs.redhat.com/en/documentation/openshift_container_platform/4.17/html-single/installing_an_on-premise_cluster_with_the_agent-based_installer/index#root-device-hints_preparing-to-install-with-agent-based-installer)

@dcibot
Copy link
Collaborator

dcibot commented Feb 6, 2025

from change #553:

  • no check (not a code change)

1 similar comment
@dcibot
Copy link
Collaborator

dcibot commented Feb 6, 2025

from change #553:

  • no check (not a code change)

Copy link

tonyskapunk
tonyskapunk previously approved these changes Feb 6, 2025
Copy link
Contributor

@tonyskapunk tonyskapunk left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!!

@dcibot
Copy link
Collaborator

dcibot commented Feb 7, 2025

from change #553:

  • no check (not a code change)

1 similar comment
@dcibot
Copy link
Collaborator

dcibot commented Feb 7, 2025

from change #553:

  • no check (not a code change)

Copy link

Copy link
Contributor

@tonyskapunk tonyskapunk left a comment

Choose a reason for hiding this comment

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

LGTM, please don't forget to squash your commits before merging.

thekad and others added 3 commits February 10, 2025 12:28
The root device hints are independent from the static network
configuration.
Keep backwards compatibility with installation_disk_path, if you specify
the installation_disk_path then a device hint is added, if you specify a
dictionary then the root hints are added as k/v pairs

TestBos2: abi
TestBos2Sno: abi-sno
Co-authored-by: Tony Garcia <[email protected]>
@dcibot
Copy link
Collaborator

dcibot commented Feb 10, 2025

from change #553:

  • no check (not a code change)

1 similar comment
@dcibot
Copy link
Collaborator

dcibot commented Feb 10, 2025

from change #553:

  • no check (not a code change)

Copy link

@thekad thekad added this pull request to the merge queue Feb 11, 2025
Merged via the queue into redhatci:main with commit 29ce4b1 Feb 11, 2025
7 checks passed
@thekad thekad deleted the root_device_hints branch February 11, 2025 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants