rootless: fail early if prerequiresites are not satisfied #2129

AkihiroSuda · 2021-03-15T06:18:45Z

kind create cluster now prints errors before running the containers.

k8s-ci-robot · 2021-03-15T06:18:53Z

Hi @AkihiroSuda. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

aojea · 2021-03-15T08:40:49Z

pkg/cluster/internal/create/create.go

@@ -66,6 +66,11 @@ type ClusterOptions struct {

 // Cluster creates a cluster
 func Cluster(logger log.Logger, p providers.Provider, opts *ClusterOptions) error {
+	// validate provider first
+	if err := validateProvider(p); err != nil {


@BenTheElder I don't know if we should validate it only on create cluster or always in DetectNodeProvider() ,

kind/pkg/cluster/provider.go

Lines 95 to 118 in 5b79090

// DetectNodeProvider allows callers to autodetect the node provider

// *without* fallback to the default.

//

// Pass the returned ProviderOption to NewProvider to pass the auto-detect Docker

// or Podman option explicitly (in the future there will be more options)

//

// NOTE: The kind *cli* also checks `KIND_EXPERIMENTAL_PROVIDER` for "podman" or

// "docker" currently and does not auto-detect / respects this if set.

//

// This will be replaced with some other mechanism in the future (likely when

// podman support is GA), in the meantime though your tool may wish to match this.

//

// In the future when this is not considered experimental,

// that logic will be in a public API as well.

func DetectNodeProvider() (ProviderOption, error) {

// auto-detect based on each node provider's IsAvailable() function

if docker.IsAvailable() {

return ProviderWithDocker(), nil

}

if podman.IsAvailable() {

return ProviderWithPodman(), nil

}

return nil, errors.WithStack(NoNodeProviderDetectedError)

}

we currently have <provider>.IsAvailable()
Should it be <provider>.IsAvailableAndValid() ?

I added this validation to create.go because deletion should not require this validation.

When a user booted the host with cgroup v2, created a rootless kind cluster, and then rebooted with cgroup v1 for running some other apps that do not support cgroup v2, the user still want to be able to remove the kind cluster.

my comment is more about the long term solution, Ben always wanted to model the providers API (as you can see in the comment that I pasted above)

So I'm wondering if this is the time to do it, to avoid start to grow it organically, 👍

When a user booted the host with cgroup v2, created a rootless kind cluster, and then rebooted with cgroup v1 for running some other apps that do not support cgroup v2, the user still want to be able to remove the kind cluster.

we should also start to think in what is supported , that is an interesting use case

I failed to click submit on my comment earlier it seems ...

Delete should not require validation indeed, the validation is wether the tagged resource exists.

It's fine for this to grow somewhat organically internally for now, we can create a better API in the future and consider it exporting it once we have a better idea what we need. It's the public APIs that we need to be more careful with (because people already depend on them and we can't refactor them easily, so we need to make any changes minimally difficult to deal with / not really remove APIs etc.). We can completely rewrite our own internal usage. I'm going to take a sledgehammer to the "actions" thing when I get some freetime someday, and the node build code ...

BenTheElder · 2021-03-16T04:11:53Z

/ok-to-test

pkg/cluster/internal/providers/podman/provider.go

aojea · 2021-03-16T10:49:44Z

/retest

aojea · 2021-03-16T11:33:03Z

/lgtm
/assign @BenTheElder

AkihiroSuda · 2021-03-17T03:40:29Z

/test pull-kind-e2e-kubernetes-1-20
/test pull-kind-e2e-kubernetes

AkihiroSuda · 2021-03-17T05:26:11Z

/test pull-kind-e2e-kubernetes

AkihiroSuda · 2021-03-23T01:20:53Z

@BenTheElder PTAL?

BenTheElder

thanks, sorry I've been prioritizing bug fixes & test coverage (k8s release cycle spilling over everywhere)

BenTheElder · 2021-03-23T01:52:36Z

pkg/cluster/internal/providers/podman/provider.go

-		Rootless: os.Geteuid() != 0,
+		Rootless: euid != 0,
+	}
+	if _, err := os.Stat("/sys/fs/cgroup/cgroup.controllers"); err == nil {


this is going to bite us someday when someone requests remote podman 🙃
(not a blocker I think, but leaving a breadcrumb @aojea)

I think that will be a future work, and probably needs some work on Podman side.

BenTheElder · 2021-03-23T01:53:16Z

pkg/cluster/internal/providers/podman/provider.go

@@ -347,8 +349,45 @@ func (p *provider) CollectLogs(dir string, nodes []nodes.Node) error {

 // Info returns the provider info.
 func (p *provider) Info() (*providers.ProviderInfo, error) {


we should probably note that this is cached.

this can not change on runtime, right?

Added a comment line about that.

this can not change on runtime, right?

Right

k8s-ci-robot · 2021-03-23T03:59:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AkihiroSuda, BenTheElder

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:

~~OWNERS~~ [BenTheElder]

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

AkihiroSuda · 2021-03-23T07:18:05Z

/test pull-kind-e2e-kubernetes

AkihiroSuda · 2021-03-23T08:26:32Z

/test pull-kind-e2e-kubernetes

aojea · 2021-03-23T10:06:47Z

/retest

aojea · 2021-03-23T10:10:36Z

something is going on with
pull-kind-e2e-kubernetes

lots of timeout and failures lately

`kind create cluster` now prints errors before running the containers. Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda · 2021-03-24T08:54:45Z

/retest

aojea · 2021-03-24T09:54:34Z

/lgtm

AkihiroSuda · 2021-03-24T17:16:04Z

/retest

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 15, 2021

k8s-ci-robot requested review from aojea and krzyzacy March 15, 2021 06:18

k8s-ci-robot added area/provider/docker Issues or PRs related to docker needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/provider/podman Issues or PRs related to podman labels Mar 15, 2021

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 15, 2021

aojea reviewed Mar 15, 2021

View reviewed changes

AkihiroSuda force-pushed the rootless-fail-early branch from e21174c to 4d7e6c9 Compare March 15, 2021 09:19

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 15, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 16, 2021

AkihiroSuda force-pushed the rootless-fail-early branch from 4d7e6c9 to ba7e05f Compare March 16, 2021 06:33

aojea reviewed Mar 16, 2021

View reviewed changes

pkg/cluster/internal/providers/podman/provider.go Outdated Show resolved Hide resolved

AkihiroSuda force-pushed the rootless-fail-early branch from ba7e05f to c3f2463 Compare March 16, 2021 07:48

k8s-ci-robot assigned BenTheElder and aojea Mar 16, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 16, 2021

Base automatically changed from master to main March 16, 2021 23:22

BenTheElder approved these changes Mar 23, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2021

rootless: fail early if prerequiresites are not satisfied

9a292c3

`kind create cluster` now prints errors before running the containers. Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda force-pushed the rootless-fail-early branch from c3f2463 to 9a292c3 Compare March 24, 2021 08:11

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 24, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 24, 2021

k8s-ci-robot merged commit 2c8c230 into kubernetes-sigs:main Mar 24, 2021

afrittoli mentioned this pull request May 7, 2021

support remote podman #2233

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rootless: fail early if prerequiresites are not satisfied #2129

rootless: fail early if prerequiresites are not satisfied #2129

AkihiroSuda commented Mar 15, 2021

k8s-ci-robot commented Mar 15, 2021

aojea Mar 15, 2021 •

edited

Loading

AkihiroSuda Mar 15, 2021 •

edited

Loading

aojea Mar 15, 2021

BenTheElder Mar 16, 2021

BenTheElder commented Mar 16, 2021

aojea commented Mar 16, 2021

aojea commented Mar 16, 2021

AkihiroSuda commented Mar 17, 2021

AkihiroSuda commented Mar 17, 2021 •

edited

Loading

AkihiroSuda commented Mar 23, 2021

BenTheElder left a comment

BenTheElder Mar 23, 2021

AkihiroSuda Mar 24, 2021

BenTheElder Mar 23, 2021

aojea Mar 23, 2021

AkihiroSuda Mar 24, 2021

k8s-ci-robot commented Mar 23, 2021

AkihiroSuda commented Mar 23, 2021

AkihiroSuda commented Mar 23, 2021

aojea commented Mar 23, 2021

aojea commented Mar 23, 2021

AkihiroSuda commented Mar 24, 2021

aojea commented Mar 24, 2021

AkihiroSuda commented Mar 24, 2021

	// DetectNodeProvider allows callers to autodetect the node provider
	// without fallback to the default.
	//
	// Pass the returned ProviderOption to NewProvider to pass the auto-detect Docker
	// or Podman option explicitly (in the future there will be more options)
	//
	// NOTE: The kind cli also checks `KIND_EXPERIMENTAL_PROVIDER` for "podman" or
	// "docker" currently and does not auto-detect / respects this if set.
	//
	// This will be replaced with some other mechanism in the future (likely when
	// podman support is GA), in the meantime though your tool may wish to match this.
	//
	// In the future when this is not considered experimental,
	// that logic will be in a public API as well.
	func DetectNodeProvider() (ProviderOption, error) {
	// auto-detect based on each node provider's IsAvailable() function
	if docker.IsAvailable() {
	return ProviderWithDocker(), nil
	}
	if podman.IsAvailable() {
	return ProviderWithPodman(), nil
	}
	return nil, errors.WithStack(NoNodeProviderDetectedError)
	}

		@@ -347,8 +349,45 @@ func (p *provider) CollectLogs(dir string, nodes []nodes.Node) error {

		// Info returns the provider info.
		func (p provider) Info() (providers.ProviderInfo, error) {

rootless: fail early if prerequiresites are not satisfied #2129

rootless: fail early if prerequiresites are not satisfied #2129

Conversation

AkihiroSuda commented Mar 15, 2021

k8s-ci-robot commented Mar 15, 2021

aojea Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

AkihiroSuda Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

aojea Mar 15, 2021

Choose a reason for hiding this comment

BenTheElder Mar 16, 2021

Choose a reason for hiding this comment

BenTheElder commented Mar 16, 2021

aojea commented Mar 16, 2021

aojea commented Mar 16, 2021

AkihiroSuda commented Mar 17, 2021

AkihiroSuda commented Mar 17, 2021 • edited Loading

AkihiroSuda commented Mar 23, 2021

BenTheElder left a comment

Choose a reason for hiding this comment

BenTheElder Mar 23, 2021

Choose a reason for hiding this comment

AkihiroSuda Mar 24, 2021

Choose a reason for hiding this comment

BenTheElder Mar 23, 2021

Choose a reason for hiding this comment

aojea Mar 23, 2021

Choose a reason for hiding this comment

AkihiroSuda Mar 24, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 23, 2021

AkihiroSuda commented Mar 23, 2021

AkihiroSuda commented Mar 23, 2021

aojea commented Mar 23, 2021

aojea commented Mar 23, 2021

AkihiroSuda commented Mar 24, 2021

aojea commented Mar 24, 2021

AkihiroSuda commented Mar 24, 2021

aojea Mar 15, 2021 •

edited

Loading

AkihiroSuda Mar 15, 2021 •

edited

Loading

AkihiroSuda commented Mar 17, 2021 •

edited

Loading