[feature] PVC template #162

ColdsteelRail · 2024-03-07T13:03:29Z

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

N
Y
"fix Enhancement: PVC template support of CollaSet #104 "

2. What is the scope of this PR (e.g. component or file name):

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

N
Y

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

6. Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

codecov · 2024-03-07T13:07:21Z

Codecov Report

Attention: Patch coverage is 74.40000% with 32 lines in your changes are missing coverage. Please review.

Project coverage is 64.39%. Comparing base (72fb78d) to head (eca6001).

Files	Patch %	Lines
pkg/controllers/collaset/utils/pvc.go	73.52%	12 Missing and 6 partials ⚠️
.../controllers/poddeletion/poddeletion_controller.go	50.00%	10 Missing and 1 partial ⚠️
pkg/controllers/collaset/collaset_controller.go	85.71%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #162      +/-   ##
==========================================
+ Coverage   64.01%   64.39%   +0.37%     
==========================================
  Files          73       74       +1     
  Lines        4708     4825     +117     
==========================================
+ Hits         3014     3107      +93     
- Misses       1419     1438      +19     
- Partials      275      280       +5

Flag	Coverage Δ
unittests	`64.39% <74.40%> (+0.37%)`	⬆️

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.

wu8685 · 2024-03-20T15:01:20Z

pkg/controllers/collaset/pvccontrol/pvc_control.go

@@ -0,0 +1,366 @@
+/*
+Copyright 2023 The KusionStack Authors.


The year should be 2024.

wu8685 · 2024-03-20T15:01:45Z

pkg/controllers/collaset/utils/pvc.go

@@ -0,0 +1,111 @@
+/*
+Copyright 2023 The KusionStack Authors.


The year should be 2024.

wu8685 · 2024-03-20T15:01:50Z

pkg/controllers/collaset/utils/pvc_test.go

@@ -0,0 +1,111 @@
+/*
+Copyright 2023 The KusionStack Authors.


The year should be 2024.

wu8685 · 2024-03-21T02:22:08Z

pkg/controllers/collaset/collaset_controller.go

@@ -19,6 +19,7 @@ package collaset
 import (
 	"context"
 	"fmt"
+	"kusionstack.io/operating/pkg/controllers/collaset/pvccontrol"


This package should be in the third package group.

ok, I'll check all imported packages

Eikykun

PTAL

Eikykun · 2024-03-22T06:19:30Z

pkg/controllers/collaset/pvccontrol/pvc_control.go

+	"context"
+	"fmt"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/sets"
+	appsv1alpha1 "kusionstack.io/operating/apis/apps/v1alpha1"
+	collasetutils "kusionstack.io/operating/pkg/controllers/collaset/utils"
+	"kusionstack.io/operating/pkg/controllers/utils/expectations"
+	refmanagerutil "kusionstack.io/operating/pkg/controllers/utils/refmanager"
+	"sigs.k8s.io/controller-runtime/pkg/client"


sort it plz. (*1)

Eikykun · 2024-03-22T06:20:17Z

pkg/controllers/collaset/pvccontrol/pvc_control.go

+	}
+	pvcList := &corev1.PersistentVolumeClaimList{}
+	// list pvcs match label selector of set
+	err = pc.client.List(context.TODO(), pvcList, &client.ListOptions{Namespace: cls.Namespace, LabelSelector: selector})


try to pass the caller's context here, instead of using context.TODO(). (*2)

Eikykun · 2024-03-22T07:23:12Z

pkg/controllers/collaset/collaset_controller.go

@@ -19,6 +19,7 @@ package collaset
 import (
 	"context"
 	"fmt"
+	"kusionstack.io/operating/pkg/controllers/collaset/pvccontrol"


Imports are not sorted. (*1)
You can config import style in settings.

Eikykun · 2024-03-22T07:23:52Z

pkg/controllers/collaset/pvccontrol/pvc_control.go

+			return nil, err
+		}
+
+		if err := c.Create(context.TODO(), claim); err != nil {


Eikykun · 2024-03-22T07:26:48Z

pkg/controllers/collaset/pvccontrol/pvc_control.go

+		}
+
+		// delete pvcs labeled same id with pod
+		if err := pc.client.Delete(context.TODO(), pvc); err != nil {


Eikykun · 2024-03-22T07:27:00Z

pkg/controllers/collaset/pvccontrol/pvc_control.go

+			continue
+		}
+		// if pvc is not claimed in pvc templates, delete it
+		if err := c.Delete(context.TODO(), pvc); err != nil {


Eikykun · 2024-03-22T07:27:11Z

pkg/controllers/collaset/pvccontrol/pvc_control.go

+		if _, newPvcExist := (*newPvcs)[pvcTmpName]; !newPvcExist {
+			continue
+		}
+		if err := c.Delete(context.TODO(), pvc); err != nil {


Eikykun · 2024-03-22T07:27:28Z

pkg/controllers/poddeletion/poddeletion_controller.go

+	cls := &appsv1alpha1.CollaSet{}
+	for _, ref := range ownerRefs {
+		if ref.Kind == "CollaSet" {
+			if err := r.Client.Get(context.TODO(), types.NamespacedName{Namespace: pod.Namespace, Name: ref.Name}, cls); err != nil {


Eikykun · 2024-03-22T07:42:11Z

pkg/controllers/collaset/collaset_controller.go

+	err = c.Watch(&source.Kind{Type: &corev1.PersistentVolumeClaim{}}, &handler.EnqueueRequestForOwner{
+		IsController: true,
+		OwnerType:    &appsv1alpha1.CollaSet{},
+	})
+	if err != nil {
+		return err
+	}


I don't think watching the PersistentVolumeClaim is necessary.

Eikykun · 2024-03-22T08:01:08Z

pkg/controllers/collaset/pvccontrol/pvc_control.go

+	var filteredPVCs []*corev1.PersistentVolumeClaim
+	ownerSelector := cls.Spec.Selector.DeepCopy()
+


unnecessary deepcopy?

all reviews are fixed

wu8685 · 2024-03-24T03:41:55Z

pkg/controllers/collaset/pvccontrol/pvc_control.go

+
+	// list pvcs match label selector of set
+	pvcList := &corev1.PersistentVolumeClaimList{}
+	err = pc.client.List(ctx, pvcList, &client.ListOptions{Namespace: cls.Namespace, LabelSelector: selector})


I wonder whether these PVCs already in used still need to manage the ownership somehow, in case another CollaSet from same namespace takes over them and rebinds to new Pods.

I'll fix this problem soon. This is because some PVCs don't have ownerReferences if user configure "whenDelete=Retain". As your advice, it is necessary to keep an persistent ownerReference for PVCs, unless their owner CollaSet is deleted and user want to retain them.

wu8685 · 2024-03-25T08:17:44Z

LGTM

ColdsteelRail changed the title ~~basic pvc template support~~ pvc template support Mar 7, 2024

ColdsteelRail changed the title ~~pvc template support~~ [feature] PVC Template support Mar 8, 2024

ColdsteelRail changed the title ~~[feature] PVC Template support~~ [feature] PVC template support Mar 8, 2024

ColdsteelRail changed the title ~~[feature] PVC template support~~ [feature] PVC template Mar 8, 2024

wu8685 reviewed Mar 22, 2024

View reviewed changes

Eikykun requested changes Mar 22, 2024

View reviewed changes

Eikykun reviewed Mar 22, 2024

View reviewed changes

wu8685 reviewed Mar 24, 2024

View reviewed changes

ColdsteelRail added 18 commits March 25, 2024 11:36

basic pvc template support

c77d520

support PVC Retention policy

eaabf13

add pvc template unit test

3b6c209

add pvc utils unit test

d1e231e

add file header lisences

908a8ac

supprt pvc with replace update

dcbb4c9

rephrase some typo

aa52814

pvc compatible with replace update + scale in

f5de776

support retain pvc when update pvc template

a889221

fix:re-mount the old pvc when pvc template rollback

11728f6

pass caller's context

9810fd4

remove redundant DeepCopy()

7c49a35

validating webhook: denied delete pvc while pending

cc69984

validating webhook: denied delete pvc (must be mounted) while pending

e99e739

rename files

2fa005e

fix: support persistent ownerReference for owned pvcs

2dc6442

fix: use latest available version

3e9aa8f

ColdsteelRail requested review from wu8685 and Eikykun March 25, 2024 03:46

ColdsteelRail added 4 commits March 25, 2024 12:23

fix:reclaim pvcs in 1 reconcile

c8fe52c

type

8a6fcea

import files

eb22d5a

always adopt pvcs

d2b0683

Eikykun previously approved these changes Mar 25, 2024

View reviewed changes

add some comment

eca6001

ColdsteelRail dismissed Eikykun’s stale review via eca6001 March 25, 2024 08:02

wu8685 approved these changes Mar 25, 2024

View reviewed changes

wu8685 merged commit 2d59b9f into KusionStack:main Mar 25, 2024
7 checks passed

github-actions bot locked and limited conversation to collaborators Mar 25, 2024

ColdsteelRail deleted the collaset-support-pvc-template branch July 8, 2024 06:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] PVC template #162

[feature] PVC template #162

ColdsteelRail commented Mar 7, 2024

codecov bot commented Mar 7, 2024 •

edited

Loading

wu8685 Mar 20, 2024

wu8685 Mar 20, 2024

wu8685 Mar 20, 2024

wu8685 Mar 21, 2024

ColdsteelRail Mar 22, 2024

Eikykun left a comment

Eikykun Mar 22, 2024

Eikykun Mar 22, 2024

Eikykun Mar 22, 2024

Eikykun Mar 22, 2024

Eikykun Mar 22, 2024

Eikykun Mar 22, 2024

Eikykun Mar 22, 2024

Eikykun Mar 22, 2024

Eikykun Mar 22, 2024

Eikykun Mar 22, 2024

ColdsteelRail Mar 22, 2024

wu8685 Mar 24, 2024

ColdsteelRail Mar 24, 2024

ColdsteelRail Mar 25, 2024

wu8685 commented Mar 25, 2024

		var filteredPVCs []*corev1.PersistentVolumeClaim
		ownerSelector := cls.Spec.Selector.DeepCopy()

[feature] PVC template #162

[feature] PVC template #162

Conversation

ColdsteelRail commented Mar 7, 2024

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

2. What is the scope of this PR (e.g. component or file name):

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

6. Release note

codecov bot commented Mar 7, 2024 • 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

Eikykun left a 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

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

wu8685 commented Mar 25, 2024

codecov bot commented Mar 7, 2024 •

edited

Loading