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 2, 2023
1 parent 3ffbd93 commit 4cf11a7
Show file tree
Hide file tree
Showing 9 changed files with 66 additions and 51 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
Expand Up @@ -4,6 +4,7 @@

import io.quarkus.deployment.annotations.BuildStep;
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 @@ -13,6 +14,6 @@ public class KubernetesClientBuildStep {

@BuildStep
public KubernetesClientBuildItem process(TlsConfig tlsConfig) {
return new KubernetesClientBuildItem(createConfig(buildConfig, tlsConfig));
return new KubernetesClientBuildItem(createConfig(buildConfig, tlsConfig), new QuarkusHttpClientFactory());
}
}
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
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@

import org.jboss.logging.Logger;

import io.dekorate.utils.Clients;
import io.dekorate.utils.Serialization;
import io.fabric8.kubernetes.api.model.APIResourceList;
import io.fabric8.kubernetes.api.model.GenericKubernetesResource;
Expand Down Expand Up @@ -90,11 +89,11 @@ public void selectDeploymentTarget(ContainerImageInfoBuildItem containerImageInf

@BuildStep
public void checkEnvironment(Optional<SelectedKubernetesDeploymentTargetBuildItem> selectedDeploymentTarget,
KubernetesClientBuildItem client,
KubernetesClientBuildItem kubernetesClientBuilder,
List<GeneratedKubernetesResourceBuildItem> resources,
BuildProducer<KubernetesDeploymentClusterBuildItem> deploymentCluster) {

if (!KubernetesDeploy.INSTANCE.checkSilently()) {
if (!KubernetesDeploy.INSTANCE.checkSilently(kubernetesClientBuilder)) {
return;
}
String target = selectedDeploymentTarget.map(s -> s.getEntry().getName()).orElse(KUBERNETES);
Expand All @@ -104,7 +103,7 @@ public void checkEnvironment(Optional<SelectedKubernetesDeploymentTargetBuildIte
}

@BuildStep(onlyIf = IsNormalNotRemoteDev.class)
public void deploy(KubernetesClientBuildItem kubernetesClientSupplier,
public void deploy(KubernetesClientBuildItem kubernetesClientBuilder,
Capabilities capabilities,
List<KubernetesDeploymentClusterBuildItem> deploymentClusters,
Optional<SelectedKubernetesDeploymentTargetBuildItem> selectedDeploymentTarget,
Expand All @@ -117,7 +116,7 @@ public void deploy(KubernetesClientBuildItem kubernetesClientSupplier,
// needed to ensure that this step runs after the container image has been built
@SuppressWarnings("unused") List<ArtifactResultBuildItem> artifactResults) {

if (!KubernetesDeploy.INSTANCE.check()) {
if (!KubernetesDeploy.INSTANCE.check(kubernetesClientBuilder)) {
return;
}

Expand All @@ -134,7 +133,7 @@ public void deploy(KubernetesClientBuildItem kubernetesClientSupplier,
return;
}

try (final KubernetesClient client = Clients.fromConfig(kubernetesClientSupplier.getConfig())) {
try (final KubernetesClient client = kubernetesClientBuilder.buildClient()) {
deploymentResult
.produce(deploy(selectedDeploymentTarget.get().getEntry(), client, outputTarget.getOutputDirectory(),
openshiftConfig, applicationInfo, optionalResourceDefinitions));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@
import io.quarkus.deployment.IsNormalNotRemoteDev;
import io.quarkus.deployment.annotations.BuildProducer;
import io.quarkus.deployment.annotations.BuildStep;
import io.quarkus.kubernetes.client.spi.KubernetesClientBuildItem;

public class KubernetesDeployerPrerequisite {

@BuildStep(onlyIf = IsNormalNotRemoteDev.class)
public void prepare(ContainerImageInfoBuildItem containerImage,
KubernetesClientBuildItem kubernetesClientBuilder,
Optional<SelectedKubernetesDeploymentTargetBuildItem> selectedDeploymentTarget,
Optional<FallbackContainerImageRegistryBuildItem> fallbackRegistry,
List<PreventImplicitContainerImagePushBuildItem> preventImplicitContainerImagePush,
Expand All @@ -23,7 +25,7 @@ public void prepare(ContainerImageInfoBuildItem containerImage,

// we don't want to throw an exception at this step and fail the build because it could prevent
// the Kubernetes resources from being generated
if (!KubernetesDeploy.INSTANCE.checkSilently() || !selectedDeploymentTarget.isPresent()) {
if (!KubernetesDeploy.INSTANCE.checkSilently(kubernetesClientBuilder) || !selectedDeploymentTarget.isPresent()) {
return;
}

Expand Down
Loading

0 comments on commit 4cf11a7

Please sign in to comment.