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

Fix handler and systemd activation errors #5158

Merged
merged 1 commit into from
Feb 18, 2020
Merged

Fix handler and systemd activation errors #5158

merged 1 commit into from
Feb 18, 2020

Conversation

jwhonce
Copy link
Member

@jwhonce jwhonce commented Feb 10, 2020 •
edited
Loading

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 jhonce@redhat.com

@jwhonce jwhonce added the kind/bug Categorizes issue or PR as related to a bug. label Feb 10, 2020
@jwhonce jwhonce self-assigned this Feb 10, 2020
@openshift-ci-robot 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 jwhonce force-pushed the issues/5151 branch 2 times, most recently from 324b7b1 to 2ca9837 Compare February 10, 2020 21:40
@jwhonce jwhonce changed the title Fix handler and systemd activation errors [CI:DOCS] Fix handler and systemd activation errors Feb 10, 2020
@jwhonce
Copy link
Member Author

jwhonce commented Feb 10, 2020

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

mheon
mheon reviewed Feb 10, 2020
View reviewed changes
pkg/api/handlers/types.go Outdated Show resolved Hide resolved
@vrothberg
Copy link
Member

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

vrothberg
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
@vrothberg
Copy link
Member

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

@openshift-ci-robot 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
@openshift-ci-robot 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
Copy link
Member Author

jwhonce commented Feb 15, 2020

@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
rhatdan approved these changes Feb 15, 2020
View reviewed changes
Copy link
Member

@rhatdan rhatdan left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
cmd/podman/service.go Outdated Show resolved Hide resolved
cmd/podman/service.go Show resolved Hide resolved
@openshift-ci-robot
Copy link
Collaborator

[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

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

@rhatdan
Copy link
Member

rhatdan commented Feb 15, 2020

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

@rh-atomic-bot
Copy link
Collaborator

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

@openshift-ci-robot 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 jwhonce changed the title [CI:DOCS] Fix handler and systemd activation errors Fix handler and systemd activation errors Feb 17, 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 Feb 17, 2020
@jwhonce
Fix handler and systemd activation errors
c0c44ae 
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 <jhonce@redhat.com>
@jwhonce jwhonce mentioned this pull request Feb 17, 2020
Closed
@rhatdan
Copy link
Member

rhatdan commented Feb 18, 2020

LGTM
@vrothberg PTAL

vrothberg
vrothberg reviewed Feb 18, 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, thanks @jwhonce !

@vrothberg
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 18, 2020
@openshift-merge-robot openshift-merge-robot merged commit bc20cb9 into containers:master Feb 18, 2020
This was referenced Feb 18, 2020
Merged
Merged
Merged
@edsantiago edsantiago mentioned this pull request Feb 24, 2020
Closed
@marusak marusak mentioned this pull request Mar 24, 2020
Closed
@jwhonce jwhonce deleted the issues/5151 branch June 30, 2021 16:11
@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

@mheon mheon mheon left review comments

@vrothberg vrothberg vrothberg left review comments

@rhatdan rhatdan rhatdan approved these changes

@baude baude Awaiting requested review from baude

@giuseppe giuseppe Awaiting requested review from giuseppe

@umohnani8 umohnani8 Awaiting requested review from umohnani8

Assignees

@jwhonce jwhonce

@vrothberg vrothberg

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. 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
8 participants
@jwhonce @vrothberg @openshift-ci-robot @rhatdan @rh-atomic-bot @mheon @baude @openshift-merge-robot