Make the configuration conversion stable #1990
Conversation
fedepaol
requested review from
rata,
russellb,
gclawes and
oribon
as code owners
fedepaol
force-pushed
the
stablereconcile
branch
4 times, most recently
from
4f440c8
to
dfdddc1
Compare
internal/bgp/frr/frr_test.go
Outdated
| if err != nil { | ||
| t.Fatalf("Could not advertise prefix: %s", err) | ||
| } | ||
| err = session1.Set(adv1, adv2) | ||
|
|
||
| rand.Shuffle(len(advs), func(i, j int) { |
There was a problem hiding this comment.
Can we add more attempts to shuffle and check like in config_conversion_test.go so we'll be more confident?
There was a problem hiding this comment.
This will require a bit of reworking, as I don't want to pay the FRR extra effort every iteration. Let me try to fix it.
| v1.ConfigMap{}) | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("conversion failed") |
| v1.ConfigMap{}) | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("conversion failed") |
| } | ||
|
|
||
| if !reflect.DeepEqual(config, oldConfig) { | ||
| t.Fatalf("conversion is not stable, seed %d, %v\n %v\n", seed, spew.Sdump(config), spew.Sdump(oldConfig)) |
There was a problem hiding this comment.
nit: let's add these small details so the output will be clearer
| t.Fatalf("conversion is not stable, seed %d, %v\n %v\n", seed, spew.Sdump(config), spew.Sdump(oldConfig)) | |
| t.Fatalf("conversion is not stable, seed %d, got: %v\n expected: %v\n", seed, spew.Sdump(config), spew.Sdump(oldConfig)) |
There was a problem hiding this comment.
The got - expected here makes less sense because we don't know which version is the expected (right) one. I'll name them current - previous
| if !reflect.DeepEqual(config, oldConfig) { | ||
| t.Fatalf("conversion is not stable, seed %d, %v\n %v\n", seed, spew.Sdump(config), spew.Sdump(oldConfig)) | ||
| } | ||
| oldConfig = config |
There was a problem hiding this comment.
I think we can omit this assignment as the value doesn't change, and keep comparing to the same oldConfig.
There was a problem hiding this comment.
yep, makes sense. Fixed
| cfg, err := toConfig(r.ValidateConfig, | ||
| ipAddressPools.Items, | ||
| []metallbv1beta2.BGPPeer{}, | ||
| []metallbv1beta1.BFDProfile{}, | ||
| []metallbv1beta1.L2Advertisement{}, | ||
| []metallbv1beta1.BGPAdvertisement{}, | ||
| addressPools.Items, | ||
| communities.Items, | ||
| map[string]corev1.Secret{}, | ||
| []corev1.Node{}, | ||
| namespaces.Items, | ||
| corev1.ConfigMap{}) |
There was a problem hiding this comment.
That's a little hard to read, plus we'll need to update the parameters list when a new CRD is added. why not reuse the ClusterResources struct, consume it as the input, and re-assign its fields with the sorted copy?
I would go even a step further and split toConfig into two methods, first only the logic of sorting the resources, so we'll have a sortedResources, and then call the config.For on it.
There was a problem hiding this comment.
That's a little hard to read, plus we'll need to update the parameters list when a new CRD is added.
What I was totally afraid of was to forget the sorting in case a new CRD is added. I wanted to make it really hard to forget, and that's why I came up with the long list. But I guess I can fix that by doing the sorting in the toConfig as you said. Let me try to get a stab at it
There was a problem hiding this comment.
Done, with this new version, assuming we add e2e tests covering new CRD, we'll be forced to add it
When translating the configuration coming from the k8s api to the internal one, the outcome depends on the order in which the cache returns us the various items. This may cause unnecessary reloads. In order to avoid this, we pre-order the list of items to feed the conversion with. The sorting is done in a conversion function in the controller (rather than the config package) in order to make it harder to forget to add the sorting action. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
fedepaol
force-pushed
the
stablereconcile
branch
from
dfdddc1
to
6359a2c
Compare
|
/lgtm |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
fedepaol
force-pushed
the
stablereconcile
branch
from
6359a2c
to
a3b3280
Compare
The resulting frr configuration was influenced by the order of the advertisements, which ultimately come from a map (which doesn't guarantee the order). Here we sort to make the configuration stable, and to allow us to catch non-changing configurations updates. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
fedepaol
force-pushed
the
stablereconcile
branch
from
a3b3280
to
9faeb1f
Compare
When translating the configuration coming from the k8s api to the internal one, the outcome depends on the order in which the cache returns us the various items. This may cause unnecessary reloads.
In order to avoid this, we pre-order the list of items to feed the conversion with.
The sorting is done in a conversion function in the controller (rather than the config package) in order to make it harder to forget to add the sorting action.