EVEREST-1182 fix install of everest operator

[recharte]

We introduced this phase check because OLM has a bug where it sometimes creates duplicate InstallPlans for the same CSV and we found a few cases where the duplicate InstallPlan wasn't reconciled correctly and abandoned by OLM. This abandoned InstallPlan was missing the status field meaning it was also missing the necessary plan to install the operator. Approving this InstallPlan would cause the operator to never be installed. By checking the phase we make sure we will be approving an InstallPlan that is actually ready to be approved. See operator-framework/kubectl-operator#13 for more details on a similar issue.

[mayankshah1607]

Were you able to test if this works?

My understanding was that in most cases, this function returns at line 581, which returns the InstallPlanRef from the subscription - when this happens, the Subscription still points at the malformed InstallPlan, and takes a while to update (basically the for-loop is never executed).

I realised that the Install flow is a bit complicated, and its hard to figure out just by looking which conditions will be triggered and which execution path our code will take 😓

[recharte]

Were you able to test if this works?

My understanding was that in most cases, this function returns at line 581, which returns the InstallPlanRef from the subscription - when this happens, the Subscription still points at the malformed InstallPlan, and takes a while to update..

I realised that the Install flow is a bit complicated, and its hard to figure out just by looking which conditions will be triggered and which execution path our code will take 😓

I did test that the install works correctly for the generic case. I'm still unable to replicate the case where the IP was missing the status but I guess this is as good as we'll get at this stage. I'll do some more tests to see if I can catch that case but it hasn't been easy...

Yes, the install flow is a bit complicated, we should probably refactor it in following releases.

[recharte]

My understanding was that in most cases, this function returns at line 581, which returns the InstallPlanRef from the subscription - when this happens, the Subscription still points at the malformed InstallPlan, and takes a while to update..

Actually, for the everest operator we are setting the startingCSV so it never returns at line 581 because in line 575 we set it.

[mayankshah1607]

Actually, for the everest operator we are setting the startingCSV so it never returns at line 581 because in line 575 we set it.

If you trace back how the req is passed, we never set the StartingCSV value there (it is empty).. So it is the same as "" and 581 gets triggered

[recharte]

Actually, for the everest operator we are setting the startingCSV so it never returns at line 581 because in line 575 we set it.

If you trace back how the req is passed, we never set the StartingCSV value there (it is empty).. So it is the same as "" and 581 gets triggered

Maybe I'm missing something but I don't think that is true. Let's take a closer look...
The getTargetInstallPlanName function is called here
In this function the req is never changed so it comes from the caller function. This call is here
The startingCSV is set here which comes from here which in turn takes the version param that comes from the caller function.
This is the call which has the v var set as long as it exists in the recommended versions from VS. These recommended versions come from here.

Is the recommended version empty in VS? 🙈

[mayankshah1607]

Is the recommended version empty in VS? 🙈

If you trace back ever further and see the implementation of ReccomendedVersions, you'll see that we never set EverestOperator there. So, its nil.. Now if you follow along from there, it eventually leads to not setting req.StartingCSV 😅

[recharte]

Is the recommended version empty in VS? 🙈

If you trace back ever further and see the implementation of ReccomendedVersions, you'll see that we never set EverestOperator there. So, its nil.. Now if you follow along from there, it eventually leads to not setting the StartingCSV 😅

But then how is it set in the Subscription? I always see it set there...

[mayankshah1607]

Is the recommended version empty in VS? 🙈

If you trace back ever further and see the implementation of ReccomendedVersions, you'll see that we never set EverestOperator there. So, its nil.. Now if you follow along from there, it eventually leads to not setting the StartingCSV 😅

But then how is it set in the Subscription? I always see it set there...

My bad, you're right. I missed this line where we're checking if it is nil, and then using the latest version:

everest/pkg/install/install.go

Line 231 in e04e882

recVer.EverestOperator = latest