quarkus icon indicating copy to clipboard operation
quarkus copied to clipboard

Remove conflicting Deployment due to label selector version

Open iocanel opened this issue 1 year ago • 5 comments

Resolves: #39180

What the pull request does is that it checks if an existing Deployment is already installed with conflicting Deployment version and provides detailed error message with instructions on how the situation should be handled.

Also, it does the checking before applied resources (e.g. Service etc) that could leave the application in a non-working state.

iocanel avatar Mar 06 '24 16:03 iocanel

@manusa: Do you have any insights on this?

iocanel avatar Mar 06 '24 16:03 iocanel

This seems kinda risky, no?

geoand avatar Mar 06 '24 16:03 geoand


:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.


Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 38f412b9ae1a9398c3e086d5152b1a2d4e563cbb.

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
Initial JDK 17 Build Build Failures Logs Raw logs :mag:

You can consult the Develocity build scans.

Failures

:gear: Initial JDK 17 Build #

- Failing: extensions/kubernetes/vanilla/deployment 
! Skipped: docs extensions/kubernetes/kind/deployment extensions/kubernetes/minikube/deployment and 6 more

:package: extensions/kubernetes/vanilla/deployment

Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.12.1:compile (default-compile) on project quarkus-kubernetes-deployment: Compilation failure

quarkus-bot[bot] avatar Mar 06 '24 16:03 quarkus-bot[bot]

@manusa: Do you have any insights on this?

I do have a recent experience about a complaint from a JKube user regarding something related.

https://github.com/eclipse/jkube/issues/2697

Starting from v1.16, we switched to use standard/recommended Kubernetes labels as opposed to those that JKube (formerly Fabric8 Maven Plugin) prescribed.

When the user upgraded to the newer JKube version with the changes, their pipelines failed due to the existent Deployment selector containing different values.

We did recommend the recreate approach (where the previous Deployment is deleted and then reapplied), but this was not possible for them. See https://github.com/eclipse/jkube/issues/2697#issuecomment-1952562420

manusa avatar Mar 06 '24 17:03 manusa


:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.


Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 6fffce9435f43af7c26714b0eda17e018a31262f.

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
Initial JDK 17 Build Build Failures Logs Raw logs :mag:

You can consult the Develocity build scans.

Failures

:gear: Initial JDK 17 Build #

- Failing: integration-tests/webjars-locator 

:package: integration-tests/webjars-locator

Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.2.1:enforce (enforce) on project quarkus-integration-test-webjars-locator: Rule 1: io.quarkus.enforcer.RequiresMinimalDeploymentDependency failed with message: 1 minimal *-deployment dependencies are superfluous and must be removed from pom.xml: io.quarkus:quarkus-webjars-locator-deployment:999-SNAPSHOT

quarkus-bot[bot] avatar Apr 23 '24 08:04 quarkus-bot[bot]

@iocanel if having version in label makes impossible to update deployments shouldn't this not be avoided?

maxandersen avatar Apr 29 '24 11:04 maxandersen

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 070107b3dcba53b9837d9f2703d15414dc31299d.

:white_check_mark: The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

quarkus-bot[bot] avatar Apr 29 '24 11:04 quarkus-bot[bot]

@iocanel if having version in label makes impossible to update deployments shouldn't this not be avoided?

Adding the version to the selector has it's pros and cons. If we just remove it then users will only be able to correlate pods by name, which can be problematic as users won't be able to route traffic between version etc. Things like canary, a/b testing it will become trickier.

So, we need give users the option to decide how they'll roll. Currently, it's enabled by default. We could possibly change that but that means that it will then bite everyone once.

iocanel avatar Apr 29 '24 11:04 iocanel

Something additional that worth mentioning ... This behavior (having the version being part of the label) is inherited by dekorate since 2019. Meaning that its not something that we just recently change. The main reason why this is has been just discovered is the transition from DeploymentConfig to Deployment which is the resource affected by it.

I suggest we move forward with this pull request. And move to an optional opt-in strategy for the next major release.

iocanel avatar May 03 '24 10:05 iocanel

@gsmet @yrodiere wdyt ? seems like good first step to inform users what to do early.

I suggest we in anohter PR make add-version-to-label-selectors be false by default. That will still cause exception on existing deployments that has label but with this PR its now informative/helpful, and any future quarkus versions wil allow rolling deployments by default.

maxandersen avatar May 16 '24 09:05 maxandersen