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

Support IPA running on top of CoreOS #188

Merged
merged 3 commits into from Jul 8, 2021
Merged

Support IPA running on top of CoreOS #188

merged 3 commits into from Jul 8, 2021

Conversation

dtantsur
Copy link
Member

@dtantsur dtantsur commented Jun 28, 2021 •
edited

  • Clean up [inspector]extra_kernel_params
  • Enable the custom-agent deploy interface
  • Allow configuring CoreOS IPA as a new entrypoint

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 28, 2021
@openshift-ci openshift-ci bot requested review from bfournie and russellb June 28, 2021 11:42
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 28, 2021
@dtantsur dtantsur changed the title [WIP] coreos-installer in ironic-image [WIP] allow configuring CoreOS IPA Jun 28, 2021
@dtantsur dtantsur force-pushed the coreos-installer branch 2 times, most recently from 8e616eb to 3ea9a8d Compare June 30, 2021 15:53
kirankt
kirankt reviewed Jun 30, 2021
View reviewed changes
scripts/coreos-ipa-common.sh

ROOTFS_FILE=/shared/html/images/ironic-python-agent.rootfs
IGNITION_FILE=/shared/html/ironic-python-agent.ign
ISO_FILE=/shared/html/images/ironic-python-agent.iso
Copy link

Choose a reason for hiding this comment

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

Can we make this configurable via argument to the script instead of a hardcoded FFILENAME?

Copy link

Choose a reason for hiding this comment

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

I'm actually not very particular about the FFILENAME here, but it does make sense that the name is ironic-python-agent because its a intermediate image. I'll apply the name change to the rhcos-downloader image and test them together.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think not supporting custom names here will make the diff smaller with the upstream version (which does hardcode ironic-python-agent).

Copy link

@kirankt kirankt Jul 1, 2021 •
edited

Choose a reason for hiding this comment

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

So...is the plan to still use the openshift fork of ironic? This has implications in the installer's startironic.sh script and in CBO for day-2 because the way ironic and its dependencies are started varies in metal3 vs. openshift versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

The openshift fork of ironic will be re-synced with metal3 soon(ish), once Riccardo comes back from PTO.

Copy link

Choose a reason for hiding this comment

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

there it is #190

@dtantsur dtantsur changed the title [WIP] allow configuring CoreOS IPA Allow configuring CoreOS IPA as a new entrypoint Jul 1, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 1, 2021
@dtantsur
Copy link
Member Author

dtantsur commented Jul 1, 2021

Tested the new entrypoint in isolation locally. Seems to work.

@dtantsur
Copy link
Member Author

dtantsur commented Jul 2, 2021

/retest

@dtantsur
Copy link
Member Author

dtantsur commented Jul 2, 2021

/test e2e-metal-ipi

@dtantsur
Clean up [inspector]extra_kernel_params
0e339a6 
This option should contain only parameters unique to inspection.
Standard kernel parameters go to boot interface configurations
(pxe_append_params, kernel_append_params).

Add missing [ilo] configuration since ilo-virtual-media is supported.

(cherry picked from commit 97eb7e4)
@dtantsur
Copy link
Member Author

dtantsur commented Jul 2, 2021

I've added two commits from metal3 that are required for this feature to work.

@dtantsur dtantsur changed the title Allow configuring CoreOS IPA as a new entrypoint Support IPA running on top of CoreOS Jul 2, 2021
@dtantsur
Copy link
Member Author

dtantsur commented Jul 2, 2021

/retest

1 similar comment
@dtantsur
Copy link
Member Author

dtantsur commented Jul 3, 2021

/retest

@elfosardo
Copy link

/test prevalidation-e2e-metal-ipi-virtualmedia-prevalidation

@elfosardo
Copy link

/test e2e-metal-ipi

@dtantsur
Allow configuring CoreOS IPA as a new entrypoint
77332d1 
Adds a new entrypoint /bin/configure-coreos-ipa to be run as an init
container after the images are downloaded.
@dtantsur
Copy link
Member Author

dtantsur commented Jul 6, 2021

/retest

1 similar comment
@dtantsur
Copy link
Member Author

dtantsur commented Jul 6, 2021

/retest

kirankt
kirankt reviewed Jul 6, 2021
View reviewed changes
scripts/runhttpd
@@ -25,6 +25,8 @@ else
INSPECTOR_EXTRA_ARGS=" ipa-inspection-callback-url=${IRONIC_BASE_URL}:5050/v1/continue"
fi

. /bin/coreos-ipa-common.sh
Copy link

Choose a reason for hiding this comment

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

This is a race condition. At least in the installer's startironic.sh script, the way to identify whether an image is fully downloaded, there exists a curl loop to check for finished file. Looks like this statement here is meant to add the kernel params, but when the httpd container is started, the images are not yet downloaded and the kernel params will not be set.

I did move the startup of the httpd container around in my installer fork as a test and it worked fine but it still feels racy.

https://github.com/kirankt/installer/blob/222922cab48056f85fb04716cb40591394200882/data/data/bootstrap/baremetal/files/usr/local/bin/startironic.sh.template#L161

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't all containers in a pod wait until initContainers are finished?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, they're not initContainers in this case. Still, won't this wait work? https://github.com/kirankt/installer/blob/222922cab48056f85fb04716cb40591394200882/data/data/bootstrap/baremetal/files/usr/local/bin/startironic.sh.template#L152

Copy link

@kirankt kirankt Jul 6, 2021 •
edited

Choose a reason for hiding this comment

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

You're right. That should catch it. But in practice the curl loops have been left in place to confirm whether the download is indeed finished. In my testing too, this has been the case.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean, the curl was required in your case? If so, that's a bug we should fix separately, I guess.

@dtantsur
Copy link
Member Author

dtantsur commented Jul 7, 2021

/retest

1 similar comment
@kirankt
Copy link

kirankt commented Jul 7, 2021

/retest

kirankt
kirankt approved these changes Jul 7, 2021
View reviewed changes
Copy link

@kirankt kirankt left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jul 7, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtantsur, kirankt

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

This was referenced Jul 7, 2021
Closed
Merged
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

10 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 4300542 into openshift:master Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elfosardo elfosardo elfosardo left review comments

@kirankt kirankt kirankt approved these changes

@bfournie bfournie Awaiting requested review from bfournie

@russellb russellb Awaiting requested review from russellb

Assignees

@kirankt kirankt

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@dtantsur @elfosardo @kirankt @openshift-bot @openshift-merge-robot