configmap based mechanism to override the admin/canary versions

[shawkins]

On the epic brief it appeared there wasn't a good way to associate this information with the strimzi deployment. So I'm assuming we need to use configmaps. I didn't see any information on in the epic brief or the presentation about how those would be created. My assumption here is that there would be 1 configmap per strimzi version. The name of the configmap would match the name of the strimzi version. The map would contain 3 key/value pairs. One for each of the images:

admin_api=quay.io/mk-ci-cd/kafka-admin-api:0.7.0
canary=...
canary_init=...

It falls back to the defaults in the application.properties - that could of course fall back in other ways such as sorting the versions, or even cause the managedkafka to be invalid.

The resync logic could be made more targeted if needed - just filtering to those managedkafkas that have matching strimzi versions.

But before doing a lot more I wanted to see if this matches up with people's new expectations based upon what was discussed in the standup.

[rareddy]

Yes, this is taking in the direction of our conversation. we already have a config map for log4j properties in the strimzi bundle, maybe we can add additional keys into it?

[shawkins]

Yes, this is taking in the direction of our conversation. we already have a config map for log4j properties in the strimzi bundle, maybe we can add additional keys into it?

Sorry I should have modeled this on that from the start.

It's been updated to use that map by checking for the app label and if the name is the same as the deployment that the strimzi operator checks for. It could be combined with either the strimzi manager or the bundle manager if we don't want another manager in the mix.

[rareddy]

thanks. now that there is config, what is your opinion on possibly introducing ENV variables along with image URLs for completeness?

[shawkins]

what is your opinion on possibly introducing ENV variables along with image URLs for completeness?

In looking at the code changes to the buildEnvVar methods, the only simplistic change I can quickly see in the last 6 months that fits this paradigm is the introduction of RECONCILE_INTERVAL_MS. However that would be fine to set across all Canary. You'd need a situation where the value would change per Canary version. The other changes seem to be dependent upon the managedkafka cr, other operator state, or are more complex involving references to the canary-config for example.

[rareddy]

lgtm

[shawkins]

Fantastic... these changes cause an out of memory error on github. That does not occur locally nor is there much of a memory footprint being introduced here. I'll see if I can track down what is happening.

[shawkins]

Fantastic... these changes cause an out of memory error on github. That does not occur locally nor is there much of a memory footprint being introduced here. I'll see if I can track down what is happening.

As far as I can tell we're hitting a quarkus issue here that's quite odd. I can strip down the class to have no logic/state, just return the default images, and I'll still either hit build or what looks to be test issues. Just the presence of this additional application bean is too much.

[MikeEdgar]

Did you get it to happen locally? Could it have been fixed in a more recent Quarkus version?

[shawkins]

Did you get it to happen locally?

Yes, a couple build iterations will consistently reproduce.

Could it have been fixed in a more recent Quarkus version?

Don't know. I've only confirmed that it's not due to any actual logic, just the introduction of the application bean.

[k-wall]

Nice work @shawkins, approach looks good.

[shawkins]

I've tacked a question about this build error onto quarkusio/quarkus#18973 (comment) - as it seems similar to the behavior we are seeing with the test server annotation. I've updated a branch to latest fabric8/quarkus/operator sdk and reproduced the test issue. In the meantime I'll confirm if this seems to still happen with that upgrade in place - and open a separate pr that contains just the upgrade work #648

[ppatierno]

can we try to have a kind of same structure for the properties configured via env var or application.properties file instead of the fleetshard_operands.yaml" with overrides? I mean naming the above as admin-server.image, canary.image and canary.init.image ? Wdyt?

[shawkins]

I mean naming the above as admin-server.image, canary.image and canary.init.image ? Wdyt?

In practical terms once this mechanism is utilized the old / default values should go unused. Turning the override into a properties file precludes easily using more structured parsing down the road (not that I'd like to overly worry about things that may not be needed). In the interest of time, I'd probably leave this as is at this point.

[shawkins]

@rareddy @k-wall As far I can there are 4-5 ways to address the build issue:

• Update the amount of resources for the build/test runs (now with this pr) - doubling basepom.test.memory seems to work - but we'll of course not be able to do this too much and may in the future do one of these others.
• change the operator tests to reuseForks=false - since the tests are not currently parallelized, this greatly impacts the time it takes to run the tests.
• Use a new module - tried with a github commit as well. It's messy but it works...
• Patching okhttp to remove/replace their timeout implementation
• Patch fabric8 to allow it to disable timeouts. That would need to include the mock server as well.

[rareddy]

I am ok with taking first approach as you already coded it as an interim approach. I am not sure what patching okhttp involves, you mean at fabric8 level?

[shawkins]

I am not sure what patching okhttp involves, you mean at fabric8 level?

Someone would have to own a jar that at least overrides the offending class.

I'll be making a separate determination of whether this affects the mock server and how to proceed from there.