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

Revert "Add set -e to all the entrypoint scripts" #83

Merged
merged 1 commit into from
Jul 30, 2019

Conversation

derekhiggins
Copy link
Member

@derekhiggins derekhiggins commented Jul 30, 2019

(updated to revert)
We have always been ignoring errors from the iptables
commands(on master nodes). Revert this while we figure
out a way to eliminate them altogether.

This reverts commit 7e7eb9a.

@dtantsur
Copy link
Member

Shouldn't we just revert the change to add set -e?

We have always been ignoring errors from the iptables
commands(on master nodes). Revert this while we figure
out a way to eliminate them altogether.

This reverts commit 7e7eb9a.
@russellb
Copy link
Member

I actually think iptables commands should be removed. They aren’t needed in the cluster. These should just be documented as network requirements for this image instead.

@russellb
Copy link
Member

I’m Ok merging this as a quick fix first but would like to follow up on removal

@derekhiggins
Copy link
Member Author

I’m Ok merging this as a quick fix first but would like to follow up on removal

ya, i suspect we dont need them at all also but want to make sure @hardys doesn't rely on them in his patch to move ironic to the bootstrap node before removing them.

@derekhiggins derekhiggins changed the title Ignore iptables errors Revert "Add set -e to all the entrypoint scripts" Jul 30, 2019
@metal3ci
Copy link

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/963/

@metal3ci
Copy link

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/964/

@russellb russellb merged commit 61e8c8a into metal3-io:master Jul 30, 2019
@russellb
Copy link
Member

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/964/

Ironic had to work on the provisioning host at least to get this far

@metal3ci
Copy link

Build SUCCESS, see build http://10.8.144.11:8080/job/dev-tools/967/

@hardys
Copy link
Member

hardys commented Jul 31, 2019

Note we do need some iptables rules for both the bootstrap VM and cluster case (I set some in the startironic.sh script but removing these may mean more rules are needed there cc @stbenjam )

Then on the cluster we need something to set rules that enable IPA access to the ironic/inspector API, tftp, http etc so that will need to happen somewhere else if we remove from the container images, cc @imain )

@hardys
Copy link
Member

hardys commented Jul 31, 2019

Also note we should consider re adding the set -e for the conductor start script, or explicitly check to ensure the dB sync worked, otherwise the systemd service on the bootstrap VM looks active, but can be failed in the case where mariadb isn't ready and the sync silently fails, we then start the conductor anyway

iurygregory pushed a commit to iurygregory/ironic-image that referenced this pull request Jul 16, 2020
Update inspection configuration to enable fast-track
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants