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

libpod, conmon: change log level for rootless #8726

Merged
merged 2 commits into from
Dec 15, 2020
Merged

libpod, conmon: change log level for rootless #8726

merged 2 commits into from
Dec 15, 2020

Conversation

giuseppe
Copy link
Member

Change the log level when running as rootless when moving conmon to a
different cgroup.

Closes: #8721

Signed-off-by: Giuseppe Scrivano [email protected]

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@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 15, 2020
@giuseppe giuseppe mentioned this pull request Dec 15, 2020
Closed
@rhatdan
Copy link
Member

rhatdan commented Dec 15, 2020

LGTM

edsantiago
edsantiago reviewed Dec 15, 2020
View reviewed changes
libpod/oci_conmon_linux.go Outdated
@@ -1433,6 +1433,14 @@ func (r *ConmonOCIRuntime) moveConmonToCgroupAndSignal(ctr *Container, cmd *exec
}

if mustCreateCgroup {
// Usually rootless users are not allowed to configure cgroupfs.
// There are cases though, where it is allowed, e.g. if the cgroup
// is manually configured and chowned). Avoid detecting all
Copy link
Member

Choose a reason for hiding this comment

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

Trailing whitespace is causing CI failure

@edsantiago edsantiago mentioned this pull request Dec 15, 2020
Merged
5 tasks
@giuseppe
libpod, conmon: change log level for rootless
08f76bf 
Change the log level when running as rootless when moving conmon to a
different cgroup.

Closes: containers#8721

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe giuseppe force-pushed the conmon-rootless-errs-to-info branch from 55e3185 to 08f76bf Compare December 15, 2020 17:56
@edsantiago
Copy link
Member

@cevich this is barfing trying to connect to mirror.chpc.utah.edu:443. That host is in

podman/contrib/cirrus/required_host_ports.txt

Line 8 in 999d40d

mirror.chpc.utah.edu 443
with the commit in question being #3832, which unfortunately only says "Confirm networking more" but gives no indication of why this host is important. Do you have any recollection of why this host is needed? Can we remove it?

@edsantiago
Copy link
Member

@giuseppe the string "chpc" does not appear anywhere else in the podman source tree, leading me to believe that this check is unnecessary. For purposes of getting CI to pass, I would recommend editing the above file and removing the chpc line, updating the commit message appropriately with something like "unrelated: remove unused mirror from list of required host/ports: the host is unreachable due to DNS misconfiguration, and it doesn't look like we need it for anything anyway"

@giuseppe
contrib: drop mirror.chpc.utah.edu:443
8bfba70 
remove unused mirror from list of required host/ports: the host is
unreachable due to DNS misconfiguration, and it doesn't look like we
need it for anything anyway.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@cevich
Copy link
Member

cevich commented Dec 15, 2020

Can we remove it?

Yes, drop it. It was a host mirroring something we use to depend on at runtime, likely a repo. for a package install or a container image.

@rhatdan
Copy link
Member

rhatdan commented Dec 15, 2020

/lgtm
/hold

@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 Dec 15, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2020
@cevich cevich mentioned this pull request Dec 15, 2020
Closed
@edsantiago
Copy link
Member

/hold cancel

@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 Dec 15, 2020
@openshift-merge-robot openshift-merge-robot merged commit e689503 into containers:master Dec 15, 2020
@edsantiago edsantiago mentioned this pull request Dec 15, 2020
Merged
@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

@edsantiago edsantiago edsantiago 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.

rootless: cgroups v1: Failed to add conmon to cgroupfs sandbox cgroup: error creating cgroup for SOMETHING
6 participants
@giuseppe @openshift-ci-robot @rhatdan @edsantiago @cevich @openshift-merge-robot