Skip to content

[FCOS] non-virtual-ip: replace the script with podman wrapper#1478

Merged
openshift-merge-robot merged 1 commit intoopenshift:fcosfrom
vrutkovs:fcos-non-virtual-ip-container
Feb 28, 2020
Merged

[FCOS] non-virtual-ip: replace the script with podman wrapper#1478
openshift-merge-robot merged 1 commit intoopenshift:fcosfrom
vrutkovs:fcos-non-virtual-ip-container

Conversation

@vrutkovs
Copy link
Copy Markdown
Contributor

@vrutkovs vrutkovs commented Feb 14, 2020 •
edited
Loading

Replace Python script with a wrapper which runs the script in a podman container (using host networking).

Requires openshift/baremetal-runtimecfg#46
Fixes #1430

Master counterpart: #1521

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 14, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2020
mandre
mandre reviewed Feb 19, 2020
View reviewed changes
Comment thread templates/worker/00-worker/baremetal/files/baremetal-non-virtual-ip.yaml Outdated
Copy link
Copy Markdown
Member

@mandre mandre Feb 19, 2020

Choose a reason for hiding this comment

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

Have you tried without attaching a TTY? I believe we shouldn't need the -ti option.

@mandre
Copy link
Copy Markdown
Member

mandre commented Feb 19, 2020

This is going to conflict with #1483, can we wait until it merges?
I'd like to keep the port clean in #1483 and resolve the conflict in your PR if you don't mind.

@vrutkovs
Copy link
Copy Markdown
Contributor Author

vrutkovs commented Feb 19, 2020

This is going to conflict with #1483, can we wait until it merges?

fcos branch is independent of master, so we'll cherry-pick it back to master if the idea works

@mandre
Copy link
Copy Markdown
Member

mandre commented Feb 19, 2020

This is going to conflict with #1483, can we wait until it merges?

fcos branch is independent of master, so we'll cherry-pick it back to master if the idea works

Ah right, I forgot this PR was against a different branch. All good then.

@mandre
Copy link
Copy Markdown
Member

mandre commented Feb 21, 2020

This should be rebased after #1483.

Also, after re-syncing with master, you should leave out ovirt platform because they're still using the older version of non-virtual-ip script.

Note that there is now another python script nodeip-finder that should be moved to baremetal-runtimecfg the same way you did in openshift/baremetal-runtimecfg#46.

@vrutkovs
Copy link
Copy Markdown
Contributor Author

vrutkovs commented Feb 21, 2020

Also, after re-syncing with master, you should leave out ovirt platform because they're still using the older version of non-virtual-ip script.

Are these incompatible? I was hoping one script could be reused for all platoforms.
cc @rgolangh

@vrutkovs
Copy link
Copy Markdown
Contributor Author

vrutkovs commented Feb 21, 2020

Updated openshift/baremetal-runtimecfg#46.

I didn't change the wrapper in fcos branch, as its still lagging behind master (it would be rebased soon)

@vrutkovs
Copy link
Copy Markdown
Contributor Author

vrutkovs commented Feb 21, 2020

This now requires openshift/baremetal-runtimecfg#48 (since fcos branch promotes to 4.4 for now)

@vrutkovs
Copy link
Copy Markdown
Contributor Author

vrutkovs commented Feb 27, 2020

/retest

@vrutkovs
Copy link
Copy Markdown
Contributor Author

vrutkovs commented Feb 28, 2020

/cc @LorbusChris

This should fix oVirt / Openstack not joining worker nodes, PTAL

@celebdor
Copy link
Copy Markdown
Contributor

celebdor commented Feb 28, 2020

It looks good. I wonder how much delay it is introduced by podman pulling the images, but with a local registry it shouldn't be much.

@LorbusChris
Copy link
Copy Markdown
Contributor

LorbusChris commented Feb 28, 2020

is this a real concern tho? After all we deliver all of OpenShift's operators as container images as well

@vrutkovs
Copy link
Copy Markdown
Contributor Author

vrutkovs commented Feb 28, 2020

This script is used by NetworkManager, so potentially we may end up with misconfigured networking

@cgwalters
Copy link
Copy Markdown
Member

cgwalters commented Feb 28, 2020

Thanks a ton for doing this! This also solves the problem of having substantial baremetal code inside the MCO, which is a double win.

@LorbusChris
Copy link
Copy Markdown
Contributor

LorbusChris commented Feb 28, 2020

We should get this into master as well asap. Do we have OpenStack e2e tests here?
/approve

@vrutkovs
Copy link
Copy Markdown
Contributor Author

vrutkovs commented Feb 28, 2020

/hold

Wrong script names

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2020
@vrutkovs vrutkovs force-pushed the fcos-non-virtual-ip-container branch from ef9f043 to 2909c79 Compare February 28, 2020 13:35
@vrutkovs
Copy link
Copy Markdown
Contributor Author

vrutkovs commented Feb 28, 2020

/hold cancel

Update entrypoint, ready for review

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2020
@LorbusChris
Copy link
Copy Markdown
Contributor

LorbusChris commented Feb 28, 2020

xref master PR: #1521
Let's wait for e2e testing on master to complete before merging here.
/approve

@vrutkovs vrutkovs force-pushed the fcos-non-virtual-ip-container branch from 2909c79 to 552ef43 Compare February 28, 2020 16:54
non-virtual-ip: replace the script with podman wrapper
918fdc7 
Avoid running python scripts on host and use `podman run --net=host` instead

Cherrypicked to master as openshift#1521
@vrutkovs vrutkovs force-pushed the fcos-non-virtual-ip-container branch from 552ef43 to 918fdc7 Compare February 28, 2020 17:03
@LorbusChris
Copy link
Copy Markdown
Contributor

LorbusChris commented Feb 28, 2020

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2020
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented Feb 28, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LorbusChris, vrutkovs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [LorbusChris,vrutkovs]

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

@vrutkovs
Copy link
Copy Markdown
Contributor Author

vrutkovs commented Feb 28, 2020

/retest

@openshift-merge-robot openshift-merge-robot merged commit 1c5ad92 into openshift:fcos Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iamemilio iamemilio Awaiting requested review from iamemilio

@celebdor celebdor Awaiting requested review from celebdor

@Fedosin Fedosin Awaiting requested review from Fedosin

@yuqi-zhang yuqi-zhang Awaiting requested review from yuqi-zhang

@LorbusChris LorbusChris Awaiting requested review from LorbusChris

1 more reviewer

@mandre mandre mandre left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@LorbusChris LorbusChris

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.

7 participants

@vrutkovs @mandre @celebdor @LorbusChris @cgwalters @openshift-ci-robot @openshift-merge-robot