feat: add PDB fields in the policy server spec. #698
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #698 +/- ##
==========================================
- Coverage 50.91% 50.56% -0.35%
==========================================
Files 26 27 +1
Lines 1970 2021 +51
==========================================
+ Hits 1003 1022 +19
- Misses 866 891 +25
- Partials 101 108 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| return err | ||
| }, timeout, pollInterval).Should(Succeed()) | ||
| Eventually(func(g Gomega) error { | ||
| return policyServerPodDisruptionBudgetExists(policyServerName) |
There was a problem hiding this comment.
We could make the tests stricter by using a getTestPDB utility function and testing that the values are set correctly see
There was a problem hiding this comment.
I've pushed a gomega matcher to address this request. It's a little bit different from what you request. During the tests I found out that if we follow the example that you sent, the tests will fail after the first failed tried in the Eventually call. As far as I can see, the example test is not failing only because the order of the tests. In other words, it's passing because the order of the execution and due how the controller set policy status. The webhook always exists when the test start. To fix the test, we can follow the approach that I've used in this PR. Or we should update the Expect calls to be g.Expect (using the argument of the function.
There was a problem hiding this comment.
I'll open an PR to address this problem.
| _, err := controllerutil.CreateOrUpdate(ctx, k8s, pdb, func() error { | ||
| pdb.Name = buildPodDisruptionBudgetName(policyServer) | ||
| pdb.Namespace = namespace | ||
| pdb.OwnerReferences = []metav1.OwnerReference{ | ||
| *metav1.NewControllerRef(policyServer, policiesv1.GroupVersion.WithKind("PolicyServer")), | ||
| } | ||
| pdb.Spec.Selector = &metav1.LabelSelector{ | ||
| MatchLabels: map[string]string{ | ||
| constants.AppLabelKey: policyServer.AppLabel(), | ||
| constants.PolicyServerLabelKey: policyServer.GetName(), | ||
| }, | ||
| } | ||
| if policyServer.Spec.MinAvailable != nil { | ||
| pdb.Spec.MinAvailable = policyServer.Spec.MinAvailable | ||
| } else { | ||
| pdb.Spec.MaxUnavailable = policyServer.Spec.MaxUnavailable | ||
| } | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
The part where we set the ownerRef can be handled in a nicer way using controllerutil.SetOwnerReference.
For example:
secret := &corev1.Secret{
TypeMeta: metav1.TypeMeta{
Kind: "Secret",
APIVersion: "v1",
},
ObjectMeta: metav1.ObjectMeta{
Namespace: app.ObjectMeta.Namespace,
Name: generatedRuntimeConfigSecretName,
Labels: map[string]string{
spinapp.NameLabelKey: app.ObjectMeta.Name,
},
},
Data: map[string][]byte{
"runtime-config.toml": tomlValue,
},
}
err = controllerutil.SetOwnerReference(app, secret, r.Scheme)
if err != nil {
return fmt.Errorf("failed to set runtimeconfig owner reference: %w", err)
}
err = r.Client.Create(ctx, secret)this is taken from here
jvanz
force-pushed
the
pod-pdb
branch
2 times, most recently
from
9373c51
to
b0541ed
Compare
Adds two new fields in the PolicyServerSpec: minAvailable and maxUnavailable. These fields are used to create a PodDisruptionBudget for the policy server pods. Both fields cannot be set together because Kubernetes does not allow setting both together in the PDB spec. Signed-off-by: José Guilherme Vanz <[email protected]>
Description
Adds two new fields in the PolicyServerSpec: minAvailable and maxUnavailable. These fields are used to create a PodDisruptionBudget for the policy server pods. Both fields cannot be set together because Kubernetes does not allow setting both together in the PDB spec.
Fix #692
Test
make testNOTE: this is a draft PR because I would like to make it "ready for review" together with the Helm chart changes. However, you can review it already.