Skip to content
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.

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

Item needs to be one of [Node, Deployment, ReplicaSet, StatefulSet, Pod, DeploymentConfig, ReplicationController], but was: [Service] #2672

Closed
Eddman opened this issue Dec 17, 2020 · 7 comments · Fixed by #2796 or jenkinsci/kubernetes-client-api-plugin#83
Assignees
Milestone

Comments

@Eddman
Copy link

Eddman commented Dec 17, 2020

We're watching service readiness using following code:

final Service service = this.kubernetesClient
                        .services()
                        .inNamespace(namespace)
                        .withName(name)
                        .waitUntilReady(60, TimeUnit.SECONDS);

We used this code with version 4.6.0 and it worked.
After upgrading to version 4.13.0 it stopped working with the following exception:

java.lang.IllegalArgumentException: Item needs to be one of [Node, Deployment, ReplicaSet, StatefulSet, Pod, ReplicationController], but was: [Service]
	at io.fabric8.kubernetes.client.internal.readiness.Readiness.isReady(Readiness.java:62)
	at io.fabric8.kubernetes.client.dsl.base.BaseOperation.lambda$waitUntilReady$6(BaseOperation.java:1109)
	at io.fabric8.kubernetes.client.dsl.base.BaseOperation.waitUntilConditionWithRetries(BaseOperation.java:1131)
	at io.fabric8.kubernetes.client.dsl.base.BaseOperation.waitUntilCondition(BaseOperation.java:1115)
	at io.fabric8.kubernetes.client.dsl.base.BaseOperation.waitUntilReady(BaseOperation.java:1109)
	at io.fabric8.kubernetes.client.dsl.internal.core.v1.ServiceOperationsImpl.waitUntilReady(ServiceOperationsImpl.java:83)
	at io.fabric8.kubernetes.client.dsl.internal.core.v1.ServiceOperationsImpl.waitUntilReady(ServiceOperationsImpl.java:37)
@rohanKanojia
Copy link
Member

Umm, I think a Service should be ready whenever it gets created. I don't think it's a Ready-able resource

@Eddman
Copy link
Author

Eddman commented Dec 17, 2020

Original implementation of ServiceOperationsImpl.waitUntilReady in 4.6.0 waited for condition notNull for the Service and then waited for Endpoints to become ready. Now it just fails calling super.waitUntilReady() that has been changed somewhere between 4.6 <-> 4.13.

@rohanKanojia
Copy link
Member

ohk, I see. You're right, there is an implementation in ServiceOperationsImpl.waitUntilReady(I wasn't aware of this):

@Override
public Service waitUntilReady(long amount, TimeUnit timeUnit) throws InterruptedException {
long started = System.nanoTime();
super.waitUntilReady(amount, timeUnit);
long alreadySpent = System.nanoTime() - started;
// if awaiting existence took very long, let's give at least 10 seconds to awaiting readiness
long remaining = Math.max(10_000, timeUnit.toNanos(amount) - alreadySpent);
EndpointsOperationsImpl endpointsOperation = new EndpointsOperationsImpl(context);
endpointsOperation.waitUntilReady(remaining, TimeUnit.MILLISECONDS);

I think we might have broken this while refactoring waitUntilCondition to use watcher instead of polling (#2239) . In v4.6.0 version of waitUntilReady I don't see Readiness.isReady being used too (which might be the reason waitUntilReady worked fine)

Since Service depends on Endpoints to become ready, maybe we can add this to our Readiness applicable resources:

public static boolean isReadinessApplicable(Class<? extends HasMetadata> itemClass) {
return Deployment.class.isAssignableFrom(itemClass)
|| io.fabric8.kubernetes.api.model.extensions.Deployment.class.isAssignableFrom(itemClass)
|| ReplicaSet.class.isAssignableFrom(itemClass)
|| Pod.class.isAssignableFrom(itemClass)
|| ReplicationController.class.isAssignableFrom(itemClass)
|| Endpoints.class.isAssignableFrom(itemClass)
|| Node.class.isAssignableFrom(itemClass)
|| StatefulSet.class.isAssignableFrom(itemClass)
;
}

@manusa
Copy link
Member

manusa commented Dec 18, 2020

This behavior describes some sort of integrated Readiness Probe, meaning both the Service and the Pod are ready.

So as a workaround, given that a valid Service configuration is provided, pods().whatever().isReady() should do the job.

Adding this to the isReadinessApplicable classes won't work as this requires polling (unless there's a different/inconsistent implementation for each resource). The watcher will only get MODIFIED events with status (spec too, but not applicable here) updates to the Service resource. Since the service status won't change, the endpoint check will never be triggered.

IMO I don't think that services().whatever().isReady() is a good name for what this does (unless kubectl/client-go/some k8s spec defines this as ready). Maybe something like services().whatever().ports().waitUntilReady() or services().whatever().probe() + services().whatever().waitUntilProbe() or anything in those lines, helps other users understand and set proper expectations.

@Eddman
Copy link
Author

Eddman commented Dec 18, 2020

So the original behavior was:

  1. wait for Service creation .waitUntilCondition(Objects::notNull, ...) (called from waitUntilReady()
  2. wait for Service Endpoints to be ready endopints().waitUntilReady(...)

But now waitUntilReady() for unknown objects throws exception instead of calling .waitUntilCondition(Objects::notNull, ...)

Somehow it makes sense cause we can wait for Service to have ready Endpoints 🤷‍♂️ Once ready we can call that service without worrying that some child components (Endpoints, Pods) are not ready...

@manusa
Copy link
Member

manusa commented Dec 18, 2020

Yes, I agree with the usefulness of the method, but not sure if the name + DSL access methods align with what it does.

@rohanKanojia rohanKanojia self-assigned this Feb 8, 2021
rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Feb 8, 2021
…lArgumentException

Use `waitUntilCondition(Objects::nonNull, amount, timeUnit)` instead of
waitUntilReady for `Service` resource
@manusa
Copy link
Member

manusa commented Feb 11, 2021

From: #2796 (comment)


Given that ServiceOperationsImpl extends BaseOperation which in turns implements the Resource interface which extends the Readiable and VersionWatchAndWaitable interfaces, the Readiness#isReady method, should never throw an Exception.

The fact is that from our current DSL point of view, we consider every Resource as waitable/readiable, so waitUntilReady() method is generated for all resources. This means that calling this method for any of the Resources that aren't currently handled in the Readiness#isReady method will throw an exception.

private static boolean isKubernetesResourceReady(HasMetadata item) {
if (item instanceof Deployment) {
return isDeploymentReady((Deployment) item);
} else if (item instanceof io.fabric8.kubernetes.api.model.extensions.Deployment) {
return isExtensionsDeploymentReady((io.fabric8.kubernetes.api.model.extensions.Deployment) item);
} else if (item instanceof ReplicaSet) {
return isReplicaSetReady((ReplicaSet) item);
} else if (item instanceof Pod) {
return isPodReady((Pod) item);
} else if (item instanceof ReplicationController) {
return isReplicationControllerReady((ReplicationController) item);
} else if (item instanceof Endpoints) {
return isEndpointsReady((Endpoints) item);
} else if (item instanceof Node) {
return isNodeReady((Node) item);
} else if (item instanceof StatefulSet) {
return isStatefulSetReady((StatefulSet) item);
}
return false;
}

public static boolean isReady(HasMetadata item) {
if (isReadiableKubernetesResource(item)) {
return isKubernetesResourceReady(item);
} else {
throw new IllegalArgumentException("Item needs to be one of [Node, Deployment, ReplicaSet, StatefulSet, Pod, ReplicationController], but was: [" + (item != null ? item.getKind() : "Unknown (null)") + "]");
}
}

The fix should be to replace line 62 by an existence check Objects:nonNull of a resource retrieved from the server, and maybe warn the user about the resource not being of what we consider to be of a "RediableKubernetesResource" type.

@manusa manusa added this to the 5.1.0 milestone Feb 11, 2021
rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Feb 12, 2021
…lArgumentException

Refactor Readiness.isReady to log a warning for Non Readiness Applicable
resources instead of throwing IllegalArgumentException.

For Readiness check, these kind of resources will only be checked with
Objects.nonNull since they become ready as soon as they get created
rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Feb 16, 2021
…lArgumentException

Refactor Readiness.isReady to log a warning for Non Readiness Applicable
resources instead of throwing IllegalArgumentException.

For Readiness check, these kind of resources will only be checked with
Objects.nonNull since they become ready as soon as they get created
manusa pushed a commit to manusa/kubernetes-client that referenced this issue Feb 17, 2021
…lArgumentException

Refactor Readiness.isReady to log a warning for Non Readiness Applicable
resources instead of throwing IllegalArgumentException.

For Readiness check, these kind of resources will only be checked with
Objects.nonNull since they become ready as soon as they get created
manusa pushed a commit to rohanKanojia/kubernetes-client that referenced this issue Feb 17, 2021
…lArgumentException

Refactor Readiness.isReady to log a warning for Non Readiness Applicable
resources instead of throwing IllegalArgumentException.

For Readiness check, these kind of resources will only be checked with
Objects.nonNull since they become ready as soon as they get created
manusa pushed a commit that referenced this issue Feb 17, 2021
…Exception

Refactor Readiness.isReady to log a warning for Non Readiness Applicable
resources instead of throwing IllegalArgumentException.

For Readiness check, these kind of resources will only be checked with
Objects.nonNull since they become ready as soon as they get created
This was referenced Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment