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

Initial support for ironic-standalone-operator #1466

Merged
merged 3 commits into from
Nov 27, 2024
Merged

Initial support for ironic-standalone-operator #1466

merged 3 commits into from
Nov 27, 2024

Conversation

dtantsur
Copy link
Member

@dtantsur dtantsur commented Oct 29, 2024 •
edited
Loading

Installs ironic-standalone-operator by default (but does not use it).

Adds a new option USE_IRSO (default false) to use the operator to provision Ironic.

Signed-off-by: Dmitry Tantsur dtantsur@protonmail.com

@metal3-io-bot
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@metal3-io-bot metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 29, 2024
@metal3-io-bot metal3-io-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 29, 2024
@dtantsur dtantsur force-pushed the ironic-standalone-operator branch 2 times, most recently from c94ffab to 76c7680 Compare October 30, 2024 11:17
@metal3-io-bot metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 30, 2024
@dtantsur dtantsur force-pushed the ironic-standalone-operator branch from 76c7680 to 2f0662b Compare October 30, 2024 13:02
@dtantsur dtantsur force-pushed the ironic-standalone-operator branch from 2f0662b to 60f36fd Compare November 6, 2024 16:32
dtantsur added a commit to dtantsur/ironic-standalone-operator that referenced this pull request Nov 6, 2024
@dtantsur
Add an environment file and a configmap to customize the manager
6e9c4fb 
Doing so simplifies downstream customizations, e.g. in metal3-dev-env:
metal3-io/metal3-dev-env#1466.

Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
@dtantsur dtantsur mentioned this pull request Nov 6, 2024
Merged
@dtantsur
Copy link
Member Author

dtantsur commented Nov 7, 2024

/test metal3-dev-env-integration-test-ubuntu-main

Should be solved now

tuminoid
tuminoid reviewed Nov 7, 2024
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

Its WIP, but two tiny nits, and one question.

The whole file needs cleanup on Bash so not picking on any of that, this PR is following convention just fine.

lib/common.sh Show resolved Hide resolved
03_launch_mgmt_cluster.sh Outdated Show resolved Hide resolved
03_launch_mgmt_cluster.sh Outdated Show resolved Hide resolved
@dtantsur dtantsur force-pushed the ironic-standalone-operator branch from 60f36fd to d615a9f Compare November 7, 2024 15:38
@metal3-io-bot metal3-io-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 7, 2024
@dtantsur dtantsur force-pushed the ironic-standalone-operator branch 5 times, most recently from 9ec2532 to e7c25d5 Compare November 8, 2024 16:22
@metal3-io-bot metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 8, 2024
@dtantsur dtantsur force-pushed the ironic-standalone-operator branch 3 times, most recently from 9720d0d to bd4eb00 Compare November 18, 2024 17:03
@dtantsur dtantsur force-pushed the ironic-standalone-operator branch 2 times, most recently from 1dcba20 to c4d9b87 Compare November 23, 2024 14:05
@dtantsur
Copy link
Member Author

/test metal3-centos-e2e-integration-test-release-1-8

@dtantsur dtantsur force-pushed the ironic-standalone-operator branch from d20194d to fe232f7 Compare November 25, 2024 12:29
@dtantsur
Copy link
Member Author

/test metal3-centos-e2e-integration-test-release-1-8

@tuminoid
Copy link
Member

It looks like you've made good progress with the dev-env part and it is now actually failing mostly to IRSO implementation. Does that sound about right?

@dtantsur
Copy link
Member Author

It looks like you've made good progress with the dev-env part and it is now actually failing mostly to IRSO implementation. Does that sound about right?

Not exactly. The previous couple of failures are related to gluing BMO and newly deployed Ironic together. I presume the last failure was because of authentication, but without metal3-io/baremetal-operator#2083 (now merged) I could not confirm that.

@dtantsur
Copy link
Member Author

Okay, correcting: the very last failure may actually be a bug in IrSO.

@dtantsur dtantsur force-pushed the ironic-standalone-operator branch from fe232f7 to a029bd7 Compare November 25, 2024 14:29
@dtantsur
Copy link
Member Author

/test metal3-centos-e2e-integration-test-release-1-8

tuminoid
tuminoid reviewed Nov 25, 2024
View reviewed changes
lib/common.sh Outdated Show resolved Hide resolved
@dtantsur
Copy link
Member Author

Yay, it works (fails only on the re-pivoting test).

@dtantsur dtantsur force-pushed the ironic-standalone-operator branch from a029bd7 to 83a4e1b Compare November 26, 2024 15:21
@dtantsur
Copy link
Member Author

/test metal3-centos-e2e-integration-test-release-1-8

@dtantsur
Allow using ironic-standalone-operator to provision Ironic
bb69cf9 
It works, but does not pass the re-pivoting test in the CAPM3 suite.

Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
@dtantsur dtantsur force-pushed the ironic-standalone-operator branch 2 times, most recently from 8ddc395 to bb69cf9 Compare November 27, 2024 07:24
@dtantsur dtantsur changed the title WIP add support for ironic-standalone-operator Initial support for ironic-standalone-operator Nov 27, 2024
@dtantsur dtantsur marked this pull request as ready for review November 27, 2024 07:25
@metal3-io-bot metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 27, 2024
@dtantsur
Copy link
Member Author

/test metal3-centos-e2e-integration-test-release-1-8
/test metal3-dev-env-integration-test-ubuntu-main

tuminoid
tuminoid reviewed Nov 27, 2024
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

Completely optional nits. Looking solid, with documented variables and generally clean addition.

This will help greatly everyone to start experimenting with IRSO!

/approve

03_launch_mgmt_cluster.sh
}

launch_ironic_via_irso() {
if [ "${IRONIC_BASIC_AUTH}" != "true" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if [ "${IRONIC_BASIC_AUTH}" != "true" ]; then
if [[ "${IRONIC_BASIC_AUTH}" != "true" ]]; then

03_launch_mgmt_cluster.sh
function update_component_image(){
IMPORT=$1
ORIG_IMAGE=$2
function get_component_image(){
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function get_component_image(){
get_component_image() {

03_launch_mgmt_cluster.sh
#
# Update the CAPM3 and BMO manifests to use local images as defined in variables
#
function update_component_image(){
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function update_component_image(){
update_component_image() {

config_example.sh
@@ -213,3 +213,6 @@
# To enable FakeIPA and run dev-env on a fake platform
# export NODES_PLATFORM="fake"
# export FAKE_IPA_IMAGE=192.168.111.1:5000/localimages/fake-ipa

# Whether to use ironic-standalone-operator to deploy Ironic.
# export USE_IRSO="true"
Copy link
Member

Choose a reason for hiding this comment

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

I think this config is supposed to have default values shown, but quickly checking others, we have already mixed it. Up to you which one you want to have here.

@metal3-io-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tuminoid

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 27, 2024
@dtantsur
Copy link
Member Author

Completely optional nits

I wonder if I should fix them in a follow-up or just wait for your reformatting of this file?

@tuminoid
Copy link
Member

Completely optional nits

I wonder if I should fix them in a follow-up or just wait for your reformatting of this file?

If the tests pass, I can fix them later, or if you have any other need to push fixes to this PR, fix them as you go?

@elfosardo
Copy link
Member

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 27, 2024
@metal3-io-bot metal3-io-bot merged commit e50c71a into metal3-io:main Nov 27, 2024
6 checks passed
@dtantsur dtantsur deleted the ironic-standalone-operator branch November 27, 2024 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tuminoid tuminoid tuminoid left review comments

@kashifest kashifest Awaiting requested review from kashifest

@smoshiur1237 smoshiur1237 Awaiting requested review from smoshiur1237

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dtantsur @metal3-io-bot @tuminoid @elfosardo