-
Notifications
You must be signed in to change notification settings - Fork 23
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
controllers: Add OcsClient and StorageClassClaim controllers #1
Conversation
Signed-off-by: Jose A. Rivera <[email protected]>
Signed-off-by: Jose A. Rivera <[email protected]>
Signed-off-by: Jose A. Rivera <[email protected]>
60c0722
to
a72dcd7
Compare
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.
Few nits and Need to rebase on top of the latest ocs-operator for the claim controller.
@@ -0,0 +1,81 @@ | |||
/* | |||
Copyright 2020 Red Hat, Inc. |
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.
2022?
type StorageClassClaimSpec struct { | ||
//+kubebuilder:validation:Enum=blockpool;sharedfilesystem | ||
Type string `json:"type"` | ||
EncryptionMethod string `json:"encryptionMethod,omitempty"` |
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.
Need to add storageProfile to the spec
For(&v1alpha1.OcsClient{}, builder.WithPredicates( | ||
predicate.GenerationChangedPredicate{}, | ||
)). | ||
Watches(&source.Kind{Type: &v1alpha1.StorageClassClaim{}}, annotationMapFunc). |
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.
Question:- Why we are watching for storageclassclaim in ocsclient controller?
// Apply status changes to the OcsClient | ||
statusErr := r.Client.Status().Update(ctx, instance) | ||
if statusErr != nil { | ||
r.Log.Info("Failed to update OcsClient status.") |
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.
log as an error?
instance.Status.Phase = v1alpha1.OcsClientOffboarding | ||
|
||
if res, err := r.offboardConsumer(instance, externalClusterClient); err != nil { | ||
r.Log.Info("Offboarding in progress.", "Status", 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.
Log as an error?
allowVolumeExpansion := true | ||
storageClass := &storagev1.StorageClass{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: r.storageClassClaim.Name, |
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.
a namespace is not required for storageclass as it is cluster scoped resources.
if reflect.DeepEqual(existing.Parameters, storageClass.Parameters) { | ||
return 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.
We should have moved this inside existing.UID
check as it make sense to have comparison only when storageclass exists
if existing.UID != "" { | ||
|
||
// Since we have to update the existing StorageClass, so we will delete the existing StorageClass and create a new one. | ||
r.log.Info("StorageClass needs to be updated, deleting it.", "StorageClass", klog.KRef(storageClass.Namespace, existing.Name)) |
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.
no namespace required for storageclass
// Delete the StorageClass. | ||
err := r.delete(existing) | ||
if err != nil { | ||
r.log.Error(err, "Failed to delete StorageClass.", "StorageClass", klog.KRef(storageClass.Namespace, existing.Name)) |
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.
no namespace required for storageclass
return err | ||
} | ||
} | ||
r.log.Info("Creating StorageClass.", "StorageClass", klog.KRef(storageClass.Namespace, existing.Name)) |
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.
no namespace required for storageclass
Closing in favor of #6. |
Signed-off-by: Jose A. Rivera [email protected]