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

feat: enhanced in-place update module to support vertical scaling #1353

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

LavenderQAQ
Copy link

@LavenderQAQ LavenderQAQ commented Aug 2, 2023

Ⅰ. Describe what this PR does

Enhanced the in-place update module to enable kruise's workload to modify resources without restarting the pod.

Ⅱ. Does this pull request fix one issue?

Fixes #1212

Ⅲ. Describe how to verify it

Need to kubernetes v1.27 version, and open feature gate InPlacePodVerticalScaling. Here is my kind configuration:

kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
featureGates:
  "InPlacePodVerticalScaling": true
nodes:
- role: control-plane
  image: kindest/node:v1.27.3@sha256:3966ac761ae0136263ffdb6cfd4db23ef8a83cba8a463690e98317add2c9ba72
- role: worker
  image: kindest/node:v1.27.3@sha256:3966ac761ae0136263ffdb6cfd4db23ef8a83cba8a463690e98317add2c9ba72
- role: worker
  image: kindest/node:v1.27.3@sha256:3966ac761ae0136263ffdb6cfd4db23ef8a83cba8a463690e98317add2c9ba72
- role: worker
  image: kindest/node:v1.27.3@sha256:3966ac761ae0136263ffdb6cfd4db23ef8a83cba8a463690e98317add2c9ba72

Build a cloneset (or other workload that includes in-place updates) and set updateStrategy to InPlaceIfPossible. Here's an example:

apiVersion: apps.kruise.io/v1alpha1
kind: CloneSet
metadata:
  name: nginx-cloneset
spec:
  updateStrategy:
    type: InPlaceIfPossible
  replicas: 3
  selector:
    matchLabels:
      app: nginx
  template:
    metadata:
      labels:
        app: nginx
    spec:
      containers:
      - name: nginx-1
        image: nginx:1.21.1
        ports:
        - containerPort: 80
        resources:
          limits:
            cpu: "1"
            memory: "256Mi"
          requests:
            cpu: "0.5"
            memory: "128Mi"

Change the workload's resources, wait a while, and you'll find that the pod's resources will change, and the pod won't restart.

Ⅳ. Special notes for reviews

The control of Status for vertical scaling requires a higher version of the k8s api, which makes this pr need to wait for kruise version upgrade. It cannot be merged at this time because it is not yet complete.

@kruise-bot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign veophi for approval by writing /assign @veophi in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@sonatype-lift
Copy link

sonatype-lift bot commented Aug 2, 2023

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

@kruise-bot kruise-bot added the size/L size/L: 100-499 label Aug 2, 2023
@LavenderQAQ LavenderQAQ changed the title feat: enhanced in-place update module to support vertical scaling feat: enhanced inplace update module to support vertical scaling Aug 2, 2023
@LavenderQAQ LavenderQAQ changed the title feat: enhanced inplace update module to support vertical scaling feat: enhanced in-place update module to support vertical scaling Aug 2, 2023
@LavenderQAQ LavenderQAQ force-pushed the feat/inplace-workload-vertical-scaling branch 4 times, most recently from 08c47c7 to 04b5eb8 Compare August 7, 2023 03:04
@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2023

Codecov Report

Attention: Patch coverage is 72.50000% with 55 lines in your changes missing coverage. Please review.

Project coverage is 49.40%. Comparing base (0d0031a) to head (eea019c).
Report is 85 commits behind head on master.

Files with missing lines Patch % Lines
pkg/util/inplaceupdate/inplace_update_defaults.go 77.00% 17 Missing and 6 partials ⚠️
pkg/util/inplaceupdate/inplace_update_vertical.go 76.74% 11 Missing and 9 partials ⚠️
pkg/controller/cloneset/core/cloneset_core.go 0.00% 6 Missing ⚠️
...kg/controller/cloneset/sync/cloneset_sync_utils.go 0.00% 3 Missing ⚠️
pkg/util/inplaceupdate/inplace_update.go 40.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1353      +/-   ##
==========================================
+ Coverage   47.91%   49.40%   +1.49%     
==========================================
  Files         162      192      +30     
  Lines       23491    19752    -3739     
==========================================
- Hits        11256     9759    -1497     
+ Misses      11014     8730    -2284     
- Partials     1221     1263      +42     
Flag Coverage Δ
unittests 49.40% <72.50%> (+1.49%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LavenderQAQ LavenderQAQ force-pushed the feat/inplace-workload-vertical-scaling branch from 0470ce2 to 4e13c0d Compare August 7, 2023 03:56
@kruise-bot kruise-bot added needs-rebase size/XL size/XL: 500-999 and removed size/L size/L: 100-499 labels Aug 8, 2023
@LavenderQAQ LavenderQAQ force-pushed the feat/inplace-workload-vertical-scaling branch from 35c4939 to 28406cb Compare August 8, 2023 11:46
@LavenderQAQ LavenderQAQ force-pushed the feat/inplace-workload-vertical-scaling branch 2 times, most recently from 48126ff to aca435f Compare August 8, 2023 12:04
@LavenderQAQ LavenderQAQ marked this pull request as ready for review August 8, 2023 13:16
@kruise-bot kruise-bot requested a review from veophi August 8, 2023 13:16
@LavenderQAQ
Copy link
Author

LavenderQAQ commented Aug 8, 2023

The complete logic needs to wait until the k8s api of kruise is upgraded.

@LavenderQAQ
Copy link
Author

/hold

@stale stale bot removed wontfix This will not be worked on labels Aug 28, 2024
@LavenderQAQ LavenderQAQ force-pushed the feat/inplace-workload-vertical-scaling branch 2 times, most recently from 1f55de3 to bff411c Compare August 28, 2024 06:07
@LavenderQAQ
Copy link
Author

I rebased it first, and I need to take some time to sort through the logic and fill in some tests. After all, this pr is a bit old.

Copy link
Member

@ABNER-1 ABNER-1 left a comment

Choose a reason for hiding this comment

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

Some methods in VerticalUpdateInterface appear to be unimplemented.

pkg/util/inplaceupdate/inplace_update_defaults.go Outdated Show resolved Hide resolved
pkg/util/inplaceupdate/inplace_update_vertical.go Outdated Show resolved Hide resolved
test/e2e/apps/daemonset.go Outdated Show resolved Hide resolved
@ABNER-1 ABNER-1 force-pushed the feat/inplace-workload-vertical-scaling branch 3 times, most recently from 3234854 to 88c2a0a Compare September 11, 2024 13:26
@ABNER-1 ABNER-1 force-pushed the feat/inplace-workload-vertical-scaling branch 11 times, most recently from 3f4af6f to a12055b Compare September 14, 2024 09:30
@ABNER-1
Copy link
Member

ABNER-1 commented Sep 14, 2024

I encountered two errors while adding e2e test cases:

  1. When modifying the image and increasing resources simultaneously, I encountered a RunContainerError. I have already submitted an issue to the Kubernetes community at [InPlacePodVerticalScaling]Got RunContainerError when patch pod image and resources kubernetes/kubernetes#127356.

  2. When I scaled the CPU limit of resources from 100m to 200m, I observed that occasionally the status was first changed by kubelet from 100m InProcess to 200m, and then it was set back to 100m InProcess by kubelet, and after a while, it finally became 200m.

@ABNER-1 ABNER-1 force-pushed the feat/inplace-workload-vertical-scaling branch from a12055b to eea019c Compare September 14, 2024 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature request] In-place udpate support resources
4 participants