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

On pod creation, if a new pod matches the SidecarSet update strategy … #1689

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

AiRanthem
Copy link
Contributor

…selector, the latest revision rather than that specified in the sidecarset.spec.injectionStrategy will be injected.

Ⅰ. Describe what this PR does

When a Pod is created, the SidecarSet's Webhook will first determine through the UpdateStrategy whether the Pod is undergoing a Sidecar canary upgrade. If so, it will directly inject the latest Sidecar and skip the logic of selecting a specified revision of the SidecarSet based on the InjectionStrategy.

Ⅱ. Does this pull request fix one issue?

fixes #1539

Ⅲ. Describe how to verify it

  1. create the sidecarset

    # sidecarset.yaml
    apiVersion: apps.kruise.io/v1alpha1
    kind: SidecarSet
    metadata:
      name: test-sidecarset
    spec:
      selector:
        matchLabels:
          app: sleep
      updateStrategy:
        type: RollingUpdate
        maxUnavailable: 1
        selector:
          matchLabels:
            canary: "true"
      containers:
        - name: sidecar
          image: curlimages/curl:8.8.0
          command: ["/bin/sleep", "infinity"]
  2. deploy the demo app

    apiVersion: apps/v1
    kind: Deployment
    metadata:
      name: sleep-canary
    spec:
      replicas: 1
      selector:
        matchLabels:
          name: sleep-canary
          app: sleep
      template:
        metadata:
          labels:
            app: sleep
            canary: "true"
            name: sleep-canary
        spec:
          terminationGracePeriodSeconds: 0
          containers:
            - name: sleep
              image: curlimages/curl:8.8.0
              command: ["/bin/sleep", "infinity"]
              imagePullPolicy: IfNotPresent
    ---
    apiVersion: apps/v1
    kind: Deployment
    metadata:
      name: sleep
    spec:
      replicas: 1
      selector:
        matchLabels:
          name: sleep
      template:
        metadata:
          labels:
            name: sleep
            app: sleep
        spec:
          terminationGracePeriodSeconds: 0
          containers:
            - name: sleep
              image: curlimages/curl:8.8.0
              command: ["/bin/sleep", "infinity"]
              imagePullPolicy: IfNotPresent
  3. upgrade the sidecarset. because of env is changed, all pods will be skipped, nothing will happen

    # upgrade.yaml
    apiVersion: apps.kruise.io/v1alpha1
    kind: SidecarSet
    metadata:
      name: test-sidecarset
    spec:
      selector:
        matchLabels:
          app: sleep
      updateStrategy:
        type: RollingUpdate
        maxUnavailable: 1
        selector:
          matchLabels:
            canary: "true"
      containers:
        - name: sidecar
          image: curlimages/curl:8.9.0
          command: ["/bin/sleep", "infinity"]
          env:
            - name: SOME_ENV
              value: "some-value"
      injectionStrategy:
        revision:
          revisionName: test-sidecarset-7dcd95bc8f # may need to be edited based on the actual situation
  4. scale both the stable and canary deployments: replicas 1 -> 2

  5. view images of sidecars injected: Only the newly created canary Pod is injected with sidecar version 8.9.0.

Ⅳ. Special notes for reviews

@kruise-bot kruise-bot added the size/L size/L: 100-499 label Aug 2, 2024
Copy link

codecov bot commented Aug 2, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.

Project coverage is 48.83%. Comparing base (0d0031a) to head (5fe5df1).
Report is 84 commits behind head on master.

Files with missing lines Patch % Lines
pkg/webhook/pod/mutating/sidecarset.go 66.66% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1689      +/-   ##
==========================================
+ Coverage   47.91%   48.83%   +0.92%     
==========================================
  Files         162      188      +26     
  Lines       23491    19298    -4193     
==========================================
- Hits        11256     9425    -1831     
+ Misses      11014     8641    -2373     
- Partials     1221     1232      +11     
Flag Coverage Δ
unittests 48.83% <66.66%> (+0.92%) ⬆️

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.

@AiRanthem AiRanthem force-pushed the feature/inject-update-selector branch 2 times, most recently from 40bb7a7 to 88c866f Compare August 5, 2024 11:05
test/e2e/apps/sidecarset.go Outdated Show resolved Hide resolved
test/e2e/apps/sidecarset.go Outdated Show resolved Hide resolved
Copy link
Member

@furykerry furykerry left a comment

Choose a reason for hiding this comment

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

/lgtm

@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 ask for approval from furykerry by writing /assign @furykerry 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

@AiRanthem AiRanthem force-pushed the feature/inject-update-selector branch 2 times, most recently from 101fdb2 to 0c45705 Compare August 6, 2024 11:55
Copy link
Member

@furykerry furykerry left a comment

Choose a reason for hiding this comment

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

/lgtm

…selector, the latest revision rather than that specified in the sidecarset.spec.injectionStrategy will be injected.

Signed-off-by: AiRanthem <zhongtianyun.zty@alibaba-inc.com>
@AiRanthem AiRanthem force-pushed the feature/inject-update-selector branch from 38d34aa to 5fe5df1 Compare August 20, 2024 09:40
Copy link
Member

@furykerry furykerry left a comment

Choose a reason for hiding this comment

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

/lgtm

@zmberg
Copy link
Member

zmberg commented Sep 10, 2024

/approve

@zmberg zmberg merged commit 2d992bf into openkruise:master Sep 10, 2024
39 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm size/L size/L: 100-499
Projects
None yet
4 participants