Fix handler and systemd activation errors by jwhonce · Pull Request #5158 · containers/podman

jwhonce · 2020-02-10T21:34:28Z

On panic from handler: log warning and stack trace, report
InternalServerError to client

Set timeouts to allow large files to be sent to/from server. Refactor idle window to support new timeouts.

When using podman system service make determining the listening endpoint deterministic.

// When determining THE listening endpoint --
// 1) User input wins always
// 2) systemd socket activation
// 3) rootless honors XDG_RUNTIME_DIR
// 4) if varlink -- adapter.DefaultVarlinkAddress
// 5) lastly adapter.DefaultAPIAddress

Fixes #5150
Fixes #5151
Fixes #5224

Signed-off-by: Jhon Honce [email protected]

jwhonce · 2020-02-10T21:46:02Z

@vrothberg I changed your package name because I added another file not related to generation.

pkg/api/handlers/types.go

vrothberg · 2020-02-11T09:09:18Z

@vrothberg I changed your package name because I added another file not related to generation.

I feel rather against changing the package name and adding somehow unrelated code to that package. I believe we have code to check what SocketActivated() does at a couple of places. I'll search for it.

pkg/api/server/handler_api.go

pkg/systemd/activation.go

vrothberg · 2020-02-11T09:25:16Z

@vrothberg I changed your package name because I added another file not related to generation.

I feel rather against changing the package name and adding somehow unrelated code to that package. I believe we have code to check what SocketActivated() does at a couple of places. I'll search for it.

There is no similar code to SocketActivated() actually. I still don't like mixing the generation code with a more generic package. The code systemdgen is awfully specific to generating service files.

jwhonce · 2020-02-15T00:51:48Z

@vrothberg I believe I understand what you are saying but do we need that granularity? Or are other developers just know it's systemd related "So let's go there?"

rhatdan

Personlly I think the directory should be pkg/systemd. I agree with @jwhonce that I as a developer would be looking for the top level directory for all systemd shared code.

If you want to keep the generate code separate then create a sub-dir
pkg/systemd/generate or pkg/systemd/unit

cmd/podman/service.go

openshift-ci-robot · 2020-02-15T12:03:05Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [jwhonce,rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhatdan · 2020-02-15T12:03:52Z

Why is this [CI:DOCS]? There is actually changed code here and we should be running the tests suites on it.

rh-atomic-bot · 2020-02-17T16:25:05Z

☔ The latest upstream changes (presumably #5093) made this pull request unmergeable. Please resolve the merge conflicts.

On panic from handler: log warning and stack trace, report InternalServerError to client When using `podman system service` make determining the listening endpoint deterministic. // When determining _*THE*_ listening endpoint -- // 1) User input wins always // 2) systemd socket activation // 3) rootless honors XDG_RUNTIME_DIR // 4) if varlink -- adapter.DefaultVarlinkAddress // 5) lastly adapter.DefaultAPIAddress Fixes containers#5150 Fixes containers#5151 Signed-off-by: Jhon Honce <[email protected]>

rhatdan · 2020-02-18T13:45:50Z

LGTM
@vrothberg PTAL

vrothberg

LGTM, thanks @jwhonce !

vrothberg · 2020-02-18T14:06:26Z

/lgtm

jwhonce added the kind/bug Categorizes issue or PR as related to a bug. label Feb 10, 2020

jwhonce requested review from baude, mheon and vrothberg February 10, 2020 21:34

jwhonce self-assigned this Feb 10, 2020

openshift-ci-robot requested review from giuseppe and umohnani8 February 10, 2020 21:34

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L labels Feb 10, 2020

jwhonce force-pushed the issues/5151 branch 2 times, most recently from 324b7b1 to 2ca9837 Compare February 10, 2020 21:40

jwhonce changed the title ~~Fix handler and systemd activation errors~~ [CI:DOCS] Fix handler and systemd activation errors Feb 10, 2020

mheon reviewed Feb 10, 2020

View reviewed changes

pkg/api/handlers/types.go Outdated Show resolved Hide resolved

vrothberg reviewed Feb 11, 2020

View reviewed changes

pkg/api/server/handler_api.go Outdated Show resolved Hide resolved

pkg/systemd/activation.go Outdated Show resolved Hide resolved

jwhonce force-pushed the issues/5151 branch from 2ca9837 to 09bf260 Compare February 14, 2020 23:26

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL and removed size/L labels Feb 14, 2020

jwhonce force-pushed the issues/5151 branch from 09bf260 to cc2fd1f Compare February 14, 2020 23:53

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2020

jwhonce force-pushed the issues/5151 branch from cc2fd1f to a2257d0 Compare February 15, 2020 00:04

rhatdan approved these changes Feb 15, 2020

View reviewed changes

cmd/podman/service.go Outdated Show resolved Hide resolved

cmd/podman/service.go Outdated Show resolved Hide resolved

cmd/podman/service.go Outdated Show resolved Hide resolved

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2020

jwhonce changed the title ~~[CI:DOCS] Fix handler and systemd activation errors~~ Fix handler and systemd activation errors Feb 17, 2020

jwhonce force-pushed the issues/5151 branch from a2257d0 to e5ebbd7 Compare February 17, 2020 17:08

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2020

jwhonce force-pushed the issues/5151 branch from e5ebbd7 to c0c44ae Compare February 17, 2020 17:29

jwhonce mentioned this pull request Feb 17, 2020

APIv2: Streaming closes after a few minutes #5224

Closed

vrothberg reviewed Feb 18, 2020

View reviewed changes

openshift-ci-robot assigned vrothberg Feb 18, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 18, 2020

openshift-merge-robot merged commit bc20cb9 into containers:master Feb 18, 2020

This was referenced Feb 18, 2020

apiv2 container create using specgen #5204

Merged

Swagger: fix inconsistencies (try #2) #5231

Merged

Update mux rules to allow slashes in image names #5235

Merged

edsantiago mentioned this pull request Feb 24, 2020

APIv2 is now super-slow #5311

Closed

marusak mentioned this pull request Mar 24, 2020

APIv2: Events streaming behaves weirdly with socket activation #5599

Closed

jwhonce deleted the issues/5151 branch June 30, 2021 16:11

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 bot locked as resolved and limited conversation to collaborators Sep 23, 2023

Conversation

jwhonce commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jwhonce commented Feb 10, 2020

Uh oh!

Uh oh!

vrothberg commented Feb 11, 2020

Uh oh!

Uh oh!

Uh oh!

vrothberg commented Feb 11, 2020

Uh oh!

jwhonce commented Feb 15, 2020

Uh oh!

rhatdan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openshift-ci-robot commented Feb 15, 2020

Uh oh!

rhatdan commented Feb 15, 2020

Uh oh!

rh-atomic-bot commented Feb 17, 2020

Uh oh!

rhatdan commented Feb 18, 2020

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

vrothberg commented Feb 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jwhonce commented Feb 10, 2020 •

edited

Loading