-
Notifications
You must be signed in to change notification settings - Fork 766
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
stage_executor: inline --network
add default
string
#4659
Conversation
@nalind @rhatdan @giuseppe @vrothberg @containers/buildah-maintainers PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LGTM |
c7eaa9f
to
ddd3c67
Compare
@TomSweeneyRedHat @vrothberg done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, giuseppe 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 |
bd4e5e5
to
bfff229
Compare
tests/bud.bats
Outdated
@@ -43,6 +43,13 @@ load helpers | |||
expect_output --substring "unsupported value" | |||
} | |||
|
|||
@test "build with inline RUN --network=default" { | |||
run_buildah build --network=host $WITH_POLICY_JSON -t source -f $BUDFILES/inline-network/Dockerfile4 | |||
build1="$output" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be the same as the network namespace that this test itself is running in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
tests/bud.bats
Outdated
run_buildah build --network=host $WITH_POLICY_JSON -t source -f $BUDFILES/inline-network/Dockerfile4 | ||
build1="$output" | ||
run_buildah build --network=private $WITH_POLICY_JSON -t source -f $BUDFILES/inline-network/Dockerfile4 | ||
assert "$output" !~ "$build1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and running with --network=private
should get a result that's both different from the --network=host
test and the previous --network=private
, unless I'm mistaken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean different with the previous --network=host
and another --network=private
? Yes I think two different builds should have different namespaces.
I can add two builds with --network=private
and compare them if you mean that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I was suggesting, since we're creating a new network namespace each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assert
probably needs to be changed - it's comparing the entire output from the previous build to the entire output of this one. Since we're not caching, I'd expect to get different image IDs in those outputs, at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems buildah re-uses private namespace and only assigns new private namespace when other one is occupied, so this happens only when two private builds are parallel otherwise consecutive private build ends up getting same private namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's surprising to me, but I'm seeing it when I run unshare -Urnmpf readlink /proc/self/ns/net
twice in succession, so the takeaway is that it's something the kernel's doing, and I was wrong to think that they'd be different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah i see its the kernel, anyways I have dropped this part from the test, only comparing a host
and private
ns build.
@flouthoc needs a rebase and it looks like the tests are still barking. |
@flouthoc the tests are still spitting up a bit. |
@TomSweeneyRedHat Looks like a flake i have restarted. |
Inline `RUN --network=default` should do nothing. This matches buildkit's behaviour here: https://github.com/moby/buildkit/blob/master/frontend/dockerfile/docs/reference.md#run---networkdefault Also added to podman's document here: containers/common#1366 Signed-off-by: Aditya R <arajan@redhat.com>
LGTM, give or take a rebase. |
/lgtm |
Inline
RUN --network=default
should do nothing. This matches buildkit's behaviour here: https://github.com/moby/buildkit/blob/master/frontend/dockerfile/docs/reference.md#run---networkdefaultAlso added to podman's document here: containers/common#1366
What type of PR is this?
What this PR does / why we need it:
How to verify it
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?