docs: add mutex improvement proposal

[isubasinghe]

Related to #8684

[sarabala1979]

@isubasinghe Can you present in the contributor meeting? We can discuss

[isubasinghe]

update: Discussed with @sarabala1979 what the next steps are for this. Bala mentioned to try replace the getResourceKey with getHolderKey. I thought during the meeting this would work as well, looking into that again this will not work without changing some data in one of the structs.

@sarabala1979 could you also please clarify here why we can't go with the config map approach? I believe it was about push back regarding introducing another resource?

The next steps for this would be to go with modifying the format of the key in the MutexHolding struct. @sarabala1979 with your okay, I will proceed with this fix.

[isubasinghe]

Hey @juliev0 or @terrytangyuan could you please give me some feedback on this if you have any time available? Mutexes have been broken since they were implemented. I have a partial solution here: #10009.

But I need to know if I can go ahead with this approach before continuing to invest more time/money in it.

Thanks!

TL;DR -> store a couple more mutex related information in config maps

[terrytangyuan]

I haven't got a chance to look into details yet but would this be a breaking change for current users who rely on mutexes?

[isubasinghe]

@terrytangyuan it shouldn't change anything for users, it will introduce an additional config map where mutex related data is stored.

[sarabala1979]

Do you think there is a security concern in multi-tenant env? If Controller updates the holder on Configmap, The controller should have cluster-level RBAC to update the config map across the namespace.

[sarabala1979]

How will these solutions form the holder keys? Will it use string format?
Can you provide the sample yaml how it will look?

[sarabala1979]

Can you add sample configmap content?

[sarabala1979]

how can we handle if holder information crosses the 1MB limit on configmap?

[Bwvolleyball]

This issue still feels relevant, right?

[agilgur5]

some formatting changes below that make the rendered version a good bit easier to read

[agilgur5]

this file is roughly alphabetical.

MutexStatus and SempahoreStatus could be written with backticks which would no longer be spell checked

[agilgur5]

Does this change a field on the CR itself? Or just the internal structs?

If it is just internal structs, a restart of the controller should get everything into the new format.

If it is on the CR, perhaps there could be a in-between option -- a struct that is encoded into a string?

[isubasinghe]

I don't think it is just internal structs unfortunately, it gets serialised into json and protobufs hence why I assumed there might be some usage here from the outside world.

Look, honestly seems very unlikely though, think we can probably just change the struct. Chucking everything in a ConfigMap would keep things consistent between different versions.

I am open to just changing the struct. This is basically an RFC to get some idea of what to do.

[agilgur5]

Now that I'm more familiar with the Controller, I believe this impacts the status.synchronization field on the CR. We technically can't change the CRD without a breaking version change (e.g. v1alpha1 -> v1alpha2 or similar).

We can add fields though, as we do quite often already. So perhaps we deprecate the existing fields and only use the new fields moving forward. In either manner, we would need to ensure backward-compat with older Workflows that used the old format.

I think that would mitigate the breaking change part and not have the drawbacks of additional ConfigMaps

[isubasinghe]

closed thanks to #13553