pkg/metrics: Add Deployment ownerReference to metrics Service object #1037
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,72 +23,78 @@ import ( | |
| v1 "k8s.io/api/core/v1" | ||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| "k8s.io/apimachinery/pkg/util/intstr" | ||
| crclient "sigs.k8s.io/controller-runtime/pkg/client" | ||
| "sigs.k8s.io/controller-runtime/pkg/client/config" | ||
| logf "sigs.k8s.io/controller-runtime/pkg/runtime/log" | ||
| ) | ||
|
|
||
| var log = logf.Log.WithName("metrics") | ||
|
|
||
| var trueVar = true | ||
|
|
||
| const ( | ||
| // PrometheusPortName defines the port name used in the metrics Service. | ||
| PrometheusPortName = "metrics" | ||
| ) | ||
|
|
||
| // ExposeMetricsPort creates a Kubernetes Service to expose the passed metrics port. | ||
| func ExposeMetricsPort(ctx context.Context, port int32) (*v1.Service, error) { | ||
| client, err := createClient() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create new client: %v", err) | ||
| } | ||
| // We do not need to check the validity of the port, as controller-runtime | ||
| // would error out and we would never get to this stage. | ||
| s, err := initOperatorService(ctx, client, port, PrometheusPortName) | ||
| if err != nil { | ||
| if err == k8sutil.ErrNoNamespace { | ||
| log.Info("Skipping metrics Service creation; not running in a cluster.") | ||
| return nil, nil | ||
| } | ||
| return nil, fmt.Errorf("failed to initialize service object for metrics: %v", err) | ||
| } | ||
| service, err := createOrUpdateService(ctx, client, s) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create or get service for metrics: %v", err) | ||
| } | ||
|
|
||
| return service, nil | ||
| } | ||
|
|
||
| func createOrUpdateService(ctx context.Context, client crclient.Client, s *v1.Service) (*v1.Service, error) { | ||
| if err := client.Create(ctx, s); err != nil { | ||
| if !apierrors.IsAlreadyExists(err) { | ||
| return nil, err | ||
| } | ||
| // Service already exists, we want to update it | ||
| // as we do not know if any fields might have changed. | ||
| existingService := &v1.Service{} | ||
| err := client.Get(ctx, types.NamespacedName{ | ||
| Name: s.Name, | ||
| Namespace: s.Namespace, | ||
| }, existingService) | ||
|
|
||
| s.ResourceVersion = existingService.ResourceVersion | ||
| if existingService.Spec.Type == v1.ServiceTypeClusterIP { | ||
| s.Spec.ClusterIP = existingService.Spec.ClusterIP | ||
| } | ||
| err = client.Update(ctx, s) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| log.V(1).Info("Metrics Service object updated", "Service.Name", s.Name, "Service.Namespace", s.Namespace) | ||
| return existingService, nil | ||
| } | ||
|
|
||
| log.Info("Metrics Service object created", "Service.Name", s.Name, "Service.Namespace", s.Namespace) | ||
| return s, nil | ||
| } | ||
|
|
||
| // initOperatorService returns the static service which exposes specifed port. | ||
| func initOperatorService(ctx context.Context, client crclient.Client, port int32, portName string) (*v1.Service, error) { | ||
| operatorName, err := k8sutil.GetOperatorName() | ||
| if err != nil { | ||
| return nil, err | ||
|
|
@@ -97,11 +103,14 @@ func initOperatorService(port int32, portName string) (*v1.Service, error) { | |
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| label := map[string]string{"name": operatorName} | ||
|
|
||
| service := &v1.Service{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: operatorName, | ||
|
There was a problem hiding this comment. Hmm. So I'm wondering if your original instinct to give the operator developer a chance to tweak the service before submitting it to the cluster was a good one. As is, if an operator developer wants to expose any other ports via a service, they will either have to create a separate service with a different name, or won't be able to enable metrics. Another option could be to have There was a problem hiding this comment. I would agree, my only fear is that it will break the API. Can open an issue to discuss this, as it's not strictly related to this PR anyways? |
||
| Namespace: namespace, | ||
| Labels: label, | ||
| }, | ||
| TypeMeta: metav1.TypeMeta{ | ||
| Kind: "Service", | ||
|
|
@@ -119,8 +128,72 @@ func initOperatorService(port int32, portName string) (*v1.Service, error) { | |
| Name: portName, | ||
| }, | ||
| }, | ||
| Selector: label, | ||
| }, | ||
| } | ||
|
|
||
| ownRef, err := getPodOwnerRef(ctx, client, namespace) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| service.SetOwnerReferences([]metav1.OwnerReference{*ownRef}) | ||
|
|
||
| return service, nil | ||
| } | ||
|
|
||
| func getPodOwnerRef(ctx context.Context, client crclient.Client, ns string) (*metav1.OwnerReference, error) { | ||
| // Get current Pod the operator is running in | ||
| pod, err := k8sutil.GetPod(ctx, client, ns) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| podOwnerRefs := metav1.NewControllerRef(pod, pod.GroupVersionKind()) | ||
| // Get Owner that the Pod belongs to | ||
| ownerRef := metav1.GetControllerOf(pod) | ||
| finalOwnerRef, err := findFinalOwnerRef(ctx, client, ns, ownerRef) | ||
|
There was a problem hiding this comment. Nit: We could simplify things further if we declare a new func getPodOwnerRef(ctx context.Context, client crclient.Client, ns string) (*metav1.OwnerReference, error) {
// Get current Pod the operator is running in
pod, err := k8sutil.GetPod(ctx, client, ns)
if err != nil {
return nil, err
}
return findFinalOwnerRef(ctx, client, ns, pod)
}
type gvkObject interface {
metav1.Object
GroupVersionKind() schema.GroupVersionKind
}
func findFinalOwnerRef(ctx context.Context, client crclient.Client, ns string, obj gvkObject) (*metav1.OwnerReference, error) {
ownerRef := metav1.GetControllerOf(obj)
if ownerRef == nil {
log.V(1).Info("Pods owner found", "Kind", ownerRef.Kind, "Name", ownerRef.Name, "Namespace", ns)
return metav1.NewControllerRef(obj, obj.GroupVersionKind()), nil
}
ownerObj := &unstructured.Unstructured{}
ownerObj.SetAPIVersion(ownerRef.APIVersion)
ownerObj.SetKind(ownerRef.Kind)
err := client.Get(ctx, types.NamespacedName{Namespace: ns, Name: ownerRef.Name}, ownerObj)
if err != nil {
return nil, err
}
return findFinalOwnerRef(ctx, client, ns, ownerObj)
}There was a problem hiding this comment. I would prefer to leave the function as is, as it has a clear entry point and you can see from the declaration what it does. |
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if finalOwnerRef != nil { | ||
| return finalOwnerRef, nil | ||
| } | ||
|
|
||
| // Default to returning Pod as the Owner | ||
| return podOwnerRefs, nil | ||
| } | ||
|
|
||
| // findFinalOwnerRef tries to locate the final controller/owner based on the owner reference provided. | ||
| func findFinalOwnerRef(ctx context.Context, client crclient.Client, ns string, ownerRef *metav1.OwnerReference) (*metav1.OwnerReference, error) { | ||
|
There was a problem hiding this comment. I really think that this function has to handle There was a problem hiding this comment. I agree, but I think we should try to do it as generically as possible. I wonder if we could consolidate the switch statement cases for known types to be: case "ReplicaSet", "ReplicationController":
rsrc := &unstructured.Unstructured{}
key := crclient.ObjectKey{Namespace: ns, Name: ownerRef.Name}
if err := client.Get(ctx, key, rsrc); err != nil {
return nil, err
}
rsrcOwner := metav1.GetControllerOf(rsrc)
// If we find an owner for the RS/RC, return that.
// Otherwise, just return the ownerRef directly.
if rsrcOwner != nil {
return rsrcOwner, nil
}
return ownerRef, nil
case "DaemonSet", "StatefulSet", "Job":
// I think we can just return this directly.
// Any reason to fetch the object first?
return ownerRef, nilI think this would cover Deployments and DeploymentConfigs generically, and it would be a bit more future-proof since we're being less explicit about some of the types we care about. There was a problem hiding this comment. I agree with @joelanford. A generic approach will reduce code significantly. There was a problem hiding this comment. Agreed! 🤦♀️ Done! There was a problem hiding this comment. This would then return the RS/RC as the owner directly, above we said we want to avoid doing that? Did I misunderstand your suggestion above maybe #1037 (comment) // If we find an owner for the RS/RC, return that.
// Otherwise, just return the ownerRef directly.
if rsrcOwner != nil {
return rsrcOwner, nil
}The reason was to avoid returning RS/RC as the owner and to avoid returning non k8s controllers owners as well. There was a problem hiding this comment. @lilic I think if the If the |
||
| if ownerRef == nil { | ||
| return nil, nil | ||
| } | ||
|
|
||
| obj := &unstructured.Unstructured{} | ||
| obj.SetAPIVersion(ownerRef.APIVersion) | ||
| obj.SetKind(ownerRef.Kind) | ||
| err := client.Get(ctx, types.NamespacedName{Namespace: ns, Name: ownerRef.Name}, obj) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| newOwnerRef := metav1.GetControllerOf(obj) | ||
| if newOwnerRef != nil { | ||
| return findFinalOwnerRef(ctx, client, ns, newOwnerRef) | ||
| } | ||
|
|
||
| log.V(1).Info("Pods owner found", "Kind", ownerRef.Kind, "Name", ownerRef.Name, "Namespace", ns) | ||
| return ownerRef, nil | ||
| } | ||
|
|
||
| func createClient() (crclient.Client, error) { | ||
| config, err := config.GetConfig() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| client, err := crclient.New(config, crclient.Options{}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return client, nil | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This suggestion isn't strictly necessary but using
corev1.SchemaGroupVersion.Versionis a better way of setting this value IMO, as it tracks dependency version changes. WDYT?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to set the TypeMeta for a concrete type when using the controller-runtime client? I thought this was only necessary with
unstructured.Unstructured.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea why this was done this way. I can remove it, but it comes originally and is used still in the
leader.gomyOwnerReffunction, so not sure if they need to set this maybe. I would leave it, as it doesn't hurt. https://github.com/operator-framework/operator-sdk/pull/1037/files#diff-a0eaeead6981b3b2f753c1ecbba711ccL145