NO-ISSUE: Recalculate operator dependencies before validations #7227
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jhernand: This pull request explicitly references no jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand 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
| } | ||
| } | ||
| for _, addedOperator := range addedOperators { | ||
| err = c.db.Save(addedOperator).Error |
There was a problem hiding this comment.
@rccrdpccl in the previous version of this pull request (now closed) @gamli75 was concerned
that saving the cluster changes directly to DB will cause inconsistent values in the elastic. I added code below to also update the feature set. What else needs to be done to ensure consistency?
There was a problem hiding this comment.
In order to keep elastic data consistent, we notify about important (cluster, hosts and infraenv, only fields we deem important as we want to keep event volume contained) state changes. Unfortunately, there isn't a centralized place for this, so the code used to do this is dispersed.
An example could be updating hosts in some circumstances, but in this case we should update the cluster state.
Related: https://github.com/openshift/assisted-service/blob/398cf47615a80714a11c2bd722681fc8c70a7cc4/internal/stream/notification_stream.go
There was a problem hiding this comment.
Added code to send the notifications, please take another look @rccrdpccl.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7227 +/- ##
==========================================
- Coverage 67.92% 67.77% -0.15%
==========================================
Files 298 298
Lines 40710 40834 +124
==========================================
+ Hits 27654 27677 +23
- Misses 10580 10667 +87
- Partials 2476 2490 +14
|
| func (r *refreshPreprocessor) recalculateOperatorDependencies(c *clusterPreprocessContext) error { | ||
| // Calculate and save the operators that have been added, updated or deleted: | ||
| previousOperators := c.cluster.MonitoredOperators | ||
| currentOperators, err := r.operatorsAPI.ResolveDependencies(c.cluster, c.cluster.MonitoredOperators) |
There was a problem hiding this comment.
c.cluster.MonitoredOperators are the current registered operators for the cluster?
operatorsAPI.ResolveDependencies returns dependencies of the current cluster operators?
I think the terms previousoperators and currentoperators are a little confusing
If I understand this code block correctly, we are doing the following
- Obtaining a list of the monitored operators for the cluster
- Finding dependencies required for these monitored operators, this is again a list of MonitoredOperators
- The resulting operators are then created, updated, deleted according to their presence in the
previous operators
I wonder if operators and operatorDependencies (you refer to currentOperators as "operator dependencies" in the error message) or something along those lines would be better?
In it's current form, it's quite hard to follow the intent of this code (but I think I got the basic gist?)
There was a problem hiding this comment.
c.cluster.MonitoredOperatorsare the current registered operators for the cluster?
Yes, I think so.
operatorsAPI.ResolveDependenciesreturns dependencies of the current cluster operators?
I beleive that ResolveDependencies calculates the dependencies of the list of operators that is passed as a parameter. Then it merges that with the complete list of operators of the cluster, and returns the result. Note it returns everything: the operators explicitly added by the user as well as the ones automatically added to resolve the dependencies.
I think the terms
previousoperatorsandcurrentoperatorsare a little confusingIf I understand this code block correctly, we are doing the following
- Obtaining a list of the monitored operators for the cluster
- Finding dependencies required for these monitored operators, this is again a list of MonitoredOperators
- The resulting operators are then created, updated, deleted according to their presence in the
previous operatorsI wonder if
operatorsandoperatorDependencies(you refer tocurrentOperatorsas "operator dependencies" in the error message) or something along those lines would be better?
The ideal names would be lostOfOperatorsBeforeRecalculatingDependencies and listOfOperatorsAfterRecalculatingDependencies, but that is too long. i can apply the renaming that you suggest, but take into account that "current..." is not only the dependencies; it is everythinng.
In it's current form, it's quite hard to follow the intent of this code (but I think I got the basic gist?)
There was a problem hiding this comment.
I am renaming the variables operatorsBeforeResolve and operatorsAfterResolve. Hope that clarifies it a bit.
| ) | ||
| } | ||
| } | ||
| for _, updatedOperator := range updatedOperators { |
There was a problem hiding this comment.
I don't get the difference between an added and an updated operator in this code, it looks like exactly the same operations are performed on each?
specifically c.db.Save(updatedOperator).Error vs c.db.Save(addedOperator).Error
Why do we have this split if the behaviour is the same?
Edit: Reading further on I can see that this may be related to feature usage. (operators you add or remove may cause a change to feature flags) I still think we could do something more condensed in this area though.
Maybe something like this could allow a single loop to handle the storage for added and updated.
for _, operator := range append(addedOperators, updatedOperators) {
I don't imagine these operator lists are really large enough for space or time complexity to be an issue with them, so this might be a nice way to compact things down a bit.
There was a problem hiding this comment.
Operators are not only a name: they have also other properties that could change during the recalculation of dependencies. updatedOperators contains the list of operators that already existed before the recalculation and have changed (other than the name) after the recalculation. Currently we happen to do almost the same for both: we save them to the database with the Save method (although internally that Save method does an INSERT for added operators and an UPDATE for updated operators). I say "almost" because the error message generated if that failed is also different: failed to add ... vs failed to update ....
When I wrote this initially I just wrote three independent loops because I didn't know at that point what I will be writing inside. The result ended up having only very minor differences. I think it makes sense, even if there was no difference. But merging them is also fine. If you insist a wee bit I will do that.
jhernand
force-pushed
the
recalculate_operator_dependencies_from_cluster_monitor_loop
branch
from
05a968f to
8b67fe0
Compare
openshift-ci
bot
added
size/XL
Currently operator dependencies are only calculated when a cluster is created or updated. But certain dependencies are dynamic, and may change when new hosts are added. For example, if a cluster has the OpenShift AI operator installed, it will also require the NVIDIA GPU operator only if there are hosts that have NVIDIA GPUs. To support those dynamic dependencies this patch modifies the cluster monitor so that it recalculates the operator dependencies before checking validations. Signed-off-by: Juan Hernandez <[email protected]>
jhernand
force-pushed
the
recalculate_operator_dependencies_from_cluster_monitor_loop
branch
from
8b67fe0 to
6eb9b99
Compare
|
@jhernand: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Currently operator dependencies are only calculated when a cluster is created or updated. But certain dependencies are dynamic, and may change when new hosts are added. For example, if a cluster has the OpenShift AI operator installed, it will also require the NVIDIA GPU operator only if there are hosts that have NVIDIA GPUs. To support those dynamic dependencies this patch modifies the cluster monitor so that it recalculates the operator dependencies before checking validations.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist