m/n/k/reconciler: implement updates
The reconciler now checks if already present object are equal to the
expected object, and else updates them. If the update fails due to
immutable fields, the object is instead deleted and recreated.
Also, the reconciler now logs create/update/delete operations.
For the CSI driver, the StorageCapacity and RequiresRepublish were added
and set to their default value. If we don't do this, the API server will
add these defaults, and then our update comparison fails. There is also
a new test which ensures that expected objects have all defaults already
applied. This test will fail if a Kubernetes upgrade adds new fields
with default values.
Closes #288.
Change-Id: Ibfb37d07b4613ae1a883ad47715feeda87135820
Reviewed-on: https://review.monogon.dev/c/monogon/+/2893
Tested-by: Jenkins CI
Reviewed-by: Lorenz Brun <lorenz@monogon.tech>
diff --git a/metropolis/node/kubernetes/reconciler/BUILD.bazel b/metropolis/node/kubernetes/reconciler/BUILD.bazel
index caa239a..f7bb47f 100644
--- a/metropolis/node/kubernetes/reconciler/BUILD.bazel
+++ b/metropolis/node/kubernetes/reconciler/BUILD.bazel
@@ -30,6 +30,9 @@
"@io_k8s_api//node/v1:node",
"@io_k8s_api//rbac/v1:rbac",
"@io_k8s_api//storage/v1:storage",
+ "@io_k8s_apimachinery//pkg/api/equality",
+ "@io_k8s_apimachinery//pkg/api/errors",
+ "@io_k8s_apimachinery//pkg/api/validation",
"@io_k8s_apimachinery//pkg/apis/meta/v1:meta",
"@io_k8s_client_go//kubernetes",
"@org_golang_google_protobuf//proto",
@@ -53,8 +56,18 @@
"//version",
"//version/spec",
"@io_etcd_go_etcd_tests_v3//integration",
+ "@io_k8s_apimachinery//pkg/api/equality",
+ "@io_k8s_apimachinery//pkg/api/errors",
+ "@io_k8s_apimachinery//pkg/api/validation",
"@io_k8s_apimachinery//pkg/apis/meta/v1:meta",
+ "@io_k8s_apimachinery//pkg/runtime",
+ "@io_k8s_apimachinery//pkg/runtime/schema",
+ "@io_k8s_apimachinery//pkg/util/validation/field",
"@io_k8s_client_go//kubernetes/fake",
+ "@io_k8s_kubernetes//pkg/apis/node/install",
+ "@io_k8s_kubernetes//pkg/apis/policy/install",
+ "@io_k8s_kubernetes//pkg/apis/rbac/install",
+ "@io_k8s_kubernetes//pkg/apis/storage/install",
"@org_golang_google_protobuf//proto",
],
)
diff --git a/metropolis/node/kubernetes/reconciler/reconciler.go b/metropolis/node/kubernetes/reconciler/reconciler.go
index 4ea2d84..3bfaa4d 100644
--- a/metropolis/node/kubernetes/reconciler/reconciler.go
+++ b/metropolis/node/kubernetes/reconciler/reconciler.go
@@ -28,10 +28,17 @@
import (
"context"
+ "errors"
"fmt"
+ "strings"
+ apiequality "k8s.io/apimachinery/pkg/api/equality"
+ apierrors "k8s.io/apimachinery/pkg/api/errors"
+ apivalidation "k8s.io/apimachinery/pkg/api/validation"
meta "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
+
+ "source.monogon.dev/metropolis/pkg/supervisor"
)
// True is a sad workaround for all the pointer booleans in K8s specs
@@ -100,8 +107,11 @@
// Create creates an object on the target. The el argument is
// an object returned by the Expected() call.
Create(ctx context.Context, el meta.Object) error
+ // Update updates an existing object, by name, on the target.
+ // The el argument is an object returned by the Expected() call.
+ Update(ctx context.Context, el meta.Object) error
// Delete deletes an object, by name, from the target.
- Delete(ctx context.Context, name string) error
+ Delete(ctx context.Context, name string, opts meta.DeleteOptions) error
// Expected returns a list of all objects expected to be present on the
// target. Objects are identified by their name, as returned by GetName.
Expected() []meta.Object
@@ -120,7 +130,7 @@
func reconcileAll(ctx context.Context, clientSet kubernetes.Interface) error {
resources := allResources(clientSet)
for name, resource := range resources {
- err := reconcile(ctx, resource)
+ err := reconcile(ctx, resource, name)
if err != nil {
return fmt.Errorf("resource %s: %w", name, err)
}
@@ -128,7 +138,8 @@
return nil
}
-func reconcile(ctx context.Context, r resource) error {
+func reconcile(ctx context.Context, r resource, rname string) error {
+ log := supervisor.Logger(ctx)
present, err := r.List(ctx)
if err != nil {
return err
@@ -143,9 +154,49 @@
expectedMap[el.GetName()] = el
}
for name, expectedEl := range expectedMap {
- if _, ok := presentMap[name]; ok {
- // TODO(#288): update the object if it is different than expected.
+ if presentEl, ok := presentMap[name]; ok {
+ // The object already exists. Update it if it is different than expected.
+
+ // The server rejects updates which don't have an up to date ResourceVersion.
+ expectedEl.SetResourceVersion(presentEl.GetResourceVersion())
+
+ // Clear out fields set by the server, such that comparison succeeds if
+ // there are no other changes.
+ presentEl.SetUID("")
+ presentEl.SetGeneration(0)
+ presentEl.SetCreationTimestamp(meta.Time{})
+ presentEl.SetManagedFields(nil)
+
+ if !apiequality.Semantic.DeepEqual(presentEl, expectedEl) {
+ log.Infof("Updating %s object %q", rname, name)
+ if err := r.Update(ctx, expectedEl); err != nil {
+ if !isImmutableError(err) {
+ return err
+ }
+ log.Infof("Failed to update object due to immutable fields; deleting and recreating: %v", err)
+
+ // Only delete if the ResourceVersion has not changed. If it has
+ // changed, that means another reconciler was faster than us and
+ // has already recreated the object.
+ resourceVersion := presentEl.GetResourceVersion()
+ deleteOpts := meta.DeleteOptions{
+ Preconditions: &meta.Preconditions{
+ ResourceVersion: &resourceVersion,
+ },
+ }
+ // ResourceVersion must be cleared when creating.
+ expectedEl.SetResourceVersion("")
+
+ if err := r.Delete(ctx, name, deleteOpts); err != nil {
+ return err
+ }
+ if err := r.Create(ctx, expectedEl); err != nil {
+ return err
+ }
+ }
+ }
} else {
+ log.Infof("Creating %s object %q", rname, name)
if err := r.Create(ctx, expectedEl); err != nil {
return err
}
@@ -153,10 +204,33 @@
}
for name := range presentMap {
if _, ok := expectedMap[name]; !ok {
- if err := r.Delete(ctx, name); err != nil {
+ log.Infof("Deleting %s object %q", rname, name)
+ if err := r.Delete(ctx, name, meta.DeleteOptions{}); err != nil {
return err
}
}
}
return nil
}
+
+// isImmutableError returns true if err indicates that an update failed because
+// of an attempt to update one or more immutable fields.
+func isImmutableError(err error) bool {
+ if !apierrors.IsInvalid(err) {
+ return false
+ }
+ var status apierrors.APIStatus
+ if !errors.As(err, &status) {
+ return false
+ }
+ details := status.Status().Details
+ if details == nil || len(details.Causes) == 0 {
+ return false
+ }
+ for _, cause := range details.Causes {
+ if !strings.Contains(cause.Message, apivalidation.FieldImmutableErrorMsg) {
+ return false
+ }
+ }
+ return true
+}
diff --git a/metropolis/node/kubernetes/reconciler/reconciler_test.go b/metropolis/node/kubernetes/reconciler/reconciler_test.go
index b72ccb9..b791dbe 100644
--- a/metropolis/node/kubernetes/reconciler/reconciler_test.go
+++ b/metropolis/node/kubernetes/reconciler/reconciler_test.go
@@ -21,9 +21,36 @@
"fmt"
"testing"
+ apiequality "k8s.io/apimachinery/pkg/api/equality"
+ apierrors "k8s.io/apimachinery/pkg/api/errors"
+ apivalidation "k8s.io/apimachinery/pkg/api/validation"
meta "k8s.io/apimachinery/pkg/apis/meta/v1"
+ "k8s.io/apimachinery/pkg/runtime"
+ "k8s.io/apimachinery/pkg/runtime/schema"
+ "k8s.io/apimachinery/pkg/util/validation/field"
+ installnode "k8s.io/kubernetes/pkg/apis/node/install"
+ installpolicy "k8s.io/kubernetes/pkg/apis/policy/install"
+ installrbac "k8s.io/kubernetes/pkg/apis/rbac/install"
+ installstorage "k8s.io/kubernetes/pkg/apis/storage/install"
+
+ "source.monogon.dev/metropolis/pkg/supervisor"
)
+// TestExpectedUniqueNames ensures that all the Expected objects of any
+// given resource type have a unique name.
+func TestExpectedUniqueNames(t *testing.T) {
+ for reconciler, r := range allResources(nil) {
+ names := make(map[string]bool)
+ for _, v := range r.Expected() {
+ if names[v.GetName()] {
+ t.Errorf("reconciler %q: duplicate name %q", reconciler, v.GetName())
+ continue
+ }
+ names[v.GetName()] = true
+ }
+ }
+}
+
// TestExpectedLabeledCorrectly ensures that all the Expected objects of all
// resource types have a Kubernetes metadata label that signifies it's a
// builtin object, to be retrieved afterwards. This contract must be met in
@@ -32,26 +59,54 @@
// selector corresponding to this label.
func TestExpectedLabeledCorrectly(t *testing.T) {
for reconciler, r := range allResources(nil) {
- for outer, v := range r.Expected() {
+ for _, v := range r.Expected() {
if data := v.GetLabels()[BuiltinLabelKey]; data != BuiltinLabelValue {
- t.Errorf("reconciler %q, object %q: %q=%q, wanted =%q", reconciler, outer, BuiltinLabelKey, data, BuiltinLabelValue)
+ t.Errorf("reconciler %q, object %q: %q=%q, wanted =%q", reconciler, v.GetName(), BuiltinLabelKey, data, BuiltinLabelValue)
continue
}
}
}
}
+// TestExpectedDefaulted ensures that all the Expected objects of all
+// resource types have defaults already applied. If this were not the case,
+// the reconciler would think that the object has changed and try to update it
+// in each iteration. If this test fails, the most likely fix is to add the
+// missing default values to the expected objects.
+func TestExpectedDefaulted(t *testing.T) {
+ scheme := runtime.NewScheme()
+ installnode.Install(scheme)
+ installpolicy.Install(scheme)
+ installrbac.Install(scheme)
+ installstorage.Install(scheme)
+
+ for reconciler, r := range allResources(nil) {
+ for _, v := range r.Expected() {
+ v_defaulted := v.(runtime.Object).DeepCopyObject()
+ if _, ok := scheme.IsUnversioned(v_defaulted); !ok {
+ t.Errorf("reconciler %q: type not installed in scheme", reconciler)
+ }
+ scheme.Default(v_defaulted)
+ if !apiequality.Semantic.DeepEqual(v, v_defaulted) {
+ t.Errorf("reconciler %q, object %q changed after defaulting\ngot: %+v\nwanted: %+v", reconciler, v.GetName(), v, v_defaulted)
+ }
+ }
+ }
+}
+
// testObject is the object type managed by testResource.
type testObject struct {
meta.ObjectMeta
+ Val int
}
-func makeTestObject(name string) *testObject {
+func makeTestObject(name string, val int) *testObject {
return &testObject{
ObjectMeta: meta.ObjectMeta{
Name: name,
Labels: builtinLabels(nil),
},
+ Val: val,
}
}
@@ -79,7 +134,12 @@
return nil
}
-func (r *testResource) Delete(ctx context.Context, name string) error {
+func (r *testResource) Update(ctx context.Context, el meta.Object) error {
+ r.current[el.GetName()] = el.(*testObject)
+ return nil
+}
+
+func (r *testResource) Delete(ctx context.Context, name string, opts meta.DeleteOptions) error {
delete(r.current, name)
return nil
}
@@ -127,28 +187,71 @@
// TestBasicReconciliation ensures that the reconcile function does manipulate
// a target state based on a set of expected resources.
func TestBasicReconciliation(t *testing.T) {
- ctx := context.Background()
- r := newTestResource(makeTestObject("foo"), makeTestObject("bar"), makeTestObject("baz"))
+ // This needs to run in a TestHarness to make logging work.
+ supervisor.TestHarness(t, func(ctx context.Context) error {
+ r := newTestResource(makeTestObject("foo", 0), makeTestObject("bar", 0), makeTestObject("baz", 0))
+ rname := "testresource"
- // nothing should have happened yet (testing the test)
- if diff := r.currentDiff(); diff != "" {
- t.Fatalf("wrong state after creation: %s", diff)
- }
+ // nothing should have happened yet (testing the test)
+ if diff := r.currentDiff(); diff != "" {
+ return fmt.Errorf("wrong state after creation: %s", diff)
+ }
- if err := reconcile(ctx, r); err != nil {
- t.Fatalf("reconcile: %v", err)
- }
- // everything requested should have been created
- if diff := r.currentDiff(makeTestObject("foo"), makeTestObject("bar"), makeTestObject("baz")); diff != "" {
- t.Fatalf("wrong state after reconciliation: %s", diff)
- }
+ if err := reconcile(ctx, r, rname); err != nil {
+ return fmt.Errorf("reconcile: %v", err)
+ }
+ // everything requested should have been created
+ if diff := r.currentDiff(makeTestObject("foo", 0), makeTestObject("bar", 0), makeTestObject("baz", 0)); diff != "" {
+ return fmt.Errorf("wrong state after reconciliation: %s", diff)
+ }
- delete(r.expected, "foo")
- if err := reconcile(ctx, r); err != nil {
- t.Fatalf("reconcile: %v", err)
+ delete(r.expected, "foo")
+ if err := reconcile(ctx, r, rname); err != nil {
+ return fmt.Errorf("reconcile: %v", err)
+ }
+ // foo should now be missing
+ if diff := r.currentDiff(makeTestObject("bar", 0), makeTestObject("baz", 0)); diff != "" {
+ return fmt.Errorf("wrong state after deleting foo: %s", diff)
+ }
+
+ r.expected["bar"] = makeTestObject("bar", 1)
+ if err := reconcile(ctx, r, rname); err != nil {
+ return fmt.Errorf("reconcile: %v", err)
+ }
+ // bar should be updated
+ if diff := r.currentDiff(makeTestObject("bar", 1), makeTestObject("baz", 0)); diff != "" {
+ return fmt.Errorf("wrong state after deleting foo: %s", diff)
+ }
+
+ return nil
+ })
+}
+
+func TestIsImmutableError(t *testing.T) {
+ gk := schema.GroupKind{Group: "someGroup", Kind: "someKind"}
+ cases := []struct {
+ err error
+ isImmutable bool
+ }{
+ {fmt.Errorf("something wrong"), false},
+ {apierrors.NewApplyConflict(nil, "conflict"), false},
+ {apierrors.NewInvalid(gk, "name", field.ErrorList{}), false},
+ {apierrors.NewInvalid(gk, "name", field.ErrorList{
+ field.Invalid(field.NewPath("field1"), true, apivalidation.FieldImmutableErrorMsg),
+ field.Invalid(field.NewPath("field2"), true, "some other error"),
+ }), false},
+ {apierrors.NewInvalid(gk, "name", field.ErrorList{
+ field.Invalid(field.NewPath("field1"), true, apivalidation.FieldImmutableErrorMsg),
+ }), true},
+ {apierrors.NewInvalid(gk, "name", field.ErrorList{
+ field.Invalid(field.NewPath("field1"), true, apivalidation.FieldImmutableErrorMsg),
+ field.Invalid(field.NewPath("field2"), true, apivalidation.FieldImmutableErrorMsg),
+ }), true},
}
- // foo should now be missing
- if diff := r.currentDiff(makeTestObject("bar"), makeTestObject("baz")); diff != "" {
- t.Fatalf("wrong state after deleting foo: %s", diff)
+ for _, c := range cases {
+ actual := isImmutableError(c.err)
+ if actual != c.isImmutable {
+ t.Errorf("Expected %v, got %v for error: %v", c.isImmutable, actual, c.err)
+ }
}
}
diff --git a/metropolis/node/kubernetes/reconciler/resources_csi.go b/metropolis/node/kubernetes/reconciler/resources_csi.go
index cec00fd..06eb3ac 100644
--- a/metropolis/node/kubernetes/reconciler/resources_csi.go
+++ b/metropolis/node/kubernetes/reconciler/resources_csi.go
@@ -52,8 +52,13 @@
return err
}
-func (r resourceCSIDrivers) Delete(ctx context.Context, name string) error {
- return r.StorageV1().CSIDrivers().Delete(ctx, name, meta.DeleteOptions{})
+func (r resourceCSIDrivers) Update(ctx context.Context, el meta.Object) error {
+ _, err := r.StorageV1().CSIDrivers().Update(ctx, el.(*storage.CSIDriver), meta.UpdateOptions{})
+ return err
+}
+
+func (r resourceCSIDrivers) Delete(ctx context.Context, name string, opts meta.DeleteOptions) error {
+ return r.StorageV1().CSIDrivers().Delete(ctx, name, opts)
}
func (r resourceCSIDrivers) Expected() []meta.Object {
@@ -68,8 +73,10 @@
AttachRequired: False(),
PodInfoOnMount: False(),
VolumeLifecycleModes: []storage.VolumeLifecycleMode{storage.VolumeLifecyclePersistent},
- // TODO(#288): Make sure this gets applied to existing clusters
- FSGroupPolicy: &fsGroupPolicy,
+ StorageCapacity: False(),
+ FSGroupPolicy: &fsGroupPolicy,
+ RequiresRepublish: False(),
+ SELinuxMount: False(),
},
},
}
diff --git a/metropolis/node/kubernetes/reconciler/resources_rbac.go b/metropolis/node/kubernetes/reconciler/resources_rbac.go
index 702ee6b..5ae5246 100644
--- a/metropolis/node/kubernetes/reconciler/resources_rbac.go
+++ b/metropolis/node/kubernetes/reconciler/resources_rbac.go
@@ -56,8 +56,13 @@
return err
}
-func (r resourceClusterRoles) Delete(ctx context.Context, name string) error {
- return r.RbacV1().ClusterRoles().Delete(ctx, name, meta.DeleteOptions{})
+func (r resourceClusterRoles) Update(ctx context.Context, el meta.Object) error {
+ _, err := r.RbacV1().ClusterRoles().Update(ctx, el.(*rbac.ClusterRole), meta.UpdateOptions{})
+ return err
+}
+
+func (r resourceClusterRoles) Delete(ctx context.Context, name string, opts meta.DeleteOptions) error {
+ return r.RbacV1().ClusterRoles().Delete(ctx, name, opts)
}
func (r resourceClusterRoles) Expected() []meta.Object {
@@ -84,7 +89,7 @@
Name: clusterRoleCSIProvisioner,
Labels: builtinLabels(nil),
Annotations: map[string]string{
- "kubernetes.io/description": "This role grants access to PersistentVolumes, PersistentVolumeClaims and StorageClassses, as used the the CSI provisioner running on nodes.",
+ "kubernetes.io/description": "This role grants access to PersistentVolumes, PersistentVolumeClaims and StorageClassses, as used by the CSI provisioner running on nodes.",
},
},
Rules: []rbac.PolicyRule{
@@ -150,8 +155,13 @@
return err
}
-func (r resourceClusterRoleBindings) Delete(ctx context.Context, name string) error {
- return r.RbacV1().ClusterRoleBindings().Delete(ctx, name, meta.DeleteOptions{})
+func (r resourceClusterRoleBindings) Update(ctx context.Context, el meta.Object) error {
+ _, err := r.RbacV1().ClusterRoleBindings().Update(ctx, el.(*rbac.ClusterRoleBinding), meta.UpdateOptions{})
+ return err
+}
+
+func (r resourceClusterRoleBindings) Delete(ctx context.Context, name string, opts meta.DeleteOptions) error {
+ return r.RbacV1().ClusterRoleBindings().Delete(ctx, name, opts)
}
func (r resourceClusterRoleBindings) Expected() []meta.Object {
diff --git a/metropolis/node/kubernetes/reconciler/resources_runtimeclass.go b/metropolis/node/kubernetes/reconciler/resources_runtimeclass.go
index b41c2c9..070cb96 100644
--- a/metropolis/node/kubernetes/reconciler/resources_runtimeclass.go
+++ b/metropolis/node/kubernetes/reconciler/resources_runtimeclass.go
@@ -45,8 +45,13 @@
return err
}
-func (r resourceRuntimeClasses) Delete(ctx context.Context, name string) error {
- return r.NodeV1().RuntimeClasses().Delete(ctx, name, meta.DeleteOptions{})
+func (r resourceRuntimeClasses) Update(ctx context.Context, el meta.Object) error {
+ _, err := r.NodeV1().RuntimeClasses().Update(ctx, el.(*node.RuntimeClass), meta.UpdateOptions{})
+ return err
+}
+
+func (r resourceRuntimeClasses) Delete(ctx context.Context, name string, opts meta.DeleteOptions) error {
+ return r.NodeV1().RuntimeClasses().Delete(ctx, name, opts)
}
func (r resourceRuntimeClasses) Expected() []meta.Object {
diff --git a/metropolis/node/kubernetes/reconciler/resources_storageclass.go b/metropolis/node/kubernetes/reconciler/resources_storageclass.go
index d8191ce..b242bbf 100644
--- a/metropolis/node/kubernetes/reconciler/resources_storageclass.go
+++ b/metropolis/node/kubernetes/reconciler/resources_storageclass.go
@@ -49,8 +49,13 @@
return err
}
-func (r resourceStorageClasses) Delete(ctx context.Context, name string) error {
- return r.StorageV1().StorageClasses().Delete(ctx, name, meta.DeleteOptions{})
+func (r resourceStorageClasses) Update(ctx context.Context, el meta.Object) error {
+ _, err := r.StorageV1().StorageClasses().Update(ctx, el.(*storage.StorageClass), meta.UpdateOptions{})
+ return err
+}
+
+func (r resourceStorageClasses) Delete(ctx context.Context, name string, opts meta.DeleteOptions) error {
+ return r.StorageV1().StorageClasses().Delete(ctx, name, opts)
}
func (r resourceStorageClasses) Expected() []meta.Object {