Update operators-installer

[thunderthome]

What is this PR About?

In the current version of the operators-installer if you set approveManualInstallPlanViaHook to false, you see the job and pods for the approver and verifier in ArgoCD and it doesn't clean them up, which isn't the problem fixed. What I did notice is that you'll see the failed pods in the OpenShift GUI as failed pods. Reason for this is mostly because the pod fails immediately when an InstallPlan isn't found and just starts a new pod for the approver until an InstallPlan is found.

What I did is make a configurable loop so you can configure the amount of times the approver needs to retry and with what interval. This way you can configure it so that the job can succeed within one pod and you don't end up with failed pods in the openshift overview.

Defaults are set to 10 retries and 5 second interval, they can be changed by changing the following values.
installPlanApproverSearchRetries
installPlanApproverSearchDelaySeconds

How do we test this?

You can use the approver as is and look at the logs. Previously you'd get something like this. (logs coming from 2 different clusters in the example).

Find next InstallPlan in Namespace (metallb-system) for CSV (metallb-operator.v4.22.0-202607151755) with Subscription (f6433a8b-0575-43ca-99aa-990082cb282f) owner

ERROR: Could not find next InstallPlan to reach CSV metallb-operator.v4.22.0-202607151755) with Subscription (metallb-operator) (f6433a8b-0575-43ca-99aa-990082cb282f) owner.
This can happen if InstallPlan isn't created yet or no valid upgrade path between current CSV and target CSV.
Try again.

And now your logs will look like this.

Find next InstallPlan in Namespace (metallb-system) for CSV (metallb-operator.v4.22.0-202607151755) with Subscription (139c41a3-8ef5-4b44-9781-f6dc266605f8) owner
	- InstallPlan not available yet. Attempt (1 of 10). Waiting (5 seconds) before trying again.
	- InstallPlan not available yet. Attempt (2 of 10). Waiting (5 seconds) before trying again.
	- InstallPlan not available yet. Attempt (3 of 10). Waiting (5 seconds) before trying again.
	- InstallPlan not available yet. Attempt (4 of 10). Waiting (5 seconds) before trying again.
	- InstallPlan not available yet. Attempt (5 of 10). Waiting (5 seconds) before trying again.
	- InstallPlan not available yet. Attempt (6 of 10). Waiting (5 seconds) before trying again.
	- InstallPlan not available yet. Attempt (7 of 10). Waiting (5 seconds) before trying again.
	- InstallPlan not available yet. Attempt (8 of 10). Waiting (5 seconds) before trying again.
	- Found InstallPlan on attempt (9 of 10)
	- Found next InstallPlan: install-2flfp

cc: @redhat-cop/day-in-the-life

[itewk]

@thunderthome this really just recreates what kubernetes jobs already does and we already have installPlanApproverRetries, and installPlanApproverActiveDeadlineSeconds. the idea is if you don't like the job resources sitting around then have the hook set to true. then all of this gets cleaned up.

so i am not really seeing the reason for this redundant logic.

what am i missing?

[itewk]

if you really want to not use the hook, and not have the re-creating jobs, then i would ask that instead of adding new fields that are more or less duplicative with installPlanApproverRetries, installPlanApproverActiveDeadlineSeconds, installPlanVerifierRetries, and installPlanVerifierActiveDeadlineSeconds create a new field called installPlanApproverLoopInsideJob and installPlanVerifierLoopInsideJob.

the default for those would be false.

if true, then set the values for the Job itself to 1 and then do your loop logic inside the script using the existing retry and timeout vars.

make sense?

[itewk]

or anther approach, add a installPlan*Delay and then just add a sleep before the first call to installplan_utils‎ .get_installplan in the scripts.

[itewk]

Please see comments.

[itewk]

also, for whichever way you choose to go, please add a new test case(s) for the new parameter(s)

[thunderthome]

@thunderthome this really just recreates what kubernetes jobs already does and we already have installPlanApproverRetries, and installPlanApproverActiveDeadlineSeconds. the idea is if you don't like the job resources sitting around then have the hook set to true. then all of this gets cleaned up.

so i am not really seeing the reason for this redundant logic.

what am i missing?

I've come to the conclusion that those 2 variables only work for waiting when the installplan is available and retrying to approve, so it won't work for the actual waiting for the installplan when there's a delay in creation of the installplan. This pod/job immediately fails when the installplan isn't found.

if you really want to not use the hook, and not have the re-creating jobs, then i would ask that instead of adding new fields that are more or less duplicative with installPlanApproverRetries, installPlanApproverActiveDeadlineSeconds, installPlanVerifierRetries, and installPlanVerifierActiveDeadlineSeconds create a new field called installPlanApproverLoopInsideJob and installPlanVerifierLoopInsideJob.

the default for those would be false.

if true, then set the values for the Job itself to 1 and then do your loop logic inside the script using the existing retry and timeout vars.

make sense?

The reason why I don't want to use the hook is because of the custom CR's and CRD's which breaks it.

And for the new fields now it waits for the installplan to be created instead of immediately failing. It's an addition before the standing logic that was already there.

So there's basically 3 steps in the installplan process:

1. Waiting for the installplan to be created (which is this PR about)
2. Waiting for the installplan to be approved (which only works if the installplan is there)
3. Waiting for the installplan to finish installing

For step 2 and 3 are those variables that you've mentioned, hence the reason why I created that extra step.

The installPlanApproverRetries and other variables are for restarting the pod, which isn't ideal due to the fact that you get lingering pods that show up on your openshift overview as failed pods.

For the suggestion, I'm not really sure what you mean. I could reuse those same variables, but that wouldn't make sense since those variables are for the pod definition and not the script itself.

or anther approach, add a installPlan*Delay and then just add a sleep before the first call to installplan_utils‎ .get_installplan in the scripts.

And for this, I see that this could be a solution but it's not configurable for the end users. That's why I made it like this, now users can tweak the waiting time and amount of retries for their respective clusters so that the waiting time is as short as possible and not longer than absolutely necessary which you create with a sleep before the check.

[itewk]

The reason why I don't want to use the hook is because of the custom CR's and CRD's which breaks it.

My solution to that is creating two different ArgoApps. one that uses this helm chart to install the operator, and a second one to do the configuration of it. then the magic of argo will keep trying the app that depends on the operator installed until it is installed, and that way you can use the hook on the operator install because its the only thing in that ArgoApp.

Butttt.....i understand the desire to put the install and configuration in the same app.

Sooo.....

Would you mind refactoring to:

• add variable installPlanApproverWaitLoopInsideJob, if true, then
  • in the approver script add your loop using installPlanApproverRetries and installPlanApproverActiveDeadlineSeconds
  • set the Job to have 1 retry
  • still use the installPlanApproverActiveDeadlineSeconds (though maybe need to add a few seconds iff the script job is going to time out on that amount)
• add variable installPlanVerifierWaitLoopInsideJob, if true, then
  • in the verify script add your loop using installPlanVerifierRetries and installPlanVerifierActiveDeadlineSeconds
  • set the Job to have 1 retry
  • use the installPlanVerifierActiveDeadlineSeconds (though maybe need to add a few seconds iff the script job is going to time out on that amount)
• update variable docs to describe this behavior
• add new test cases

Make sense? cool with that?

[thunderthome]

I've made a story on our board so I can make some time for it in the upcoming sprint. I think I understand where you'd like it to go so I'll see what I can do and then get back to you.

[itewk]

@thunderthome thanks!

[thunderthome]

@itewk I've applied the requested changes to the code. I've also tested if it's working in two different simultaneously deployed applications. If the installPlanApproverWaitLoopInsideJob is set to true, the retries and activedeadlineseconds are redirected to the single pod.

Logs of the grafana deployment with installPlanApproverWaitLoopInsideJob: true

Find next InstallPlan in Namespace (openshift-operators) for CSV (grafana-operator.v5.21.2) with Subscription (8e19d479-03e4-4cf6-a13b-88411d5d6904) owner
	- InstallPlan not available yet. Attempt (1 of 20). Waiting (5.0 seconds) before trying again.
	- Found InstallPlan on attempt (2 of 20)
	- Found next InstallPlan: install-xqwvp

Logs for the loki deployment with the default setting which is false, restarting a new pod for every retry until an installplan is found or the retries time out.

Get Subscription (loki-operator) UID
	- Subscription (loki-operator) UID: e2673bf4-a4ae-4d12-beee-b2069ce73069
Find and approve every InstallPlan between currently installed CSV and target CSV (loki-operator.v6.5.1)
Find next InstallPlan in Namespace (openshift-operators-redhat) for CSV (loki-operator.v6.5.1) with Subscription (e2673bf4-a4ae-4d12-beee-b2069ce73069) owner
ERROR: Could not find InstallPlan for CSV (loki-operator.v6.5.1) with Subscription (loki-operator) (e2673bf4-a4ae-4d12-beee-b2069ce73069) owner.
This can happen if the InstallPlan wasn't created yet or no valid upgrade path exists between the current CSV and target CSV.

The readme is also updated accordingly.

[itewk]

@thunderthome thanks! need you to fix a merge issue with the version which i put a suggestion in there for. could you also squash down all your commits?

[itewk]

@thunderthome can you add at least one new test that specifically tests the new attributes?

https://github.com/redhat-cop/helm-charts/tree/main/charts/operators-installer/ci

[thunderthome]

@itewk I've added the ci and squashed the commits

[itewk]

@thunderthome thank you so much for the contribution and sticking through all my asks!