Skip to content

Commit

Permalink
Fix randomly failures when deploying to OpenShift/K8s on JVM/Native
Browse files Browse the repository at this point in the history
There were several places where a new KubernetesClient was being created:
- Some places use `Clients.fromConfig`.
- Other places use `KubernetesClientUtils.createConfig`.
- And other places use `KubernetesClientBuildItem.getClient`

And the problem is that every time we invoke any of these methods, Fabric8 KubernetesClient tries to locate one HttpClient.Factory and it seems that the logic to get one HttpClient.Factory sometimes gets the `VertxHttpClientFactory` implementation over the expected one `QuarkusHttpClientFactory`

With this solution, it avoids to find a HttpClient.Factory using the ServiceLoader logic in Fabric8 Kubernetes Client, but it provides the expected `QuarkusHttpClientFactory` implementation always.

Fix quarkusio#31476
  • Loading branch information
Sgitario committed Mar 3, 2023
1 parent 9579d9d commit 928266d
Show file tree
Hide file tree
Showing 11 changed files with 84 additions and 55 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,17 @@

import org.jboss.logging.Logger;

import io.dekorate.utils.Clients;
import io.dekorate.utils.Packaging;
import io.dekorate.utils.Serialization;
import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.KubernetesList;
import io.fabric8.kubernetes.api.model.Secret;
import io.fabric8.kubernetes.client.Config;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.KubernetesClientBuilder;
import io.fabric8.kubernetes.client.KubernetesClientException;
import io.fabric8.kubernetes.client.dsl.LogWatch;
import io.fabric8.kubernetes.client.http.HttpClient;
import io.fabric8.openshift.api.model.Build;
import io.fabric8.openshift.api.model.BuildConfig;
import io.fabric8.openshift.api.model.ImageStream;
Expand Down Expand Up @@ -231,7 +232,7 @@ public void openshiftRequirementsNative(OpenshiftConfig openshiftConfig,
public void openshiftBuildFromJar(OpenshiftConfig openshiftConfig,
S2iConfig s2iConfig,
ContainerImageConfig containerImageConfig,
KubernetesClientBuildItem kubernetesClientSupplier,
KubernetesClientBuildItem kubernetesClientBuilder,
ContainerImageInfoBuildItem containerImage,
ArchiveRootBuildItem archiveRoot, OutputTargetBuildItem out, PackageConfig packageConfig,
List<GeneratedFileSystemResourceBuildItem> generatedResources,
Expand Down Expand Up @@ -263,7 +264,7 @@ public void openshiftBuildFromJar(OpenshiftConfig openshiftConfig,
return;
}

try (KubernetesClient kubernetesClient = kubernetesClientSupplier.getClient().get()) {
try (KubernetesClient kubernetesClient = kubernetesClientBuilder.buildClient()) {
String namespace = Optional.ofNullable(kubernetesClient.getNamespace()).orElse("default");
LOG.info("Starting (in-cluster) container image build for jar using: " + config.buildStrategy + " on server: "
+ kubernetesClient.getMasterUrl() + " in namespace:" + namespace + ".");
Expand All @@ -272,14 +273,16 @@ public void openshiftBuildFromJar(OpenshiftConfig openshiftConfig,
//For s2i kind of builds where jars are expected directly in the '/' we have to use null.
String outputDirName = out.getOutputDirectory().getFileName().toString();
String contextRoot = getContextRoot(outputDirName, packageConfig.isFastJar(), config.buildStrategy);
KubernetesClientBuilder clientBuilder = newClientBuilderWithoutHttp2(kubernetesClient.getConfiguration(),
kubernetesClientBuilder.getHttpClientFactory());
if (packageConfig.isFastJar()) {
createContainerImage(kubernetesClient, openshiftYml.get(), config, contextRoot, jar.getPath().getParent(),
createContainerImage(clientBuilder, openshiftYml.get(), config, contextRoot, jar.getPath().getParent(),
jar.getPath().getParent());
} else if (jar.getLibraryDir() != null) { //When using uber-jar the libraryDir is going to be null, potentially causing NPE.
createContainerImage(kubernetesClient, openshiftYml.get(), config, contextRoot, jar.getPath().getParent(),
createContainerImage(clientBuilder, openshiftYml.get(), config, contextRoot, jar.getPath().getParent(),
jar.getPath(), jar.getLibraryDir());
} else {
createContainerImage(kubernetesClient, openshiftYml.get(), config, contextRoot, jar.getPath().getParent(),
createContainerImage(clientBuilder, openshiftYml.get(), config, contextRoot, jar.getPath().getParent(),
jar.getPath());
}
artifactResultProducer.produce(new ArtifactResultBuildItem(null, "jar-container", Collections.emptyMap()));
Expand All @@ -300,7 +303,7 @@ private String getContextRoot(String outputDirName, boolean isFastJar, BuildStra
@BuildStep(onlyIf = { IsNormalNotRemoteDev.class, OpenshiftBuild.class, NativeBuild.class })
public void openshiftBuildFromNative(OpenshiftConfig openshiftConfig, S2iConfig s2iConfig,
ContainerImageConfig containerImageConfig,
KubernetesClientBuildItem kubernetesClientSupplier,
KubernetesClientBuildItem kubernetesClientBuilder,
ContainerImageInfoBuildItem containerImage,
ArchiveRootBuildItem archiveRoot, OutputTargetBuildItem out, PackageConfig packageConfig,
List<GeneratedFileSystemResourceBuildItem> generatedResources,
Expand All @@ -321,7 +324,7 @@ public void openshiftBuildFromNative(OpenshiftConfig openshiftConfig, S2iConfig
return;
}

try (KubernetesClient kubernetesClient = kubernetesClientSupplier.getClient().get()) {
try (KubernetesClient kubernetesClient = kubernetesClientBuilder.buildClient()) {
String namespace = Optional.ofNullable(kubernetesClient.getNamespace()).orElse("default");

LOG.info("Starting (in-cluster) container image build for jar using: " + config.buildStrategy + " on server: "
Expand All @@ -340,14 +343,16 @@ public void openshiftBuildFromNative(OpenshiftConfig openshiftConfig, S2iConfig
//For docker kind of builds where we use instructions like: `COPY target/*.jar /deployments` it using '/target' is a requirement.
//For s2i kind of builds where jars are expected directly in the '/' we have to use null.
String contextRoot = config.buildStrategy == BuildStrategy.DOCKER ? "target" : null;
createContainerImage(kubernetesClient, openshiftYml.get(), config, contextRoot, out.getOutputDirectory(),
nativeImage.getPath());
createContainerImage(
newClientBuilderWithoutHttp2(kubernetesClient.getConfiguration(),
kubernetesClientBuilder.getHttpClientFactory()),
openshiftYml.get(), config, contextRoot, out.getOutputDirectory(), nativeImage.getPath());
artifactResultProducer.produce(new ArtifactResultBuildItem(null, "native-container", Collections.emptyMap()));
containerImageBuilder.produce(new ContainerImageBuilderBuildItem(OPENSHIFT));
}
}

public static void createContainerImage(KubernetesClient kubernetesClient,
public static void createContainerImage(KubernetesClientBuilder kubernetesClientBuilder,
GeneratedFileSystemResourceBuildItem openshiftManifests,
OpenshiftConfig openshiftConfig,
String base,
Expand All @@ -364,10 +369,7 @@ public static void createContainerImage(KubernetesClient kubernetesClient,
throw new RuntimeException("Error creating the openshift binary build archive.", e);
}

Config config = kubernetesClient.getConfiguration();
//Let's disable http2 as it causes issues with duplicate build triggers.
config.setHttp2Disable(true);
try (KubernetesClient client = Clients.fromConfig(config)) {
try (KubernetesClient client = kubernetesClientBuilder.build()) {
OpenShiftClient openShiftClient = toOpenshiftClient(client);
KubernetesList kubernetesList = Serialization
.unmarshalAsList(new ByteArrayInputStream(openshiftManifests.getData()));
Expand Down Expand Up @@ -533,6 +535,14 @@ private static void display(Reader logReader, Logger.Level level) throws IOExcep
}
}

private static KubernetesClientBuilder newClientBuilderWithoutHttp2(Config configuration,
HttpClient.Factory httpClientFactory) {
//Let's disable http2 as it causes issues with duplicate build triggers.
configuration.setHttp2Disable(true);

return new KubernetesClientBuilder().withConfig(configuration).withHttpClientFactory(httpClientFactory);
}

// visible for test
static String concatUnixPaths(String... elements) {
StringBuilder result = new StringBuilder();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ public void s2iRequirementsNative(S2iConfig s2iConfig,

@BuildStep(onlyIf = { IsNormalNotRemoteDev.class, S2iBuild.class }, onlyIfNot = NativeBuild.class)
public void s2iBuildFromJar(S2iConfig s2iConfig, ContainerImageConfig containerImageConfig,
KubernetesClientBuildItem kubernetesClientSupplier,
KubernetesClientBuildItem kubernetesClientBuilder,
ContainerImageInfoBuildItem containerImage,
ArchiveRootBuildItem archiveRoot, OutputTargetBuildItem out, PackageConfig packageConfig,
List<GeneratedFileSystemResourceBuildItem> generatedResources,
Expand Down Expand Up @@ -201,7 +201,7 @@ public void s2iBuildFromJar(S2iConfig s2iConfig, ContainerImageConfig containerI
"No Openshift manifests were generated so no s2i process will be taking place");
return;
}
try (KubernetesClient kubernetesClient = kubernetesClientSupplier.getClient().get()) {
try (KubernetesClient kubernetesClient = kubernetesClientBuilder.buildClient()) {
String namespace = Optional.ofNullable(kubernetesClient.getNamespace()).orElse("default");
LOG.info("Performing s2i binary build with jar on server: " + kubernetesClient.getMasterUrl()
+ " in namespace:" + namespace + ".");
Expand All @@ -215,7 +215,7 @@ public void s2iBuildFromJar(S2iConfig s2iConfig, ContainerImageConfig containerI

@BuildStep(onlyIf = { IsNormalNotRemoteDev.class, S2iBuild.class, NativeBuild.class })
public void s2iBuildFromNative(S2iConfig s2iConfig, ContainerImageConfig containerImageConfig,
KubernetesClientBuildItem kubernetesClientSupplier,
KubernetesClientBuildItem kubernetesClientBuilder,
ContainerImageInfoBuildItem containerImage,
ArchiveRootBuildItem archiveRoot, OutputTargetBuildItem out, PackageConfig packageConfig,
List<GeneratedFileSystemResourceBuildItem> generatedResources,
Expand All @@ -234,7 +234,7 @@ public void s2iBuildFromNative(S2iConfig s2iConfig, ContainerImageConfig contain
return;
}

try (KubernetesClient kubernetesClient = kubernetesClientSupplier.getClient().get()) {
try (KubernetesClient kubernetesClient = kubernetesClientBuilder.buildClient()) {
String namespace = Optional.ofNullable(kubernetesClient.getNamespace()).orElse("default");
LOG.info("Performing s2i binary build with native image on server: " + kubernetesClient.getMasterUrl()
+ " in namespace:" + namespace + ".");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package io.quarkus.kubernetes.client.deployment;

import static io.quarkus.kubernetes.client.runtime.KubernetesClientUtils.*;
import static io.quarkus.kubernetes.client.runtime.KubernetesClientUtils.createConfig;

import io.quarkus.deployment.annotations.BuildStep;
import io.quarkus.deployment.builditem.QuarkusBuildCloseablesBuildItem;
import io.quarkus.kubernetes.client.runtime.KubernetesClientBuildConfig;
import io.quarkus.kubernetes.client.runtime.QuarkusHttpClientFactory;
import io.quarkus.kubernetes.client.spi.KubernetesClientBuildItem;
import io.quarkus.runtime.TlsConfig;

Expand All @@ -12,7 +14,9 @@ public class KubernetesClientBuildStep {
private KubernetesClientBuildConfig buildConfig;

@BuildStep
public KubernetesClientBuildItem process(TlsConfig tlsConfig) {
return new KubernetesClientBuildItem(createConfig(buildConfig, tlsConfig));
public KubernetesClientBuildItem process(TlsConfig tlsConfig, QuarkusBuildCloseablesBuildItem closeablesBuildItem) {
QuarkusHttpClientFactory httpClientFactory = new QuarkusHttpClientFactory();
closeablesBuildItem.add(httpClientFactory);
return new KubernetesClientBuildItem(createConfig(buildConfig, tlsConfig), httpClientFactory);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import static io.vertx.core.spi.resolver.ResolverProvider.DISABLE_DNS_RESOLVER_PROP_NAME;

import java.io.Closeable;

import jakarta.enterprise.inject.spi.CDI;

import io.fabric8.kubernetes.client.Config;
Expand All @@ -10,17 +12,20 @@
import io.vertx.core.Vertx;
import io.vertx.core.VertxOptions;

public class QuarkusHttpClientFactory implements HttpClient.Factory {
public class QuarkusHttpClientFactory implements HttpClient.Factory, Closeable {

private Vertx vertx;
private boolean closeVertxOnExit;

public QuarkusHttpClientFactory() {
// The client might get initialized outside a Quarkus context that can provide the Vert.x instance
// This is the case for the MockServer / @KubernetesTestServer where the server provides the KubernetesClient instance
try {
this.vertx = CDI.current().select(Vertx.class).get();
this.closeVertxOnExit = false;
} catch (Exception e) {
this.vertx = createVertxInstance();
this.closeVertxOnExit = true;
}
}

Expand Down Expand Up @@ -59,4 +64,11 @@ public VertxHttpClientBuilder<QuarkusHttpClientFactory> newBuilder() {
public int priority() {
return 1;
}

@Override
public void close() {
if (closeVertxOnExit) {
vertx.close();
}
}
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,26 +1,30 @@
package io.quarkus.kubernetes.client.spi;

import java.util.function.Supplier;

import io.fabric8.kubernetes.client.Config;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.KubernetesClientBuilder;
import io.fabric8.kubernetes.client.http.HttpClient;
import io.quarkus.builder.item.SimpleBuildItem;

public final class KubernetesClientBuildItem extends SimpleBuildItem {

private final Config config;
private final HttpClient.Factory httpClientFactory;

public KubernetesClientBuildItem(Config config) {
public KubernetesClientBuildItem(Config config, HttpClient.Factory httpClientFactory) {
this.config = config;
}

public Supplier<KubernetesClient> getClient() {
return () -> new KubernetesClientBuilder().withConfig(config).build();
this.httpClientFactory = httpClientFactory;
}

public Config getConfig() {
return config;
}

public HttpClient.Factory getHttpClientFactory() {
return httpClientFactory;
}

public KubernetesClient buildClient() {
return new KubernetesClientBuilder().withConfig(config).withHttpClientFactory(httpClientFactory).build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,14 @@ public class KnativeDeployer {
@BuildStep
public void checkEnvironment(Optional<SelectedKubernetesDeploymentTargetBuildItem> selectedDeploymentTarget,
List<GeneratedKubernetesResourceBuildItem> resources,
KubernetesClientBuildItem clientSupplier, BuildProducer<KubernetesDeploymentClusterBuildItem> deploymentCluster) {
KubernetesClientBuildItem kubernetesClientBuilder,
BuildProducer<KubernetesDeploymentClusterBuildItem> deploymentCluster) {
selectedDeploymentTarget.ifPresent(target -> {
if (!KubernetesDeploy.INSTANCE.checkSilently()) {
if (!KubernetesDeploy.INSTANCE.checkSilently(kubernetesClientBuilder)) {
return;
}
if (target.getEntry().getName().equals(KNATIVE)) {
try (DefaultKnativeClient client = clientSupplier.getClient().get().adapt(DefaultKnativeClient.class)) {
try (DefaultKnativeClient client = kubernetesClientBuilder.buildClient().adapt(DefaultKnativeClient.class)) {
if (client.isSupported()) {
deploymentCluster.produce(new KubernetesDeploymentClusterBuildItem(KNATIVE));
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.VersionInfo;
import io.quarkus.kubernetes.client.runtime.KubernetesClientUtils;
import io.quarkus.kubernetes.client.spi.KubernetesClientBuildItem;

public class KubernetesDeploy {

Expand All @@ -27,8 +27,8 @@ private KubernetesDeploy() {
*
* @throws RuntimeException if there was an error while communicating with the Kubernetes API server
*/
public boolean check() {
Result result = doCheck();
public boolean check(KubernetesClientBuildItem clientBuilder) {
Result result = doCheck(clientBuilder);

if (result.getException().isPresent()) {
throw result.getException().get();
Expand All @@ -41,11 +41,11 @@ public boolean check() {
* @return {@code true} if @{code quarkus.kubernetes.deploy=true} AND the target Kubernetes API server is reachable
* {@code false} otherwise or if there was an error while communicating with the Kubernetes API server
*/
public boolean checkSilently() {
return doCheck().isAllowed();
public boolean checkSilently(KubernetesClientBuildItem clientBuilder) {
return doCheck(clientBuilder).isAllowed();
}

private Result doCheck() {
private Result doCheck(KubernetesClientBuildItem clientBuilder) {
if (!KubernetesConfigUtil.isDeploymentEnabled()) {
return Result.notConfigured();
}
Expand All @@ -55,9 +55,8 @@ private Result doCheck() {
return Result.enabled();
}

KubernetesClient client = KubernetesClientUtils.createClient();
String masterURL = client.getConfiguration().getMasterUrl();
try {
String masterURL = clientBuilder.getConfig().getMasterUrl();
try (KubernetesClient client = clientBuilder.buildClient()) {
//Let's check if we can connect.
VersionInfo version = client.getVersion();
if (version == null) {
Expand All @@ -83,8 +82,6 @@ private Result doCheck() {
+ masterURL + "'",
e));
}
} finally {
client.close();
}
}

Expand Down
Loading

0 comments on commit 928266d

Please sign in to comment.