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

RHEL: Do not pull rootless CNI infra image, and instead request that user build it. #8671

Merged
merged 2 commits into from
Jan 5, 2021
Merged

RHEL: Do not pull rootless CNI infra image, and instead request that user build it. #8671

merged 2 commits into from
Jan 5, 2021

Conversation

mheon
Copy link
Member

@mheon mheon commented Dec 9, 2020

Instead, we want to advise users to manually build the image. We cannot distribute the existing image for RHEL 8.3.1, and the feature will be tech preview, so this degraded user experience will have to be sufficient until we can get a better solution in place.

Ref: https://issues.redhat.com/browse/RUN-1127

Please note that this is a RHEL only change and should not be included in non-RHEL branches.

Also, cherry-pick #8652 into the RHEL tree.

@mheon
Correct port range logic for port generation
26b3357 
The existing logic (Range > 0) always triggered, because range is
guaranteed to be at least 1 (a single port has a range of 1, a
two port range (e.g. 80-81) has a range of 2, and so on). As such
this could cause ports that had a host port assigned to them by
the user to randomly assign one instead.

Fixes containers#8650
Fixes containers#8651

Signed-off-by: Matthew Heon <[email protected]>
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 9, 2020
@mheon
Copy link
Member Author

mheon commented Dec 9, 2020

@baude @TomSweeneyRedHat @rhatdan PTAL

@rhatdan
Copy link
Member

rhatdan commented Dec 9, 2020

@mheon Looks like you have a PR in here that is not supposed to be here on Ports.

@mheon
Copy link
Member Author

mheon commented Dec 9, 2020

@rhatdan Deliberate cherry-pick - that's a pretty significant bugfix we should get into RHEL. We're not frozen yet, so I grabbed it.

@rhatdan
Copy link
Member

rhatdan commented Dec 9, 2020

LGTM

TomSweeneyRedHat
TomSweeneyRedHat reviewed Dec 9, 2020
View reviewed changes
libpod/rootless_cni_linux.go Outdated
if err != nil {
if errors.Cause(err) == define.ErrNoSuchImage {
return nil, errors.Errorf("rootless CNI infra image not present - please build image from https://github.com/containers/podman/blob/v2.2.1/contrib/rootless-cni-infra/Containerfile and tag as %q", imageName)
Copy link
Member

Choose a reason for hiding this comment

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

I know it's long already, but could we give them the command to use to build the image instead of just the Containerfile?

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 don't think so - the actual build process is getting very complicated because we can't just do a podman build $URL (you need to pull down not just the containerfile, but the directory containing it, and then do a podman build on that)

Copy link
Member Author

Choose a reason for hiding this comment

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

We have a separate card to document this elsewhere, which is where I think that sort of documentation will need to land.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about letting Podman itself execute git clone ... && git checkout ... && podman build -t rootless-cni-infra .

Copy link
Member Author

Choose a reason for hiding this comment

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

Problem is disconnected environments - we can't guarantee the system has a working connection to the internet.

@mheon mheon force-pushed the rhel_no_pull_infra branch 2 times, most recently from 0a596dd to fefb663 Compare December 11, 2020 18:39
@vrothberg
Copy link
Member

@AkihiroSuda PTAL.

At some point we'd love to ship multi-arch images but the local build seems like the best way forward for now.

@vrothberg
Copy link
Member

@AkihiroSuda PTAL.

At some point we'd love to ship multi-arch images but the local build seems like the best way forward for now.

Ah, that's a RHEL-only branch

AkihiroSuda
AkihiroSuda reviewed Dec 14, 2020
View reviewed changes
test/e2e/network_create_test.go Outdated
@@ -74,6 +74,7 @@ var _ = Describe("Podman network create", func() {
)

BeforeEach(func() {
SkipIfRootless("rootless CNI is tech preview in 8.3.1")
Copy link
Collaborator

Choose a reason for hiding this comment

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

8.3.1 -> RHEL 8.3.1

@AkihiroSuda
Copy link
Collaborator

Ref: https://issues.redhat.com/browse/RUN-1127

I don't have visibility in this URL, but I'm wondering Red Hat could build and upload the infra image by themselves if they have concern in the image built outside Red Hat.

@vrothberg
Copy link
Member

Ref: https://issues.redhat.com/browse/RUN-1127

I don't have visibility in this URL, but I'm wondering Red Hat could build and upload the infra image by themselves if they have concern in the image built outside Red Hat.

I think that's the long-term goal. Thanks for checking!

@AkihiroSuda AkihiroSuda mentioned this pull request Dec 14, 2020
Closed
@mheon mheon force-pushed the rhel_no_pull_infra branch from fefb663 to c2ad5a8 Compare January 4, 2021 19:48
@mheon
Copy link
Member Author

mheon commented Jan 4, 2021

Re-pushed. Added some extra docs to the README on Github about building, which should help people get started.

@rhatdan @TomSweeneyRedHat PTAL

@mheon
Do not pull rootless CNI infra image
4007554 
Instead, we want to advise users to manually build the image. We
cannot distribute the existing image for RHEL 8.3.1, and the
feature will be tech preview, so this degraded user experience
will have to be sufficient until we can get a better solution in
place.

Ref: https://issues.redhat.com/browse/RUN-1127

Please note that this is a RHEL only change and should not be
included in non-RHEL branches.

Signed-off-by: Matthew Heon <[email protected]>
@mheon mheon force-pushed the rhel_no_pull_infra branch from c2ad5a8 to 4007554 Compare January 4, 2021 20:55
@mheon
Copy link
Member Author

mheon commented Jan 4, 2021

Good to merge

vrothberg
vrothberg reviewed Jan 5, 2021
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@mheon
Copy link
Member Author

mheon commented Jan 5, 2021

@containers/podman-maintainers PTAL

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AkihiroSuda, mheon

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

@rhatdan
Copy link
Member

rhatdan commented Jan 5, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 5, 2021
@openshift-merge-robot openshift-merge-robot merged commit 14c35f6 into containers:v2.2.1-rhel Jan 5, 2021
@mheon mheon mentioned this pull request Mar 1, 2021
Merged
mheon added a commit to mheon/libpod that referenced this pull request Mar 1, 2021
@mheon
Do not pull rootless CNI infra image
ab5ed82 
Instead, we want to advise users to manually build the image. We
cannot distribute the existing image for RHEL 8.4.0, and the
feature will be tech preview, so this degraded user experience
will have to be sufficient until we can get a better solution in
place.

Ref: https://issues.redhat.com/browse/RUN-1127

Please note that this is a RHEL only change and should not be
included in non-RHEL branches.

This is a forward-port of the original changes in containers#8671 to the
new v3.0.1-rhel branch.

Signed-off-by: Matthew Heon <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@vrothberg vrothberg vrothberg left review comments

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees

@rhatdan rhatdan

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. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mheon @rhatdan @vrothberg @AkihiroSuda @openshift-ci-robot @TomSweeneyRedHat @openshift-merge-robot