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

OCPBUGS-15259: Updates to ignition config and site-config handling #98

Merged

Conversation

donpenney
Copy link
Collaborator

This update provides the following enhancements and fixes, related to site-config and ignitionConfigOverride handling:

  • Fix ignition config file mode values - original code is using octal string, where field is a decimal value. This results in incorrect permissions being set, but coincidentally still executable
  • Drop the agent-fix override script, as the issue being addressed by the override has been resolved as of ACM 2.6.3
  • Eliminate the duplicated image extract scripts, in favour of a single script sourced in the repo that is renamed in the ignitionConfigOverride entries
  • Enhance update-site-config.sh dev utility to support merging prestaging hooks into existing ignitionConfigOverride entries
  • Add a new command "siteconfig" to "factory-precaching-cli" tool to merge prestaging hooks into a given site-config.yaml file

In support of the new "siteconfig" command, go-bindata has been added to generate GO code that incorporates the ignition config files.

Signed-off-by: Don Penney <dpenney@redhat.com>
Signed-off-by: Don Penney <dpenney@redhat.com>
As the content of the extract-ai.sh and extract-ocp.sh scripts are now
the same, this update replaces them with a single extract-images.sh as
the source code. The original names are maintained in the ignition
config files, as the name is used to differentiate which installation
stage is triggering the image extraction.

Signed-off-by: Don Penney <dpenney@redhat.com>
This commit rewrites the update-site-config.sh helper utility to use yq
and jq commands to merge the prestaging ignition configs and installer
args into a site-config, including purging prestaging entries that are
no longer required (ie. dropping override of agent-fix script) from the
site-config.

This also adds support for using a custom partition label.

Signed-off-by: Don Penney <dpenney@redhat.com>
This update introduces a new factory-precaching-cli command, siteconfig,
which takes an existing site-config yaml as input and adds the required
prestaging hooks. If the site-config already has ignitionConfigOverride
or installerArgs entries, the prestaging hooks are merged into the
existing values. Additionally, the tool looks for stale prestaging hooks
that may have changed or been dropped and prunes them from the
site-config.

To support this tool, the prestaging ignition config files are now
used to generate go-bindata source, requiring a developer to run the
"make update-bindata" command when updating these files.

Additionally, regression tests have been added to verify the siteconfig
command, along with truncated testmode ignition files to avoid requiring
test updates whenever the prestaging ignition files or image extract
script are updated.

Signed-off-by: Don Penney <dpenney@redhat.com>
@openshift-ci-robot
Copy link

@donpenney: This pull request references Jira Issue OCPBUGS-15259, which is invalid:

  • expected the bug to target the "4.14.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This update provides the following enhancements and fixes, related to site-config and ignitionConfigOverride handling:

  • Fix ignition config file mode values - original code is using octal string, where field is a decimal value. This results in incorrect permissions being set, but coincidentally still executable
  • Drop the agent-fix override script, as the issue being addressed by the override has been resolved as of ACM 2.6.3
  • Eliminate the duplicated image extract scripts, in favour of a single script sourced in the repo that is renamed in the ignitionConfigOverride entries
  • Enhance update-site-config.sh dev utility to support merging prestaging hooks into existing ignitionConfigOverride entries
  • Add a new command "siteconfig" to "factory-precaching-cli" tool to merge prestaging hooks into a given site-config.yaml file

In support of the new "siteconfig" command, go-bindata has been added to generate GO code that incorporates the ignition config files.

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/test-infra repository.

@openshift-ci openshift-ci bot requested review from browsell and tliu2021 July 7, 2023 13:29
@openshift-ci
Copy link

openshift-ci bot commented Jul 7, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: donpenney

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 Jul 7, 2023
@donpenney donpenney changed the title OCPBUGS-15259: OCPBUGS-15259: Updates to ignition config and site-config handling Jul 7, 2023
@donpenney
Copy link
Collaborator Author

/jira refresh

@openshift-ci-robot
Copy link

@donpenney: This pull request references Jira Issue OCPBUGS-15259, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.14.0) matches configured target version for branch (4.14.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (obochan@redhat.com), skipping review request.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

/jira refresh

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/test-infra repository.

Signed-off-by: Don Penney <dpenney@redhat.com>
@donpenney
Copy link
Collaborator Author

/cc @alosadagrande @nishant-parekh

#
function update_installer_args {
local installer_args='"--save-partlabel","'"${PARTITION_LABEL}"'"'
if ! yq "${NODE_INSTALLER_ARGS}" "${SITECFG}" | jq -cM | grep "${installer_args}"; then
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 safer to use jq to check if the option exists? Just in case the installer_args contain special characters

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you have some example in mind? As far as I know, installerArgs should be a json array of args passed to the installer. Grepping the compact format of this json array for the desired pair of args should be safe, I would think (although this could/should be "grep -q" here).

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of any examples with a special character that couldn't be handled by grep. This is a more general comment since the script deals with JSON data rather than text files. However, considering the specific use case and the nature of the data being processed, I'm fine with using grep in this context.

@tliu2021
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2023
@openshift-merge-robot openshift-merge-robot merged commit 98d9e22 into openshift-kni:main Jul 12, 2023
4 checks passed
@openshift-ci-robot
Copy link

@donpenney: Jira Issue OCPBUGS-15259: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-15259 has been moved to the MODIFIED state.

In response to this:

This update provides the following enhancements and fixes, related to site-config and ignitionConfigOverride handling:

  • Fix ignition config file mode values - original code is using octal string, where field is a decimal value. This results in incorrect permissions being set, but coincidentally still executable
  • Drop the agent-fix override script, as the issue being addressed by the override has been resolved as of ACM 2.6.3
  • Eliminate the duplicated image extract scripts, in favour of a single script sourced in the repo that is renamed in the ignitionConfigOverride entries
  • Enhance update-site-config.sh dev utility to support merging prestaging hooks into existing ignitionConfigOverride entries
  • Add a new command "siteconfig" to "factory-precaching-cli" tool to merge prestaging hooks into a given site-config.yaml file

In support of the new "siteconfig" command, go-bindata has been added to generate GO code that incorporates the ignition config files.

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/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants