Add dev services for kubernetes #28305
Conversation
|
Thanks for your pull request! The title of your pull request does not follow our editorial rules. Could you have a look?
|
scrocquesel
changed the title
scrocquesel
force-pushed
the
gh-28189
branch
3 times, most recently
from
fd12643 to
922af72
Compare
|
1/, I find out how to disable dev services when quarkus-test-kubernetes-client is imported. For now, it is one or another. |
scrocquesel
force-pushed
the
gh-28189
branch
2 times, most recently
from
11e2723 to
1328373
Compare
This comment has been minimized.
This comment has been minimized.
|
Thanks for this! We'll have to take a close look and get back to you. |
| <dependency> | ||
| <groupId>com.dajudge.kindcontainer</groupId> | ||
| <artifactId>kindcontainer</artifactId> | ||
| <version>1.3.0</version> |
There was a problem hiding this comment.
I think all the versions we provide should be declared in bom/application/pom.xml
There was a problem hiding this comment.
Does this work on all platforms?
There was a problem hiding this comment.
Yes, but AFAIK it can't be tested with windows container, so test should be disable in Windows test profile.
There was a problem hiding this comment.
Apart from my comments, I think we should cover the new dev services as part of the existing
integration-tests/kubernetes-clientmodule. There is an internal testing framework we can use for this (see@QuarkusDevModeTest).
@Sgitario integration-tests/kubernetes-client use quarkus-test-kubernetes-client and its AbstractKubernetesTestResource which already set fabric8 systemProps and dev services may interfere. Moreover, from my understanding, @QuarkusDevModeTest start the apps in Dev Mode after dev services already starts in Test mode as part of the buildStep.
| import java.util.Arrays; | ||
| import java.util.function.BooleanSupplier; | ||
|
|
||
| class NoQuarkusTestKubernetesClient implements BooleanSupplier { |
There was a problem hiding this comment.
I don't see what is the purpose of this logic, can you please clarify?
There was a problem hiding this comment.
This refer to the first point in description. io.quarkus.test.kubernetes.client package contains all quarkus mock server test class used to actually mock kubernetes. This is the package that I think most people use to write unit test. It is not compatible with dev services because they barely do the same thing to configure a KubernetesClient to connect to the test server. Dev services wins because quarkus properties override the system props properties, but it can't detect it because systemProps are set latter by a test resource class. Thus, all existing unit test would fail if we don't disable dev services when the mock package is used.
There was a problem hiding this comment.
I think that we need a couple of things here:
- a more descriptive name
- a comment that explain(s) why?
- documentation
- (optionally) create a feature requrest to allow for multiple clients with qualifiers.
There was a problem hiding this comment.
Added documentation and guide. First time doing this.
| .orElseGet(() -> latest(KindContainerVersion.class))); | ||
| break; | ||
| default: | ||
| throw new RuntimeException(); |
There was a problem hiding this comment.
We should throw a descriptive message here.
| user.getClientCertificateData(), | ||
| "quarkus.kubernetes-client.client-key-data", user.getClientKeyData(), | ||
| "quarkus.kubernetes-client.client-key-algo", Config.getKeyAlgorithm(null, user.getClientKeyData()), | ||
| "quarkus.kubernetes-client.namespace", "default"); |
There was a problem hiding this comment.
Are we always using the "default" namespace on purpose? This can be set from kubeconfig as well.
There was a problem hiding this comment.
Actually, the test container starts with only the default namespace. We may support additional namespace by adding additional devservices config. But it can be done in a second step, there is a lot that can be configured at startup like applying yaml resource.
| return new KubernetesDevServiceCfg(devServicesConfig); | ||
| } | ||
|
|
||
| private static final class KubernetesDevServiceCfg { |
There was a problem hiding this comment.
I never implemented a dev service, but why do we need this class instead of using directly KubernetesClientBuildConfig?
There was a problem hiding this comment.
I reuse the logic from elsewhere. The difference is that the class must implements equals to be able to detect changes.
|
|
||
| public Map<String, String> getKubeconfig() { | ||
| var image = getContainerInfo().getConfig().getImage(); | ||
| if (image.contains("rancher/k3s")) { |
There was a problem hiding this comment.
shouldn't we use the flavor instead of the image to do this logic?
There was a problem hiding this comment.
Unfortunately, when attaching to a running container we can just describe the container and do some reverse logic to guess what dev services config was used to actually build it. I think of image name or a label applied at creation and choose the former.
There was a problem hiding this comment.
In theory one could bring their own image, so generally a label is preferable.
| return getKubernetesClientConfigFromKubeConfig(getKubeconfigFromApiContainer(containerAddress.getUrl())); | ||
| } | ||
|
|
||
| throw new RuntimeException(); |
There was a problem hiding this comment.
I think this logic should be moved into here: https://github.com/quarkusio/quarkus/pull/28305/files#diff-1f1a987103950b5c6975b74afefa56dea0d2f9a5e592db110493311faf2309abR195
There was a problem hiding this comment.
@Sgitario I don't see where the link point at.
quarkus-bot
bot
added
the
area/dependencies
There was a problem hiding this comment.
Overall looks good.
I am a bit sceptic about the handling of the conflict with the mock client.
Maybe the simplest solution would be to just log a warning and if user decides to disable the devservices. That might be simpler / cleaner. I don't mind the current approach as long as we document it.
So both ways are fine by me.
| import java.util.Arrays; | ||
| import java.util.function.BooleanSupplier; | ||
|
|
||
| class NoQuarkusTestKubernetesClient implements BooleanSupplier { |
There was a problem hiding this comment.
I think that we need a couple of things here:
- a more descriptive name
- a comment that explain(s) why?
- documentation
- (optionally) create a feature requrest to allow for multiple clients with qualifiers.
|
One last blocking issue, I can't find out how to not run integration test for JDK 11 Windows Container job. It fails because docker is not available in those jobs. Help wanted. |
This comment has been minimized.
This comment has been minimized.
@geoand? Any ideas? |
|
I'll have a look later on |
Not sure what you mean here. We intentionally don't run container related tests on Windows. Is there a test that is running that shouldn't be? |
That's what I observed on CI but not locally see https://github.com/scrocquesel/quarkus/actions/runs/3256150273 |
The test that should not run are those added by this PR |
|
I still don't see which Kubernetes tests you say are failing on the Windows run. |
You are right. As the job keep failing, I thought it was the new test. |
|
Can you please rebase onto main so we can be sure nothing has broken in the meantime? |
geoand
added
triage/waiting-for-ci
This comment has been minimized.
This comment has been minimized.
Failing Jobs - Building 4f32580
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 Windows #- Failing: extensions/opentelemetry/deployment
! Skipped: integration-tests/micrometer-prometheus integration-tests/opentelemetry integration-tests/opentelemetry-grpc and 5 more📦 extensions/opentelemetry/deployment✖ ⚙️ JVM Tests - JDK 18 #- Failing: integration-tests/elytron-undertow
📦 integration-tests/elytron-undertow✖ |
quarkus-bot
bot
added
kind/enhancement
|
Hmmm, I think it's too big of a behavioral change to get backported post CR1, especially since I saw some CI failures that might be related to it. Let's give it some bake time. |
These tests are executed in the Windows machine where there is no Docker instance running. Related to the changes in quarkusio#28305 Related to CI failures like https://github.com/quarkusio/quarkus/runs/9165337690#user-content-test-failure-org.acme.applicationconfigresourcetest-1
| //// | ||
| = Dev Services for Kubernetes | ||
| include::_attributes.adoc[] | ||
| :categories: messaging |
There was a problem hiding this comment.
A little late for this but I suspect this category isn't correct…
There was a problem hiding this comment.
@metacosm I will have a look thanks
There was a problem hiding this comment.
My bad. Should be cloud I guess like other k8s pages.
Closes #28189
Some design advices are more than welcome to adjust some behavior of dev service for kubernetes:
Currently some integration tests like
openshift-clientuse k8s mock server provided byAbstractKubernetesTestResource. This set systemProps to be read by fabric8 client and return config properties with key of the same key name. Thekubernetes-clientextension override those values withquarkus.*properties if any. The issue is that Dev services return the configuration as quarkus properties and if not disabled, it will override the configuration to connect to the Mock Server. Disabling dev services when mock server is used seems not doable.I guess a lot of people are using the Mock Server do to their tests and would experience a breaking change.
Should dev service be disabled by default ?
Should
AbstractKubernetesTestResourcebe updated to return ``quarkus.` prefixed properties instead of fabric8 systemProps properties (not sure if this would work) ?Should we disable dev services manually in those project assuming the breaking change in behavior ?
dev services for Kubernetes do its best to detect if a kubernetes configuration is defined so that the developer or test runner can use its own current kubeconfig. While, I think this is a valid default use case, it is not so easy to force dev services to start a cluster. This differs from how other dev services work because there is generally no such thing as a config file in home directory or env var for client of databases/message broker/..., and it should be rare a user defines
master-urlmanually to disable dev services.Quarkus extension first build a fabric8 Config object and call autoconfigure on it inconditionally. If a kubeconfig is found,
master-urlwill be set. Therefore, user needs to pass-Dkubernetes.auth.tryKubeConfig=falsewhen running test or dev mode to bypass any kubeconfig present in home directory. Dev services use same logic to guess if master-url would be set at runtime and decide if a cluster should be started.In my case, and I think some user will be in the same case, I have a valid k8s cluster but still would prefer to use dev service cluster to isolate dev and tests and have automatic cleanup when stopping dev mode.
Should a
quarkus.kubernetes-client.auto-configure(or quarkus.kubernetes-client.devservices.override-kubeconfig`) be added to disable the fabric8 autoconfigure behavior ? At least, this property could be set in properties or dotenv file.Regarding docker image name, current testcontainer implementation used to start a api server only cluster do not allow to pass a docker image name for the api server and etcd containers. It is possible to specify a version if it is supported by the package (see https://github.com/dajudge/kindcontainer/blob/master/k8s-versions.json). If a user needs to change the registry name/path, it must provide a testcontainer image name substitutor. Do you have any concern about this ?