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

Kube like pods should share ipc,net,uts by default #10268

Merged
merged 1 commit into from
May 10, 2021
Merged

Kube like pods should share ipc,net,uts by default #10268

merged 1 commit into from
May 10, 2021

Conversation

flouthoc
Copy link
Collaborator

@flouthoc flouthoc commented May 7, 2021 •
edited
Loading

podman play kube ignores sharing ipc,net,uts namespaces whenever shareProcessNamespace: True is specified. However it works fine when shareProcessNamespace is not defined at all. While kube like pods must share ipc,net,uts in both cases.

Ref Issue: #9128

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: flouthoc
To complete the pull request process, please assign tomsweeneyredhat after the PR has been reviewed.
You can assign the PR to them by writing /assign @tomsweeneyredhat in a comment when ready.

The full list of commands accepted by this bot can be found 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

@flouthoc flouthoc force-pushed the kube-default-shared-namespace branch from 5feb478 to 6c614d0 Compare May 7, 2021 19:30
@flouthoc flouthoc mentioned this pull request May 7, 2021
Closed
@flouthoc flouthoc force-pushed the kube-default-shared-namespace branch 3 times, most recently from 9f7be20 to e11fb9a Compare May 8, 2021 07:34
@zhangguanzhang
Copy link
Collaborator

should use SkipIfRootless()

@flouthoc flouthoc force-pushed the kube-default-shared-namespace branch from e11fb9a to 99dd454 Compare May 8, 2021 09:01
@zhangguanzhang
Copy link
Collaborator

Close: #9128

@flouthoc
Copy link
Collaborator Author

flouthoc commented May 8, 2021 •
edited
Loading

@zhangguanzhang Just waiting for tests to pass. Then i guess we could request to close this issue.

@flouthoc
Copy link
Collaborator Author

flouthoc commented May 8, 2021

Checking why this test is failing.

@flouthoc flouthoc marked this pull request as draft May 8, 2021 09:59
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2021
@flouthoc flouthoc force-pushed the kube-default-shared-namespace branch 3 times, most recently from 70c6f12 to e862855 Compare May 8, 2021 11:56
@flouthoc flouthoc marked this pull request as ready for review May 8, 2021 13:06
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2021
@flouthoc
Copy link
Collaborator Author

flouthoc commented May 8, 2021

@zhangguanzhang @mheon @rhatdan Above test is fixed issue was with registry url but now this existing test is failing podman cp the root directory from the ctr to an existing directory on the host which seems to be unrelated to this commit.

Is this an issue with upstream or a flake ?

@flouthoc flouthoc force-pushed the kube-default-shared-namespace branch from e862855 to c105f2b Compare May 8, 2021 13:28
@flouthoc
Copy link
Collaborator Author

flouthoc commented May 8, 2021

Doing a rebase with master to confirm if this is not a flake.

@flouthoc
Copy link
Collaborator Author

flouthoc commented May 8, 2021

@zhangguanzhang This is fixed :)

TomSweeneyRedHat
TomSweeneyRedHat reviewed May 8, 2021
View reviewed changes
test/e2e/play_kube_test.go
inspect := podmanTest.Podman([]string{"inspect", "testpod1", "--format", "'{{ .SharedNamespaces }}'"})
inspect.WaitWithDefaultTimeout()
sharednamespaces := inspect.OutputToString()
Expect(sharednamespaces).To(ContainSubstring("ipc"))
Copy link
Member

Choose a reason for hiding this comment

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

can you add tests for uts and net too, or am I being oblivious and I'm missing them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@TomSweeneyRedHat You are right I missed checking string for uts and net. Thanks for pointing this out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah seems like CI flake is still there will open/close PR to re-kick CI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@TomSweeneyRedHat this is resolved.

Copy link
Collaborator

Choose a reason for hiding this comment

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

also need pid namespace

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@zhangguanzhang Bug was with only ipc, net, uts but sure case should also pass with pid

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@zhangguanzhang @TomSweeneyRedHat Added pid as well let me know if anything else needs to be done.

I think rdoproject.org/github-check is a flake , repush should fix it. But let me know if this check is mandatory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@zhangguanzhang ah it was just a flake force pushed made checks happy again.

@flouthoc flouthoc force-pushed the kube-default-shared-namespace branch 2 times, most recently from 50a91a8 to 70996cc Compare May 8, 2021 19:30
@flouthoc flouthoc closed this May 9, 2021
@flouthoc flouthoc reopened this May 9, 2021
@flouthoc flouthoc force-pushed the kube-default-shared-namespace branch 3 times, most recently from b2eb143 to 1415278 Compare May 9, 2021 04:38
@flouthoc flouthoc force-pushed the kube-default-shared-namespace branch 3 times, most recently from 135898a to 1b90fcd Compare May 10, 2021 04:09
@flouthoc flouthoc force-pushed the kube-default-shared-namespace branch from 1b90fcd to 14a1a45 Compare May 10, 2021 07:11
@flouthoc
Copy link
Collaborator Author

@zhangguanzhang @TomSweeneyRedHat this is ready for review all the comments seems to be resolved. CI is also happy now.

@rhatdan
Copy link
Member

rhatdan commented May 10, 2021

@haircommander @umohnani8 PTAL

@haircommander
Copy link
Collaborator

LGTM, thanks @flouthoc

@rhatdan
Copy link
Member

rhatdan commented May 10, 2021

/approve
/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 10, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, rhatdan

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

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 10, 2021
@openshift-merge-robot openshift-merge-robot merged commit 195895e into containers:master May 10, 2021
@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

@zhangguanzhang zhangguanzhang zhangguanzhang left review comments

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
@flouthoc @openshift-ci-robot @zhangguanzhang @rhatdan @haircommander @TomSweeneyRedHat @openshift-merge-robot