Skip to content

drop loading image when its already present #1763

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

drop loading image when its already present #1763

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
255c8b7
test
wind57 Oct 15, 2024
d06a36f
checkstyle
wind57 Oct 15, 2024
f2f9d31
simpler
wind57 Oct 15, 2024
6452efe
simpler
wind57 Oct 15, 2024
9daeaf5
Merge branch '3.1.x' into no-load-for-it
wind57 Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...ingframework/cloud/kubernetes/fabric8/client/discovery/Fabric8DiscoveryPodMetadataIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,8 +279,8 @@ private static void manifests(Phase phase) {
ClusterRoleBinding clusterRoleBinding = Serialization.unmarshal(getAdminRole(), ClusterRoleBinding.class);
if (phase.equals(Phase.CREATE)) {
client.rbac().clusterRoleBindings().resource(clusterRoleBinding).create();
util.createAndWait(NAMESPACE, IMAGE_NAME, deployment, discoveryService, ingress, true);
util.createAndWait(NAMESPACE, null, null, externalServiceName, null, true);
util.createAndWait(NAMESPACE, deployment, discoveryService, ingress, true);
util.createAndWait(NAMESPACE, null, externalServiceName, null, true);
}
else {
client.rbac().clusterRoleBindings().resource(clusterRoleBinding).delete();
Expand Down
2 changes: 1 addition & 1 deletion ...c/test/java/org/springframework/cloud/kubernetes/fabric8/client/istio/Fabric8IstioIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ private static void appManifests(Phase phase) {
Ingress ingress = Serialization.unmarshal(ingressStream, Ingress.class);

if (phase.equals(Phase.CREATE)) {
util.createAndWait(NAMESPACE, null, deployment, service, ingress, true);
util.createAndWait(NAMESPACE, deployment, service, ingress, true);
}
else {
util.deleteAndWait(NAMESPACE, deployment, service, ingress);
Expand Down
2 changes: 1 addition & 1 deletion ...java/org/springframework/cloud/kubernetes/fabric8/client/reload/Fabric8EventReloadIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ private static void manifests(Phase phase) {
util.createAndWait("right", rightConfigMap, null);
util.createAndWait("right", rightWithLabelConfigMap, null);
util.createAndWait(NAMESPACE, configMap, secret);
util.createAndWait(NAMESPACE, null, deployment, service, ingress, true);
util.createAndWait(NAMESPACE, deployment, service, ingress, true);
}
else {
util.deleteAndWait("left", leftConfigMap, null);
Expand Down
2 changes: 1 addition & 1 deletion ...framework/cloud/kubernetes/k8s/client/catalog/watcher/KubernetesClientCatalogWatchIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ private static void app(Phase phase) {
V1Ingress ingress = (V1Ingress) util.yaml("app/watcher-ingress.yaml");

if (phase.equals(Phase.CREATE)) {
util.createAndWait(NAMESPACE, null, deployment, service, ingress, true);
util.createAndWait(NAMESPACE, deployment, service, ingress, true);
}
else if (phase.equals(Phase.DELETE)) {
util.deleteAndWait(NAMESPACE, deployment, service, ingress);
Expand Down
2 changes: 1 addition & 1 deletion ...st/java/org/springframework/cloud/kubernetes/configuration/watcher/ActuatorRefreshIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ private static void configWatcher(Phase phase) {

if (phase.equals(Phase.CREATE)) {
util.createAndWait(NAMESPACE, configMap, null);
util.createAndWait(NAMESPACE, null, deployment, service, null, true);
util.createAndWait(NAMESPACE, deployment, service, null, true);
}
else {
util.deleteAndWait(NAMESPACE, configMap, null);
Expand Down
2 changes: 1 addition & 1 deletion ...framework/cloud/kubernetes/configuration/watcher/ActuatorRefreshMultipleNamespacesIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ private static void configWatcher(Phase phase) {

if (phase.equals(Phase.CREATE)) {
util.createAndWait(DEFAULT_NAMESPACE, configMap, null);
util.createAndWait(DEFAULT_NAMESPACE, null, deployment, service, null, true);
util.createAndWait(DEFAULT_NAMESPACE, deployment, service, null, true);
}
else {
util.deleteAndWait(DEFAULT_NAMESPACE, configMap, null);
Expand Down
4 changes: 2 additions & 2 deletions .../test/java/org/springframework/cloud/kubernetes/discoveryclient/it/DiscoveryClientIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ private static void discoveryClient(Phase phase) {
V1Ingress ingress = (V1Ingress) util.yaml("ingress.yaml");

if (phase.equals(Phase.CREATE)) {
util.createAndWait(NAMESPACE, null, deployment, service, ingress, true);
util.createAndWait(NAMESPACE, deployment, service, ingress, true);
}
else {
util.deleteAndWait(NAMESPACE, deployment, service, ingress);
Expand All @@ -299,7 +299,7 @@ private static void discoveryServer(Phase phase) {
V1Service service = (V1Service) util.yaml("server/spring-cloud-kubernetes-discoveryserver-service.yaml");

if (phase.equals(Phase.CREATE)) {
util.createAndWait(NAMESPACE, null, deployment, service, null, true);
util.createAndWait(NAMESPACE, deployment, service, null, true);
}
else {
util.deleteAndWait(NAMESPACE, deployment, service, null);
Expand Down
4 changes: 2 additions & 2 deletions ...ingframework/cloud/kubernetes/k8s/client/discovery/KubernetesClientDiscoveryClientIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,8 +488,8 @@ private static void manifests(Phase phase) {
envVars.add(commonsLevel);
deployment.getSpec().getTemplate().getSpec().getContainers().get(0).setEnv(envVars);

util.createAndWait(NAMESPACE, null, deployment, service, ingress, true);
util.createAndWait(NAMESPACE, null, null, externalNameService, null, true);
util.createAndWait(NAMESPACE, deployment, service, ingress, true);
util.createAndWait(NAMESPACE, null, externalNameService, null, true);
}

}
Expand Down
2 changes: 1 addition & 1 deletion ...cloud/kubernetes/k8s/client/discovery/KubernetesClientDiscoverySelectiveNamespacesIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ private static void manifests(Phase phase) {
envVars.add(debugLevel);
envVars.add(selectiveNamespaceA);
deployment.getSpec().getTemplate().getSpec().getContainers().get(0).setEnv(envVars);
util.createAndWait(NAMESPACE, null, deployment, service, ingress, true);
util.createAndWait(NAMESPACE, deployment, service, ingress, true);
}
}

Expand Down
6 changes: 3 additions & 3 deletions ...ud/kubernetes/configuration/watcher/multiple/apps/ConfigurationWatcherMultipleAppsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ private void appA(Phase phase) {
.yaml("ingress/spring-cloud-kubernetes-configuration-watcher-multiple-apps-ingress.yaml");

if (phase.equals(Phase.CREATE)) {
util.createAndWait(NAMESPACE, null, deployment, service, ingress, true);
util.createAndWait(NAMESPACE, deployment, service, ingress, true);
}
else if (phase.equals(Phase.DELETE)) {
util.deleteAndWait(NAMESPACE, deployment, service, ingress);
Expand All @@ -175,7 +175,7 @@ private void appB(Phase phase) {
V1Service service = (V1Service) util.yaml("app-b/app-b-service.yaml");

if (phase.equals(Phase.CREATE)) {
util.createAndWait(NAMESPACE, null, deployment, service, null, true);
util.createAndWait(NAMESPACE, deployment, service, null, true);
}
else if (phase.equals(Phase.DELETE)) {
util.deleteAndWait(NAMESPACE, deployment, service, null);
Expand All @@ -189,7 +189,7 @@ private void configWatcher(Phase phase) {
.yaml("config-watcher/spring-cloud-kubernetes-configuration-watcher-service.yaml");

if (phase.equals(Phase.CREATE)) {
util.createAndWait(NAMESPACE, null, deployment, service, null, true);
util.createAndWait(NAMESPACE, deployment, service, null, true);
}
else if (phase.equals(Phase.DELETE)) {
util.deleteAndWait(NAMESPACE, deployment, service, null);
Expand Down
6 changes: 3 additions & 3 deletions ...oud/kubernetes/configuration/watcher/multiple/apps/ConfigurationWatcherMultipleAppIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ private void appA(Phase phase) {
.yaml("ingress/spring-cloud-kubernetes-configuration-watcher-multiple-apps-ingress.yaml");

if (phase.equals(Phase.CREATE)) {
util.createAndWait(NAMESPACE, null, deployment, service, ingress, true);
util.createAndWait(NAMESPACE, deployment, service, ingress, true);
}
else if (phase.equals(Phase.DELETE)) {
util.deleteAndWait(NAMESPACE, deployment, service, ingress);
Expand All @@ -173,7 +173,7 @@ private void appB(Phase phase) {
V1Service service = (V1Service) util.yaml("app-b/app-b-service.yaml");

if (phase.equals(Phase.CREATE)) {
util.createAndWait(NAMESPACE, null, deployment, service, null, true);
util.createAndWait(NAMESPACE, deployment, service, null, true);
}
else if (phase.equals(Phase.DELETE)) {
util.deleteAndWait(NAMESPACE, deployment, service, null);
Expand All @@ -187,7 +187,7 @@ private void configWatcher(Phase phase) {
.yaml("config-watcher/spring-cloud-kubernetes-configuration-watcher-service.yaml");

if (phase.equals(Phase.CREATE)) {
util.createAndWait(NAMESPACE, null, deployment, service, null, true);
util.createAndWait(NAMESPACE, deployment, service, null, true);
}
else if (phase.equals(Phase.DELETE)) {
util.deleteAndWait(NAMESPACE, deployment, service, null);
Expand Down
2 changes: 1 addition & 1 deletion ...ingframework/cloud/kubernetes/k8s/client/reload/configmap/K8sClientConfigMapReloadIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ private static void manifests(Phase phase) {
util.createAndWait(NAMESPACE, mountConfigMap, null);
util.createAndWait("left", leftConfigMap, null);
util.createAndWait("right", rightConfigMap, null);
util.createAndWait(NAMESPACE, null, deployment, service, ingress, true);
util.createAndWait(NAMESPACE, deployment, service, ingress, true);
}

if (phase.equals(Phase.DELETE)) {
Expand Down
2 changes: 1 addition & 1 deletion ...g/springframework/cloud/kubernetes/k8s/client/reload/secret/K8sClientSecretsReloadIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ private static void configK8sClientIt(Phase phase) {
V1Secret secret = (V1Secret) util.yaml("secret.yaml");

if (phase.equals(Phase.CREATE)) {
util.createAndWait(NAMESPACE, null, deployment, service, ingress, true);
util.createAndWait(NAMESPACE, deployment, service, ingress, true);
util.createAndWait(NAMESPACE, null, secret);
}
else if (phase.equals(Phase.DELETE)) {
Expand Down
24 changes: 18 additions & 6 deletions .../src/main/java/org/springframework/cloud/kubernetes/integration/tests/commons/Images.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,27 +85,39 @@ public static void loadBusybox(K3sContainer container) {
}

public static void loadWiremock(K3sContainer container) {
Commons.load(container, WIREMOCK_TAR, WIREMOCK, wiremockVersion());
if (!imageAlreadyInK3s(container, WIREMOCK_TAR)) {
Copy link
Contributor Author

@wind57 wind57 Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we did this check in a previous PR for busybox, let's do it for every other image

Copy link
Contributor

@ryanjbaxter ryanjbaxter Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just move the check to Commons.load instead?

Commons.load(container, WIREMOCK_TAR, WIREMOCK, wiremockVersion());
}
}

public static void loadIstioCtl(K3sContainer container) {
Commons.load(container, ISTIOCTL_TAR, ISTIOCTL, istioVersion());
if (!imageAlreadyInK3s(container, ISTIOCTL_TAR)) {
Commons.load(container, ISTIOCTL_TAR, ISTIOCTL, istioVersion());
}
}

public static void loadIstioProxyV2(K3sContainer container) {
Commons.load(container, ISTIO_PROXY_V2_TAR, ISTIO_PROXY_V2, istioVersion());
if (!imageAlreadyInK3s(container, ISTIO_PROXY_V2_TAR)) {
Commons.load(container, ISTIO_PROXY_V2_TAR, ISTIO_PROXY_V2, istioVersion());
}
}

public static void loadIstioPilot(K3sContainer container) {
Commons.load(container, ISTIO_PILOT_TAR, ISTIO_PILOT, istioVersion());
if (!imageAlreadyInK3s(container, ISTIO_PILOT_TAR)) {
Commons.load(container, ISTIO_PILOT_TAR, ISTIO_PILOT, istioVersion());
}
}

public static void loadKafka(K3sContainer container) {
Commons.load(container, KAFKA_TAR, KAFKA, kafkaVersion());
if (!imageAlreadyInK3s(container, KAFKA_TAR)) {
Commons.load(container, KAFKA_TAR, KAFKA, kafkaVersion());
}
}

public static void loadRabbitmq(K3sContainer container) {
Commons.load(container, RABBITMQ_TAR, RABBITMQ, rabbitMqVersion());
if (!imageAlreadyInK3s(container, RABBITMQ_TAR)) {
Commons.load(container, RABBITMQ_TAR, RABBITMQ, rabbitMqVersion());
}
}

private static boolean imageAlreadyInK3s(K3sContainer container, String tarName) {
Expand Down
21 changes: 4 additions & 17 deletions ...a/org/springframework/cloud/kubernetes/integration/tests/commons/fabric8_client/Util.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,7 @@

import static org.awaitility.Awaitility.await;
import static org.junit.jupiter.api.Assertions.fail;
import static org.springframework.cloud.kubernetes.integration.tests.commons.Commons.loadImage;
import static org.springframework.cloud.kubernetes.integration.tests.commons.Commons.pomVersion;
import static org.springframework.cloud.kubernetes.integration.tests.commons.Commons.pullImage;

/**
* @author wind57
Expand All @@ -65,15 +63,9 @@ public final class Util {

private static final Log LOG = LogFactory.getLog(Util.class);

/** Image we get {@code istioctl} from in order to install Istio. */
public static final String ISTIO_ISTIOCTL = "istio/istioctl";

private final K3sContainer container;

private final KubernetesClient client;

public Util(K3sContainer container) {
this.container = container;
this.client = new KubernetesClientBuilder().withConfig(Config.fromKubeconfig(container.getKubeConfigYaml()))
.build();
}
Expand All @@ -85,7 +77,7 @@ public Util(K3sContainer container) {
* tight as possible, providing reasonable defaults.
*
*/
public void createAndWait(String namespace, String name, @Nullable Deployment deployment, @Nullable Service service,
public void createAndWait(String namespace, @Nullable Deployment deployment, @Nullable Service service,
@Nullable Ingress ingress, boolean changeVersion) {
try {

Expand All @@ -104,11 +96,6 @@ public void createAndWait(String namespace, String name, @Nullable Deployment de
.get(0)
.setImage(imageFromDeployment + ":" + pomVersion());
}
else {
Copy link
Contributor Author

@wind57 wind57 Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recall the history of why we needed this, but we do not anymore. At this point in time the images are already un-tared/downloaded and loaded in k3s, as such we can drop this code

Copy link
Contributor

@ryanjbaxter ryanjbaxter Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was needed for Jenkins because on our end we hit threshold limits from Dockerhub inside of K3S. If we pull the images and load them in in the test iself we use our Dockerhub credentials and we don't run into the threshold limits from Dockerhub

Copy link
Contributor Author

@wind57 wind57 Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I get it, will close this PR cause its a lot easier to make a new one then re-work this one. thx!

String[] image = imageFromDeployment.split(":", 2);
pullImage(image[0], image[1], container);
loadImage(image[0], image[1], name, container);
}

client.apps().deployments().inNamespace(namespace).resource(deployment).create();
waitForDeployment(namespace, deployment);
Expand Down Expand Up @@ -141,7 +128,7 @@ public void busybox(String namespace, Phase phase) {
Service service = client.services().load(serviceStream).item();

if (phase.equals(Phase.CREATE)) {
createAndWait(namespace, "busybox", deployment, service, null, false);
createAndWait(namespace, deployment, service, null, false);
}
else if (phase.equals(Phase.DELETE)) {
deleteAndWait(namespace, deployment, service, null);
Expand Down Expand Up @@ -311,7 +298,7 @@ public void setUpIstioctl(String namespace, Phase phase) {
istioctlDeployment.getSpec().getTemplate().getSpec().getContainers().get(0).setImage(imageWithVersion);

if (phase.equals(Phase.CREATE)) {
createAndWait(namespace, null, istioctlDeployment, null, null, false);
createAndWait(namespace, istioctlDeployment, null, null, false);
}
else {
deleteAndWait(namespace, istioctlDeployment, null, null);
Expand Down Expand Up @@ -362,7 +349,7 @@ public void wiremock(String namespace, String path, Phase phase, boolean withIng

deployment.getMetadata().setNamespace(namespace);
service.getMetadata().setNamespace(namespace);
createAndWait(namespace, "wiremock", deployment, service, ingress, false);
createAndWait(namespace, deployment, service, ingress, false);
}
else {

Expand Down
22 changes: 6 additions & 16 deletions ...va/org/springframework/cloud/kubernetes/integration/tests/commons/native_client/Util.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,7 @@

import static org.awaitility.Awaitility.await;
import static org.junit.jupiter.api.Assertions.fail;
import static org.springframework.cloud.kubernetes.integration.tests.commons.Commons.loadImage;
import static org.springframework.cloud.kubernetes.integration.tests.commons.Commons.pomVersion;
import static org.springframework.cloud.kubernetes.integration.tests.commons.Commons.pullImage;

/**
* @author wind57
Expand All @@ -84,8 +82,6 @@ public final class Util {

private final RbacAuthorizationV1Api rbacApi;

private final K3sContainer container;

public Util(K3sContainer container) {

ApiClient client;
Expand All @@ -99,7 +95,6 @@ public Util(K3sContainer container) {
client.setDebugging(false);
Configuration.setDefaultApiClient(client);

this.container = container;
this.coreV1Api = new CoreV1Api();
this.appsV1Api = new AppsV1Api();
this.networkingV1Api = new NetworkingV1Api();
Expand All @@ -113,8 +108,8 @@ public Util(K3sContainer container) {
* tight as possible, providing reasonable defaults.
*
*/
public void createAndWait(String namespace, String name, V1Deployment deployment, V1Service service,
@Nullable V1Ingress ingress, boolean changeVersion) {
public void createAndWait(String namespace, V1Deployment deployment, V1Service service, @Nullable V1Ingress ingress,
boolean changeVersion) {
try {

coreV1Api.createNamespacedService(namespace, service, null, null, null, null);
Expand All @@ -134,11 +129,6 @@ public void createAndWait(String namespace, String name, V1Deployment deployment
.get(0)
.setImage(imageFromDeployment + ":" + pomVersion());
}
else {
Copy link
Contributor Author

@wind57 wind57 Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above

Copy link
Contributor

@ryanjbaxter ryanjbaxter Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above ;)

String[] image = imageFromDeployment.split(":", 2);
pullImage(image[0], image[1], container);
loadImage(image[0], image[1], name, container);
}

appsV1Api.createNamespacedDeployment(namespace, deployment, null, null, null, null);
waitForDeployment(namespace, deployment);
Expand Down Expand Up @@ -252,7 +242,7 @@ public void busybox(String namespace, Phase phase) {

V1Service service = (V1Service) yaml("busybox/service.yaml");
if (phase.equals(Phase.CREATE)) {
createAndWait(namespace, "busybox", deployment, service, null, false);
createAndWait(namespace, deployment, service, null, false);
}
else if (phase.equals(Phase.DELETE)) {
deleteAndWait(namespace, deployment, service, null);
Expand All @@ -271,7 +261,7 @@ public void kafka(String namespace, Phase phase) {

if (phase.equals(Phase.CREATE)) {
createAndWait(namespace, configMap, null);
createAndWait(namespace, "kafka", deployment, service, null, false);
createAndWait(namespace, deployment, service, null, false);
}
else if (phase.equals(Phase.DELETE)) {
deleteAndWait(namespace, configMap, null);
Expand All @@ -289,7 +279,7 @@ public void rabbitMq(String namespace, Phase phase) {
V1Service service = (V1Service) yaml("rabbitmq/rabbitmq-service.yaml");

if (phase.equals(Phase.CREATE)) {
createAndWait(namespace, "rabbitmq", deployment, service, null, false);
createAndWait(namespace, deployment, service, null, false);
}
else if (phase.equals(Phase.DELETE)) {
deleteAndWait(namespace, deployment, service, null);
Expand Down Expand Up @@ -479,7 +469,7 @@ public void wiremock(String namespace, String path, Phase phase, boolean withIng

deployment.getMetadata().setNamespace(namespace);
service.getMetadata().setNamespace(namespace);
createAndWait(namespace, "wiremock", deployment, service, ingress, false);
createAndWait(namespace, deployment, service, ingress, false);
}
else {
if (withIngress) {
Expand Down
Loading