(refactor) Move csv set and replace to a package #860
Conversation
openshift-ci-robot
added
the
size/L
|
/test unit |
2 similar comments
|
/test unit |
|
/test unit |
|
/retest |
1 similar comment
|
/retest |
openshift-ci-robot
added
lgtm
|
/retest |
|
I forgot to change |
Yeah, I could not find a better name either. :) |
|
/retest |
|
|
||
| // NewSetGenerator returns a new instance of SetGenerator. | ||
| func NewSetGenerator(logger *logrus.Logger, lister operatorlister.OperatorLister) SetGenerator { | ||
| return &csvSet{ |
There was a problem hiding this comment.
It might make sense to make this an exported type
| // is replacing the given CSV specified. | ||
| // | ||
| // If the corresponding ClusterServiceVersion is not found nil is returned. | ||
| func (r *replace) IsBeingReplaced(in *v1alpha1.ClusterServiceVersion, csvsInNamespace map[string]*v1alpha1.ClusterServiceVersion) (replacedBy *v1alpha1.ClusterServiceVersion) { |
There was a problem hiding this comment.
if the CSVSet type were exported, could just reference it here
There was a problem hiding this comment.
You mean, instead of passing the map csvsInNamespace map[string]*v1alpha1.ClusterServiceVersion, call csvSet.WithNamespace from within the function to get the list of namespaces?
I thought that the original intention was to pass the map as a parameter so that it does not need to know how to get it, and I wanted to keep that loose coupling.
I also was thinking of the following use case for my next PR on cluster operator
selector := labels.SelectorFromSet(labels.Set{
"olm.clusteroperator.name": "packageserver",
})
related := csvSet.WithNamespaceAndLabels(in.GetNamespace(), v1alpha1.CSVPhaseAny, selector)
replacedBy := replaceFinder.IsBeingReplaced(in, related)
Thoughts?
There was a problem hiding this comment.
@ecordell I was going to make this change but then came across this - https://github.com/operator-framework/operator-lifecycle-manager/blob/master/pkg/controller/operators/olm/operator_test.go#L4321
| @@ -163,15 +164,22 @@ func NewFakeOperator(clientObjs []runtime.Object, k8sObjs []runtime.Object, extO | |||
|
|
|||
| // Create the new operator | |||
| queueOperator, err := queueinformer.NewOperatorFromClient(opClientFake, logrus.StandardLogger()) | |||
|
|
|||
| lister := operatorlister.NewLister() | |||
| csvSetGenerator := csvutility.NewSetGenerator(logrus.StandardLogger(), lister) | |||
There was a problem hiding this comment.
if you're passing in the logger here, you should construct it ahead of time and pass the same instance to NewOperator, NewSetGenerator, and NewReplaceFinder so that when we pass in configuration (i.e. in the e2e tests) it gets distributed everywhere
There was a problem hiding this comment.
sounds good, I will make the change. I noticed that logrus.StandardLogger() was being called every time for a logger inside the function so went ahead with that.
pkg/lib/csv/replace_finder.go
Outdated
| r.logger.Infof("checking %s", csv.GetName()) | ||
| if csv.Spec.Replaces == in.GetName() { | ||
| r.logger.Infof("%s replaced by %s", in.GetName(), csv.GetName()) | ||
| replacedBy = csv.DeepCopy() |
There was a problem hiding this comment.
It would be nice to review where we're calling this and see if the copy is really necessary. This can be quite a lot of objects in a larger cluster.
Another thought: we might want to filter out Copied CSVs here - but again that will require a review of where we call this.
There was a problem hiding this comment.
I was thinking whether deep copy was necessary for this but didn't spend time since I thought it would be outside the scope of this PR.
But I am happy to do the search and see whether we can filter out copied CSVs and not deep copy.
|
@ecordell please review my comments when you have time. |
| // If the corresponding ClusterServiceVersion is not found nil is returned. | ||
| func (r *replace) IsBeingReplaced(in *v1alpha1.ClusterServiceVersion, csvsInNamespace map[string]*v1alpha1.ClusterServiceVersion) (replacedBy *v1alpha1.ClusterServiceVersion) { | ||
| for _, csv := range csvsInNamespace { | ||
| if csv.IsCopied() { |
There was a problem hiding this comment.
I checked the call sites of this function, it just requeues both the given CSV and the one replacing it old -> next if next.Status.Phase ~= v1alpha1.CSVPhaseSucceeded. Let me know if we don't need to requeue copied CSV here.
|
|
||
| if csv.Spec.Replaces == in.GetName() { | ||
| r.logger.Infof("%s replaced by %s", in.GetName(), csv.GetName()) | ||
| replacedBy = csv |
There was a problem hiding this comment.
No more deep copying - I removed replacedBy = csv.DeepCopy(). The call sites do not modify the next CSV.
|
/retest |
|
/test e2e-aws-console-olm |
|
/retest |
1 similar comment
|
/retest |
Promote csv set and replace logic within olm operator to a reusable package in lib so that it can be reused by other units. Note: - No behavior of the operator has changed as part of this refactor.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ecordell, tkashem 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 |
|
/retest |
Promote csv set and replace logic within olm operator to a reusable
package in lib so that it can be reused by other units.
Note: