create a scaler controller to watch and trigger the events on CR creation #529
Conversation
Signed-off-by: Nitin Goyal <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: iamniting 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 |
openshift-ci
bot
added
the
approved
iamniting
force-pushed
the
watch-crds
branch
10 times, most recently
from
c89fa74 to
1f10e0b
Compare
Signed-off-by: Nitin Goyal <[email protected]>
Signed-off-by: Nitin Goyal <[email protected]>
iamniting
force-pushed
the
watch-crds
branch
from
1f10e0b to
89fb155
Compare
| } | ||
|
|
||
| //nolint:unused | ||
| kindList = []metav1.PartialObjectMetadata{ |
There was a problem hiding this comment.
maybe remove unused code? not sure where it is planned, storing pointers would be good if used at a later point.
| csvCephcsiOperator = "cephcsi-operator" //nolint:unused | ||
| csvMcgOperator = "mcg-operator" | ||
| csvOcsClientOperator = "ocs-client-operator" //nolint:unused | ||
| csvOcsOperator = "ocs-operator" | ||
| csvOdfCsiAddonsOperator = "odf-csi-addons-operator" //nolint:unused | ||
| csvOdfDependencies = "odf-dependencies" //nolint:unused | ||
| csvOdfOperator = "odf-operator" //nolint:unused | ||
| csvOdfPrometheusOperator = "odf-prometheus-operator" //nolint:unused | ||
| csvRecipe = "recipe" //nolint:unused | ||
| csvRookCephOperator = "rook-ceph-operator" | ||
|
|
||
| csvIbmStorageOdfOperator = "ibm-storage-odf-operator" |
There was a problem hiding this comment.
aren't these name of packages rather than CSV?
There was a problem hiding this comment.
yes, i Just named as CSV as we refer to csv mostly.
There was a problem hiding this comment.
No, I mean these represent pkg and should be named accordingly.
|
|
||
| var csvLabelKey = "operators.coreos.com/%s.%s" | ||
|
|
||
| var ( |
There was a problem hiding this comment.
separation is already being followed w/ new lines and so why to use var at multiple places again?
There was a problem hiding this comment.
I am doing that to keep different logical variables separate.
There was a problem hiding this comment.
That isn't helping anything I believe, a single var w/ a comment around each segment would inform the intent more appropriately.
In current state, reviewers (future & current) should ensure ntg in b/n should be written.
| crdStorageCluster = "storageclusters.ocs.openshift.io" | ||
| crdCephCluster = "cephclusters.ceph.rook.io" | ||
| crdNoobaa = "noobaas.noobaa.io" | ||
| crdFlashSystemCluster = "flashsystemclusters.odf.ibm.com" | ||
|
|
||
| crdList = []string{ | ||
| crdStorageCluster, | ||
| crdCephCluster, | ||
| crdNoobaa, | ||
| crdFlashSystemCluster, | ||
| } |
There was a problem hiding this comment.
these are names of CRDs and crdXXName, crdNamesList would be more apt?
| ) | ||
|
|
||
| var ( | ||
| crdToKindMapping = map[string]metav1.PartialObjectMetadata{ |
There was a problem hiding this comment.
here the value is copied into map which has two issues
- only shallow copy is being performed which makes the nested structures being shared
- map size increases without any clear benefits
Either
- do a deep copy if you want exclusive ownership of the obj (or)
- store pointers as values
I prefer 2 as I don't see why map needs exclusive ownership of these.
| replicas = 0 | ||
| } | ||
|
|
||
| for _, csvName := range csvNames { |
There was a problem hiding this comment.
maybe it's better to run this in parallel or let the loop go over all CSVs trying to update CSV as a failure in one stops everything else now, why do want to put a break if all operators can be run in parallel in the cluster?
| for kind, csvNames := range kindToCsvsMapping { | ||
|
|
||
| var replicas int32 = 1 | ||
| objects := &metav1.PartialObjectMetadataList{} |
There was a problem hiding this comment.
possible to give a more concrete name here? these are CRs, isn't it? crList?
|
|
||
| var replicas int32 = 1 | ||
| objects := &metav1.PartialObjectMetadataList{} | ||
| objects.TypeMeta = kind.TypeMeta |
There was a problem hiding this comment.
does this work 🤔? isn't List and individual resource distinct? this line here seems like assigning Pod typeMeta to PodList typeMeta.
| if err != nil { | ||
| if meta.IsNoMatchError(err) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
is client.IgnoreNotFound(err) != nil different than IsNoMatchError?
There was a problem hiding this comment.
Yes they are different IsNoMatchError is when there is no CRD.
| }, | ||
| predicate.GenerationChangedPredicate{}, | ||
| ), | ||
| ). |
There was a problem hiding this comment.
i'm just thinking, do we want to add a watch for CSV as we are effectively managing it's replicas?
There was a problem hiding this comment.
currently i dont want to because if we do that we don't leave any room for user to make the change to the replicas of the CSV also they wont be able to scale down the odf-operator because of the webhook.
|
/test odf-operator-e2e-aws |
Signed-off-by: Nitin Goyal [email protected]