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

Quarkus with liquibase on Kubernetes #33097

Closed
edeandrea opened this issue May 3, 2023 · 24 comments · Fixed by #33409
Closed

Quarkus with liquibase on Kubernetes #33097

edeandrea opened this issue May 3, 2023 · 24 comments · Fixed by #33409
Labels
Milestone

Comments

@edeandrea
Copy link
Contributor

edeandrea commented May 3, 2023

Describe the bug

Hi - I've noticed when using the Quarkus Kubernetes/OpenShift extension when an app uses MongoDB that there is this new liquibase-mongodb-init kubernetes Job added.

Why is that, especially if my app has quarkus.liquibase-mongodb.migrate-at-start=false set?

when i run ./mvnw clean package -Dquarkus.kubernetes.deployment-target=openshift -Dquarkus.liquibase-mongodb.migrate-at-start=false I still see this in the resulting target/openshift.yml (note I've removed some things to make it shorter & more readable).

Why do I need that init container if my app doesn't need it? Furthermore, on Openshift 4.12, that liquibase-mongodb-init container never actually starts up and causes my entire app not to work.

The image groundnuty/k8s-wait-for:1.3 seems that the version of kubctl is too old.

But in any case, if my app specifies quarkus.liquibase-mongodb.migrate-at-start=false why do I even need all this init container stuff?

apiVersion: apps.openshift.io/v1
kind: DeploymentConfig
metadata:
  labels:
    app.kubernetes.io/name: rest-fights
    app.kubernetes.io/part-of: fights-service
    app.kubernetes.io/version: java17-latest
    app.openshift.io/runtime: quarkus
    app.kubernetes.io/managed-by: quarkus
    system: quarkus-super-heroes
    application: fights-service
    app: rest-fights
  name: rest-fights
spec:
  replicas: 1
  selector:
    app.kubernetes.io/version: java17-latest
    app.kubernetes.io/name: rest-fights
    app.kubernetes.io/part-of: fights-service
  template:
    metadata:
      labels:
        app.kubernetes.io/version: java17-latest
        app.kubernetes.io/name: rest-fights
        app.kubernetes.io/part-of: fights-service
        app.openshift.io/runtime: quarkus
        app.kubernetes.io/managed-by: quarkus
        system: quarkus-super-heroes
        application: fights-service
        app: rest-fights
    spec:
      containers:
        - env:
            - name: KUBERNETES_NAMESPACE
              valueFrom:
                fieldRef:
                  fieldPath: metadata.namespace
            - name: QUARKUS_LIQUIBASE_MONGODB_ENABLED
              value: "false"
          envFrom:
            - configMapRef:
                name: rest-fights-config
            - secretRef:
                name: rest-fights-config-creds
          image: rest-fights:jaa17-latest
          imagePullPolicy: Always
          name: rest-fights
          ports:
            - containerPort: 8082
              name: http
              protocol: TCP
            - containerPort: 8443
              name: https
              protocol: TCP
          resources:
            limits:
              memory: 768Mi
            requests:
              memory: 256Mi
      initContainers:
        - args:
            - job
            - liquibase-mongodb-init
          image: groundnuty/k8s-wait-for:1.3
          imagePullPolicy: IfNotPresent
          name: init
  triggers:
    - imageChangeParams:
        automatic: true
        containerNames:
          - rest-fights
        from:
          kind: ImageStreamTag
          name: rest-fights:jaa17-latest
      type: ImageChange
---
apiVersion: batch/v1
kind: Job
metadata:
  name: liquibase-mongodb-init
spec:
  completionMode: OnFailure
  template:
    metadata: {}
    spec:
      containers:
        - env:
            - name: QUARKUS_INIT_AND_EXIT
              value: "true"
            - name: QUARKUS_LIQUIBASE_MONGODB_ENABLED
              value: "true"
          envFrom:
            - configMapRef:
                name: rest-fights-config
            - secretRef:
                name: rest-fights-config-creds
          image: quay.io/quarkus-super-heroes/rest-fights:jaa17-latest
          name: liquibase-mongodb-init
      restartPolicy: OnFailure

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

  1. Clone https://github.com/quarkusio/quarkus-super-heroes
  2. cd into rest-fights
  3. Log into an openshift cluster
  4. Run
./mvnw clean package -DskipTests -Dmaven.compiler.release=17 \
  -Dquarkus.container-image.tag=java17-latest \
  -Dquarkus.liquibase-mongodb.migrate-at-start=false \
  -Dquarkus.kubernetes.deployment-target=openshift \
  -Dquarkus.kubernetes.version=java17-latest \
  -Dquarkus.kubernetes.ingress.expose=true \
  -Dquarkus.kubernetes.resources.limits.memory=768Mi \
  -Dquarkus.kubernetes.resources.requests.memory=256Mi \
  -Dquarkus.openshift.version=java17-latest \
  -Dquarkus.openshift.route.expose=true \
  -Dquarkus.openshift.resources.limits.memory=768Mi \
  -Dquarkus.openshift.resources.requests.memory=256Mi \
  -Dquarkus.kubernetes.deploy=true  \
  -Dquarkus.container-image.group=$(oc project -q) \
  -Dquarkus.container-image.registry= \
  -Dquarkus.profile=openshift-17

Notice in target/kubernetes/openshift.yml that init container is still there and actually never starts up when deployed.

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

@edeandrea edeandrea added the kind/bug Something isn't working label May 3, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented May 3, 2023

/cc @Sgitario (kubernetes), @andrejpetras (liquibase), @geoand (kubernetes,liquibase,openshift), @gsmet (liquibase), @iocanel (kubernetes,openshift)

@Sgitario
Copy link
Contributor

Sgitario commented May 3, 2023

To give more context, this was introduced by #29026:

The pull request introduces an initialization phase where init tasks like Flyway & Liquibase migration take place. It allows users to run those tasks in isolation (without running the whole application).

Yet having quarkus.liquibase-mongodb.migrate-at-start=false to disable the Job generation makes a lot of sense to me.

@edeandrea
Copy link
Contributor Author

THank you @Sgitario . I'm finding that the job also doesn't actually start on OpenShift 4.12. It errors out with a kubectl version error being too old. I had to change the job to use groundnuty/k8s-wait-for:no-root-v1.7.

But in any case, if I don't want the migration I shouldn't need to even have that job/init container.

@Sgitario
Copy link
Contributor

Sgitario commented May 3, 2023

THank you @Sgitario . I'm finding that the job also doesn't actually start on OpenShift 4.12. It errors out with a kubectl version error being too old. I had to change the job to use groundnuty/k8s-wait-for:no-root-v1.7.

Thanks! This should be definitely fixed for OpenShift.

But in any case, if I don't want the migration I shouldn't need to even have that job/init container.

+100

@edeandrea
Copy link
Contributor Author

I also tried to change the image the init container uses by using quarkus.kubernetes.init-containers."init".image but that didn't work either

Sgitario added a commit to Sgitario/quarkus that referenced this issue May 4, 2023
The `quarkus.[flyway or liquibase or liquibase-mongodb].enabled` and `quarkus.[flyway or liquibase or liquibase-mongodb].migrate-at-start` properties are runtime properties, so I have to create the following build time properties to read these values and allow users configurating it:

- `quarkus.[flyway or liquibase or liquibase-mongodb].generate-init-task` with default value `quarkus.[flyway or liquibase or liquibase-mongodb].enabled`
- `quarkus.[flyway or liquibase or liquibase-mongodb].migrate-with-init-task` with default value `quarkus.[flyway or liquibase or liquibase-mongodb].migrate-at-start`

Plus, I've extended the coverage of these extensions with the K8s tests. 

Fix quarkusio#33097
Sgitario added a commit to Sgitario/quarkus that referenced this issue May 4, 2023
To groundnuty/k8s-wait-for:no-root-v1.7.

Related to quarkusio#33097 (comment)
@Sgitario
Copy link
Contributor

Sgitario commented May 4, 2023

@edeandrea all the issues you spotted should be fixed by #33116.

@edeandrea
Copy link
Contributor Author

Thank you for the quick turnaround!

Sgitario added a commit to Sgitario/quarkus that referenced this issue May 4, 2023
The `quarkus.[flyway or liquibase or liquibase-mongodb].enabled` and `quarkus.[flyway or liquibase or liquibase-mongodb].migrate-at-start` properties are runtime properties, so I have to create the following build time properties to read these values and allow users configurating it:

- `quarkus.[flyway or liquibase or liquibase-mongodb].generate-init-task` with default value `quarkus.[flyway or liquibase or liquibase-mongodb].enabled`
- `quarkus.[flyway or liquibase or liquibase-mongodb].migrate-with-init-task` with default value `quarkus.[flyway or liquibase or liquibase-mongodb].migrate-at-start`

Plus, I've extended the coverage of these extensions with the K8s tests. 

Fix quarkusio#33097
Sgitario added a commit to Sgitario/quarkus that referenced this issue May 4, 2023
To groundnuty/k8s-wait-for:no-root-v1.7.

Related to quarkusio#33097 (comment)
@iocanel
Copy link
Contributor

iocanel commented May 6, 2023

FWIW, quarkus.liquibase-mongodb.migrate-at-start is a runtime configuration option and is not meant to be used a build time where the actual generation takes place.

@edeandrea
Copy link
Contributor Author

Hmmm. Maybe we need a build time property? I still think the requirement is valid.

@iocanel
Copy link
Contributor

iocanel commented May 7, 2023

Hmmm. Maybe we need a build time property? I still think the requirement is valid.

Absolutely. There requirement is perfectly valid we do need a feature flag for this.

@Sgitario
Copy link
Contributor

Sgitario commented May 8, 2023

Hmmm. Maybe we need a build time property? I still think the requirement is valid.

Absolutely. There requirement is perfectly valid we do need a feature flag for this.

I do think that having a build-time property will make this feature mostly unused. But taking into account the comments, I'm closing #33116 in favor of a new build-time property.

@iocanel I'm delegating this issue to you, so you can fix it with what you have in mind.

Thanks all for your comments!

@iocanel
Copy link
Contributor

iocanel commented May 8, 2023

I don't think that you should close your pull request. You have put time to create it and I have put time to review it, we better work out the points were we disaagree and move forward.

I also don't understand the point about the build time property thing. Your pull request introduces a new build time property as a feature flag quarkus.liquibase.generate-init-task and I agree with it. The only part were we disagree is on where it's default value should originate and if it should be dependant on quarkus.liquibase.migrate-at-startup.

@Sgitario
Copy link
Contributor

Sgitario commented May 8, 2023

I don't think that you should close your pull request. You have put time to create it and I have put time to review it, we better work out the points were we disaagree and move forward.

I really appreciate your time, but I think it would be better to start over with a new pull request as the solution of adding only a new property would be much simpler. Also, it's the thing about the new decorators I added, which I agree with you that it over-complicates the solution, tho I don't see any other easier solution, so the user config takes precedence over the auto-generated configuration :/

@Sgitario
Copy link
Contributor

Sgitario commented May 8, 2023

About adding only one build-time property generate-init-task whose default value is false. Would this solution work for you? @edeandrea

About:

Also, it's the thing about the new decorators I added, which I agree with you that it over-complicates the solution, tho I don't see any other easier solution, so the user config takes precedence over the auto-generated configuration :/

Perhaps, the easiest solution is not to do this for now. So, the generated init-container would not be configurable yet. Wdyt? @iocanel

@edeandrea
Copy link
Contributor Author

edeandrea commented May 8, 2023

There were 2 things that I was hoping for

  1. correcting the image used in the case the job is present

  2. making it so that I can control whether the job and the init container are generated.

In the interim I've put in a hack which strips this out: https://github.com/quarkusio/quarkus-super-heroes/blob/main/scripts/generate-k8s-resources.sh#L97_L99

@iocanel
Copy link
Contributor

iocanel commented May 8, 2023

About adding only one build-time property generate-init-task whose default value is false. Would this solution work for you? @edeandrea

About:

Also, it's the thing about the new decorators I added, which I agree with you that it over-complicates the solution, tho I don't see any other easier solution, so the user config takes precedence over the auto-generated configuration :/

Perhaps, the easiest solution is not to do this for now. So, the generated init-container would not be configurable yet. Wdyt? @iocanel

I think that this is the pefect sized solution!

I am not 100% sure if the default value for generate-init-task should be false however. I am open about it, but if its set to false then we also need to update the docs. @maxandersen @edeandrea thoughts?

@Sgitario
Copy link
Contributor

Sgitario commented May 8, 2023

I am not 100% sure if the default value for generate-init-task should be false however.

Just to extend my reasoning about why it should be false by default. It's because the migrate-at-start runtime property is also false by default.

There were 2 things that I was hoping for

  1. correcting the image used in the case the job is present

I guess you mean to fix the image to be used, so it works in OpenShift, right? If so, this should be also addressed by a536cb9.

  1. making it so that I can control whether the job and the init container are generated.

@edeandrea I think adding the new build time property would cover the second point.

@iocanel
Copy link
Contributor

iocanel commented May 8, 2023

Just to extend my reasoning about why it should be false by default. It's because the migrate-at-start runtime property is also false by default.

I understand the reasoning. However, at build time the value of migrate-at-start doesn't really mean much.
I would like to avoid to have two different properties needed to enable a feture that we currently have on by default.

Also note that beyond liquibase, liquibase-mongo and flyway, this feature is currently being opened up to users, allowing them to bring their own initialization code. This means that if we disable by default it will require way too much configuration to enable everyting.

I'd rather have everything by default and let users decide what to disable, than having everything disabled practically the feature.

@iocanel
Copy link
Contributor

iocanel commented May 10, 2023

@edeandrea: Until we get this sorted out, there are the following options you can already use to globally disable creating Jobs:

The fact that I had to look multiple times to find it (even though I added that myself) is a strong indication, that the name is not good enough.

@Sgitario
Copy link
Contributor

@edeandrea: Until we get this sorted out, there are the following options you can already use to globally disable creating Jobs:

The fact that I had to look multiple times to find it (even though I added that myself) is a strong indication, that the name is not good enough.

I was not aware of it. Thanks @iocanel !

@edeandrea
Copy link
Contributor Author

@iocanel those properties seem to do the trick!

@edeandrea
Copy link
Contributor Author

Although these flags seem to not be specific to Liquibase/mongo/etc. They seem to be global for all extensions that may generate k8s resources?

@Sgitario
Copy link
Contributor

Although these flags seem to not be specific to Liquibase/mongo/etc. They seem to be global for all extensions that may generate k8s resources?

Yes, though I don't think there are many extensions using this. I'm working in a pull request to provide a new property so we can exclude only one init task.

@Sgitario
Copy link
Contributor

Sgitario commented May 16, 2023

@iocanel @edeandrea I've provided a new pull request without adding the controversial changes (won't add any new property in the flyway, liquibase and liquibase-mongodb extensions). The pull request includes the changes described in the description: #33409 (comment)

Let me know if you feel better with the changes on it!

Sgitario added a commit to Sgitario/dekorate that referenced this issue May 17, 2023
Sgitario added a commit to Sgitario/dekorate that referenced this issue May 17, 2023
Sgitario added a commit to Sgitario/dekorate that referenced this issue May 17, 2023
Sgitario added a commit to Sgitario/quarkus that referenced this issue May 18, 2023
To groundnuty/k8s-wait-for:no-root-v1.7.

Related to quarkusio#33097 (comment)
Sgitario added a commit to dekorateio/dekorate that referenced this issue May 24, 2023
Sgitario added a commit to Sgitario/quarkus that referenced this issue May 26, 2023
To groundnuty/k8s-wait-for:no-root-v1.7.

Related to quarkusio#33097 (comment)
Sgitario added a commit to Sgitario/quarkus that referenced this issue May 26, 2023
@quarkus-bot quarkus-bot bot added this to the 3.2 - main milestone May 26, 2023
michelle-purcell pushed a commit to michelle-purcell/quarkus that referenced this issue May 29, 2023
To groundnuty/k8s-wait-for:no-root-v1.7.

Related to quarkusio#33097 (comment)
michelle-purcell pushed a commit to michelle-purcell/quarkus that referenced this issue May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment