EVEREST-1618: affinity per component #592
Conversation
| @@ -126,6 +126,9 @@ func (p *applier) Engine() error { | |||
| if p.DB.Status.Status == everestv1alpha1.AppStateReady { | |||
| pxc.Spec.PXC.PodSpec.Affinity = p.currentPerconaXtraDBClusterSpec.PXC.Affinity | |||
| } | |||
|
|
|||
| p.configureEngineAffinity() | |||
There was a problem hiding this comment.
I think we need to do some cleanup above because we were setting some default affinity that we got from common.DefaultAffinitySettings.
Besides cleaning up those defaults, we should also ensure that we don't change the affinity for existing clusters (see the if condition 4 lines above this comment) otherwise all DB clusters would restart when upgrading the everest operator to 1.5.
There was a problem hiding this comment.
I'm not sure I follow.
Re restarts - the p.configureEngineAffinity() triggers a change only in case you configure affinity configuration for an Engine.
Since the spec does not support affinity at the moment, existing clusters shall not be affected.
Re cleanup - the line
cleans up previous affinity settings. Is there anything else you had in mind?There was a problem hiding this comment.
You are right when you say that this keeps the backward compatibility and won't lead to any restarts because the field will be empty in existing DBCs but please read on...
the p.configureEngineAffinity() triggers a change only in case you configure affinity configuration for an Engine.
This is actually a deviation from what we envisioned as the intended behavior.
The initial idea was that the affinity field in the DBC spec would be the source of truth, if it was empty there would be no affinity rules applied to the PXC CR. With the current implementation, this is not possible, we'll always have at least the default rule from common.DefaultAffinitySettings() and it will get overwritten by whatever the user inputs in the affinity field. Ultimately, this means that users won't be able to delete all affinity rules.
Always taking the affinity from the DBC spec would be an easy implementation change. However, I don't know how we would be able to ensure that existing DBs wouldn't be affected. The affinity field would need to be automatically populated with the value from common.DefaultAffinitySettings() in order to keep the existing behavior. 😕
There was a problem hiding this comment.
You're right.
I thought it's better not to restart the DB and keep the behavior consistent across minor versions.
Happy to change this in a way which makes more sense. It doesn't seem to be possible to determine if the default affinity settings are expected if affinity is not provided.
There was a problem hiding this comment.
@michal-kralik since this PR will apply the affinity configuration that we get from the UI we need to hold this PR until the FE has also implemented this change 🙂 (Disabled auto-merge)
The change will not have any effect if an affinity configuration is not provided. Without the UI change, the behavior will be the same regardless if the PR gets merged. |
CHANGE DESCRIPTION
Problem:
EVEREST-1618
Short explanation of the problem.
Add configuration of affinity per component.
The affinity configuration is forwarded to the respective operator.
CHECKLIST
Jira
Tests