Fix RemoveDeploymentTriggerDecorator order #30768

Sgitario · 2023-02-01T07:17:51Z

The decorator RemoveDeploymentTriggerDecorator was not configured to be triggered before ChangeDeploymentTriggerDecorator, so sometimes the image stream tag was wrong.

This change fixes the failure spotted by the test OpenshiftWithDockerAndImageTest where the image stream tag was sometimes 0.1-SNAPSHOT and other times 1.0 when it should be always 1.0.

Relates to the random failures in https://github.com/dekorateio/dekorate/actions/runs/4023231521/jobs/6913879180 and also in the jakarta-rewrite branch

Sgitario · 2023-02-01T08:11:40Z

it seems the issue is still there:

Error:  Failures: 
Error:    OpenshiftWithDockerAndImageTest.assertGeneratedResources:46 
[List check single element] (1 failure)
-- failure 1 --
expected: "openshift-with-docker-and-image:1.0"
 but was: "openshift-with-docker-and-image:0.1-SNAPSHOT"
at OpenshiftWithDockerAndImageTest.lambda$assertGeneratedResources$5(OpenshiftWithDockerAndImageTest.java:56)

iocanel · 2023-02-01T08:34:23Z

...loyment/src/main/java/io/quarkus/kubernetes/deployment/RemoveDeploymentTriggerDecorator.java

@@ -16,7 +15,7 @@ public void andThenVisit(DeploymentConfigSpecFluent<?> deploymentConfigSpec, Obj
    }

    @Override
-    public Class<? extends Decorator>[] after() {


This should not be removed.

The purpose of RemoveDeploymentTrigger is to remove triggers either provided by the user or by the ApplyDeploymentTrigger decorator. By removing this condition it's possible that RemoveDeploymentTrigger runs before there is anything to remove.

iocanel

There is an issue with the implementation as commented directly on the code.
Also this is fixed as part of #30566 so we could possibly close this one.

Sgitario · 2023-02-01T08:44:57Z

There is an issue with the implementation as commented directly on the code. Also this is fixed as part of #30566 so we could possibly close this one.

Unfortunately, this is not fixing the issue even using your changes because the ApplyDeploymentTriggerDecorator from decorator is being applied the last:

[INFO] Applying decorator 'io.quarkus.kubernetes.deployment.RemoveDeploymentTriggerDecorator' -- removes the image triggers if any
[INFO] Applying decorator 'io.quarkus.kubernetes.deployment.ChangeDeploymentTriggerDecorator' -- add the correct image
[INFO] Applying decorator 'io.dekorate.openshift.decorator.ApplyDeploymentTriggerDecorator' -- overwrite the wrong image

Sgitario · 2023-02-01T09:02:06Z

@iocanel I've just updated the changes that seem to consistently apply the decorators in the correct order.
Also, I've followed your suggestion to keep using ApplyDeploymentTriggerDecorator in RemoveDeploymentTriggerDecorator. Additionally, I've updated the RemoveDeploymentTriggerDecorator decorator to only do that to the DeploymentConfig with the correct name.

Sgitario · 2023-02-01T10:55:50Z

I'm running out of ideas here since locally my changes worked consistently ok, but not in CI.
Now, I'm doing exactly what was done by @iocanel in #30566 in spite of that locally it was not working for me, but just in case CI accepts it.

Sgitario · 2023-02-01T14:53:05Z

Something really odd is happening with the dekorate logic that orders the decorators to be run.
In spite of the CI being green, these changes are consistently not working on my local machine. On the other hand, the changes that indeed fix the actual issue locally do not work for the CI. so, we need further investigation here. Moving to Draft.

iocanel · 2023-02-01T19:09:21Z

Something really odd is happening with the dekorate logic that orders the decorators to be run. In spite of the CI being green, these changes are consistently not working on my local machine. On the other hand, the changes that indeed fix the actual issue locally do not work for the CI. so, we need further investigation here. Moving to Draft.

Are you sure that tests on CI are passing? I am seeing them as skipped.
Eitherway here are some thoughts on how we could deal with it:

Simplify the original decorator in dekorate. The decorator seems to be quite complex due to a long fixed issue in sundrio, so we could possibly try to simplify it.
Fix the ordering algorithm in dekorate. The issue here is that our comparator is not transitive which is most likely what is causing our ordering issues.
Register a configurator that sets the right image so that no matter what decorator is run last it will use ther correct value.

Sgitario · 2023-02-02T07:35:14Z

Something really odd is happening with the dekorate logic that orders the decorators to be run. In spite of the CI being green, these changes are consistently not working on my local machine. On the other hand, the changes that indeed fix the actual issue locally do not work for the CI. so, we need further investigation here. Moving to Draft.

Are you sure that tests on CI are passing? I am seeing them as skipped. Eitherway here are some thoughts on how we could deal with it:

Simplify the original decorator in dekorate. The decorator seems to be quite complex due to a long fixed issue in sundrio, so we could possibly try to simplify it.

Fix the ordering algorithm in dekorate. The issue here is that our comparator is not transitive which is most likely what is causing our ordering issues.

Register a configurator that sets the right image so that no matter what decorator is run last it will use ther correct value.

+100 It makes totally sense to fix this from the root.
I will try to use a custom configurator to do this.

Sgitario · 2023-02-02T08:37:26Z

I've just updated the pull request by providing a custom configurator, so we can fix this issue from the root. I reported a couple of issues in Dekorate, but not blockers.

Sgitario · 2023-02-02T10:53:11Z

It seems that dekorateio/dekorate#1147 is indeed a blocker because for some reason, it expects a build service factory (??) in Kind projects.
So, I don't know how we cannot overwrite the default container image at the moment. @iocanel can you help here please?

Sgitario · 2023-02-02T10:59:38Z

I'm going to confirm that this pull request #30591 introduced this issue and if so, I will revert those changes.

Sgitario · 2023-02-02T12:48:58Z

I've just confirmed that this issue has nothing to do with #30591.
I guess this issue has been always there, and for some reason, now it's more frequent. The only fix I foresee is to fix the logic of the decorator execution at Dekorate side.

quarkus-bot · 2023-02-02T13:41:05Z

Failing Jobs - Building `e820786`

Status	Name	Step	Failures	Logs	Raw logs
✖	JVM Tests - JDK 11	`Build`	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 17
✖	JVM Tests - JDK 18	`Build`	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: integration-tests/kubernetes/quarkus-standard-way

📦 integration-tests/kubernetes/quarkus-standard-way

✖ io.quarkus.it.kubernetes.OpenshiftWithDockerAndImageTest.assertGeneratedResources line 46 - More details - Source on GitHub

org.assertj.core.error.AssertJMultipleFailuresError: 

[List check single element] (1 failure)

⚙️ JVM Tests - JDK 18 #

- Failing: integration-tests/kubernetes/quarkus-standard-way

📦 integration-tests/kubernetes/quarkus-standard-way

✖ io.quarkus.it.kubernetes.OpenshiftWithDockerAndImageTest.assertGeneratedResources line 46 - More details - Source on GitHub

org.assertj.core.error.AssertJMultipleFailuresError: 

[List check single element] (1 failure)

iocanel · 2023-02-02T14:06:21Z

I've just confirmed that this issue has nothing to do with #30591. I guess this issue has been always there, and for some reason, now it's more frequent. The only fix I foresee is to fix the logic of the decorator execution at Dekorate side.

I think that issue was always there but only manifests itself under certain decorator combinations.
I agree that the best course of action, is to deal with the issue in the dekorate side. I'll be able to work on that later today.

Sgitario · 2023-02-03T05:09:29Z

Closing as this will be addressed in Dekorate.

Related to quarkusio#30768 I don't know why but it's failing systematically in the jakarta-rewrite branch and I don't want it to be in the way of the migration.

quarkus-bot bot added the area/kubernetes label Feb 1, 2023

Sgitario requested a review from gsmet February 1, 2023 07:19

Sgitario force-pushed the fix_order_remove_depl_trigger branch from 9167104 to 9e190ce Compare February 1, 2023 07:20

gsmet approved these changes Feb 1, 2023

View reviewed changes