Skip to content

Commit 77040f2

Browse files
efiacorefiacor
authored
CRD E2E Ginkgo Suite (#964)
* Add CRD e2e suite Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech> * Update CRD test Readme Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech> * Add e2e tests for deletion git cleanup and zombie prevention * Addtional test and fixes Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech> * Fix data race in FunctionConfigStore exec cache by adding missing write lock and synchronized lookup * fix: pre-populate FunctionConfigStore on startup to prevent empty exec cache after pod restart * fix: stabilize e2e tests against PRR conflicts, render-lifecycle race, and SSA patch timing * Stabilize post-render PRR assertions with Eventually to handle cache propagation delay * Fix nil map panic in updatePRRResources after controller restart * Re-read PR after render to prevent stale lifecycle revert * Always check render staleness to prevent source-triggered render from overwriting a concurrent push * Guard against DB dual-writer race with waitForPRRVisible and sleep before push in e2e tests * Requeue on lifecycle transition failure to retry transient git push errors * Skip rapid-push stale detection test pending render cancellation (GH #1125) * Extract prePopulateFunctionConfigStore and add unit tests for coverage * Add sleep to PRR edge case to avoid db race Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech> * Fix lifecycle transition wiping resources: only save resources to DB when modified * Wrap DB resource writes in transaction to prevent concurrent interleaving corruption * Disable ctr restart resilience test temp Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech> * Fix FnConfig deploy race Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech> * Fix FnConf test fails Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech> --------- Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech>
1 parent 7cc5141 commit 77040f2

39 files changed

Lines changed: 4946 additions & 38 deletions

‎.github/workflows/porch-e2e-ci-jobs.yaml‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,11 @@ jobs:
140140
test_path: "${GITHUB_WORKSPACE}/test/e2e/cli"
141141
test_env: "E2E=1"
142142
log_prefix: "porch-cli-e2e-dbcache"
143+
- name: "Porch CRD E2E Tests (v1alpha2)"
144+
make_target: "run-in-kind-v1alpha2"
145+
test_path: "${GITHUB_WORKSPACE}/test/e2e/crd -ginkgo.v"
146+
test_env: "E2E=1"
147+
log_prefix: "porch-crd-e2e"
143148

144149
steps:
145150
- name: Checkout Porch
@@ -171,7 +176,25 @@ jobs:
171176
kind load docker-image ${IMAGE_REPO}/${PORCH_FUNCTION_RUNNER_IMAGE}:${{ needs.build.outputs.image-tag }} -n ${KIND_CONTEXT_NAME}
172177
kind load docker-image ${IMAGE_REPO}/${PORCH_WRAPPER_SERVER_IMAGE}:${{ needs.build.outputs.image-tag }} -n ${KIND_CONTEXT_NAME}
173178
- name: Deploy porch kpt pkg
179+
timeout-minutes: 10
174180
run: IMAGE_TAG=${{ needs.build.outputs.image-tag }} SKIP_IMG_BUILD=true make ${{ matrix.make_target }}
181+
- name: Dump cluster state on deploy failure
182+
if: failure()
183+
run: |
184+
echo "=== Pods (all namespaces) ==="
185+
kubectl get pods -A
186+
echo "=== Events (porch-system) ==="
187+
kubectl get events -n porch-system --sort-by='.lastTimestamp' | tail -40
188+
echo "=== Non-running pods details ==="
189+
kubectl get pods -A -o json | jq -r '.items[] | select(.status.phase != "Running" and .status.phase != "Succeeded") | "\n--- \(.metadata.namespace)/\(.metadata.name) (\(.status.phase)) ---"'
190+
for pod in $(kubectl get pods -A -o json | jq -r '.items[] | select(.status.phase != "Running" and .status.phase != "Succeeded") | "\(.metadata.namespace)/\(.metadata.name)"'); do
191+
ns=$(echo $pod | cut -d/ -f1)
192+
name=$(echo $pod | cut -d/ -f2)
193+
echo "--- Describe $ns/$name ---"
194+
kubectl describe pod -n $ns $name | tail -20
195+
echo "--- Logs $ns/$name ---"
196+
kubectl logs -n $ns $name --all-containers --tail=30 2>/dev/null || true
197+
done
175198
- name: Run E2E tests
176199
run: ${{ matrix.test_env }} go test -v -timeout 20m ${{ matrix.test_path }}
177200
- name: Export porch server logs

‎controllers/functionconfigs/reconciler/functionconfigreconciler.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,9 @@ func (s *FunctionConfigStore) UpdateBinaryCache(_ string, obj *configapi.Functio
125125
}
126126

127127
func (s *FunctionConfigStore) UpdateExecCache(name string, functionConfig *configapi.FunctionConfig) {
128+
s.mu.Lock()
129+
defer s.mu.Unlock()
130+
128131
functionAliases := map[string][]string{}
129132

130133
id := name
@@ -201,6 +204,14 @@ func (s *FunctionConfigStore) GetExecCache() map[string]fnsdk.ResourceListProces
201204
return s.builtInExecutorCache
202205
}
203206

207+
// GetProcessorFromCache looks up a function processor by image, holding the read lock for the duration of the lookup.
208+
func (s *FunctionConfigStore) GetProcessorFromCache(image string) (fnsdk.ResourceListProcessor, bool) {
209+
s.mu.RLock()
210+
defer s.mu.RUnlock()
211+
processor, found := s.builtInExecutorCache[image]
212+
return processor, found
213+
}
214+
204215
func (s *FunctionConfigStore) List() []*configapi.FunctionConfig {
205216
s.mu.Lock()
206217
defer s.mu.Unlock()
@@ -268,6 +279,9 @@ func (r *FunctionConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reque
268279

269280
if err := r.Client.Status().Patch(ctx, obj, patch); err != nil {
270281
klog.Errorf("Failed to update status of FunctionConfig %q: %v", obj.Name, err)
282+
if finalErr == nil {
283+
finalErr = err
284+
}
271285
}
272286
}()
273287

‎controllers/functionconfigs/reconciler/functionconfigreconciler_test.go‎

Lines changed: 138 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ func TestFunctionConfigReconciler(t *testing.T) {
194194
for _, tt := range tests {
195195
tt := tt // pin for closure
196196
t.Run(tt.name, func(t *testing.T) {
197-
c := fake.NewClientBuilder().WithObjects(tt.objs...).WithScheme(scheme).Build()
197+
c := fake.NewClientBuilder().WithObjects(tt.objs...).WithScheme(scheme).WithStatusSubresource(&configapi.FunctionConfig{}).Build()
198198

199199
functionConfigStore := NewFunctionConfigStore(defaultImagePrefix, functionCacheDir)
200200
reconciler := &FunctionConfigReconciler{
@@ -259,7 +259,7 @@ func TestFinalizersAdded(t *testing.T) {
259259
},
260260
}
261261

262-
c := fake.NewClientBuilder().WithScheme(schemeWithFunctionConfig(t)).WithObjects(obj).Build()
262+
c := fake.NewClientBuilder().WithScheme(schemeWithFunctionConfig(t)).WithObjects(obj).WithStatusSubresource(&configapi.FunctionConfig{}).Build()
263263
r := &FunctionConfigReconciler{
264264
Client: c,
265265
FunctionConfigStore: NewFunctionConfigStore(defaultImagePrefix, functionCacheDir),
@@ -279,6 +279,141 @@ func TestFinalizersAdded(t *testing.T) {
279279
}
280280
}
281281

282+
func TestGetProcessorFromCache(t *testing.T) {
283+
store := NewFunctionConfigStore(defaultImagePrefix, functionCacheDir)
284+
285+
// Populate via UpdateExecCache (same path as reconciler)
286+
obj := &configapi.FunctionConfig{
287+
ObjectMeta: metav1.ObjectMeta{Name: "set-namespace", Namespace: testNamespace},
288+
Spec: configapi.FunctionConfigSpec{
289+
Image: "set-namespace",
290+
Prefixes: []string{""},
291+
GoExecutor: &configapi.GoExecutorConfig{
292+
Tags: []string{"v0.4.1"},
293+
},
294+
},
295+
}
296+
store.UpdateExecCache(obj.Name, obj)
297+
298+
// Found with full prefix
299+
processor, found := store.GetProcessorFromCache("ghcr.io/kptdev/krm-functions-catalog/set-namespace:v0.4.1")
300+
assert.True(t, found)
301+
assert.NotNil(t, processor)
302+
303+
// Found without prefix (short form)
304+
processor, found = store.GetProcessorFromCache("set-namespace:v0.4.1")
305+
assert.True(t, found)
306+
assert.NotNil(t, processor)
307+
308+
// Not found for unknown tag
309+
_, found = store.GetProcessorFromCache("set-namespace:v9.9.9")
310+
assert.False(t, found)
311+
312+
// Not found for unknown image
313+
_, found = store.GetProcessorFromCache("nonexistent:v1.0.0")
314+
assert.False(t, found)
315+
}
316+
317+
func TestPrePopulationPattern(t *testing.T) {
318+
// Simulates what setupFunctionConfigReconciler does on cold start:
319+
// list all FunctionConfigs and populate the store synchronously
320+
// without going through the reconcile loop.
321+
store := NewFunctionConfigStore(defaultImagePrefix, functionCacheDir)
322+
323+
configs := []configapi.FunctionConfig{
324+
{
325+
ObjectMeta: metav1.ObjectMeta{Name: "set-namespace", Namespace: testNamespace},
326+
Spec: configapi.FunctionConfigSpec{
327+
Image: "set-namespace",
328+
Prefixes: []string{""},
329+
GoExecutor: &configapi.GoExecutorConfig{Tags: []string{"v0.4.1"}},
330+
},
331+
},
332+
{
333+
ObjectMeta: metav1.ObjectMeta{Name: "apply-replacements", Namespace: testNamespace},
334+
Spec: configapi.FunctionConfigSpec{
335+
Image: "apply-replacements",
336+
Prefixes: []string{""},
337+
GoExecutor: &configapi.GoExecutorConfig{Tags: []string{"v0.1.1"}},
338+
},
339+
},
340+
{
341+
ObjectMeta: metav1.ObjectMeta{Name: "set-image", Namespace: testNamespace},
342+
Spec: configapi.FunctionConfigSpec{
343+
Image: "set-image",
344+
Prefixes: []string{""},
345+
BinaryExecutor: &configapi.BinaryExecutorConfig{
346+
Tags: []string{"v0.1.4"},
347+
Path: "set-image",
348+
},
349+
},
350+
},
351+
}
352+
353+
// Pre-populate (mirrors the code in setupFunctionConfigReconciler)
354+
for i := range configs {
355+
obj := &configs[i]
356+
store.UpsertFunctionConfig(obj.Name, obj)
357+
if obj.Spec.GoExecutor != nil {
358+
store.UpdateExecCache(obj.Name, obj)
359+
}
360+
if obj.Spec.BinaryExecutor != nil {
361+
store.UpdateBinaryCache(obj.Name, obj)
362+
}
363+
}
364+
365+
// Verify exec cache is populated
366+
_, found := store.GetProcessorFromCache("ghcr.io/kptdev/krm-functions-catalog/set-namespace:v0.4.1")
367+
assert.True(t, found, "set-namespace should be in exec cache after pre-population")
368+
369+
_, found = store.GetProcessorFromCache("ghcr.io/kptdev/krm-functions-catalog/apply-replacements:v0.1.1")
370+
assert.True(t, found, "apply-replacements should be in exec cache after pre-population")
371+
372+
// Verify binary cache is populated
373+
path, found := store.GetBinaryFromCache("ghcr.io/kptdev/krm-functions-catalog/set-image:v0.1.4")
374+
assert.True(t, found, "set-image should be in binary cache after pre-population")
375+
assert.Equal(t, "/functions/set-image", path)
376+
377+
// Verify function configs are stored
378+
assert.Len(t, store.List(), 3)
379+
}
380+
381+
func TestConcurrentAccessSafety(t *testing.T) {
382+
// Verifies no data race when UpdateExecCache and GetProcessorFromCache
383+
// are called concurrently (the fix for the data race bug).
384+
store := NewFunctionConfigStore(defaultImagePrefix, functionCacheDir)
385+
386+
obj := &configapi.FunctionConfig{
387+
ObjectMeta: metav1.ObjectMeta{Name: "set-namespace", Namespace: testNamespace},
388+
Spec: configapi.FunctionConfigSpec{
389+
Image: "set-namespace",
390+
Prefixes: []string{""},
391+
GoExecutor: &configapi.GoExecutorConfig{Tags: []string{"v0.4.1"}},
392+
},
393+
}
394+
395+
done := make(chan struct{})
396+
397+
// Writer goroutine
398+
go func() {
399+
defer close(done)
400+
for i := 0; i < 100; i++ {
401+
store.UpdateExecCache(obj.Name, obj)
402+
}
403+
}()
404+
405+
// Reader goroutine (concurrent with writer)
406+
for i := 0; i < 100; i++ {
407+
store.GetProcessorFromCache("ghcr.io/kptdev/krm-functions-catalog/set-namespace:v0.4.1")
408+
}
409+
410+
<-done
411+
412+
// After all writes complete, the entry should be present
413+
_, found := store.GetProcessorFromCache("ghcr.io/kptdev/krm-functions-catalog/set-namespace:v0.4.1")
414+
assert.True(t, found)
415+
}
416+
282417
func TestFinalizersRemoved(t *testing.T) {
283418
now := metav1.Now()
284419
const testFinalizer = "config.porch.kpt.dev/test-hold"
@@ -318,7 +453,7 @@ func TestFinalizersRemoved(t *testing.T) {
318453
},
319454
}
320455

321-
c := fake.NewClientBuilder().WithScheme(schemeWithFunctionConfig(t)).WithObjects(obj).Build()
456+
c := fake.NewClientBuilder().WithScheme(schemeWithFunctionConfig(t)).WithObjects(obj).WithStatusSubresource(&configapi.FunctionConfig{}).Build()
322457
store := NewFunctionConfigStore(defaultImagePrefix, functionCacheDir)
323458
store.UpsertFunctionConfig(objName, obj)
324459

‎controllers/main.go‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -319,10 +319,35 @@ func setupFunctionConfigReconciler(mgr ctrl.Manager) (*reconciler.FunctionConfig
319319
return nil, fmt.Errorf("error creating FunctionConfig controller: %w", err)
320320
}
321321

322+
prePopulateFunctionConfigStore(mgr.GetAPIReader(), functionConfigStore)
323+
322324
klog.Infof("FunctionConfig reconciler registered (for: %s)", reconciler.ReconcilerForController)
323325
return functionConfigStore, nil
324326
}
325327

328+
// prePopulateFunctionConfigStore loads all FunctionConfigs into the store
329+
// synchronously so the exec cache is ready before the PR controller starts.
330+
// Without this, a pod restart leaves the cache empty until the async
331+
// informer triggers reconciliation.
332+
func prePopulateFunctionConfigStore(reader client.Reader, store *reconciler.FunctionConfigStore) {
333+
var fcList configapi.FunctionConfigList
334+
if err := reader.List(context.Background(), &fcList); err != nil {
335+
klog.Warningf("FunctionConfig pre-population failed (non-fatal): %v", err)
336+
return
337+
}
338+
for i := range fcList.Items {
339+
obj := &fcList.Items[i]
340+
store.UpsertFunctionConfig(obj.Name, obj)
341+
if obj.Spec.GoExecutor != nil {
342+
store.UpdateExecCache(obj.Name, obj)
343+
}
344+
if obj.Spec.BinaryExecutor != nil {
345+
store.UpdateBinaryCache(obj.Name, obj)
346+
}
347+
}
348+
klog.Infof("FunctionConfig store pre-populated with %d configs", len(fcList.Items))
349+
}
350+
326351

327352
// --- Helpers ---
328353

‎controllers/main_test.go‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,15 @@ import (
1919
"flag"
2020
"testing"
2121

22+
configapi "github.com/nephio-project/porch/api/porchconfig/v1alpha1"
23+
"github.com/nephio-project/porch/controllers/functionconfigs/reconciler"
24+
mockclient "github.com/nephio-project/porch/test/mockery/mocks/external/sigs.k8s.io/controller-runtime/pkg/client"
2225
"github.com/stretchr/testify/assert"
26+
"github.com/stretchr/testify/mock"
2327
"github.com/stretchr/testify/require"
2428
"k8s.io/apimachinery/pkg/runtime/schema"
2529
ctrl "sigs.k8s.io/controller-runtime"
30+
"sigs.k8s.io/controller-runtime/pkg/client"
2631
"sigs.k8s.io/controller-runtime/pkg/reconcile"
2732
)
2833

@@ -132,3 +137,69 @@ func TestReconcilersMapContainsAllReconcilers(t *testing.T) {
132137
}
133138
assert.Len(t, reconcilers, len(expected))
134139
}
140+
141+
// --- prePopulateFunctionConfigStore ---
142+
143+
func TestPrePopulateFunctionConfigStore_Success(t *testing.T) {
144+
items := []configapi.FunctionConfig{
145+
{
146+
Spec: configapi.FunctionConfigSpec{
147+
GoExecutor: &configapi.GoExecutorConfig{
148+
Tags: []string{"v0.4.1"},
149+
},
150+
},
151+
},
152+
{
153+
Spec: configapi.FunctionConfigSpec{
154+
BinaryExecutor: &configapi.BinaryExecutorConfig{
155+
Tags: []string{"v1.0.0"},
156+
Path: "/usr/local/bin/starlark",
157+
},
158+
},
159+
},
160+
{
161+
Spec: configapi.FunctionConfigSpec{},
162+
},
163+
}
164+
items[0].Name = "set-namespace"
165+
items[1].Name = "starlark"
166+
items[2].Name = "no-executor"
167+
168+
mockReader := mockclient.NewMockReader(t)
169+
mockReader.EXPECT().List(mock.Anything, mock.AnythingOfType("*v1alpha1.FunctionConfigList"), mock.Anything).
170+
Run(func(_ context.Context, list client.ObjectList, _ ...client.ListOption) {
171+
list.(*configapi.FunctionConfigList).Items = items
172+
}).Return(nil)
173+
174+
store := reconciler.NewFunctionConfigStore("ghcr.io/kptdev", "/tmp/bins")
175+
prePopulateFunctionConfigStore(mockReader, store)
176+
177+
_, ok := store.GetFunctionConfig("set-namespace")
178+
assert.True(t, ok, "set-namespace should be in store")
179+
_, ok = store.GetFunctionConfig("starlark")
180+
assert.True(t, ok, "starlark should be in store")
181+
_, ok = store.GetFunctionConfig("no-executor")
182+
assert.True(t, ok, "no-executor should be in store")
183+
}
184+
185+
func TestPrePopulateFunctionConfigStore_ListError(t *testing.T) {
186+
mockReader := mockclient.NewMockReader(t)
187+
mockReader.EXPECT().List(mock.Anything, mock.Anything, mock.Anything).Return(assert.AnError)
188+
189+
store := reconciler.NewFunctionConfigStore("ghcr.io/kptdev", "/tmp/bins")
190+
prePopulateFunctionConfigStore(mockReader, store)
191+
192+
_, ok := store.GetFunctionConfig("anything")
193+
assert.False(t, ok, "store should be empty after list error")
194+
}
195+
196+
func TestPrePopulateFunctionConfigStore_EmptyList(t *testing.T) {
197+
mockReader := mockclient.NewMockReader(t)
198+
mockReader.EXPECT().List(mock.Anything, mock.AnythingOfType("*v1alpha1.FunctionConfigList"), mock.Anything).
199+
Return(nil)
200+
201+
store := reconciler.NewFunctionConfigStore("ghcr.io/kptdev", "/tmp/bins")
202+
prePopulateFunctionConfigStore(mockReader, store)
203+
204+
assert.Equal(t, 0, len(store.List()))
205+
}

‎controllers/packagerevisions/pkg/controllers/packagerevision/packagerevision_controller.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,14 @@ func (r *PackageRevisionReconciler) Reconcile(ctx context.Context, req ctrl.Requ
9494
return resultOrDefault(result), nil
9595
}
9696

97+
// Re-read to pick up spec changes (e.g. lifecycle transitions) that
98+
// occurred while render was in-flight. A validating webhook should
99+
// eventually block lifecycle transitions during render, making this
100+
// re-read unnecessary.
101+
if err := r.Get(ctx, req.NamespacedName, &pr); err != nil {
102+
return ctrl.Result{}, client.IgnoreNotFound(err)
103+
}
104+
97105
return r.reconcileLifecycle(ctx, &pr, repoKey)
98106
}
99107

@@ -135,7 +143,7 @@ func (r *PackageRevisionReconciler) reconcileLifecycle(ctx context.Context, pr *
135143
if err != nil {
136144
log.Error(err, "lifecycle transition failed")
137145
r.updateStatus(ctx, pr, nil, "", readyCondition(pr.Generation, metav1.ConditionFalse, porchv1alpha2.ReasonFailed, err.Error()))
138-
return ctrl.Result{}, nil
146+
return ctrl.Result{Requeue: true}, nil
139147
}
140148

141149
r.updateStatus(ctx, pr, updated, "", readyCondition(pr.Generation, metav1.ConditionTrue, porchv1alpha2.ReasonReady, ""))

0 commit comments

Comments
 (0)