operator: fix upgradeImages

[hj-johannes-lee]

fixes: #1873
Operatorhub bundle can have sha256 image tags that are put through env vars. When operator controller manager gets upgraded, its operands (plugin daemonsets) should be updated to the image in the env vars. But it has not been working properly because of wrong parsing.

Fix it to parse the image names that have sha256 tags correctly so env vars in operator can be used as intended.

[mythi]

For sha256 image tags, it has not been working properly.

Can you write how you expect it to work and how it works now. IIRC we decided SHA pinning is only for the env var approach.

[hj-johannes-lee]

If image is intel-qat-plugin@sha256:69858421d6abd38867f61844db2c3fa7275bf6c089efd547e7de98dfc0c90a1a

Before:
name was intel-qat-plugin@sha256
and upgradeImage was not working even though envVar is properly set.

For example, let's say there is 0.30.0 version of deployed operatorhub bundle using olm (as written in the link), and then let's say we deploy a qat plugin (0.30.0 version).
And then we update the bundle with the comamnd:

operator-sdk run bundle-upgrade intel/intel-deviceplugins-operator:0.31.0

Then, the qat-plugin is expected to be upgraded automatically according to the envVar, but it does not because the parsing has been wrong (intel-qat-plugin@sha256)

So,
I expect name to be intel-qat-plugin so that it can be updated when another sha256 is found from envVar.
And it works now like that, and tested with olm.

IIRC we decided SHA pinning is only for the env var approach.

Yeah, this fix is for fixing the misimplementation with env var approach.
Maybe I need to make the commit message more clear.

[mythi]

Thanks for noticing this! TestUpgrade unit test needs updating to get this covered. Also .1 release is needed

[hj-johannes-lee]

Yeah, I think we should have had it already... It's my mistake.
I pushed the commit for the testUpgrade fix too.

[mythi]

for full coverage, we'd also need a case where image is same as the corresponding envVar and when the envVar is not in the pinned/sha256 format.

[hj-johannes-lee]

@mythi

for full coverage, we'd also need a case where image is same as the corresponding envVar and when the envVar is not in the pinned/sha256 format.

Umm, we do not check in reconciler.go whether the envVar value is sha256 format or not.
In our current implementation, we always expect the envvar value for the image would be in sha256 format as the name of the envvar itself is like INTEL_DSA_PLUGIN_SHA

So, do you want to improve the implementation? Or... did I misunderstand what you mean?

[mythi]

So, do you want to improve the implementation?

we can focus on the current functionality and make sure all scenarios are covered

[hj-johannes-lee]

@mythi
Thanks for the comment. I think we all cover for the "current implementation."
edit: We need to cover the case of 'same image given in env var'. Let me add it.

Anyway, I opened an issue because we should improve the functionality of UpgradeImage at some point.
#1881

[hj-johannes-lee]

Fixed.!

And,, added some comments in the code to make it understandable more easily (actually for myself to understand next time.. I need to always follow the process and understand again...). If you think it's not good, let me konw.!

[mythi]

LGTM, thanks!