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.

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

NOMAD-236 userns podman configuration option #212

Merged
merged 6 commits into from
Mar 20, 2023

Conversation

acornies
Copy link
Contributor

@acornies acornies commented Feb 3, 2023

I haven't been able to run the tests locally, but I want to get a general sense on if this is the right approach or not.

  • add userns to task config
  • add parsing for userns config option
  • add the driver configuration, uncomment api portion
  • tests

- add userns to task config
- add parsing for userns config option
- add the driver configuration, uncomment api portion
- tests
@acornies acornies marked this pull request as ready for review February 7, 2023 17:50
@acornies acornies changed the title NOMAD-236 --userns podman configuration option: NOMAD-236 userns podman configuration option Feb 9, 2023
@shoenig shoenig self-assigned this Feb 10, 2023
Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

Hi @acornies 👋

Thanks for the PR! I left some comments for further discussion. You mentioned not being able to run the tests, is there any problem in particular?

driver.go Outdated Show resolved Hide resolved
driver.go Outdated Show resolved Hide resolved
driver.go Outdated Show resolved Hide resolved
driver_test.go Outdated
Comment on lines 1488 to 1504
// check userns mode configuration (single value)
func TestPodmanDriver_UsernsMode(t *testing.T) {
taskCfg := newTaskConfig("", busyboxLongRunningCmd)
taskCfg.UserNS = "host"
inspectData := startDestroyInspect(t, taskCfg, "userns")

require.Equal(t, "host", inspectData.HostConfig.UsernsMode)
}

// check userns mode configuration (parsed value)
func TestPodmanDriver_UsernsModeParsed(t *testing.T) {
taskCfg := newTaskConfig("", busyboxLongRunningCmd)
taskCfg.UserNS = "keep-id:uid=200,gid=210"
inspectData := startDestroyInspect(t, taskCfg, "userns")

require.Equal(t, "keep-id:uid=200,gid=210", inspectData.HostConfig.UsernsMode)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why the Github Action didn't run (it does this some times 😞), but I ran it locally and I think these need to run with Podman in rootless mode? The first test always returns an empty string:

--- FAIL: TestPodmanDriver_UsernsMode (6.28s)
    driver_test.go:1494:
                Error Trace:    /Users/laoqui/go/src/github.com/hashicorp/nomad-driver-podman/driver_test.go:1494
                Error:          Not equal:
                                expected: "host"
                                actual  : ""

                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -host
                                +
                Test:           TestPodmanDriver_UsernsMode
FAIL
FAIL    github.com/hashicorp/nomad-driver-podman        6.313s
FAIL

The second test says the mode is not supported:

--- FAIL: TestPodmanDriver_UsernsModeParsed (4.11s)
    driver_test.go:2253:
                Error Trace:    /Users/laoqui/go/src/github.com/hashicorp/nomad-driver-podman/driver_test.go:2253
                                                        /Users/laoqui/go/src/github.com/hashicorp/nomad-driver-podman/driver_test.go:1501
                Error:          Received unexpected error:
                                rpc error: code = Unknown desc = failed to start task, could not create container: cannot create container, status code: 500: {"cause":"keep-id is only supported in rootless mode","message":"keep-id is only supported in rootless mode","response":500}
                Test:           TestPodmanDriver_UsernsModeParsed

I think our tests run as root so we may need to skip these in CI.

Copy link
Contributor Author

@acornies acornies Mar 14, 2023

Choose a reason for hiding this comment

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

Part of the problem is I'm running a Macbook M1 so let me figure this out...

Copy link
Contributor

Choose a reason for hiding this comment

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

Same 😅

I've been using Lima with this configuration, and so far it has been working well:
https://gist.github.com/lgfa29/f24eec7bb39f4bda28d7765bec8740cc#file-podman-yaml

I think you may need to run the tests as sudo.

acornies and others added 2 commits March 14, 2023 14:44
Used suggested changes from Luis - clearer intention

Co-authored-by: Luiz Aoqui <luiz@hashicorp.com>
- remove redundant mapping code to map NamespaceMode
- Switch to SplitN usage to limit split operation
Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @acornies. I fixed a CI issue and also set the tests to skip for now since our CI runs them in rootful mode and userns seems to be "" unless Podman is running in rootless mode.

Is there anything else missing before we merge this? 🙂

@acornies
Copy link
Contributor Author

Thanks @lgfa29 you beat me to it (skipping the tests). I don't think there's anything else atm. It's an effort to iterate on additional config options that would be useful to podman users. The referenced internal issue has some other stuff that might be useful but those can happen in another PR.

@lgfa29
Copy link
Contributor

lgfa29 commented Mar 20, 2023

Cool, thanks! Feel free to open more PRs in the future as needed 😄

@lgfa29 lgfa29 merged commit 736560e into hashicorp:main Mar 20, 2023
@acornies acornies deleted the feature/additional_config branch March 31, 2023 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants