store history revisions for sidecarset #715

veophi · 2021-08-18T09:40:23Z

Signed-off-by: veophi [email protected]

Ⅰ. Describe what this PR does

Store history revisions for sidecarset.

Store sidecarset.spec using controllerRevision;
All controllerRevisions of sidecarset will be stored in the namespace of kruise-manager;
We set a limited number of stored revisions (default is 10).

Add revision annotation for matched pods.

This annotation can remind users which sidecar is not the latest;
This annotation may be used in the future(e.g., for the future plan of replacing update operation with patch).

Ⅱ. Does this pull request fix one issue?

fixes #710

Ⅲ. What does it look like?

sonatype-lift · 2021-08-18T09:56:21Z

pkg/controller/sidecarset/sidecarset_processor.go

+func (p *Processor) updatePodAnnotationsAboutSidecarNameAndRevision(control sidecarcontrol.SidecarControl, pod *corev1.Pod, latestRevision *apps.ControllerRevision) {
+	sidecarSet := control.GetSidecarset()
+	sidecarSetRevisions := make(map[string]string)
+	if revisions, _ := pod.Annotations[sidecarcontrol.SidecarSetRevisionAnnotation]; len(revisions) > 0 {


S1005: unnecessary assignment to the blank identifier
(at-me in a reply with help or ignore)

sonatype-lift · 2021-08-18T09:56:23Z

pkg/controller/sidecarset/sidecarset_processor_test.go

+
+	showme := map[string]string{"s1": "latest"}
+	by, _ := json.Marshal(showme)
+	fmt.Printf(string(by))


SA1006: printf-style function with dynamic format string and no further arguments should use print-style function instead
(at-me in a reply with help or ignore)

sonatype-lift · 2021-08-18T09:56:24Z

pkg/controller/sidecarset/sidecarset_processor_test.go

+	// case 4
+	for i := 0; i < 100; i++ {
+		sidecarSet.Spec.Containers[0].Image = fmt.Sprintf("%d", i)
+		latestRevision, _, err = processor.registerLatestRevision(sidecarSet, selector)


SA4006: this value of err is never used
(at-me in a reply with help or ignore)

sonatype-lift · 2021-08-18T09:56:25Z

pkg/controller/sidecarset/sidecarset_processor_test.go

+	// case 4
+	for i := 0; i < 100; i++ {
+		sidecarSet.Spec.Containers[0].Image = fmt.Sprintf("%d", i)
+		latestRevision, _, err = processor.registerLatestRevision(sidecarSet, selector)


SA4006: this value of latestRevision is never used
(at-me in a reply with help or ignore)

codecov-commenter · 2021-08-19T04:04:35Z

Codecov Report

Merging #715 (1e30627) into master (8d73235) will decrease coverage by 0.27%.
The diff coverage is 41.61%.

@@            Coverage Diff             @@
##           master     #715      +/-   ##
==========================================
- Coverage   48.02%   47.74%   -0.28%     
==========================================
  Files         118      119       +1     
  Lines       10854    10999     +145     
==========================================
+ Hits         5213     5252      +39     
- Misses       4839     4939     +100     
- Partials      802      808       +6

Flag	Coverage Δ
unittests	`47.74% <41.61%> (-0.28%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/control/sidecarcontrol/history_control.go	`0.00% <0.00%> (ø)`
pkg/controller/sidecarset/sidecarset_controller.go	`13.88% <ø> (ø)`
pkg/control/sidecarcontrol/util.go	`43.68% <71.42%> (-0.32%)`	⬇️
pkg/controller/sidecarset/sidecarset_processor.go	`72.80% <73.07%> (-1.34%)`	⬇️
pkg/controller/daemonset/update.go	`24.24% <0.00%> (-2.53%)`	⬇️
pkg/controller/daemonset/daemonset_util.go	`60.00% <0.00%> (-2.31%)`	⬇️
pkg/controller/uniteddeployment/revision.go	`66.15% <0.00%> (-1.54%)`	⬇️
...tating/container_launch_priority_initialization.go	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d73235...1e30627. Read the comment docs.

apis/apps/v1alpha1/sidecarset_types.go

FillZpp · 2021-08-19T06:39:44Z

pkg/control/sidecarcontrol/history_control.go

+	var raw map[string]interface{}
+	_ = json.Unmarshal(str, &raw)
+	objCopy := make(map[string]interface{})
+	objCopy["spec"] = raw["spec"].(map[string]interface{})


Should not use the whole spec as patch, fields like selector, updateStrategy should be excluded.

FillZpp · 2021-08-19T06:41:18Z

pkg/control/sidecarcontrol/history_control.go

+	if cr.Labels == nil {
+		cr.Labels = make(map[string]string)
+	}
+	cr.SetLabels(r.GetRevisionLabelSelector(s).MatchLabels)


Do not use SetLabels, it will replace the original labels in it.

Why we need GetRevisionLabelSelector? How about directly cr.Labels["SidecarSet"] = s.Name here?

Rename "SidecarSet" to kruise.io/sidecarset-name and make it a variable in util.go.

FillZpp · 2021-08-19T06:56:47Z

pkg/controller/sidecarset/sidecarset_processor.go

+	sidecarSetClone := s.DeepCopy()
+	// set controllerRevision namespace, this namespace must have very
+	// strict permissions for average users. Here use namespace of kruise-manager
+	sidecarSetClone.SetNamespace(webhookutil.GetNamespace())


Do not set the namespace in SidecarSet. Just use webhookutil.GetNamespace() in the places need it.

in kubernetes/pkg/controller/history/controller_history.go, func CreateControllerRevision uses parent's namespace to create ControllerRevision, so we have to set sidecarset namespace here.

Or we might copy these codes and modify them.

FillZpp · 2021-08-19T07:09:14Z

pkg/controller/sidecarset/sidecarset_processor.go

+	if len(revisions) == 0 || len(revisions) <= limited {
+		return nil
+	}
+	if err := p.historyController.DeleteControllerRevision(revisions[0]); err != nil {


Should not delete the ControllerRevisions that have existing pods in these revisions.

So should we count references for each revisions?

FillZpp · 2021-08-19T07:14:33Z

pkg/controller/sidecarset/sidecarset_processor.go

+	return nil
+}
+
+func getCurrentStatus(control sidecarcontrol.SidecarControl, pods []*corev1.Pod,


Just add latestRevision and collisionCount into calculateStatus. No need to define getCurrentStatus and setStatusRevisionInfo

FillZpp · 2021-08-19T07:35:34Z

pkg/controller/sidecarset/sidecarset_processor.go

+			// in the future, we will support revision control for sidecar injection,
+			// some pods may be injected old sidecarSet
+			if _, ok := sidecarSetRevisions[sidecar]; !ok {
+				sidecarSetRevisions[sidecar] = sidecarcontrol.SidecarLatestRevision


Don't really understand what does the SidecarLatestRevision for?

I realize I'm wrong and I'll fix it as soon as possible.

FillZpp · 2021-08-19T07:38:14Z

pkg/controller/sidecarset/sidecarset_processor.go

+	sidecarCount := len(strings.Split(sidecarSetNames, ","))
+	if !nameOk || sidecarCount == 0 || len(sidecarSetRevisions) != sidecarCount {
+		sidecarSets := p.listMatchedSidecarSets(pod)
+		for _, sidecar := range sidecarSets {


Maybe here should only manage the sidecar revision of this sidecarset in the pod annotation.

I continue to use the name list annotation just for forward compatibility.

zmberg · 2021-08-20T02:21:14Z

pkg/control/sidecarcontrol/util.go

@@ -38,6 +38,10 @@ const (
 	// SidecarSetHashWithoutImageAnnotation represents the key of a sidecarset hash without images of sidecar
 	SidecarSetHashWithoutImageAnnotation = "kruise.io/sidecarset-hash-without-image"

+	// SidecarSetHashAnnotation represents the revision of a sidecarSet
+	SidecarSetRevisionAnnotation = "kruise.io/sidecarset-injected-revision"


Why not put the revision in SidecarSetUpgradeSpec?

zmberg · 2021-08-20T02:31:10Z

pkg/controller/sidecarset/sidecarset_processor.go

@@ -204,7 +250,7 @@ func (p *Processor) listMatchedSidecarSets(pod *corev1.Pod) string {
 		}
 	}

-	return strings.Join(sidecarSetNames, ",")
+	return sidecarSetNames
 }

 func (p *Processor) updateSidecarSetStatus(sidecarSet *appsv1alpha1.SidecarSet, status *appsv1alpha1.SidecarSetStatus) error {


Is function inconsistentStatus necessary to determine revision filed?

zmberg · 2021-08-20T02:34:59Z

pkg/controller/sidecarset/sidecarset_processor.go

@@ -127,13 +142,13 @@ func (p *Processor) UpdateSidecarSet(sidecarSet *appsv1alpha1.SidecarSet) (recon
 	}

 	// 7. upgrade pod sidecar
-	if err := p.updatePods(control, pods); err != nil {
+	if err := p.updatePods(control, pods, latestRevision); err != nil {


sidecarSet.status already contains the latestRevison, so don't need to add additional parameters latestRevision

Do you mean we should execute "get status" in this function?

zmberg · 2021-08-20T02:36:40Z

pkg/controller/sidecarset/sidecarset_processor.go

+		// update SidecarSetRevisionAnnotation and SidecarSetRevisionAnnotation of pod
+		// These annotations improve the performance of the sidecarSet controller
+		// In the future, SidecarSetRevisionAnnotation may help sidecarSet controller replace update operation with patch
+		p.updatePodAnnotationsAboutSidecarNameAndRevision(control, podClone, latestRevision)


I think you can update revision in function control.updatePodSidecarSetHash

zmberg · 2021-08-20T07:38:39Z

/lgtm

sonatype-lift · 2021-09-18T05:49:33Z

pkg/controller/sidecarset/sidecarset_processor_test.go

+	kubeSysNs := &corev1.Namespace{}
+	kubeSysNs.SetName(webhookutil.GetNamespace()) //Note that util.GetKruiseManagerNamespace() return "" here
+	kubeSysNs.SetNamespace(webhookutil.GetNamespace())
+	fakeClient := fake.NewFakeClientWithScheme(scheme, sidecarSet, kubeSysNs)


SA1019: fake.NewFakeClientWithScheme is deprecated: Please use NewClientBuilder instead.
(at-me in a reply with help or ignore)

zmberg · 2021-10-22T02:55:35Z

/lgtm

FillZpp · 2021-10-27T03:47:35Z

apis/apps/v1alpha1/sidecarset_types.go

+	// RevisionHistoryLimit indicates the maximum quantity of stored revisions about SidecarSet.
+	// This value is set by SidecarSet controller.
+	// default value is 10
+	RevisionHistoryLimit int32 `json:"revisionHistoryLimit,omitempty"`


It should be in SidecarSet spec instead of status...

FillZpp · 2021-10-27T03:55:24Z

pkg/controller/sidecarset/sidecarset_controller.go

@@ -47,6 +47,7 @@ func init() {
 }

 var (
+	maxStoredRevisions   = 10


We should use spec.revisionHistoryLimit instead of a fixed number.

Signed-off-by: veophi <[email protected]>

FillZpp · 2021-12-20T09:22:25Z

/lgtm

FillZpp · 2021-12-29T03:28:17Z

/approve

kruise-bot · 2021-12-29T03:28:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FillZpp

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:

~~OWNERS~~ [FillZpp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: veophi <[email protected]>

kruise-bot requested review from Fei-Guo and zmberg August 18, 2021 09:40

kruise-bot added the size/L size/L: 100-499 label Aug 18, 2021

sonatype-lift bot reviewed Aug 18, 2021

View reviewed changes

veophi force-pushed the sidecar-revision-control branch 2 times, most recently from f1753ac to 467a183 Compare August 19, 2021 03:54

veophi force-pushed the sidecar-revision-control branch 3 times, most recently from 63116fc to ec1e026 Compare August 19, 2021 07:21

kruise-bot added size/XL size/XL: 500-999 and removed size/L size/L: 100-499 labels Aug 19, 2021

veophi force-pushed the sidecar-revision-control branch from ec1e026 to 7a9617c Compare August 19, 2021 07:27

FillZpp reviewed Aug 19, 2021

View reviewed changes

zmberg reviewed Aug 20, 2021

View reviewed changes

veophi force-pushed the sidecar-revision-control branch 4 times, most recently from 2686690 to 6801438 Compare August 20, 2021 07:33

kruise-bot assigned zmberg Aug 20, 2021

kruise-bot added the lgtm label Aug 20, 2021

veophi force-pushed the sidecar-revision-control branch from 6801438 to fe454ed Compare August 20, 2021 07:50

kruise-bot removed the lgtm label Aug 20, 2021

sonatype-lift bot reviewed Sep 18, 2021

View reviewed changes

veophi force-pushed the sidecar-revision-control branch 2 times, most recently from 56432c3 to 50e99f0 Compare September 18, 2021 06:27

kruise-bot added the lgtm label Oct 22, 2021

veophi mentioned this pull request Oct 27, 2021

[feature request] Support SidecarSet update strategy when pod recreate or inject #783

Closed

FillZpp reviewed Oct 27, 2021

View reviewed changes

veophi force-pushed the sidecar-revision-control branch from 50e99f0 to 3be668f Compare October 27, 2021 06:50

kruise-bot removed the lgtm label Oct 27, 2021

veophi force-pushed the sidecar-revision-control branch 3 times, most recently from ceef177 to bbd46e0 Compare October 28, 2021 04:13

kruise-bot added size/XXL and removed size/XL size/XL: 500-999 labels Oct 28, 2021

veophi force-pushed the sidecar-revision-control branch from 1e30627 to 209141e Compare October 28, 2021 05:58

kruise-bot added size/XL size/XL: 500-999 and removed size/XXL labels Oct 28, 2021

veophi force-pushed the sidecar-revision-control branch from 209141e to b273cfc Compare December 14, 2021 09:37

store history revisions for sidecarset

b95ead0

Signed-off-by: veophi <[email protected]>

veophi force-pushed the sidecar-revision-control branch from b273cfc to b95ead0 Compare December 20, 2021 08:08

kruise-bot assigned FillZpp Dec 20, 2021

kruise-bot added the lgtm label Dec 20, 2021

kruise-bot added the approved label Dec 29, 2021

kruise-bot merged commit 766a7d1 into openkruise:master Dec 29, 2021

FillZpp mentioned this pull request Dec 29, 2021

[feature request]Support version control for SidecarSet #710

Closed

FillZpp pushed a commit that referenced this pull request Jan 21, 2022

store history revisions for sidecarset (#715)

9e2847b

Signed-off-by: veophi <[email protected]>

ppbits pushed a commit to ppbits/kruise that referenced this pull request Apr 4, 2024

store history revisions for sidecarset (openkruise#715)

008f682

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store history revisions for sidecarset #715

store history revisions for sidecarset #715

veophi commented Aug 18, 2021 •

edited

Loading

sonatype-lift bot Aug 18, 2021

sonatype-lift bot Aug 18, 2021

sonatype-lift bot Aug 18, 2021

sonatype-lift bot Aug 18, 2021

codecov-commenter commented Aug 19, 2021 •

edited

Loading

FillZpp Aug 19, 2021

veophi Aug 19, 2021

FillZpp Aug 19, 2021

veophi Aug 19, 2021

FillZpp Aug 19, 2021

veophi Aug 19, 2021 •

edited

Loading

veophi Aug 19, 2021

FillZpp Aug 19, 2021

veophi Aug 19, 2021

FillZpp Aug 19, 2021

veophi Aug 19, 2021

FillZpp Aug 19, 2021

veophi Aug 20, 2021

FillZpp Aug 19, 2021

veophi Aug 19, 2021 •

edited

Loading

zmberg Aug 20, 2021

zmberg Aug 20, 2021

zmberg Aug 20, 2021

veophi Aug 20, 2021

zmberg Aug 20, 2021

zmberg commented Aug 20, 2021

sonatype-lift bot Sep 18, 2021

zmberg commented Oct 22, 2021

FillZpp Oct 27, 2021

FillZpp Oct 27, 2021

veophi Oct 27, 2021

FillZpp commented Dec 20, 2021

FillZpp commented Dec 29, 2021

kruise-bot commented Dec 29, 2021

store history revisions for sidecarset #715

store history revisions for sidecarset #715

Conversation

veophi commented Aug 18, 2021 • edited Loading

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. What does it look like?

sonatype-lift bot Aug 18, 2021

Choose a reason for hiding this comment

sonatype-lift bot Aug 18, 2021

Choose a reason for hiding this comment

sonatype-lift bot Aug 18, 2021

Choose a reason for hiding this comment

sonatype-lift bot Aug 18, 2021

Choose a reason for hiding this comment

codecov-commenter commented Aug 19, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veophi Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veophi Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmberg commented Aug 20, 2021

sonatype-lift bot Sep 18, 2021

Choose a reason for hiding this comment

zmberg commented Oct 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FillZpp commented Dec 20, 2021

FillZpp commented Dec 29, 2021

kruise-bot commented Dec 29, 2021

veophi commented Aug 18, 2021 •

edited

Loading

codecov-commenter commented Aug 19, 2021 •

edited

Loading

veophi Aug 19, 2021 •

edited

Loading

veophi Aug 19, 2021 •

edited

Loading