Refactored kubelet's kuberuntime_sandbox #114185

claudiubelu · 2022-11-29T17:38:57Z

What type of PR is this?

/kind cleanup
/sig windows

/milestone v1.27

What this PR does / why we need it:

Refactors platform specific code into separate files (*_linux.go / *_windows.go / *_others.go)

Adds unit tests for the kuberuntime_sandbox changes.

Co-Authored-By: Claudiu Belu cbelu@cloudbasesolutions.com

Which issue(s) this PR fixes:

Partial cleanup of issue #60338

Special notes for your reviewer:

Original PR: #109761

This PR contains only the kuberuntime_sandbox changes. Contains a few additional unit tests and cleanups.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2022-11-29T17:39:01Z

Please note that we're already in Test Freeze for the release-1.26 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.26.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Tue Nov 29 15:38:53 UTC 2022.

matthyx · 2022-11-30T14:05:54Z

/remove-sig node
/uncc

claudiubelu · 2022-12-02T13:59:53Z

/retest

claudiubelu · 2022-12-05T15:47:02Z

/retest

claudiubelu · 2022-12-08T12:04:44Z

/retest

claudiubelu · 2022-12-08T12:06:07Z

/assign @endocrimes

claudiubelu · 2022-12-12T12:49:44Z

/retest

claudiubelu · 2022-12-13T13:18:16Z

/retest

claudiubelu · 2022-12-14T12:49:43Z

/retest

marosset · 2022-12-15T17:54:15Z

/retest

marosset · 2022-12-15T17:54:21Z

/assign

marosset · 2022-12-15T17:54:42Z

/triage accepted
/priority important-soon

k8s-ci-robot · 2024-04-25T00:04:50Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

claudiubelu · 2024-04-25T07:08:17Z

/reopen

k8s-ci-robot · 2024-04-25T07:08:22Z

@claudiubelu: Reopened this PR.

In response to this:

/reopen

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.

claudiubelu · 2024-04-25T07:08:38Z

/remove-lifecycle rotten

claudiubelu · 2024-04-25T07:09:00Z

/test pull-kubernetes-e2e-capz-windows-master

claudiubelu · 2024-04-25T14:19:11Z

/test pull-kubernetes-e2e-capz-windows-master

claudiubelu · 2024-04-26T12:17:01Z

/test pull-kubernetes-e2e-capz-windows-master

SergeyKanzhelev · 2024-04-26T22:56:14Z

pkg/kubelet/kuberuntime/kuberuntime_sandbox.go

-		}
-		if sc.RunAsGroup != nil && runtime.GOOS != "windows" {
-			lc.SecurityContext.RunAsGroup = &runtimeapi.Int64Value{Value: int64(*sc.RunAsGroup)}
-		}
 		namespaceOptions, err := runtimeutil.NamespacesForPod(pod, m.runtimeHelper, m.runtimeClassManager)


is this one linux only? Is there something we configure for Windows for user namespaces?

Took a quick glance in NamespaceForPod: it would seem so. First of all, it does have the Network field, which may be runtimeapi.NamespaceMode_NODE (host network) or runtimeapi.NamespaceMode_POD, which is relevant for Windows as well. Not sure about the other fields though (UsernsOptions may be relevant, since it's based on the Spec's RuntimeClassName). I'd leave this here for the time being. But yeah, it's a bit strange that we need LinuxPodSandboxConfig for Windows.

SergeyKanzhelev · 2024-04-26T22:59:18Z

pkg/kubelet/kuberuntime/kuberuntime_sandbox.go

@@ -141,14 +140,11 @@ func (m *kubeGenericRuntimeManager) generatePodSandboxConfig(pod *v1.Pod, attemp
 		return nil, err
 	}
 	podSandboxConfig.Linux = lc


do we need to set linux context on Windows node? Looks counterintuitive

It seems so, according to the other comment I've left.

SergeyKanzhelev · 2024-04-26T23:06:02Z

pkg/kubelet/kuberuntime/kuberuntime_sandbox.go

@@ -141,14 +140,11 @@ func (m *kubeGenericRuntimeManager) generatePodSandboxConfig(pod *v1.Pod, attemp
 		return nil, err
 	}


I have similar desire to refactor this file when I look at it. This PR is taking a very shallow step in refactoring, which makes it a matter of taste as later comment of @bart0sh indicated.

I would suggest to make a little bit bigger effort of separating clearly Windows-only, Linux-only, and shared settings. So when somebody adds a new feature they will not be confused where they put code for it

Sure, added a bit more to this PR. Keep in mind that most of this comes from another PR (#109761), which was split into 3 (so it's easier to merge, one of them merged, this is still in review: #114186), some comments were addressed, a bit extra cleaning.

claudiubelu · 2024-05-13T19:58:14Z

/test pull-kubernetes-e2e-capz-windows-master

claudiubelu · 2024-05-16T12:37:40Z

/test pull-kubernetes-e2e-capz-windows-master

claudiubelu · 2024-06-03T08:56:14Z

/test pull-kubernetes-unit

k8s-ci-robot · 2024-06-03T09:37:08Z

@claudiubelu: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-inplace-pod-resize-containerd-main-v2	9de08903ee28d8649915e040c16686ee893ab90d	link	false	`/test pull-kubernetes-e2e-inplace-pod-resize-containerd-main-v2`
pull-kubernetes-e2e-capz-windows-containerd	`5e605d8`	link	false	`/test pull-kubernetes-e2e-capz-windows-containerd`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

Refactors platform specific code into separate files (*_linux.go / *_windows.go / *_others.go) Adds unit tests for the kuberuntime_sandbox changes. Co-Authored-By: Claudiu Belu <cbelu@cloudbasesolutions.com>

k8s-ci-robot added this to the v1.27 milestone Nov 29, 2022

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 29, 2022

k8s-ci-robot requested review from krmayankk and matthyx November 29, 2022 17:39

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 29, 2022

k8s-ci-robot removed the request for review from matthyx November 30, 2022 14:05

k8s-ci-robot removed the sig/node Categorizes an issue or PR as relevant to SIG Node. label Nov 30, 2022

k8s-ci-robot assigned endocrimes Dec 8, 2022

k8s-ci-robot assigned marosset Dec 15, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Dec 15, 2022

k8s-ci-robot closed this Apr 25, 2024

SIG Node PR Triage automation moved this from Needs Reviewer to Done Apr 25, 2024

k8s-ci-robot reopened this Apr 25, 2024

SIG Node PR Triage automation moved this from Done to Triage Apr 25, 2024

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Apr 25, 2024

bart0sh moved this from Triage to Needs Reviewer in SIG Node PR Triage Apr 25, 2024

SergeyKanzhelev reviewed Apr 26, 2024

View reviewed changes

claudiubelu force-pushed the refactor-platform-deps-1 branch from dbabfc7 to 8c08682 Compare May 7, 2024 16:01

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 7, 2024

claudiubelu force-pushed the refactor-platform-deps-1 branch 4 times, most recently from 5c0e21b to 129626e Compare May 13, 2024 15:47

claudiubelu force-pushed the refactor-platform-deps-1 branch from 129626e to cae7b6e Compare May 31, 2024 08:15

Refactored kubelet's kuberuntime_sandbox

18acb05

Refactors platform specific code into separate files (*_linux.go / *_windows.go / *_others.go) Adds unit tests for the kuberuntime_sandbox changes. Co-Authored-By: Claudiu Belu <cbelu@cloudbasesolutions.com>

claudiubelu force-pushed the refactor-platform-deps-1 branch from cae7b6e to 18acb05 Compare June 4, 2024 06:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored kubelet's kuberuntime_sandbox #114185

Refactored kubelet's kuberuntime_sandbox #114185

claudiubelu commented Nov 29, 2022

k8s-ci-robot commented Nov 29, 2022

matthyx commented Nov 30, 2022

claudiubelu commented Dec 2, 2022

claudiubelu commented Dec 5, 2022

claudiubelu commented Dec 8, 2022

claudiubelu commented Dec 8, 2022

claudiubelu commented Dec 12, 2022

claudiubelu commented Dec 13, 2022

claudiubelu commented Dec 14, 2022

marosset commented Dec 15, 2022

marosset commented Dec 15, 2022

marosset commented Dec 15, 2022

k8s-ci-robot commented Apr 25, 2024

claudiubelu commented Apr 25, 2024

k8s-ci-robot commented Apr 25, 2024

claudiubelu commented Apr 25, 2024

claudiubelu commented Apr 25, 2024

claudiubelu commented Apr 25, 2024

claudiubelu commented Apr 26, 2024

SergeyKanzhelev Apr 26, 2024

claudiubelu May 7, 2024

SergeyKanzhelev Apr 26, 2024

claudiubelu May 27, 2024

SergeyKanzhelev Apr 26, 2024

claudiubelu May 7, 2024

claudiubelu commented May 13, 2024

claudiubelu commented May 16, 2024

claudiubelu commented Jun 3, 2024

k8s-ci-robot commented Jun 3, 2024 •

edited

		@@ -141,14 +140,11 @@ func (m kubeGenericRuntimeManager) generatePodSandboxConfig(pod v1.Pod, attemp
		return nil, err
		}

Refactored kubelet's kuberuntime_sandbox #114185

Are you sure you want to change the base?

Refactored kubelet's kuberuntime_sandbox #114185

Conversation

claudiubelu commented Nov 29, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Nov 29, 2022

matthyx commented Nov 30, 2022

claudiubelu commented Dec 2, 2022

claudiubelu commented Dec 5, 2022

claudiubelu commented Dec 8, 2022

claudiubelu commented Dec 8, 2022

claudiubelu commented Dec 12, 2022

claudiubelu commented Dec 13, 2022

claudiubelu commented Dec 14, 2022

marosset commented Dec 15, 2022

marosset commented Dec 15, 2022

marosset commented Dec 15, 2022

k8s-ci-robot commented Apr 25, 2024

claudiubelu commented Apr 25, 2024

k8s-ci-robot commented Apr 25, 2024

claudiubelu commented Apr 25, 2024

claudiubelu commented Apr 25, 2024

claudiubelu commented Apr 25, 2024

claudiubelu commented Apr 26, 2024

SergeyKanzhelev Apr 26, 2024

Choose a reason for hiding this comment

claudiubelu May 7, 2024

Choose a reason for hiding this comment

SergeyKanzhelev Apr 26, 2024

Choose a reason for hiding this comment

claudiubelu May 27, 2024

Choose a reason for hiding this comment

SergeyKanzhelev Apr 26, 2024

Choose a reason for hiding this comment

claudiubelu May 7, 2024

Choose a reason for hiding this comment

claudiubelu commented May 13, 2024

claudiubelu commented May 16, 2024

claudiubelu commented Jun 3, 2024

k8s-ci-robot commented Jun 3, 2024 • edited

k8s-ci-robot commented Jun 3, 2024 •

edited