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

Rename operator deployment #432

Merged

Conversation

andreasgerstmayr
Copy link
Collaborator

Commit 7fa448a added additional labels to the operator deployment and its selector field.

Unfortunately the selector field is immutable, and when OLM tries to patch the deployment while upgrading, Kubernetes will reject this update.

A workaround is to rename the deployment, as suggested here: operator-framework/operator-lifecycle-manager#952

@andreasgerstmayr andreasgerstmayr force-pushed the rename-operator-deployment branch from 0de1528 to babb2df Compare May 31, 2023 10:28
Copy link
Collaborator

@iblancasa iblancasa left a comment

Choose a reason for hiding this comment

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

I have some questions:

  1. This is something we need to do just because the selector field was modified. Not something we'll need to do for each new version, right?
  2. Are we going to bump the operator to version 2 and that is the reason for v2 postfix?

Also... maybe it's ok to not support the upgrade from the 0.1.0 version. That will simplify everything

@codecov-commenter
Copy link

codecov-commenter commented May 31, 2023

Codecov Report

Merging #432 (cd5bd6d) into main (d556798) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #432   +/-   ##
=======================================
  Coverage   77.93%   77.93%           
=======================================
  Files          55       55           
  Lines        4252     4252           
=======================================
  Hits         3314     3314           
  Misses        793      793           
  Partials      145      145           
Flag Coverage Δ
unittests 77.93% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@andreasgerstmayr
Copy link
Collaborator Author

I have some questions:

1. This is something we need to do just because the selector field was modified. Not something we'll need to do for each new version, right?

Yes, exactly. The selectors shouldn't change in the future.

2. Are we going to bump the operator to version 2 and that is the reason for `v2` postfix?

I choose v2 in a sense of "v2-of-the-deployment", not of the operator. The name of the operator deployment should be an internal detail and shouldn't matter much.

Also... maybe it's ok to not support the upgrade from the 0.1.0 version. That will simplify everything

That's also an option. What are your concerns about renaming the deployment? Does anything rely on the name of the deployment (note, I didn't rename the labels of the deployment, only the name)?

Afaics nothing should break.. except one thing, if you install the operator via manifests, than the old deployment is not pruned. imho that's a general problem and will happen every time when we drop or rename any component (Service, ConfigMap, etc).

@andreasgerstmayr
Copy link
Collaborator Author

Also... maybe it's ok to not support the upgrade from the 0.1.0 version. That will simplify everything

Hm, yeah it also changes the pod name, then the blog post example output won't match anymore. Maybe it's not that important to support upgrades from 0.1.0, I don't know :)

@iblancasa
Copy link
Collaborator

iblancasa commented May 31, 2023

That's also an option. What are your concerns about renaming the deployment? Does anything rely on the name of the deployment (note, I didn't rename the labels of the deployment, only the name)?

I think the v2 can be a little bit misleading because we can think relates somehow to version 2 of the operator. Also, I'm not sure if we want to keep that v2 postfix forever because of one change between the first and second versions.

It's not a strong opinion.

@andreasgerstmayr
Copy link
Collaborator Author

I agree, I'm also not too happy about the rename with -v2. I'd like it renamed to just tempo-operator (I'm not a big fan of controller-manager), but that's also not possible because the kustomize files prefix everything with tempo-operator, so the deployment name in the kustomize files would be empty then.

Copy link
Collaborator

@frzifus frzifus left a comment

Choose a reason for hiding this comment

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

Just some thoughts on it. ^^

.chloggen/rename_operator_deployment.yaml Outdated Show resolved Hide resolved
.chloggen/rename_operator_deployment.yaml Outdated Show resolved Hide resolved
config/manager/manager.yaml Outdated Show resolved Hide resolved
Commit 7fa448a added additional labels
to the operator deployment and its selector field.

Unfortunately the selector field is immutable, and when OLM tries to patch
the deployment while upgrading, Kubernetes will reject this update.

A workaround is to rename the deployment, as suggested here:
operator-framework/operator-lifecycle-manager#952

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
@andreasgerstmayr andreasgerstmayr force-pushed the rename-operator-deployment branch from cd5bd6d to a999f2a Compare June 1, 2023 14:00
@andreasgerstmayr andreasgerstmayr merged commit df6e565 into grafana:main Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants