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

Document containers.conf settings for remote connections #8285

Merged
merged 1 commit into from
Nov 20, 2020
Merged

Document containers.conf settings for remote connections #8285

merged 1 commit into from
Nov 20, 2020

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Nov 9, 2020

Currently we don't document which end of the podman-remote client server
operations uses the containers.conf. This PR begins documenting this
and then testing to make sure the defaults follow the rules.

Fixes: #7657

Signed-off-by: Daniel J Walsh [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 Nov 9, 2020
@TomSweeneyRedHat
Copy link
Member

@rhatdan test aren't hip

TomSweeneyRedHat
TomSweeneyRedHat reviewed Nov 10, 2020
View reviewed changes
Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2020
@rhatdan
Copy link
Member Author

rhatdan commented Nov 17, 2020

@containers/podman-maintainers PTAL
@vrothberg We talked in a different issue about adding some tests to make sure that the server side is using containers.conf, This PR begins that and fixes some issues, where we were relying on the client side and ignoring the server side.

@vrothberg
Copy link
Member

On my review list for tomorrow. Brain isn't sharp enough to review so many line changes.

vrothberg
vrothberg reviewed Nov 18, 2020
View reviewed changes
cmd/podman/common/create.go Outdated
@@ -791,3 +791,66 @@ func DefineCreateFlags(cmd *cobra.Command, cf *ContainerCLIOpts) {
_ = cmd.RegisterFlagCompletionFunc(cgroupConfFlagName, completion.AutocompleteNone)

}

func ulimits() []string {
Copy link
Member

Choose a reason for hiding this comment

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

Could we move these into a new file create_flag_defaults.go?

Copy link
Member Author

Choose a reason for hiding this comment

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

Rather then create a fourth create*.go file, how about I move them to create_opts.go.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

docs/source/markdown/podman-create.1.md Outdated
@@ -18,6 +18,10 @@ any point.

The initial status of the container created with **podman create** is 'created'.

Default settings for flags are defined in `containers.conf`. Most settings for
Remote connections use the server's containers.conf, except when documented in
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Remote connections use the server's containers.conf, except when documented in
remote connections use the server's containers.conf, except when documented in

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

docs/source/markdown/podman-create.1.md Outdated
@@ -817,6 +821,7 @@ Signal to stop a container. Default is SIGTERM.
#### **--stop-timeout**=*seconds*

Timeout (in seconds) to stop a container. Default is 10.
Remote connections, use local containers.conf for defaults
Copy link
Member

Choose a reason for hiding this comment

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

I think the comma is wrong but I also have a hard time understanding it. Are we using the server or client conf? Maybe we can always speak about server and client?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

Choose a reason for hiding this comment

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

Some cases are left

pkg/specgen/volumes.go
"path/filepath"
"strings"

"github.com/containers/buildah/pkg/parse"
Copy link
Member

Choose a reason for hiding this comment

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

This seems like another candidate for c/common?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do this in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Awsome, thanks!

pkg/specgen/volumes.go
ReadWrite bool
}

func GenVolumeMounts(volumeFlag []string) (map[string]spec.Mount, map[string]*NamedVolume, map[string]*OverlayVolume, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Doc comments are missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@rhatdan rhatdan force-pushed the containers.conf branch 3 times, most recently from 3513133 to 2784ed2 Compare November 18, 2020 15:45
@rhatdan
Copy link
Member Author

rhatdan commented Nov 18, 2020

@giuseppe @mheon @edsantiago Any idea what is going on here. Ubuntu (cgroupv1 pod_stats_test,go is blowing up, even though these tests were not touched.

[+1082s] Error: unable to obtain cgroup stats: open /sys/fs/cgroup/pids/pids.current: no such file or directory

Have you ever seen anything like this before?

@edsantiago
Copy link
Member

I just saw and reported the same thing in 8170. Pressing Re-run made it go away, which does not fill me with warm fuzzies.

@rhatdan
Document containers.conf settings for remote connections
9770947 
Currently we don't document which end of the podman-remote client server
operations uses the containers.conf.  This PR begins documenting this
and then testing to make sure the defaults follow the rules.

Fixes: containers#7657

Signed-off-by: Daniel J Walsh <[email protected]>
vrothberg
vrothberg reviewed Nov 20, 2020
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

saschagrunert
saschagrunert approved these changes Nov 20, 2020
View reviewed changes
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2020
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

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

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-merge-robot openshift-merge-robot merged commit 042d488 into containers:master Nov 20, 2020
@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 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 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

@saschagrunert saschagrunert saschagrunert approved these changes

Assignees

@saschagrunert saschagrunert

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.

CLI defaults should come from server not client
7 participants
@rhatdan @TomSweeneyRedHat @vrothberg @edsantiago @openshift-ci-robot @saschagrunert @openshift-merge-robot