-
Notifications
You must be signed in to change notification settings - Fork 11
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
[feature] PVC template #162
[feature] PVC template #162
Conversation
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
6cb4ecb
to
04de789
Compare
@@ -0,0 +1,366 @@ | |||
/* | |||
Copyright 2023 The KusionStack Authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The year should be 2024
.
@@ -0,0 +1,111 @@ | |||
/* | |||
Copyright 2023 The KusionStack Authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The year should be 2024
.
@@ -0,0 +1,111 @@ | |||
/* | |||
Copyright 2023 The KusionStack Authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The year should be 2024
.
@@ -19,6 +19,7 @@ package collaset | |||
import ( | |||
"context" | |||
"fmt" | |||
"kusionstack.io/operating/pkg/controllers/collaset/pvccontrol" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This package should be in the third package group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I'll check all imported packages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL
"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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sort it plz. (*1)
} | ||
pvcList := &corev1.PersistentVolumeClaimList{} | ||
// list pvcs match label selector of set | ||
err = pc.client.List(context.TODO(), pvcList, &client.ListOptions{Namespace: cls.Namespace, LabelSelector: selector}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try to pass the caller's context
here, instead of using context.TODO()
. (*2)
@@ -19,6 +19,7 @@ package collaset | |||
import ( | |||
"context" | |||
"fmt" | |||
"kusionstack.io/operating/pkg/controllers/collaset/pvccontrol" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imports are not sorted. (*1)
You can config import style in settings.
return nil, err | ||
} | ||
|
||
if err := c.Create(context.TODO(), claim); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(*2)
} | ||
|
||
// delete pvcs labeled same id with pod | ||
if err := pc.client.Delete(context.TODO(), pvc); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(*2)
continue | ||
} | ||
// if pvc is not claimed in pvc templates, delete it | ||
if err := c.Delete(context.TODO(), pvc); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(*2)
if _, newPvcExist := (*newPvcs)[pvcTmpName]; !newPvcExist { | ||
continue | ||
} | ||
if err := c.Delete(context.TODO(), pvc); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(*2)
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(*2)
err = c.Watch(&source.Kind{Type: &corev1.PersistentVolumeClaim{}}, &handler.EnqueueRequestForOwner{ | ||
IsController: true, | ||
OwnerType: &appsv1alpha1.CollaSet{}, | ||
}) | ||
if err != nil { | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think watching the PersistentVolumeClaim
is necessary.
var filteredPVCs []*corev1.PersistentVolumeClaim | ||
ownerSelector := cls.Spec.Selector.DeepCopy() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary deepcopy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all reviews are fixed
|
||
// list pvcs match label selector of set | ||
pvcList := &corev1.PersistentVolumeClaimList{} | ||
err = pc.client.List(ctx, pvcList, &client.ListOptions{Namespace: cls.Namespace, LabelSelector: selector}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
1245d89
to
413e546
Compare
6866796
to
3e9aa8f
Compare
LGTM |
1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):
"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):
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.