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

Impossible to replace ConfigMap #2445

Closed
sknot-rh opened this issue Aug 27, 2020 · 26 comments · Fixed by #2453
Closed

Impossible to replace ConfigMap #2445

sknot-rh opened this issue Aug 27, 2020 · 26 comments · Fixed by #2453
Assignees
Labels

Comments

@sknot-rh
Copy link
Contributor

sknot-rh commented Aug 27, 2020

Hi.
I've upgraded to version 4.11.0 and I started having an issue with replacing ConfigMaps.

Reproducer

public class Main {
    public static final String RESOURCE_NAME = "my-resource";
    public static final String NAMESPACE = "myproject";
    protected static KubernetesClient client = new DefaultKubernetesClient();

    public static ConfigMap getOriginalCM()  {
        return new ConfigMapBuilder()
                .withNewMetadata()
                .withName(RESOURCE_NAME)
                .withLabels(singletonMap("state", "new"))
                .endMetadata()
                .build();
    }

    public static void main(String[] args) {
        ConfigMap newResource = getOriginalCM();
        client.configMaps().inNamespace(NAMESPACE).withName(RESOURCE_NAME).create(newResource);
        client.configMaps().inNamespace(NAMESPACE).createOrReplace(newResource);
    }
}

Output of createOrReplace(ConfigMap)

io.fabric8.kubernetes.client.dsl.base.BaseOperation.createOrReplace(BaseOperation.java:83)
java.lang.ClassCastException: [Ljava.lang.Object; cannot be cast to [Lio.fabric8.kubernetes.api.model.HasMetadata;

Expected output
updated CM

$ oc version
oc v3.11.82
kubernetes v1.11.0+d4cacc0
features: Basic-Auth GSSAPI Kerberos SPNEGO

Server https://10.37.144.54:8443
kubernetes v1.11.0+d4cacc0

Did the API changed in 4.11.0 or is it a bug?

Thank you!

@rohanKanojia
Copy link
Member

rohanKanojia commented Aug 27, 2020

Strange, the code you shared is working for me on OpenShift 3.11 and minikube v1.12.2 on FKC 4.11.0

@sknot-rh
Copy link
Contributor Author

The code runs ok. The exception is visible in debug mode.
exception

@rohanKanojia
Copy link
Member

ohk, I can reproduce

@manusa
Copy link
Member

manusa commented Sep 1, 2020

ClassCastException issue (IntelliJ Expression evaluation)

Affects versions: >= 4.10.2
Probably caused by #2239 by making T generic type extend HasMetadata

Sames issue can be observed when client.configMaps().inNamespace(NAMESPACE).delete(newResource);

Issue won't happen if argument is wrapped with List:

client.configMaps().inNamespace(NAMESPACE).delete(Collections.singleonList(newResource));
// Or
client.configMaps().inNamespace(NAMESPACE).delete(new HasMetadata[]{newResource});

I'm not sure about this, but I'm strongly thinking IntelliJ's Debug->Evaluate tools makes a mess with type inference and Generics in BaseOperation class (when compiling the evaluated expression, wrong inference for varargs).
So this only affects IDEA's expression evaluator:

image
image
image
image

As you can see in the previous screenshots, when performing unnecessary casting, Evaluate works as expected.

See also

@sknot-rh
Copy link
Contributor Author

sknot-rh commented Sep 1, 2020

Ah, makes sense. Thanks for the investigation. So I guess this can be closed now?

@manusa
Copy link
Member

manusa commented Sep 1, 2020

So I guess this can be closed now?

I'm still working on it. See if I can find some solution to the problem. It worries me that we hit into this problem not just when debugging 😟

@Ladicek
Copy link
Contributor

Ladicek commented Sep 1, 2020

I'd like to point out that I'm also seeing this issue, and it's not in a debugger. If you want to see that, clone https://github.com/thorntail-examples/configmap, run mvn clean verify -Popenshift,openshift-it and observe a success. Then update Fabric8 OpenShift Client to 4.11.0 and run again -- it will take a long time and 2 tests will eventually fail. The reason is that the ConfigMap update code here https://github.com/thorntail-examples/configmap/blob/aca3b64/src/test/java/io/thorntail/example/OpenshiftIT.java#L138-L144 doesn't work.

@manusa
Copy link
Member

manusa commented Sep 1, 2020

Thx @Ladicek, this helps a lot. BTW, I can track the affecting issue to 4.10.2. Don't your tests fail with this version?

@Ladicek
Copy link
Contributor

Ladicek commented Sep 1, 2020

No, it's OK with 4.10.3. Perhaps it's a different issue, but manifests in the same way :-)

@Ladicek
Copy link
Contributor

Ladicek commented Sep 1, 2020

Just to be sure, I reran the test with the latest master branch of the repo mentioned above, and it passed. The POM defines Fabric8 OpenShift Client version 4.10.3, and mvn dependency:tree confirms that all Fabric8 Kubernetes Client related dependencies are indeed 4.10.3 -- with an exception of io.fabric8:kubernetes-openshift-uberjar. That's only there for Arquillian Cube, and since the classes in that uberjar use a different package name, it shouldn't interfere.

@manusa
Copy link
Member

manusa commented Sep 1, 2020

Hi @Ladicek

I'm not sure your issue is related to this ClassCastException. I'm thinking it's related to the behavior of createOrReplace method being changed in the latest release.

I'm able to see the test failures, but it's related to a timeout (probably because the configmaps never get replaced). I think this is caused by the comparison methods used to determine if the resource must be replaced or left as is. But this has nothing to do with the issue at hand.

Did you actually see some similar message (ClassCastException)? In a nutshell, problem for ClassCastException issue is that the generated bytecode is createOrReplace(HasMetadata[] items) instead of createOrReplace(Object[] items)

@thmshmm
Copy link

thmshmm commented Sep 1, 2020

Hi, I came across this issue while searching why the ConfigMaps are not updated in the cluster. Same exceptions while debugging in IDEA and if executing on OpenShift 3.11 it just didn't replace the ConfigMaps.

@Ladicek
Copy link
Contributor

Ladicek commented Sep 1, 2020

@manusa yes, the tests fail because the ConfigMap is never replaced. Whether there's a ClassCastException or not, to be fair, I don't know. I found that my ConfigMaps are not getting replaced, went to the issue tracker, and the first issue I saw was "impossible to replace ConfigMap", so I thought someone found the problem already :-) Sorry for the noise.

@sknot-rh
Copy link
Contributor Author

sknot-rh commented Sep 1, 2020

TBH I had the same issue as @Ladicek. While debugging I saw the exception and I was thinking "Yeah, that's it".

@manusa
Copy link
Member

manusa commented Sep 1, 2020

ConfigMaps not getting replaced are related to:

if (ResourceCompare.equals(itemFromServer, itemToCreateOrReplace)) {
// Nothing changed, ignore
return itemToCreateOrReplace;
} else {

/**
* This method returns true when left Kubernetes resource contains
* all data that's present in right Kubernetes resource, this method
* won't consider fields that are missing in right parameters. Values
* which are present in right would only be compared.
*
* @param left kubernetes resource (fetched from cluster)
* @param right kubernetes resource (provided as input by user)
* @param <T> type for kubernetes resource
*
* @return boolean value whether both resources are actually equal or not
*/
public static <T> boolean equals(T left, T right) {

createOrReplace method was probably "overoptimized", and now replace method is only hit in case differences are detected.

#2372 #2354 --> (#2292)

Probably your failing cases are due to this (this was actually introduced in 4.11.0).

Could you please confirm in a debug session that your configmap replacements are getting skipped?

@sknot-rh
Copy link
Contributor Author

sknot-rh commented Sep 1, 2020

In the simple reproducer, I provided, the CM is replaced. In much more complex IT or even ST it happens it is not replaced.

@thmshmm
Copy link

thmshmm commented Sep 1, 2020

Hi @manusa , confirmed. For me it's running into line 441 and skipping while compared it's definitly different data.

@thmshmm
Copy link

thmshmm commented Sep 1, 2020

Line 107 and 108 in ResourceCompare return 'null'. Therefore isEqualSpec returns true in line 111.

@Ladicek
Copy link
Contributor

Ladicek commented Sep 1, 2020

I can confirm that my ConfigMap isn't updated because the createOrReplace method ends up here: https://github.com/fabric8io/kubernetes-client/blob/v4.11.0/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java#L287

@manusa
Copy link
Member

manusa commented Sep 1, 2020

Thx all for your collaboration, sent a PR reverting that behavior.

@manusa
Copy link
Member

manusa commented Sep 2, 2020

Patch should be available in https://github.com/fabric8io/kubernetes-client/releases/tag/v4.11.1

@wind57
Copy link

wind57 commented Jun 15, 2022

I can reproduce this issue in version 5.12.1 that we currently use in spring-cloud-kubernetes. Unfortunately the code is not yet part of any PR for you to try or look at, but as soon as it will - I will link it in here.

@wind57
Copy link

wind57 commented Jul 4, 2022

here is where I had to apply the cast : https://github.com/spring-cloud/spring-cloud-kubernetes/pull/1036/files#diff-4ddeab245bfc52856d8238ad19d4bcb38b009a73be2c3b8acd97d878e20f15a1

It's fairly easy to test, but you require the source code locally. You need to build the image (by simply running mvn clean install - requires docker) and run the test method. If I remove the cast - it fails.

@manusa
Copy link
Member

manusa commented Sep 22, 2022

Hi @wind57
I just came by your comments. Is this issue still applicable?
We refactored most of the affected parts for 6.x, so it's very likely that in that version, either the error is no longer there, or that it corresponds to something else.

Could you please open a new issue if the problem persists for you so we can keep track of it, and implement a fix if necessary?

@wind57
Copy link

wind57 commented Sep 22, 2022

sure! If I see it again, will create a reproducible example.

@wind57
Copy link

wind57 commented Dec 23, 2022

After upgrading to 6.2, we no longer see this issue! thx a lot @manusa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants