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: add PersistentPodState custom workload support #1063

Merged
merged 1 commit into from
Dec 9, 2022

Conversation

baxiaoshi
Copy link
Contributor

Ⅰ. Describe what this PR does

PPS(PersistentPodState) currently has support for standard statefulset and kruise's statefulSet. However, some companies will customize workloads(statefulset like) with stable identification pods, support such workloads, and add support for custom workloads to pps.

@kruise-bot
Copy link

Welcome @baxiaoshi! It looks like this is your first PR to openkruise/kruise 🎉

@kruise-bot kruise-bot added the size/L size/L: 100-499 label Aug 24, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2022

Codecov Report

Merging #1063 (6033887) into master (bdd3efb) will decrease coverage by 1.08%.
The diff coverage is 40.36%.

❗ Current head 6033887 differs from pull request most recent head 0f47468. Consider uploading reports for the commit 0f47468 to get more accurate results

@@            Coverage Diff             @@
##           master    #1063      +/-   ##
==========================================
- Coverage   50.36%   49.28%   -1.09%     
==========================================
  Files         125      137      +12     
  Lines       17637    19410    +1773     
==========================================
+ Hits         8883     9566     +683     
- Misses       7782     8790    +1008     
- Partials      972     1054      +82     
Flag Coverage Δ
unittests 49.28% <40.36%> (-1.09%) ⬇️

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

Impacted Files Coverage Δ
pkg/control/pubcontrol/pub_control.go 26.21% <0.00%> (ø)
pkg/control/pubcontrol/pub_control_utils.go 52.02% <0.00%> (ø)
pkg/control/sidecarcontrol/history_control.go 0.00% <0.00%> (ø)
...ontroller/daemonset/daemonset_predownload_image.go 0.00% <0.00%> (ø)
pkg/controller/daemonset/daemonset_update.go 57.36% <0.00%> (-0.93%) ⬇️
...troller/nodepodprobe/nodepodprobe_event_handler.go 0.00% <0.00%> (ø)
...tentpodstate/persistent_pod_state_event_handler.go 0.00% <0.00%> (ø)
...ler/podprobemarker/podprobemarker_event_handler.go 0.00% <0.00%> (ø)
...availablebudget/podunavailablebudget_controller.go 39.22% <0.00%> (ø)
...ller/podunavailablebudget/pub_pod_event_handler.go 26.47% <0.00%> (ø)
... and 54 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

1)Get the APIResourceList in the cluster through the ServerPreferredResources interface of the dynamic client. Because we can't distinguish those workloads, we can only watch all of them here, which will have a lot of unnecessary watch overhead.
2)Find the corresponding workload by the reverse of the pod's ownerReferences. There is a time delay.

It is recommended to use method 2 to get the owner's gvk by reverse lookup and create Informer if there is no watch before.
Copy link
Member

Choose a reason for hiding this comment

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

PPS rely on the ordinal maintenance of statefulset, and actually pps can works for statefulset-like workload how to ensure custom workload can provide similar capability and avoid incorrect usage? maybe adding a new config in kruise-configuration configmaps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add PPS_Watch_Custom_Workload_WhiteList in kruise-configuration to declare the Workload that needs to be watched

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 kruise-bot added lgtm and removed lgtm labels Aug 30, 2022
@kruise-bot kruise-bot added size/XL size/XL: 500-999 and removed size/L size/L: 100-499 labels Sep 20, 2022
}

// 2.add watch
for gvk, _ := range ownerReferencesGVK {
Copy link

Choose a reason for hiding this comment

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

S1005: unnecessary assignment to the blank identifier


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

}

cli := s.createStatefulSetLikeCli(sts.Namespace)
data, err := cli.Get(context.TODO(), sts.Name, metav1.GetOptions{})
Copy link

Choose a reason for hiding this comment

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

SA4006: this value of err is never used


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

}

// 2.add watch
for gvk, _ := range ownerReferencesGVK {
Copy link

Choose a reason for hiding this comment

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

gofmt: File is not gofmt-ed with -s


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -90,6 +92,22 @@
UID types.UID `json:"uid" protobuf:"bytes,4,opt,name=uid,casttype=k8s.io/apimachinery/pkg/types.UID"`
}

Copy link

Choose a reason for hiding this comment

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

💬 2 similar findings have been found in this PR


U1000: type statefulSetLike is unused


🔎 Expand here to view all instances of this finding
File Path Line Number
pkg/util/controllerfinder/controller_finder.go 101
pkg/util/controllerfinder/controller_finder.go 107

Visit the Lift Web Console to find more details in your report.


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -0,0 +1,67 @@
package util

import (
Copy link

Choose a reason for hiding this comment

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

💬 2 similar findings have been found in this PR


goimports: File is not goimports-ed


🔎 Expand here to view all instances of this finding
File Path Line Number
pkg/util/controllerfinder/controller_finder.go 20
pkg/controller/persistentpodstate/persistent_pod_state_event_handler.go 22

Visit the Lift Web Console to find more details in your report.


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

return false, nil
}

if _, ok := watcherMap.Load(gvk); ok {
Copy link
Member

Choose a reason for hiding this comment

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

plz swap this line with DiscoverGVK

@@ -90,6 +93,22 @@ type ControllerReference struct {
UID types.UID `json:"uid" protobuf:"bytes,4,opt,name=uid,casttype=k8s.io/apimachinery/pkg/types.UID"`
}

type statefulSetLike struct {
Copy link

Choose a reason for hiding this comment

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

💬 2 similar findings have been found in this PR


U1000: type statefulSetLike is unused


🔎 Expand here to view all instances of this finding
File Path Line Number
pkg/util/controllerfinder/controller_finder.go 102
pkg/util/controllerfinder/controller_finder.go 108

Visit the Lift Web Console to find more details in your report.


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -19,6 +19,7 @@ package configuration
import (
"context"
"encoding/json"
Copy link

Choose a reason for hiding this comment

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

💬 2 similar findings have been found in this PR


goimports: File is not goimports-ed


🔎 Expand here to view all instances of this finding
File Path Line Number
pkg/webhook/persistentpodstate/validating/persistent_create_update_handler.go 22
pkg/webhook/persistentpodstate/validating/persistent_validating_test.go 21

Visit the Lift Web Console to find more details in your report.


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@baxiaoshi baxiaoshi force-pushed the master branch 2 times, most recently from c6625e5 to 5a0f954 Compare September 28, 2022 02:57

whiteList, err := configuration.GetPPSWatchWatchCustomWorkloadWhiteList(h.Client)
if err != nil {
err = fmt.Errorf("Failed to get persistent pod state config white list, error: %v\n", err.Error())
Copy link

Choose a reason for hiding this comment

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

ST1005: error strings should not be capitalized


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]


whiteList, err := configuration.GetPPSWatchWatchCustomWorkloadWhiteList(h.Client)
if err != nil {
err = fmt.Errorf("Failed to get persistent pod state config white list, error: %v\n", err.Error())
Copy link

Choose a reason for hiding this comment

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

ST1005: error strings should not end with punctuation or newlines


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]


whiteList, err := configuration.GetPPSWatchWatchCustomWorkloadWhiteList(h.Client)
if err != nil {
err = fmt.Errorf("failed to get persistent pod state config white list, error: %v\n", err.Error())
Copy link

Choose a reason for hiding this comment

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

ST1005: error strings should not end with punctuation or newlines


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -17,11 +17,11 @@
package mutating

import (
Copy link

Choose a reason for hiding this comment

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

goimports: File is not goimports-ed


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -44,6 +48,9 @@ type PersistentPodStateSpec struct {
// current only support StatefulSet
TargetReference TargetReference `json:"targetRef"`

// Persist the annotations information of the pods that need to be saved
PersistentPodAnnotations []string `json:"persistentPodAnnotations,omitempty"`
Copy link
Member

@zmberg zmberg Oct 17, 2022

Choose a reason for hiding this comment

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

// PersistentPodStateSpec defines the desired state of PersistentPodState
type PersistentPodStateSpec struct {
	PersistentPodAnnotations []PersistentPodAnnotation `json:"persistentPodAnnotations,omitempty"`
}

type PersistentPodAnnotation struct {
	Key string `json:"key"`
}

continue
}

_, err := util.AddWatcherDynamically(runtimeController, workloadHandler, gvk)
Copy link
Member

Choose a reason for hiding this comment

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

It is recommended not to implement the watch dynamic crd method in the event handler, but in reconcile, please refer to: https://github.com/openkruise/rollouts/blob/master/pkg/controller/batchrelease/ batchrelease_controller.go#L165

od, _ := json.Marshal(oWorkload)
nd, _ := json.Marshal(nWorkload)
klog.V(3).Infof("enqueueRequestForStatefulSetLike(%s) old workload", oWorkload.GetName(), string(od))
klog.V(3).Infof("enqueueRequestForStatefulSetLike(%s) new workload", oWorkload.GetName(), string(nd))
Copy link
Member

Choose a reason for hiding this comment

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

These two logs remove it, it is estimated that will brush the screen.

}
}

if nAnnotations[appsv1alpha1.AnnotationAutoGeneratePersistentPodState] == "true" &&
Copy link
Member

Choose a reason for hiding this comment

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

Can this be removed?

@@ -20,6 +20,8 @@ import (
"context"
"encoding/json"

v1 "k8s.io/apimachinery/pkg/apis/meta/v1"

Copy link
Member

Choose a reason for hiding this comment

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

go imports -ed

allErrs = append(allErrs, field.Invalid(fldPath.Child("TargetReference"), spec.TargetReference, "TargetReference.Kind must be StatefulSet"))

apiVersion, kind := spec.TargetReference.APIVersion, spec.TargetReference.Kind
if spec.TargetReference.Kind != "StatefulSet" && !whiteList.ValidateAPIVersionAndKind(apiVersion, kind) {
Copy link
Member

Choose a reason for hiding this comment

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

I think not the native or kruise StatefulSet, all other CRD(e.g. other StatefulSet) should judge whiteList.

@@ -20,6 +20,8 @@ import (
"context"
"testing"

corev1 "k8s.io/api/core/v1"

Copy link
Member

Choose a reason for hiding this comment

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

go imports -ed

ref := metav1.GetControllerOf(pod)
if ref == nil || ref.Kind != "StatefulSet" {
if ref == nil || (ref.Kind != "StatefulSet" && !whiteList.ValidateAPIVersionAndKind(ref.APIVersion, ref.Kind)) {
Copy link
Member

Choose a reason for hiding this comment

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

If Custom Kind StatefulSet but not in whitelist, i think should skip the webhook.

@kruise-bot kruise-bot added size/XXL and removed size/XL size/XL: 500-999 labels Nov 9, 2022
@@ -23,6 +23,10 @@ import (
"strconv"
"strings"

"github.com/openkruise/kruise/pkg/util/configuration"

Copy link
Member

Choose a reason for hiding this comment

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

plz format import

@@ -20,16 +20,20 @@ import (
"context"
"fmt"

"github.com/openkruise/kruise/pkg/util/configuration"

"k8s.io/klog/v2"
Copy link
Member

Choose a reason for hiding this comment

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

plz format imports


var watcherMap = sync.Map{}

func DiscoverGVK(gvk schema.GroupVersionKind) bool {
Copy link
Member

Choose a reason for hiding this comment

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

this func has been defined in [pkg/util/discovery/discovery.go](https://github.com/openkruise/kruise/blob/14e21f5bcfd40790861e6db3dc75655c9c37b189/pkg/util/discovery/discovery.go)

@@ -49,12 +51,30 @@ func GetSidecarSetPatchMetadataWhiteList(client client.Client) (*SidecarSetPatch
return whiteList, nil
}

func GetPPSWatchWatchCustomWorkloadWhiteList(client client.Client) (*PPSWatchWatchCustomWorkloadWhiteList, error) {
whiteList := &PPSWatchWatchCustomWorkloadWhiteList{Workloads: make([]schema.GroupVersionKind, 0)}
data, err := getKruiseConfiguration(client)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe, we can just initialize the whiteList in a init function at the moment of kruise starting?

@veophi
Copy link
Member

veophi commented Nov 11, 2022

Very awesome work, thanks! @baxiaoshi

@zmberg
Copy link
Member

zmberg commented Dec 9, 2022

/lgtm

@furykerry
Copy link
Member

/approve

@kruise-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: furykerry

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

@kruise-bot kruise-bot merged commit cdb3b02 into openkruise:master Dec 9, 2022
@zmberg zmberg added this to the v1.4 milestone Feb 14, 2023
@zmberg zmberg added the kind/enhancement New feature or request label Feb 14, 2023
ppbits pushed a commit to ppbits/kruise that referenced this pull request Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants