Allow applying manifests on Kubernetes Dev Service startup

[LarsSven]

This contribution adds a new configuration parameter to the kubernetes-client extension that allows users to deploy manifests to their Kubernetes cluster Dev Service (https://quarkus.io/guides/kubernetes-dev-services).

This is useful because certain applications expect certain resources to be present in the cluster. For our specific use case for example, we expect a custom resource definition to be present such that our Quarkus application can deploy custom resources.

I have for now left out the tests and documentation. I plan to add them as soon as the implementation looks good, but I would first like to figure out if the Quarkus team likes the idea and likes the implementation of it.

You can only deploy manifests using this method and not helm charts. However helm charts are still supported with this feature, since one of the Kubernetes flavors is k3s, which has a helm chart controller built in. I will add this to the documentation with an example of how to deploy a helm chart once I write the documentation for this PR.

[LarsSven]

Would it also be possible to make this a non-optional parameter with a default value of []? Then I could replace

if (configuration.manifests.isPresent())
  deployManifests(configuration.manifests.get(), devService.getConfig());

with

deployManifests(configuration.manifests.get(), devService.getConfig());

How would it look like to use an @WithDefault for a list?

[geoand]

cc @metacosm

[gsmet]

This looks like a very interesting contribution, thanks. @metacosm as @geoand mentioned, we are interested in your feedback!

[LarsSven]

I rebased it against main since there were quite a few big changes in the dev service class in main that conflicted with this change.

[metacosm]

This looks like a very interesting contribution, thanks. @metacosm as @geoand mentioned, we are interested in your feedback!

Yes, started looking at it, will try to finish the review tomorrow.

[metacosm]

OK, so initial comments: I would separate the deploy phase into a separate build step. Look at

quarkus/extensions/kubernetes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/KubernetesDeployer.java

Line 116 in 6316ec2

public void deploy(KubernetesClientBuildItem kubernetesClientBuilder,

for example (in particular, look into using KubernetesClientBuildItem). This way, you wouldn't have to deal with the client configuration and the deploy step would be done at an appropriate step (at least, I think, I haven't checked).

[LarsSven]

I've never written a Quarkus extension before so I've been reading through the extension documentation. I get the concept of BuildSteps and the dependencies between them, but I don't quite get how to get a BuildStep to run if no other BuildStep consumes it (since this BuildStep is more a followup of the Dev Service creation).

Obviously this doesn't work:

    @BuildStep
    public void deployManifests(KubernetesClientBuildItem kubernetesClientBuilder) {
    }

But when I follow the error message

does not produce any build item and thus will never get executed. 
Either change the return type of the method to a build item type, add a parameter of type BuildProducer<[some build item type]>/Consumer<[some build item type]>, or annotate the method with @Produces. Use @Produce(ArtifactResultBuildItem.class) if you want to always execute this step.

and do:

    @Produce(ArtifactResultBuildItem.class)
    @BuildStep
    public void deployManifests(KubernetesClientBuildItem kubernetesClientBuilder) {
        log.info("Deploying manifests...");
    }

It no longer crashes but doesn't run.

Also how do I add a depenendency on the previous setupKubernetesDevService BuildStep? Since it produces a generic DevServicesResultBuildItem, how can I ensure the BuildStep runs specifically after the setupKubernetesDevService when it doesn't return a specific KubernetesDevService build item?

[LarsSven]

@gsmet @geoand @metacosm I can figure out most of it out myself I think, but how would I make sure the BuildStep runs if it has no other BuildSteps consuming it? I just want a BuildStep that runs after another BuildStep but does not have any BuildStep consuming it itself.

[geoand]

Does

    @BuildStep
    public void deployManifests(KubernetesClientBuildItem kubernetesClientBuilder, BuildProducer<ArtifactResultBuildItem> fakeProducer) {
        log.info("Deploying manifests...");
    }

work?

[metacosm]

Since the dev service configuration step produces KubernetesDevServiceInfoBuildItem, the step that deploys the manifests should probably consume it (even if it does nothing with it) to make sure it runs after. Producing ArtifactResultBuildItem should indeed ensure your steps runs even if it doesn't produce anything that another step would consume.

[LarsSven]

I am sorry for being such a nuisance and not understanding the build steps framework, but I added the following code to the project to the DevServicesKubernetesProcessor class:

@BuildStep
public void deployManifests(
        KubernetesDevServiceInfoBuildItem kubernetesDevServiceInfoBuildItem,
        BuildProducer<ArtifactResultBuildItem> fakeProducer) {
    log.info("Deploying manifests...");
}

This still doesn't do anything. It doesn't matter if I just take the BuildProducer as the only parameter, or if I take the direct example graciously provided by @geoand. I simply never see this log message appear even though I see the log message from the other build step within the same class appear.

[LarsSven]

I've pushed the new build step to this MR if anyone would like to take a look? I can deal with all the code, tests and documentation surrounding this MR, I just need the new build step to actually run, which I am really struggling with because I am missing something about how this all works.

I did check that none of the class level IfOnly/IfOnlyNots were blocking the new build step, but that doesn't seem to be the case.

[metacosm]

I am sorry for being such a nuisance and not understanding the build steps framework…

You're not being a nuisance. How build steps work is not always as easy or intuitive as we think they might be. Don't worry about it. I will take a look and see if we can make it work! 😅

[LarsSven]

You're not being a nuisance. How build steps work is not always as easy or intuitive as we think they might be. Don't worry about it. I will take a look and see if we can make it work! 😅

Thank you, I appreciate the help :)

[geoand]

I just tried this and the reason why

    @BuildStep
    public void deployManifests(
            KubernetesDevServiceInfoBuildItem kubernetesDevServiceInfoBuildItem,
            BuildProducer<ArtifactResultBuildItem> fakeProducer) {
        log.info("Deploying manifests...");
    }

doesn't work in mvn quarkus:dev is because in Dev mode we don't consume ArtifactResultBuildItem at all.
If you really want to have this work in Dev mode as well, you can use ServiceStartBuildItem instead.

[metacosm]

Ah, this is what I was starting to wonder about because, while debugging, I didn't see anything producing ArtifactResultBuildItem being added to the build steps. Probably something we should document as well for extension developers…

[LarsSven]

@metacosm @geoand I switched the manifests deployment over to a separate build step. However, using the KubernetesClientBuildItem did not work because the configuration of the built client is not at all aware of the dev service. It just points to the Kubernetes default endpoint (kubernetes.default.svc rather than the actual dev service being spun up. So I had to keep the existing manual configuration of the client based on the dev service.

[LarsSven]

Things that I still need to do:

• Tests
• Documentation
• Test against test, dev and normal profile

[metacosm]

@metacosm @geoand I switched the manifests deployment over to a separate build step. However, using the KubernetesClientBuildItem did not work because the configuration of the built client is not at all aware of the dev service. It just points to the Kubernetes default endpoint (kubernetes.default.svc rather than the actual dev service being spun up. So I had to keep the existing manual configuration of the client based on the dev service.

This is probably something that we should look into, the client build item should probably take the dev service into account.

[metacosm]

Ah, this is what I was starting to wonder about because, while debugging, I didn't see anything producing ArtifactResultBuildItem being added to the build steps. Probably something we should document as well for extension developers…

Created #48748

[LarsSven]

I manually tested this against the normal, test and dev profiles and it works as expected. Some automated tests seem to be all that's left.

[LarsSven]

I think we could also use a reduction in the number of commits that go into this change

Will squash to a single commit once I'm fully happy with the PR 👍

[LarsSven]

@holly-cummins how am I supposed to run the DevServicesKubernetesITest? I'm trying to do what the contributing guide says which is ./mvnw test -f integration-tests/kubernetes-client-devservices/, but that doesn't start up any test from the class.

Edit: I figured out I need to add the -Dtest-containers profile

[LarsSven]

I migrated the test over to an integration test and rebased the commits. I am happy with the PR in its current state. This is the only comment that I think may have a nicer solution still: #48536 (comment)

[metacosm]

Nit picking but maybe this check could be done before so that the "internal" config is not re-created if there are no manifests present? (Sorry, should have noticed this before)

[LarsSven]

No worries, that's indeed a good point. I moved this to the top.

[LarsSven]

No wait that doesn't work, because the code is now:

if (cfg.manifests.isEmpty())
  return;

if (cfg == null)
  cfg = getConfiguration(kubernetesClientBuildTimeConfig);

Which throws an NPE if cfg == null

[LarsSven]

I went back to the old approach

[metacosm]

Sorry wasn't clear enough, indeed doing it that way would cause an NPE. What I meant was to check fro the presence of the manifests directly in KubernetesDevServicesBuildTimeConfig, which I thought was injected in the method already. That said, thinking about it some more, it should probably be injected instead of using cfg because that internal object is only used to check for equality of a potentially already present one. In this step, we only need to get the manifests list from the configuration, and we do not care about the rest. Things definitely work the way they currently are, just doing extra unneeded work. Could definitely be addressed in a subsequent PR if you'd rather be done with this one, which I could understand… 😅

[LarsSven]

No worries at all, I'm more than happy to improve this PR before merging it.

The manifests are already available from the KubernetesClientBuildConfig through kubernetesClientBuildTimeConfig.devservices().manifests(), so I used that rather than also injecting KubernetesDevServicesBuildTimeConfig. I hope that's an okay solution? I pushed it.

[LarsSven]

@metacosm it is ready for another round of review :)

[metacosm]

I don't think this check is needed since this step won't be called if the build item is not produced.

[LarsSven]

Quarkus acts really weird around starting up a Quarkus dev service when the containers from its previous run are still in the process of terminating. I've seen this happen a few times now.

If this happens, we already get other exceptions anyways, but I'd rather not forward a "KubernetesDevServiceInfoBuildIte is null" exception to the users if this happens, because it'll be confusing.

[metacosm]

Ah, ok. Wasn't aware of this. Thanks for the explanation, and, indeed, it makes more sense to have some information instead of a simple NPE.

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 44d1530.

✅ 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.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 44d1530.

✅ 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.