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

[FG:InPlacePodVerticalScaling] Drop InPlacePodVerticalScaling support in windows #128623

Merged
merged 1 commit into from
Nov 7, 2024
Merged

[FG:InPlacePodVerticalScaling] Drop InPlacePodVerticalScaling support in windows #128623

merged 1 commit into from
Nov 7, 2024

Conversation

AnishShah
Copy link
Contributor

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This change drops support for InPlacePodVerticalScaling feature in Windows.

Which issue(s) this PR fixes:

Fixes #128617

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Drop support for InPlacePodVerticalScaling feature in Windows.

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

- KEP: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/1287-in-place-update-pod-resources

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 6, 2024
@AnishShah
Copy link
Contributor Author

/assign @tallclair

@k8s-ci-robot k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 6, 2024
@AnishShah AnishShah force-pushed the ippr-windows branch 2 times, most recently from 758101f to f67468b Compare November 6, 2024 19:35
tallclair
tallclair reviewed Nov 6, 2024
View reviewed changes
pkg/kubelet/kubelet.go
@@ -220,10 +220,12 @@ var (
// ContainerLogsDir can be overwritten for testing usage
ContainerLogsDir = DefaultContainerLogsDir
etcHostsPath = getContainerEtcHostsPath()

goos = sysruntime.GOOS
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd prefer to keep this inline, rather than assigning to a variable. It's just a constant, no cost to calling it multiple times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is mainly for unit tests. If I mutate runtime.GOOS during unit tests, then it might affect other unit tests running in parallel. Let me verify this.

Copy link
Contributor Author

@AnishShah AnishShah Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like runtime.GOOS is a constant. So I won't be able to write unit tests for this. WDYT?

./kubelet_test.go:2753:19: cannot assign to runtime.GOOS (neither addressable nor a map index expression)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, makes sense. Can you add a comment then? Something like // exposed for testing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the comment. I'm not sure if there's any security risk by switching between runtime.GOOS constant and goos variable

pkg/kubelet/kubelet.go Outdated Show resolved Hide resolved
@AnishShah AnishShah force-pushed the ippr-windows branch 2 times, most recently from f41362a to 1b4510b Compare November 6, 2024 19:49
@tallclair
Copy link
Member

/triage accepted
/priority important-soon
/milestone v1.32

@k8s-ci-robot k8s-ci-robot added this to the v1.32 milestone Nov 6, 2024
@k8s-ci-robot 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. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 6, 2024
@tallclair
Copy link
Member

/assign @thockin
For API validation approval.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2024
thockin
thockin reviewed Nov 6, 2024
View reviewed changes
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

/approve for API

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2024
@AnishShah
Copy link
Contributor Author

/retest

@tallclair
Copy link
Member

/lgtm
/approve

Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: cbba31765cea4ed8680b84c1c1b9c76292e73efb

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AnishShah, tallclair, thockin

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

@kannon92
Copy link
Contributor

kannon92 commented Nov 6, 2024

Did we check if there are any test-infra or e2e tests we will need to clean up?

@kannon92
Copy link
Contributor

kannon92 commented Nov 6, 2024

I saw that we have some windows related presubmits for this feature.

https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes-sigs/sig-windows/release-master-windows-presubmits.yaml#L248

@AnishShah
Copy link
Contributor Author

Did we check if there are any test-infra or e2e tests we will need to clean up?

We skip pod resize tests for windows nodes today -

kubernetes/test/e2e/node/pod_resize.go

Lines 377 to 379 in 9ba42a5

if framework.NodeOSDistroIs("windows") || e2enode.IsARM64(node) {
e2eskipper.Skipf("runtime does not support InPlacePodVerticalScaling -- skipping")
}

@AnishShah
Copy link
Contributor Author

I saw that we have some windows related presubmits for this feature.

https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes-sigs/sig-windows/release-master-windows-presubmits.yaml#L248

Those failures might be due to #128598 ?

@knabben
Copy link
Member

knabben commented Nov 6, 2024

Hey @thockin @tallclair @AnishShah
⚠️ Do we still intend to merge this for v1.32? Just a reminder that the code freeze is starting 02:00 UTC Friday November 8th 2024 (a little less than 1 week from now). Please make sure the PR has both lgtm and approved labels before the code freeze. Thanks!

@kannon92
Copy link
Contributor

kannon92 commented Nov 7, 2024

Those failures might be due to #128598 ?

These aren't failures but I saw that we had windows jobs that target this.

@kannon92
Copy link
Contributor

kannon92 commented Nov 7, 2024

/test

@k8s-ci-robot
Copy link
Contributor

@kannon92: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

  • /test pull-cos-containerd-e2e-ubuntu-gce
  • /test pull-kubernetes-conformance-kind-ga-only-parallel
  • /test pull-kubernetes-coverage-unit
  • /test pull-kubernetes-dependencies
  • /test pull-kubernetes-dependencies-go-canary
  • /test pull-kubernetes-e2e-gce
  • /test pull-kubernetes-e2e-gce-100-performance
  • /test pull-kubernetes-e2e-gce-cos
  • /test pull-kubernetes-e2e-gce-cos-canary
  • /test pull-kubernetes-e2e-gce-cos-no-stage
  • /test pull-kubernetes-e2e-gce-network-proxy-http-connect
  • /test pull-kubernetes-e2e-gce-pull-through-cache
  • /test pull-kubernetes-e2e-kind
  • /test pull-kubernetes-e2e-kind-ipv6
  • /test pull-kubernetes-integration
  • /test pull-kubernetes-integration-go-canary
  • /test pull-kubernetes-kubemark-e2e-gce-scale
  • /test pull-kubernetes-node-e2e-containerd
  • /test pull-kubernetes-typecheck
  • /test pull-kubernetes-unit
  • /test pull-kubernetes-unit-go-canary
  • /test pull-kubernetes-update
  • /test pull-kubernetes-verify
  • /test pull-kubernetes-verify-go-canary
  • /test pull-kubernetes-verify-lint

The following commands are available to trigger optional jobs:

  • /test check-dependency-stats
  • /test pull-crio-cgroupv1-node-e2e-eviction
  • /test pull-crio-cgroupv1-node-e2e-eviction-kubetest2
  • /test pull-crio-cgroupv1-node-e2e-features
  • /test pull-crio-cgroupv1-node-e2e-features-kubetest2
  • /test pull-crio-cgroupv1-node-e2e-hugepages
  • /test pull-crio-cgroupv1-node-e2e-hugepages-kubetest2
  • /test pull-crio-cgroupv1-node-e2e-resource-managers
  • /test pull-crio-cgroupv1-node-e2e-resource-managers-kubetest2
  • /test pull-crio-cgroupv2-imagefs-separatedisktest
  • /test pull-crio-cgroupv2-imagefs-separatedisktest-kubetest2
  • /test pull-crio-cgroupv2-node-e2e-eviction
  • /test pull-crio-cgroupv2-node-e2e-eviction-kubetest2
  • /test pull-crio-cgroupv2-node-e2e-hugepages
  • /test pull-crio-cgroupv2-node-e2e-hugepages-kubetest2
  • /test pull-crio-cgroupv2-node-e2e-resource-managers
  • /test pull-crio-cgroupv2-node-e2e-resource-managers-kubetest2
  • /test pull-crio-cgroupv2-splitfs-separate-disk
  • /test pull-crio-cgroupv2-splitfs-separate-disk-kubetest2
  • /test pull-e2e-gce-cloud-provider-disabled
  • /test pull-e2e-gci-gce-alpha-enabled-default
  • /test pull-kubernetes-apidiff
  • /test pull-kubernetes-conformance-image-test
  • /test pull-kubernetes-conformance-kind-ga-only
  • /test pull-kubernetes-conformance-kind-ipv6-parallel
  • /test pull-kubernetes-cos-cgroupv1-containerd-node-e2e
  • /test pull-kubernetes-cos-cgroupv1-containerd-node-e2e-features
  • /test pull-kubernetes-cos-cgroupv2-containerd-node-e2e
  • /test pull-kubernetes-cos-cgroupv2-containerd-node-e2e-eviction
  • /test pull-kubernetes-cos-cgroupv2-containerd-node-e2e-features
  • /test pull-kubernetes-cos-cgroupv2-containerd-node-e2e-serial
  • /test pull-kubernetes-crio-node-memoryqos-cgrpv2
  • /test pull-kubernetes-crio-node-memoryqos-cgrpv2-kubetest2
  • /test pull-kubernetes-cross
  • /test pull-kubernetes-e2e-autoscaling-hpa-cm
  • /test pull-kubernetes-e2e-autoscaling-hpa-cpu
  • /test pull-kubernetes-e2e-capz-azure-disk
  • /test pull-kubernetes-e2e-capz-azure-disk-vmss
  • /test pull-kubernetes-e2e-capz-azure-file
  • /test pull-kubernetes-e2e-capz-azure-file-vmss
  • /test pull-kubernetes-e2e-capz-conformance
  • /test pull-kubernetes-e2e-capz-master-windows-nodelogquery
  • /test pull-kubernetes-e2e-capz-windows-alpha-feature-vpa
  • /test pull-kubernetes-e2e-capz-windows-alpha-features
  • /test pull-kubernetes-e2e-capz-windows-master
  • /test pull-kubernetes-e2e-capz-windows-serial-slow
  • /test pull-kubernetes-e2e-capz-windows-serial-slow-hpa
  • /test pull-kubernetes-e2e-containerd-gce
  • /test pull-kubernetes-e2e-ec2
  • /test pull-kubernetes-e2e-ec2-arm64
  • /test pull-kubernetes-e2e-ec2-conformance
  • /test pull-kubernetes-e2e-ec2-conformance-arm64
  • /test pull-kubernetes-e2e-ec2-device-plugin-gpu
  • /test pull-kubernetes-e2e-gce-canary
  • /test pull-kubernetes-e2e-gce-correctness
  • /test pull-kubernetes-e2e-gce-cos-alpha-features
  • /test pull-kubernetes-e2e-gce-csi-serial
  • /test pull-kubernetes-e2e-gce-device-plugin-gpu
  • /test pull-kubernetes-e2e-gce-disruptive-canary
  • /test pull-kubernetes-e2e-gce-kubelet-credential-provider
  • /test pull-kubernetes-e2e-gce-network-policies
  • /test pull-kubernetes-e2e-gce-network-proxy-grpc
  • /test pull-kubernetes-e2e-gce-serial
  • /test pull-kubernetes-e2e-gce-serial-canary
  • /test pull-kubernetes-e2e-gce-storage-disruptive
  • /test pull-kubernetes-e2e-gce-storage-selinux
  • /test pull-kubernetes-e2e-gce-storage-slow
  • /test pull-kubernetes-e2e-gce-storage-snapshot
  • /test pull-kubernetes-e2e-gci-gce-autoscaling
  • /test pull-kubernetes-e2e-gci-gce-ingress
  • /test pull-kubernetes-e2e-gci-gce-ipvs
  • /test pull-kubernetes-e2e-gci-gce-nftables
  • /test pull-kubernetes-e2e-inplace-pod-resize-containerd-main-v2
  • /test pull-kubernetes-e2e-kind-alpha-beta-features
  • /test pull-kubernetes-e2e-kind-alpha-features
  • /test pull-kubernetes-e2e-kind-beta-features
  • /test pull-kubernetes-e2e-kind-canary
  • /test pull-kubernetes-e2e-kind-cloud-provider-loadbalancer
  • /test pull-kubernetes-e2e-kind-dual-canary
  • /test pull-kubernetes-e2e-kind-evented-pleg
  • /test pull-kubernetes-e2e-kind-ipv6-canary
  • /test pull-kubernetes-e2e-kind-ipvs
  • /test pull-kubernetes-e2e-kind-kms
  • /test pull-kubernetes-e2e-kind-multizone
  • /test pull-kubernetes-e2e-kind-nftables
  • /test pull-kubernetes-e2e-relaxed-environment-variable-validation
  • /test pull-kubernetes-e2e-storage-kind-alpha-beta-features
  • /test pull-kubernetes-e2e-storage-kind-disruptive
  • /test pull-kubernetes-e2e-storage-kind-volume-group-snapshots
  • /test pull-kubernetes-kind-dra
  • /test pull-kubernetes-kind-dra-all
  • /test pull-kubernetes-kind-json-logging
  • /test pull-kubernetes-kind-text-logging
  • /test pull-kubernetes-kubemark-e2e-gce-big
  • /test pull-kubernetes-linter-hints
  • /test pull-kubernetes-local-e2e
  • /test pull-kubernetes-node-arm64-e2e-containerd-ec2
  • /test pull-kubernetes-node-arm64-e2e-containerd-serial-ec2
  • /test pull-kubernetes-node-arm64-ubuntu-serial-gce
  • /test pull-kubernetes-node-crio-cgrpv1-evented-pleg-e2e
  • /test pull-kubernetes-node-crio-cgrpv1-evented-pleg-e2e-kubetest2
  • /test pull-kubernetes-node-crio-cgrpv2-e2e
  • /test pull-kubernetes-node-crio-cgrpv2-e2e-kubetest2
  • /test pull-kubernetes-node-crio-cgrpv2-imagefs-e2e
  • /test pull-kubernetes-node-crio-cgrpv2-imagefs-e2e-kubetest2
  • /test pull-kubernetes-node-crio-cgrpv2-imagevolume-e2e
  • /test pull-kubernetes-node-crio-cgrpv2-imagevolume-e2e-kubetest2
  • /test pull-kubernetes-node-crio-cgrpv2-splitfs-e2e
  • /test pull-kubernetes-node-crio-cgrpv2-splitfs-e2e-kubetest2
  • /test pull-kubernetes-node-crio-cgrpv2-userns-e2e-serial
  • /test pull-kubernetes-node-crio-cgrpv2-userns-e2e-serial-kubetest2
  • /test pull-kubernetes-node-crio-e2e
  • /test pull-kubernetes-node-crio-e2e-kubetest2
  • /test pull-kubernetes-node-e2e-alpha-ec2
  • /test pull-kubernetes-node-e2e-containerd-1-7-dra
  • /test pull-kubernetes-node-e2e-containerd-alpha-features
  • /test pull-kubernetes-node-e2e-containerd-ec2
  • /test pull-kubernetes-node-e2e-containerd-features
  • /test pull-kubernetes-node-e2e-containerd-features-kubetest2
  • /test pull-kubernetes-node-e2e-containerd-kubetest2
  • /test pull-kubernetes-node-e2e-containerd-serial-ec2
  • /test pull-kubernetes-node-e2e-containerd-serial-ec2-eks
  • /test pull-kubernetes-node-e2e-containerd-standalone-mode
  • /test pull-kubernetes-node-e2e-containerd-standalone-mode-all-alpha
  • /test pull-kubernetes-node-e2e-cri-proxy-serial
  • /test pull-kubernetes-node-e2e-crio-cgrpv1-dra
  • /test pull-kubernetes-node-e2e-crio-cgrpv1-dra-kubetest2
  • /test pull-kubernetes-node-e2e-crio-cgrpv2-dra
  • /test pull-kubernetes-node-e2e-crio-cgrpv2-dra-kubetest2
  • /test pull-kubernetes-node-e2e-resource-health-status
  • /test pull-kubernetes-node-kubelet-containerd-flaky
  • /test pull-kubernetes-node-kubelet-credential-provider
  • /test pull-kubernetes-node-kubelet-serial-containerd
  • /test pull-kubernetes-node-kubelet-serial-containerd-alpha-features
  • /test pull-kubernetes-node-kubelet-serial-containerd-kubetest2
  • /test pull-kubernetes-node-kubelet-serial-containerd-sidecar-containers
  • /test pull-kubernetes-node-kubelet-serial-cpu-manager
  • /test pull-kubernetes-node-kubelet-serial-cpu-manager-kubetest2
  • /test pull-kubernetes-node-kubelet-serial-crio-cgroupv1
  • /test pull-kubernetes-node-kubelet-serial-crio-cgroupv1-kubetest2
  • /test pull-kubernetes-node-kubelet-serial-crio-cgroupv2
  • /test pull-kubernetes-node-kubelet-serial-crio-cgroupv2-kubetest2
  • /test pull-kubernetes-node-kubelet-serial-hugepages
  • /test pull-kubernetes-node-kubelet-serial-memory-manager
  • /test pull-kubernetes-node-kubelet-serial-podresize
  • /test pull-kubernetes-node-kubelet-serial-podresources
  • /test pull-kubernetes-node-kubelet-serial-topology-manager
  • /test pull-kubernetes-node-kubelet-serial-topology-manager-kubetest2
  • /test pull-kubernetes-node-swap-conformance-fedora-serial
  • /test pull-kubernetes-node-swap-conformance-ubuntu-serial
  • /test pull-kubernetes-node-swap-fedora
  • /test pull-kubernetes-node-swap-fedora-serial
  • /test pull-kubernetes-node-swap-ubuntu-serial
  • /test pull-kubernetes-scheduler-perf
  • /test pull-kubernetes-unit-experimental
  • /test pull-publishing-bot-validate

Use /test all to run the following jobs that were automatically triggered:

  • pull-kubernetes-conformance-kind-ga-only-parallel
  • pull-kubernetes-dependencies
  • pull-kubernetes-e2e-ec2
  • pull-kubernetes-e2e-ec2-conformance
  • pull-kubernetes-e2e-gce
  • pull-kubernetes-e2e-gce-canary
  • pull-kubernetes-e2e-inplace-pod-resize-containerd-main-v2
  • pull-kubernetes-e2e-kind
  • pull-kubernetes-e2e-kind-ipv6
  • pull-kubernetes-integration
  • pull-kubernetes-linter-hints
  • pull-kubernetes-node-e2e-containerd
  • pull-kubernetes-typecheck
  • pull-kubernetes-unit
  • pull-kubernetes-verify
  • pull-kubernetes-verify-lint

In response to this:

/test

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.

@kannon92
Copy link
Contributor

kannon92 commented Nov 7, 2024

/test pull-kubernetes-e2e-capz-windows-alpha-feature-vpa

@kannon92
Copy link
Contributor

kannon92 commented Nov 7, 2024

cc @marosset

@kannon92
Copy link
Contributor

kannon92 commented Nov 7, 2024

/test pull-kubernetes-e2e-capz-windows-alpha-feature-vpa
/test pull-kubernetes-e2e-capz-windows-alpha-features
/test pull-kubernetes-e2e-capz-windows-master
/test pull-kubernetes-e2e-capz-windows-serial-slow

@AnishShah
Copy link
Contributor Author

Failures from these windows jobs seem unrelated - https://testgrid.k8s.io/sig-windows-master-release#capz-windows-master-alpha-features

InPlacePodVerticalScaling tests are being skipped.

@k8s-ci-robot k8s-ci-robot merged commit dff657a into kubernetes:master Nov 7, 2024
14 of 18 checks passed
@AnishShah AnishShah deleted the ippr-windows branch November 7, 2024 00:25
@esotsal esotsal mentioned this pull request Nov 22, 2024
Open
@esotsal
Copy link

esotsal commented Nov 22, 2024 •
edited
Loading

Failures from these windows jobs seem unrelated - https://testgrid.k8s.io/sig-windows-master-release#capz-windows-master-alpha-features

InPlacePodVerticalScaling tests are being skipped.

Please check #128897 (comment) , WDYT @AnishShah

@marosset marosset mentioned this pull request Dec 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tallclair tallclair tallclair left review comments

@thockin thockin thockin left review comments

@bart0sh bart0sh Awaiting requested review from bart0sh

@caesarxuchao caesarxuchao Awaiting requested review from caesarxuchao

Assignees

@thockin thockin

@tallclair tallclair

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Apps
Archived in project
SIG Node: In Place Pod Vertical Scaling
Status: Done
SIG Node: code and documentation PRs
Status: Done
[sig-release] Bug Triage
Archived in project
Milestone
v1.32
Development

Successfully merging this pull request may close these issues.

[FG:InPlacePodVerticalScaling] drop support for windows
7 participants
@AnishShah @tallclair @k8s-ci-robot @kannon92 @knabben @esotsal @thockin