Installation: add installation_priority parameter #2182
Conversation
kosstennbl
requested review from
denverwilliams and
agentpoyo
as code owners
|
@kosstennbl please note observability is failing right now (I'm cleaning the volumes/networks used by the runners and it raises a side effect on this particular test). it's also longer as I reduced thread to 1 to make it predictive. Sorry for that. |
| @@ -150,6 +134,10 @@ deployments: | |||
| ... | |||
| ``` | |||
|
|
|||
| ##### Installation priority | |||
|
|
|||
| If deployments needed to be installed in specific order, installation_priority parameter should be used. All of the deployments would be installed in order from highest installation priority to lowest. This parameter also affects uninstallation order, it would be reversed: from lowest installation_priority to highest. If not specified, installation priority for deployment is 0. | |||
There was a problem hiding this comment.
Maybe run this paragraph through some AI to make it a bit more concise, it does not read very well. Also it would be good to explicitly mention that this parameter is optional (unless it is not).
And I have a minor issue with the fact that the installation priority is in the order from highest to lowest. From a usage standpoint it would make sense to have the lowest number (0) be the one with most priority. As I expect the user to logically start ordering from it and move up. In the current case there would be some unnecessary thinking required (e.g. I have 10 deployments that need to be in specific order, what number do I need to start with so that I can end with 0 as the least important?)
There was a problem hiding this comment.
Currently, it works in the way that 0 is default, -1,-2,-3... would be lower priority deployments and 1,2,3 would be higher priority deployments.
In my opinion, default value shouldn't be the highest priority as it would require more changes to cnf-config for prioritized deployments (writing values for any priorities lower then the highest one seems to add more work for CNF configuration)
Naming can be also discussed, but from my thoughts - it will always be at least a bit confusing (if used without documentation), and to me it seemed that installation_priority is the best option.
There was a problem hiding this comment.
We've discussed this with @martin-mat and come to the conclusion that lowest numbers = highest priority is the way to go. For example the kubernetes priorityClass property also works this way. Additionally users would not have to go into the negative numbers which is a plus.
There was a problem hiding this comment.
Ok, it should be carefully considered what is default value then and documentation with code is needed to be adapted
| ensure | ||
| result = ShellCmd.cnf_cleanup() | ||
| result[:status].success?.should be_true | ||
| (/All CNF deployments were uninstalled/ =~ result[:output]).should_not be_nil |
There was a problem hiding this comment.
I am wondering if we could do a more thorough check, we should verify that the install and uninstall messages come in correct order.
| installation_priority: 2 | ||
| helm_repo_name: elastic | ||
| helm_repo_url: https://helm.elastic.co | ||
| helm_chart_name: elasticsearch | ||
| helm_values: "--set replicas=1" | ||
| - name: logstash | ||
| installation_priority: 1 |
There was a problem hiding this comment.
This sample also displays the problem with prioritizing from highest to lowest, it would be much more intuitive to have logstash and kibana both with installation_priority: 1, whilst elastic could have implicit 0.
kosstennbl
force-pushed
the
#2176-deployment-ordering
branch
from
081c44b to
0457316
Compare
Add installation_priority parameter for specification of order for installation of deployments. Refs: #2176 Signed-off-by: Konstantin Yarovoy <[email protected]>
kosstennbl
force-pushed
the
#2176-deployment-ordering
branch
from
0457316 to
98f99bb
Compare
Description
Add installation_priority parameter for specification of order for installation of deployments.
More info in documentation changes and issue.
Issues:
Refs: #2176