Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move controller revision to annotations #1234

Merged
merged 12 commits into from
Jul 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 43 additions & 12 deletions operators/Gopkg.lock

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

4 changes: 0 additions & 4 deletions operators/config/crds/apm_v1alpha1_apmserver.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -195,10 +195,6 @@ spec:
type: object
status:
properties:
controllerVersion:
description: ControllerVersion is the version of the controller that
last updated the ApmServer instance
type: string
health:
type: string
secretTokenSecret:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -244,10 +244,6 @@ spec:
properties:
clusterUUID:
type: string
controllerVersion:
description: ControllerVersion is the version of the controller that
last updated the Elasticsearch cluster
type: string
health:
type: string
masterNode:
Expand Down
4 changes: 0 additions & 4 deletions operators/config/crds/kibana_v1alpha1_kibana.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -189,10 +189,6 @@ spec:
properties:
associationStatus:
type: string
controllerVersion:
description: ControllerVersion is the version of the controller that
last updated the Kibana instance
type: string
health:
type: string
type: object
Expand Down
2 changes: 0 additions & 2 deletions operators/pkg/apis/apm/v1alpha1/apmserver_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,6 @@ type ApmServerStatus struct {
SecretTokenSecretName string `json:"secretTokenSecret,omitempty"`
// Association is the status of any auto-linking to Elasticsearch clusters.
Association commonv1alpha1.AssociationStatus
// ControllerVersion is the version of the controller that last updated the ApmServer instance
ControllerVersion string `json:"controllerVersion,omitempty"`
}

// IsDegraded returns true if the current status is worse than the previous.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,6 @@ type ElasticsearchStatus struct {
MasterNode string `json:"masterNode,omitempty"`
ExternalService string `json:"service,omitempty"`
ZenDiscovery ZenDiscoveryStatus `json:"zenDiscovery,omitempty"`
// ControllerVersion is the version of the controller that last updated the Elasticsearch cluster
ControllerVersion string `json:"controllerVersion,omitempty"`
}

type ZenDiscoveryStatus struct {
Expand Down
2 changes: 0 additions & 2 deletions operators/pkg/apis/kibana/v1alpha1/kibana_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,6 @@ type KibanaStatus struct {
commonv1alpha1.ReconcilerStatus
Health KibanaHealth `json:"health,omitempty"`
AssociationStatus commonv1alpha1.AssociationStatus `json:"associationStatus,omitempty"`
// ControllerVersion is the version of the controller that last updated the Kibana instance
ControllerVersion string `json:"controllerVersion,omitempty"`
}

// IsDegraded returns true if the current status is worse than the previous.
Expand Down
8 changes: 6 additions & 2 deletions operators/pkg/controller/apmserver/apmserver_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"github.com/elastic/cloud-on-k8s/operators/pkg/controller/apmserver/labels"
apmname "github.com/elastic/cloud-on-k8s/operators/pkg/controller/apmserver/name"
"github.com/elastic/cloud-on-k8s/operators/pkg/controller/common"
"github.com/elastic/cloud-on-k8s/operators/pkg/controller/common/annotation"
"github.com/elastic/cloud-on-k8s/operators/pkg/controller/common/certificates"
"github.com/elastic/cloud-on-k8s/operators/pkg/controller/common/certificates/http"
"github.com/elastic/cloud-on-k8s/operators/pkg/controller/common/defaults"
Expand Down Expand Up @@ -195,9 +196,12 @@ func (r *ReconcileApmServer) Reconcile(request reconcile.Request) (reconcile.Res
return reconcile.Result{}, nil
}

state := NewState(request, as)
state.UpdateApmServerControllerVersion(r.OperatorInfo.BuildInfo.Version)
err = annotation.UpdateControllerVersion(r.Client, as, r.OperatorInfo.BuildInfo.Version)
if err != nil {
return reconcile.Result{}, err
}

state := NewState(request, as)
svc, err := common.ReconcileService(r.Client, r.scheme, NewService(*as), as)
if err != nil {
return reconcile.Result{}, err
Expand Down
8 changes: 0 additions & 8 deletions operators/pkg/controller/apmserver/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,3 @@ func (s State) UpdateApmServerState(deployment v1.Deployment, apmServerSecret co
func (s State) UpdateApmServerExternalService(svc corev1.Service) {
s.ApmServer.Status.ExternalService = svc.Name
}

func (s *State) UpdateApmServerControllerVersion(version string) {
s.ApmServer.Status.ControllerVersion = version
}

func (s *State) GetApmServerControllerVersion() string {
return s.ApmServer.Status.ControllerVersion
}
52 changes: 52 additions & 0 deletions operators/pkg/controller/common/annotation/controller_version.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
// or more contributor license agreements. Licensed under the Elastic License;
// you may not use this file except in compliance with the Elastic License.

package annotation

import (
"github.com/elastic/cloud-on-k8s/operators/pkg/utils/k8s"
"k8s.io/apimachinery/pkg/api/meta"
"k8s.io/apimachinery/pkg/runtime"
)

// ControllerVersionAnnotation is the annotation name that indicates the last controller version to update a resource
const ControllerVersionAnnotation = "common.k8s.elastic.co/controller-version"

// UpdateControllerVersion updates the controller version annotation to the current version if necessary
func UpdateControllerVersion(client k8s.Client, obj runtime.Object, version string) error {
accessor := meta.NewAccessor()
namespace, err := accessor.Namespace(obj)
if err != nil {
log.Error(err, "error getting namespace", "kind", obj.GetObjectKind().GroupVersionKind().Kind)
return err
}
name, err := accessor.Name(obj)
if err != nil {
log.Error(err, "error getting name", "namespace", namespace, "kind", obj.GetObjectKind().GroupVersionKind().Kind)
return err
}
annotations, err := accessor.Annotations(obj)
if err != nil {
log.Error(err, "error getting annotations", "namespace", namespace, "name", name, "kind", obj.GetObjectKind().GroupVersionKind().Kind)
return err
}
if annotations == nil {
annotations = make(map[string]string)
}

// do not send unnecessary update if the value would not change
if annotations[ControllerVersionAnnotation] == version {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not also return if there is any version set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we would want to use this func because the caller decided that we do want to proceed with an update -- for instance it might be okay with going from 0.9.0 to 0.9.1. We might need additional funcs before calling this

log.V(1).Info("Skipping controller version annotation update, version already matches", "namespace", namespace, "name", name, "kind", obj.GetObjectKind())
return nil
}

annotations[ControllerVersionAnnotation] = version
err = accessor.SetAnnotations(obj, annotations)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not reuse metaObject.SetAnnotations here, instead of creating a new accessor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so because accessor's SetAnnotations(obj, annotations) returns a runtime.Object, which is what we need for the Update call, while metaObject.SetAnnotations() modifies the metav1.Object receiver.

But your question did make me realize that I didn't look closely enough at the funcs available to the metav1 Accessor so there's some small bit of refactoring I can do here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

metaObject.SetAnnotations() modifies the metav1.Object receiver

I think it also modifies the original object you passed to it, which you can the use for the update call.
But I'm also fine with the way you used it here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that was not my understanding just from scanning through the code but it's deeeeefinitely possible I am misunderstanding

if err != nil {
log.Error(err, "error updating controller version annotation", "namespace", namespace, "name", name, "kind", obj.GetObjectKind())
return err
}
log.V(1).Info("updating controller version annotation", "namespace", namespace, "name", name, "kind", obj.GetObjectKind())
return client.Update(obj)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
// or more contributor license agreements. Licensed under the Elastic License;
// you may not use this file except in compliance with the Elastic License.

package annotation

import (
"testing"

kibanav1alpha1 "github.com/elastic/cloud-on-k8s/operators/pkg/apis/kibana/v1alpha1"
"github.com/elastic/cloud-on-k8s/operators/pkg/utils/k8s"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client/fake"

apmtype "github.com/elastic/cloud-on-k8s/operators/pkg/apis/apm/v1alpha1"
assoctype "github.com/elastic/cloud-on-k8s/operators/pkg/apis/associations/v1alpha1"
estype "github.com/elastic/cloud-on-k8s/operators/pkg/apis/elasticsearch/v1alpha1"
"k8s.io/client-go/kubernetes/scheme"
)

// Test UpdateControllerVersion updates annotation if there is an older version
func TestAnnotationUpdated(t *testing.T) {
kibana := kibanav1alpha1.Kibana{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns",
Name: "kibana",
Annotations: map[string]string{
ControllerVersionAnnotation: "oldversion",
},
},
}
obj := kibana.DeepCopy()
sc := setupScheme(t)
client := k8s.WrapClient(fake.NewFakeClientWithScheme(sc, obj))
err := UpdateControllerVersion(client, obj, "newversion")
require.NoError(t, err)
require.Equal(t, obj.GetAnnotations()[ControllerVersionAnnotation], "newversion")
}

// Test UpdateControllerVersion creates an annotation even if there are no current annotations
func TestAnnotationCreated(t *testing.T) {
kibana := kibanav1alpha1.Kibana{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns",
Name: "kibana",
},
}

obj := kibana.DeepCopy()
sc := setupScheme(t)
client := k8s.WrapClient(fake.NewFakeClientWithScheme(sc, obj))
err := UpdateControllerVersion(client, obj, "newversion")
require.NoError(t, err)
actualKibana := &kibanav1alpha1.Kibana{}
err = client.Get(types.NamespacedName{
Namespace: obj.Namespace,
Name: obj.Name,
}, actualKibana)
require.NoError(t, err)
require.NotNil(t, actualKibana.GetAnnotations())
assert.Equal(t, actualKibana.GetAnnotations()[ControllerVersionAnnotation], "newversion")
}

// setupScheme creates a scheme to use for our fake clients so they know about our custom resources
// TODO move this into one of the upper level common packages and make public, refactor out this code that's in a lot of our tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, can you create an issue for that one?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically you only need to register the Kibana types here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's true @pebrc, wanted to make it easy to just copy it later

func setupScheme(t *testing.T) *runtime.Scheme {
sc := scheme.Scheme
if err := assoctype.SchemeBuilder.AddToScheme(sc); err != nil {
assert.Fail(t, "failed to add Association types")
}
if err := apmtype.SchemeBuilder.AddToScheme(sc); err != nil {
assert.Fail(t, "failed to add APM types")
}
if err := estype.SchemeBuilder.AddToScheme(sc); err != nil {
assert.Fail(t, "failed to add ES types")
}
if err := kibanav1alpha1.SchemeBuilder.AddToScheme(sc); err != nil {
assert.Fail(t, "failed to add Kibana types")
}
return sc
}
Loading