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

Allow setting sshkey and kernel params for debugging IPA #226

Merged
merged 2 commits into from
Dec 16, 2020
Merged

Allow setting sshkey and kernel params for debugging IPA #226

merged 2 commits into from
Dec 16, 2020

Conversation

derekhiggins
Copy link
Member

No description provided.

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekhiggins

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

@metal3-io-bot metal3-io-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 23, 2020
@derekhiggins
Copy link
Member Author

The previous PR to do this is here #150
but it went stale and was before we moved to jinja for the config

@knfoo knfoo mentioned this pull request Nov 30, 2020
Closed
@metal3-io-bot metal3-io-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 1, 2020
@derekhiggins
Copy link
Member Author

cc @maelk , includes a proposal to remove console=ttyS0 we had hardcoded in, and use IRONIC_KERNEL_PARAMS instead

@elfosardo
Copy link
Member

hey @derekhiggins are this method or any of the parameters documented anywhere?

@derekhiggins
Copy link
Member Author

hey @derekhiggins are this method or any of the parameters documented anywhere?

Nope, I'll add some. thanks

@maelk
Copy link
Member

maelk commented Dec 1, 2020

Would you mind creating a PR in BMO to maintain the current behaviour when merging this change ? There's two deployment methods in BMO, through ironic-deployment folder and with tools/run_local_ironic.sh. both refer to a configmap that need to be updated . There is also one in metal3-dev-env, in the 03_* script. Would you be ok modifying those ? thank you

@derekhiggins
Copy link
Member Author

Would you mind creating a PR in BMO to maintain the current behaviour when merging this change ? There's two deployment methods in BMO, through ironic-deployment folder and with tools/run_local_ironic.sh. both refer to a configmap that need to be updated . There is also one in metal3-dev-env, in the 03_* script. Would you be ok modifying those ? thank you

Yup, my intention is that it would only be maintained for virtual environments, I'll update those

@derekhiggins derekhiggins changed the title Allow setting sshkey for debugging IPA Allow setting sshkey and kernel params for debugging IPA Dec 3, 2020
@stbenjam
Copy link
Member

stbenjam commented Dec 3, 2020

/test-integration

@maelk
Copy link
Member

maelk commented Dec 3, 2020

/hold until the behaviour is maintained via PR in other repos

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 3, 2020
This was referenced Dec 4, 2020
Merged
Merged
dtantsur
dtantsur reviewed Dec 9, 2020
View reviewed changes
runhttpd.sh
@@ -26,7 +26,7 @@ else
fi

# Copy files to shared mount
cp /tmp/inspector.ipxe /shared/html/inspector.ipxe
render_j2_config /tmp/inspector.ipxe.j2 /shared/html/inspector.ipxe
Copy link
Member

Choose a reason for hiding this comment

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

Handling of options below should be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you elaborate on this, I'm not sure what you mean?

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, github does not allow me to comment on the code a bit below. It has the remaining bits of the old approach to templating, they should be removed.

Copy link
Member Author

@derekhiggins derekhiggins Dec 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

cp /tmp/dualboot.ipxe /shared/html/dualboot.ipxe
cp /tmp/uefi_esp.img /shared/html/uefi_esp.img

these lines? Those aren't templates

@dtantsur dtantsur mentioned this pull request Dec 9, 2020
Closed
@stbenjam
Copy link
Member

Needs rebase. metal3-io/baremetal-operator#736 should land soon. Anything else we need to keep current behaviour?

@derekhiggins
Allow setting sshkey for debugging IPA
160b694
@derekhiggins
Add env variable to set IPA kernel params
f41e88f 
Add IRONIC_KERNEL_PARAMS to set extra kernel params.
Also remove console=ttyS0 as it can now be set with IRONIC_KERNEL_PARAMS
where needed. Having it hardcoded results in Nothing being sent to the
VGA console in real baremetal environments, making it difficult to debug
problems.
@derekhiggins
Copy link
Member Author

Needs rebase. metal3-io/baremetal-operator#736 should land soon. Anything else we need to keep current behaviour?

rebase done, I'm not aware of anything else yet to merge to keep current behaviour.

@stbenjam
Copy link
Member

/test-integration

maelk
maelk reviewed Dec 15, 2020
View reviewed changes
Copy link
Member

@maelk maelk left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2020
@stbenjam
Copy link
Member

/hold cancel

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2020
@metal3-io-bot metal3-io-bot merged commit fd22729 into metal3-io:master Dec 16, 2020
elfosardo pushed a commit to elfosardo/ironic-image that referenced this pull request Oct 22, 2021
@openshift-merge-robot
Merge pull request metal3-io#226 from elfosardo/fix-long-name
a682c3a 
Bug 2014630: Update ironic to fix image provisioning fails with file name too long
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtantsur dtantsur dtantsur left review comments

@maelk maelk maelk left review comments

Assignees
No one assigned
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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@derekhiggins @metal3-io-bot @elfosardo @maelk @stbenjam @dtantsur