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

Annotations cannot be removed by operator #1171

Closed
f41gh7 opened this issue Nov 27, 2024 · 3 comments
Closed

Annotations cannot be removed by operator #1171

f41gh7 opened this issue Nov 27, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@f41gh7
Copy link
Collaborator

f41gh7 commented Nov 27, 2024

Currently, operator merges annotations exists at managed objects with annotations defined at CRDs definition.

Consider the following example:

apiVersion: operator.victoriametrics.com/v1beta1
kind: VMSingle
metadata:
  name: example
  annotations:
      some-key: value
spec:
  # Add fields here
  retentionPeriod: "1"

Created deployment has annotation:

apiVersion: apps/v1
kind: Deployment
metadata:
  annotations:
    some-key: value

Due to labels.Merge logic at reconcile package: https://github.com/VictoriaMetrics/operator/blob/master/internal/controller/operator/factory/reconcile/deploy.go#L51

Operator will always keep some-key annotations.

Main reason behind that is preserving annotations added by 3rd party components, such as mutation webhooks.

Possible solutions:

  1. use 3-way merge to properly delete annotations added and managed by operator
  2. override all annotations and use CRD as source of truth.
@f41gh7 f41gh7 added the bug Something isn't working label Nov 27, 2024
@f41gh7
Copy link
Collaborator Author

f41gh7 commented Dec 2, 2024

Based on quick research there is only 1 option - use 3-way merge.

In this case, operator must keep last applied metadata into the special annotation. And perform decision to remove annotation based on previous state.

Operator cannot remove any annotations manually assigned. Since it breaks logic for other controllers. Such as serviceAccount and deployment controllers at controller-manager.

For instance, controller-manager assigns deployment.kubernetes.io/revision: "N" annotations to deployments.

@f41gh7
Copy link
Collaborator Author

f41gh7 commented Dec 5, 2024

I find out that labels and annotations inherit was a design mistake. At the first glance, it looks simple and natural to forward metadata from parent to the child resources.

But in terms of Kubernetes it has major down-sides and it makes changes hard to track and actually use.

The biggest concern for that is object.Generation field and resource changes tracking. Kubernetes api-server increments generation each time configuration of spec changes. It must trigger some actions based on this changes.

In addition, many deployment tools, such as helm and argocd uses labels and annotations to track resources managed by it. It's misleading to add metadata to the objects created and managed by operator. It requires some workarounds, that were already added ignoreAnnotationPrefixes and some other.

It leads to the following upcoming changes:

  • labels and annotations inheritance will be deprecated at v0.51.0 release of operator and will be removed at v0.52.0 version
  • v0.51.0 version will add special field spec.managedMetadata for labels and annotations that must be added to resources created by operator.

FYI @Haleygo

Related Kubernetes issue:
kubernetes/kubernetes#67428

f41gh7 added a commit that referenced this issue Dec 6, 2024
 Previously, labels and annotations metadata was inherited from base CRD
object to the all children objects created operator. Such as
Deployment, ServiceAccount and etc.

  This commit reworks `labels` and `annotations`. Since v0.52.0 release
operator will no longer use add base CRD object metadata to the created
objects. It will use `managedMetadata` as the source of it.

It has the following benefits:
* it's no longer needed to filter-out labels and annotations to be
   compatible with application deployment tools. Such as helm or argocd
* changes to `managedMetadata` will trigger `generation` field change
  for the CRD and it'll possible to properly trigger update status.
* it'd be possible to remove annotations managed by operator and
  preserve 3rd party annotations.

Related issue:
#1171
@f41gh7 f41gh7 added the waiting for release The change was merged to upstream, but wasn't released yet. label Dec 6, 2024
@f41gh7
Copy link
Collaborator Author

f41gh7 commented Dec 19, 2024

The issue was fixed at v0.51.1 release

@f41gh7 f41gh7 closed this as completed Dec 19, 2024
@f41gh7 f41gh7 removed the waiting for release The change was merged to upstream, but wasn't released yet. label Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant