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

Merge ironic and ironic-inspector repositories #179

Merged

Conversation

elfosardo
Copy link

This change moves Dockerfile, configuration and scripts from the
ironic-inspector container image repository to the ironic one.

This approach provides one Dockerfile that builds an image that contains
all the services necessary to run both ironic and ironic-inspector.
The scripts and configurations are kept separated for the time being.

Those changes will need to be reflected in CI as well.

@openshift-ci openshift-ci bot requested review from hardys and russellb June 9, 2021 15:21
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2021
@elfosardo
Copy link
Author

/test prevalidation-e2e-metal-ipi-prevalidation

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2021
@elfosardo
Copy link
Author

/retest

@dtantsur
Copy link
Member

I'm quite sure we'll need to do it together with some CBO changes.

@elfosardo
Copy link
Author

I'm quite sure we'll need to do it together with some CBO changes.

I don't think so, it doesn't change anything on ironic side, I'm just moving inspector config from the inspector repo to this one here
The change on CBO and BMO should happen after this has merged

@elfosardo
Copy link
Author

/test prevalidation-e2e-metal-ipi-prevalidation

1 similar comment
@elfosardo
Copy link
Author

/test prevalidation-e2e-metal-ipi-prevalidation

@elfosardo
Copy link
Author

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

@elfosardo
Copy link
Author

/test prevalidation-e2e-metal-ipi-prevalidation

@elfosardo
Copy link
Author

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

@elfosardo
Copy link
Author

/test prevalidation-e2e-metal-ipi-prevalidation

@elfosardo
Copy link
Author

/retest

1 similar comment
@elfosardo
Copy link
Author

/retest

@elfosardo
Copy link
Author

/test prevalidation-e2e-metal-ipi-prevalidation

@elfosardo
Copy link
Author

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

@elfosardo
Copy link
Author

/test prevalidation-e2e-metal-ipi-prevalidation

@elfosardo
Copy link
Author

/test prevalidation-e2e-metal-ipi-prevalidation

@elfosardo
Copy link
Author

/retest

1 similar comment
@elfosardo
Copy link
Author

/retest

do
until ls "${LOG_DIR}"/*.tar.gz 1> /dev/null 2>&1
do
sleep 5

Choose a reason for hiding this comment

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

We could potentially get stuck in this loop checking one directory and not swapping back to check the other.
e.g. if we are just redeploying nodes that have been previously inspected. Then this will get stuck waiting for a file
in /shared/log/ironic-inspector/ramdisk and never return to /shared/log/ironic/deploy

Copy link
Author

Choose a reason for hiding this comment

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

I realize that, maybe we can do it as a follow-up?
we need to fix it first in https://github.com/metal3-io/ironic-image/blob/master/scripts/runlogwatch.sh

Choose a reason for hiding this comment

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

ack,

@@ -1,20 +1,24 @@

Choose a reason for hiding this comment

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

I think this newline might be the reason the CI tests are failing

Looking at the CI job logs, I noticed this to be failing
failed: (1.3s) 2021-06-18T10:05:50 "[sig-installer][Feature:baremetal] Baremetal platform should have a metal3 deployment [Suite:openshift/conformance/parallel]"

which fails if the replica set for the metal3 pod isn't 1
https://github.com/openshift/origin/blob/d389c2654/test/extended/baremetal/hosts.go#L72-L74

Looking at the must-gather, I could see that the "ironic-deploy-ramdisk-logs" was restarted 27 times, the logs for this container have an error

$ cat metal3-5444948969-w98zp/ironic-deploy-ramdisk-logs/ironic-deploy-ramdisk-logs/logs/current.log
2021-06-18T10:50:25.394447661Z standard_init_linux.go:219: exec user process caused: exec format error

Copy link
Author

Choose a reason for hiding this comment

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

oh snap, I didn't see that! thanks!

This change moves Dockerfile, configuration and scripts from the
ironic-inspector container image repository to the ironic one.

This approach provides one Dockerfile that builds an image that contains
all the services necessary to run both ironic and ironic-inspector.
The scripts and configurations are kept separated for the time being.

Those changes will need to be reflected in CI as well.
@derekhiggins
Copy link

/lgtm
Doesn't look like the prevalidation-e2e-metal-ipi-prevalidation failure is ironic related

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

openshift-ci bot commented Jun 21, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekhiggins, elfosardo

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:
  • OWNERS [derekhiggins,elfosardo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@derekhiggins
Copy link

/test prevalidation-e2e-metal-ipi-prevalidation

@openshift-bot
Copy link

/retest

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

2 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-merge-robot openshift-merge-robot merged commit ea1e99f into openshift:master Jun 21, 2021
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/ironic-image that referenced this pull request Oct 25, 2022
Set the hash ring algorithm to SHA256
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

5 participants