Skip to content

Commit

Permalink
Add manager package and do some housekeeping (#11)
Browse files Browse the repository at this point in the history
- Manager package constructs a `ctrl.Manager` for both integration tests
and production use, and owns indices since they are manager-scoped (so
shouldn't be owned by controllers)
- Converts `PodCreation` a pointer because there will be a period of
time between allocating the struct and populating the creation timestamp
- Adds `LastRolloutTime` to synthesizer status in anticipation of the
rollout mechanism
- Adds a custom log constructor to add controller name to the logs of
each controller. This is better than just adding the fields within the
`Reconcile` function because they will also be included when
controller-runtime logs errors.
- Adds k8s version matrix to the integration tests in CI
  • Loading branch information
jveski authored Nov 17, 2023
1 parent c4dbbd4 commit d106616
Show file tree
Hide file tree
Showing 14 changed files with 193 additions and 82 deletions.
12 changes: 10 additions & 2 deletions .github/workflows/unit.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,20 @@ on: [push]
jobs:
test:
runs-on: ubuntu-latest
env:
APISERVER_VERSION: 1.28.x
steps:
- uses: actions/checkout@v3

- name: Set up Go
uses: actions/setup-go@v4
with:
go-version: '1.21'
- name: Test
run: KUBEBUILDER_ASSETS=$(go run sigs.k8s.io/controller-runtime/tools/setup-envtest@latest use -p path 1.28.x) go test -v ./...

- name: Download kubebuilder assets
run: |
echo "KUBEBUILDER_ASSETS=$(go run sigs.k8s.io/controller-runtime/tools/setup-envtest@latest use -p path $APISERVER_VERSION)" >> $GITHUB_ENV
- name: Run tests
run: go test -v -race ./...

12 changes: 6 additions & 6 deletions api/v1/composition.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,13 @@ type CompositionStatus struct {

// Synthesis represents a Synthesizer's specific synthesis of a given Composition.
type Synthesis struct {
// metadata.generation of the Composition at the time of synthesis.
ObservedGeneration int64 `json:"observedGeneration,omitempty"`
ObservedCompositionGeneration int64 `json:"observedCompositionGeneration,omitempty"`
ObservedSynthesizerGeneration int64 `json:"observedSynthesizerGeneration,omitempty"`

// Number of resulting resource slices. Since they are immutable, this provides adequate timing signal to avoid stale informer caches.
ResourceSliceCount int64 `json:"resourceSliceCount,omitempty"`
ResourceSliceCount *int64 `json:"resourceSliceCount,omitempty"`

Ready bool `json:"ready,omitempty"`
Synced bool `json:"synced,omitempty"`
PodCreation metav1.Time `json:"podCreation,omitempty"`
Ready bool `json:"ready,omitempty"`
Synced bool `json:"synced,omitempty"`
PodCreation *metav1.Time `json:"podCreation,omitempty"`
}
20 changes: 14 additions & 6 deletions api/v1/config/crd/eno.azure.io_compositions.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ spec:
synthesizer:
description: Compositions are synthesized by a Synthesizer.
properties:
minGeneration:
description: Compositions will be resynthesized if their status.currentState.observedSynthesizerGeneration
is < the referenced synthesizer's generation. Used to slowly
roll out synthesizer updates across compositions.
format: int64
type: integer
name:
type: string
type: object
Expand All @@ -70,9 +76,10 @@ spec:
description: Synthesis represents a Synthesizer's specific synthesis
of a given Composition.
properties:
observedGeneration:
description: metadata.generation of the Composition at the time
of synthesis.
observedCompositionGeneration:
format: int64
type: integer
observedSynthesizerGeneration:
format: int64
type: integer
podCreation:
Expand All @@ -93,9 +100,10 @@ spec:
description: Synthesis represents a Synthesizer's specific synthesis
of a given Composition.
properties:
observedGeneration:
description: metadata.generation of the Composition at the time
of synthesis.
observedCompositionGeneration:
format: int64
type: integer
observedSynthesizerGeneration:
format: int64
type: integer
podCreation:
Expand Down
6 changes: 6 additions & 0 deletions api/v1/config/crd/eno.azure.io_synthesizers.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ spec:
complete.
format: int64
type: integer
lastRolloutTime:
description: LastRolloutTime is the timestamp of the last pod creation
caused by a change to this resource. Should not be updated due to
Composotion changes. Used to calculate rollout cooldown period.
format: date-time
type: string
type: object
type: object
served: true
Expand Down
13 changes: 12 additions & 1 deletion api/v1/synthesizer.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package v1

import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// +kubebuilder:object:root=true
type SynthesizerList struct {
Expand Down Expand Up @@ -29,9 +31,18 @@ type SynthesizerStatus struct {
// The metadata.generation of this resource at the oldest version currently used by any Generations.
// This will equal the current generation when slow rollout of an update to the Generations is complete.
CurrentGeneration int64 `json:"currentGeneration,omitempty"`

// LastRolloutTime is the timestamp of the last pod creation caused by a change to this resource.
// Should not be updated due to Composotion changes.
// Used to calculate rollout cooldown period.
LastRolloutTime *metav1.Time `json:"lastRolloutTime,omitempty"`
}

type SynthesizerRef struct {
// +required
Name string `json:"name,omitempty"`

// Compositions will be resynthesized if their status.currentState.observedSynthesizerGeneration is < the referenced synthesizer's generation.
// Used to slowly roll out synthesizer updates across compositions.
MinGeneration int64 `json:"minGeneration,omitempty"`
}
16 changes: 14 additions & 2 deletions api/v1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

75 changes: 75 additions & 0 deletions internal/manager/manager.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package manager

import (
"context"
"fmt"

"github.com/go-logr/logr"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/rest"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/manager"
"sigs.k8s.io/controller-runtime/pkg/metrics/server"
"sigs.k8s.io/controller-runtime/pkg/reconcile"

apiv1 "github.com/Azure/eno/api/v1"
)

const (
IdxSlicesByCompositionGeneration = ".metadata.ownerReferences.compositionGen" // see: NewSlicesByCompositionGenerationKey
)

type Options struct {
Rest *rest.Config
HealthProbeAddr string
MetricsAddr string
}

func New(logger logr.Logger, opts *Options) (ctrl.Manager, error) {
mgr, err := ctrl.NewManager(opts.Rest, manager.Options{
Logger: logger,
HealthProbeBindAddress: opts.HealthProbeAddr,
Metrics: server.Options{
BindAddress: opts.MetricsAddr,
},
})
if err != nil {
return nil, err
}

err = apiv1.SchemeBuilder.AddToScheme(mgr.GetScheme())
if err != nil {
return nil, err
}

err = mgr.GetFieldIndexer().IndexField(context.Background(), &apiv1.ResourceSlice{}, IdxSlicesByCompositionGeneration, func(o client.Object) []string {
slice := o.(*apiv1.ResourceSlice)
owner := metav1.GetControllerOf(slice)
if owner == nil || owner.Kind != "Composition" {
return nil
}
return []string{NewSlicesByCompositionGenerationKey(owner.Name, slice.Spec.CompositionGeneration)}
})
if err != nil {
return nil, err
}

return mgr, nil
}

func NewLogConstructor(mgr ctrl.Manager, controllerName string) func(*reconcile.Request) logr.Logger {
return func(req *reconcile.Request) logr.Logger {
l := mgr.GetLogger().WithValues("controller", controllerName)
if req != nil {
l.WithValues("requestName", req.Name, "requestNamespace", req.Namespace)
}
return l
}
}

// NewSlicesByCompositionGenerationKey documents the key structure used by IdxSlicesByCompositionGeneration.
func NewSlicesByCompositionGenerationKey(compName string, compGeneration int64) string {
// keys will not collide because k8s doesn't allow slashes in names
return fmt.Sprintf("%s/%d", compName, compGeneration)
}
6 changes: 3 additions & 3 deletions internal/reconstitution/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func (c *cache) Get(ctx context.Context, ref *ResourceRef, gen int64) (*Resource
func (c *cache) HasSynthesis(ctx context.Context, comp types.NamespacedName, synthesis *apiv1.Synthesis) bool {
key := synthesisKey{
Composition: comp,
Generation: synthesis.ObservedGeneration,
Generation: synthesis.ObservedCompositionGeneration,
}

c.mut.Lock()
Expand All @@ -74,7 +74,7 @@ func (c *cache) Fill(ctx context.Context, comp types.NamespacedName, synthesis *
c.mut.Lock()
defer c.mut.Unlock()

synKey := synthesisKey{Composition: comp, Generation: synthesis.ObservedGeneration}
synKey := synthesisKey{Composition: comp, Generation: synthesis.ObservedCompositionGeneration}
c.resources[synKey] = resources
c.synthesesByComposition[comp] = append(c.synthesesByComposition[comp], synKey.Generation)

Expand Down Expand Up @@ -165,7 +165,7 @@ func (c *cache) Purge(ctx context.Context, compNSN types.NamespacedName, comp *a
remainingSyns := []int64{}
for _, syn := range c.synthesesByComposition[compNSN] {
// Don't touch any syntheses still referenced by the composition
if comp != nil && ((comp.Status.CurrentState != nil && comp.Status.CurrentState.ObservedGeneration == syn) || (comp.Status.PreviousState != nil && comp.Status.PreviousState.ObservedGeneration == syn)) {
if comp != nil && ((comp.Status.CurrentState != nil && comp.Status.CurrentState.ObservedCompositionGeneration == syn) || (comp.Status.PreviousState != nil && comp.Status.PreviousState.ObservedCompositionGeneration == syn)) {
remainingSyns = append(remainingSyns, syn)
continue // still referenced by the Generation
}
Expand Down
22 changes: 11 additions & 11 deletions internal/reconstitution/cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,12 @@ func TestCacheBasics(t *testing.T) {
assert.True(t, c.HasSynthesis(ctx, comp, synth))

// negative
assert.False(t, c.HasSynthesis(ctx, comp, &apiv1.Synthesis{ObservedGeneration: 123}))
assert.False(t, c.HasSynthesis(ctx, comp, &apiv1.Synthesis{ObservedCompositionGeneration: 123}))
})

t.Run("get", func(t *testing.T) {
// positive
resource, exists := c.Get(ctx, &expectedReqs[0].ResourceRef, synth.ObservedGeneration)
resource, exists := c.Get(ctx, &expectedReqs[0].ResourceRef, synth.ObservedCompositionGeneration)
require.True(t, exists)
assert.NotEmpty(t, resource.Manifest)
assert.Equal(t, "ConfigMap", resource.Object().GetKind())
Expand All @@ -56,7 +56,7 @@ func TestCacheBasics(t *testing.T) {
c.Purge(ctx, comp, nil)

// confirm
_, exists := c.Get(ctx, &expectedReqs[0].ResourceRef, synth.ObservedGeneration)
_, exists := c.Get(ctx, &expectedReqs[0].ResourceRef, synth.ObservedCompositionGeneration)
assert.False(t, exists)

assert.Len(t, c.resources, 0)
Expand Down Expand Up @@ -103,7 +103,7 @@ func TestCacheSecret(t *testing.T) {
assert.Equal(t, expectedReqs, reqs)

// Confirm cache was filled correctly
resource, exists := c.Get(ctx, &expectedReqs[0].ResourceRef, synth.ObservedGeneration)
resource, exists := c.Get(ctx, &expectedReqs[0].ResourceRef, synth.ObservedCompositionGeneration)
require.True(t, exists)
assert.NotEmpty(t, resource.Manifest)
assert.Equal(t, "ConfigMap", resource.Object().GetKind())
Expand Down Expand Up @@ -150,7 +150,7 @@ func TestCacheReconcileInterval(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, expectedReqs, reqs)

resource, exists := c.Get(ctx, &expectedReqs[0].ResourceRef, synth.ObservedGeneration)
resource, exists := c.Get(ctx, &expectedReqs[0].ResourceRef, synth.ObservedCompositionGeneration)
require.True(t, exists)
assert.Equal(t, interval, resource.ReconcileInterval)
}
Expand All @@ -165,12 +165,12 @@ func TestCachePartialPurge(t *testing.T) {
compNSN, synth, resources, _ := newCacheTestFixtures(3, 4)
_, err := c.Fill(ctx, compNSN, synth, resources)
require.NoError(t, err)
originalGen := synth.ObservedGeneration
originalGen := synth.ObservedCompositionGeneration

// Add another resource to the composition but synthesized from a newer generation
_, _, resources, expectedReqs := newCacheTestFixtures(1, 1)
synth.ObservedGeneration++
resources[0].Spec.CompositionGeneration = synth.ObservedGeneration
synth.ObservedCompositionGeneration++
resources[0].Spec.CompositionGeneration = synth.ObservedCompositionGeneration
expectedReqs[0].Composition = compNSN
_, err = c.Fill(ctx, compNSN, synth, resources)
require.NoError(t, err)
Expand All @@ -191,7 +191,7 @@ func TestCachePartialPurge(t *testing.T) {
c.Purge(ctx, compNSN, comp)

// The newer resource should still exist
_, exists := c.Get(ctx, &expectedReqs[0].ResourceRef, synth.ObservedGeneration)
_, exists := c.Get(ctx, &expectedReqs[0].ResourceRef, synth.ObservedCompositionGeneration)
assert.True(t, exists)

// The older resource is not referenced by the composition and should have been removed
Expand All @@ -208,7 +208,7 @@ func TestCachePartialPurge(t *testing.T) {

func newCacheTestFixtures(sliceCount, resPerSliceCount int) (types.NamespacedName, *apiv1.Synthesis, []apiv1.ResourceSlice, []*Request) {
comp := types.NamespacedName{Namespace: string(uuid.NewUUID()), Name: string(uuid.NewUUID())}
synth := &apiv1.Synthesis{ObservedGeneration: 3} // just not 0
synth := &apiv1.Synthesis{ObservedCompositionGeneration: 3} // just not 0

resources := make([]apiv1.ResourceSlice, sliceCount)
requests := []*Request{}
Expand All @@ -217,7 +217,7 @@ func newCacheTestFixtures(sliceCount, resPerSliceCount int) (types.NamespacedNam
slice.Name = string(uuid.NewUUID())
slice.Namespace = "slice-ns"
slice.Spec.Resources = make([]apiv1.Manifest, resPerSliceCount)
slice.Spec.CompositionGeneration = synth.ObservedGeneration
slice.Spec.CompositionGeneration = synth.ObservedCompositionGeneration

for j := 0; j < resPerSliceCount; j++ {
resource := &corev1.ConfigMap{}
Expand Down
Loading

0 comments on commit d106616

Please sign in to comment.