CPU resource unit comparison error

[10000-ki]

Bug Report

In Kubernetes, some resource units are automatically converted to a more convenient format internally. For example:

resources:
    limits:
      cpu: 2000m

Even if the CPU is set to 2000m, when you retrieve this configuration using kubectl, you might see:

resources:
    limits:
      cpu: 2

This indicates that the unit has been converted.

The problem arises when an Operator sets the CPU unit as 2000m, but the Kubernetes API server keeps converting it back to 2. This leads to a situation where, during the DependentResource.match process, the Operator and the API server identify the resources as different, causing match to return false.

The reason for this issue is that when comparing the actual and desired states, the values are checked through simple string comparison.

As a result, the operator keeps sending patch requests, causing the resourceVersion to increase infinitely.

Environment

$ java -version

17

$ kubectl version

v1.24

Possible Solution

Quantity("2000m") == Quantity("2")

In fact, when using Quantity in fabric8 for comparison, it can be determined that these two values are the same. It might be better to use Quantity for resource comparisons. However, given the current structure, this doesn't seem straightforward. Do you think it might be possible?

[10000-ki]

This is also same for storage unit comparisons between Gi and Ti. For example:
Quantity("1024Gi") == to Quantity("1Ti").

[metacosm]

The problem is that the matcher is generic so it'd have to introspect the structure to figure out the type and do the appropriate conversions before checking for equality so it might be difficult to do in a generic way.

[metacosm]

One option could be to have specific matchers for specific types to avoid this issue but that would add quite a maintenance burden as we'd need to ensure that these matchers are properly updated whenever the associated type changes in Kubernetes

[10000-ki]

@metacosm

One option could be to have specific matchers for specific types to avoid this issue but that would add quite a maintenance burden as we'd need to ensure that these matchers are properly updated whenever the associated type changes in Kubernetes

yes right

While it is possible to compare resources like StatefulSet and Deployment separately, I also anticipate that it would be challenging to handle this across different K8S versions.

I will also take some time to think about whether there might be a better approach.

[csviri]

It would be good to handle such cases in the framework, unfortunatelly the current SSA Matcher is not prepared for that. But would be nice to do such experiments, and add such fixes even on-demand bases.

[10000-ki]

It would be good to handle such cases in the framework, unfortunatelly the current SSA Matcher is not prepared for that. But would be nice to do such experiments, and add such fixes even on-demand bases.

yes right so
i will try to do some experiments to find if there is a good way.

[Donnerbart]

We are facing the same issue and solved this by actually creating our desired resource in the format that K8s would transform into. With this approach the actual and desired are the same and the built-in matchers work (SSA and also the generic one).

This is also same for storage unit comparisons between Gi and Ti. For example: Quantity("1024Gi") == to Quantity("1Ti").

Good to know, I think I have to build that into our utility class then.

Our use case is a CRUDKubernetesDependentResource<StatefulSet, OurCustomResource> that is created from a StatefulSetSpec that we have in our custom resource. In the desired StatefulSet we go over the containers and modify the resources like this:

        if (container.getResources() != null) {
            convertResourcesQuantity(container.getResources().getRequests(),
                    (requests) -> container.getResources().setRequests(requests));
            convertResourcesQuantity(container.getResources().getLimits(),
                    (limits) -> container.getResources().setLimits(limits));
        }

This is the utility class:

import io.fabric8.kubernetes.api.model.Quantity;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

import java.util.HashMap;
import java.util.Map;
import java.util.function.Consumer;

public class QuantityUtil {

    private QuantityUtil() {
    }

    public static void convertResourcesQuantity(
            final @Nullable Map<String, Quantity> resource,
            final @NotNull Consumer<Map<String, Quantity>> resourceConsumer) {
        if (resource == null || resource.isEmpty()) {
            return;
        }
        final var newResource = new HashMap<String, Quantity>(resource.size());
        resource.forEach((key, quantity) -> {
            // workaround to deal with the implicit conversion of, e.g. "cpu=1000m" to "cpu=1" or "cpu=0.5" to
            // "cpu=500m" in Fabric8 and K8s, that lead to constant mismatches between the desired and actual
            // values (triggers infinite rolling restarts)
            if ("m".equals(quantity.getFormat())) {
                // a Quantity of, e.g. 512m or 1001m has an intValue() of 0 or 1 (the fractional part is lost), so
                // we can only replace the Quantity if the full numerical amount of the candidate is still the same
                final var numericalAmount = quantity.getNumericalAmount();
                final var candidate = new Quantity(String.valueOf(numericalAmount.intValue()));
                if (numericalAmount.compareTo(candidate.getNumericalAmount()) == 0) {
                    newResource.put(key, candidate);
                    return;
                }
            } else {
                // a Quantity of e.g. 0.5 has a fractional part, so we can only replace the Quantity if the intValue()
                // of the numerical amount doesn't equal the original amount, and the full numerical amount of the
                // candidate is still the same
                final var numericalAmount = quantity.getNumericalAmount();
                if (!quantity.getAmount().equals(String.valueOf(numericalAmount.intValue()))) {
                    final var milliCoreAmount = Math.round(numericalAmount.floatValue() * 1000);
                    final var candidate = new Quantity(String.valueOf(milliCoreAmount), "m");
                    if (numericalAmount.compareTo(candidate.getNumericalAmount()) == 0) {
                        newResource.put(key, candidate);
                        return;
                    }
                }
            }
            newResource.put(key, quantity);
        });
        resourceConsumer.accept(newResource);
    }
}

class QuantityUtilTest {

    private final @NotNull AtomicReference<Map<String, Quantity>> resourcesRef = new AtomicReference<>();
    private final @NotNull Consumer<Map<String, Quantity>> resourceConsumer = resourcesRef::set;

    @Test
    @Timeout(10)
    void convertResourcesQuantity_withNullResource_doNotApplyResources() {
        QuantityUtil.convertResourcesQuantity(null, resourceConsumer);
        assertThat(resourcesRef).hasNullValue();
    }

    @Test
    @Timeout(10)
    void convertResourcesQuantity_withEmptyResource_doNotApplyResources() {
        QuantityUtil.convertResourcesQuantity(Map.of(), resourceConsumer);
        assertThat(resourcesRef).hasNullValue();
    }

    @Test
    @Timeout(10)
    void convertResourcesQuantity_withConvertableMilliFormat_applyConvertedResources() {
        // test Quantity with and without a separate format argument
        final var resource = Map.of("cpu1", new Quantity("1000m"), "cpu2", new Quantity("2000", "m"));
        QuantityUtil.convertResourcesQuantity(resource, resourceConsumer);
        assertThat(resourcesRef).hasValueSatisfying(resources -> assertThat(resources).hasSize(2)
                .containsEntry("cpu1", new Quantity("1"))
                .containsEntry("cpu2", new Quantity("2")));
    }

    @Test
    @Timeout(10)
    void convertResourcesQuantity_withNonConvertableMilliFormat_applyOriginalResources() {
        // test Quantity with and without a separate format argument
        final var resource = Map.of("cpu1", new Quantity("42m"), "cpu2", new Quantity("2342", "m"));
        QuantityUtil.convertResourcesQuantity(resource, resourceConsumer);
        assertThat(resourcesRef).hasValueSatisfying(resources -> assertThat(resources).hasSize(2)
                .containsEntry("cpu1", new Quantity("42m"))
                .containsEntry("cpu2", new Quantity("2342m")));
    }

    @Test
    @Timeout(10)
    void convertResourcesQuantity_withConvertableFormat_applyConvertedResources() {
        final var resource = Map.of("cpu", new Quantity("0.42"));
        QuantityUtil.convertResourcesQuantity(resource, resourceConsumer);
        assertThat(resourcesRef).hasValueSatisfying(resources -> assertThat(resources).hasSize(1)
                .containsEntry("cpu", new Quantity("420m")));
    }

    @Test
    @Timeout(10)
    void convertResourcesQuantity_withNonConvertableFormat_applyOriginalResources() {
        final var resource = Map.of("cpu", new Quantity("0.5"));
        QuantityUtil.convertResourcesQuantity(resource, resourceConsumer);
        assertThat(resourcesRef).hasValueSatisfying(resources -> assertThat(resources).hasSize(1)
                .containsEntry("cpu", new Quantity("0.5")));
    }
}

I know this is far from being perfect, but it solved the infinite update loop issue for us. But of course I'd love to see this fixed in the operator SDK or even the fabric8 client, so there are more eyes on this code (e.g. to cover other missing cases like the storage conversion).

[Donnerbart]

I'm really bad at reading GoLang code. Could someone help me find the code in K8s that does this transformation? Maybe we can re-implement that 1:1 in Java to have the same conversion code as Java utility. This of course might need maintenance, but it would at least be a better workaround than my util, that was just built upon observations.

[metacosm]

Maybe we should add your utility method (or some form of it) on the Quantity class in the client, @Donnerbart?

[metacosm]

I'm really bad at reading GoLang code. Could someone help me find the code in K8s that does this transformation? Maybe we can re-implement that 1:1 in Java to have the same conversion code as Java utility. This of course might need maintenance, but it would at least be a better workaround than my util, that was just built upon observations.

The code seems to be here: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/api/resource/quantity.go

[Donnerbart]

I think that would be a great solution. Maybe something like public Quantity asKubernetesFormat() for the Quantity class and public A asKubernetesFormat() for the QuantityBuilder? Or maybe asKubernetesSerializationFormat()?

(Note that 1024 = 1Ki but 1000 = 1k; I didn't choose the capitalization.)

I love that comment on the K8s code 😅

Since I anyway have to improve my utility class, I could make a contribution to the fabric8 client (assuming that I can copy the whole formatting logic from the GoLang code into Java).

[metacosm]

I think that would be a great solution. Maybe something like public Quantity asKubernetesFormat() for the Quantity class and public A asKubernetesFormat() for the QuantityBuilder? Or maybe asKubernetesSerializationFormat()?

Yes. Please open an issue on the client's repo

(Note that 1024 = 1Ki but 1000 = 1k; I didn't choose the capitalization.)

I love that comment on the K8s code 😅

😁

Since I anyway have to improve my utility class, I could make a contribution to the fabric8 client (assuming that I can copy the whole formatting logic from the GoLang code into Java).

That would be ideal and we would appreciate the contribution! 😉

[10000-ki]

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/api/resource/quantity.go

I also agree with the opinion that the quantity parsing part code is implemented in the same way on the josdk side.

[Donnerbart]

I had a first look a this, and I'm not sure anymore if this is the right way to go.

The canonical format conversion is complex with a lot of rules and open TODOs in the K8s GoLang code. The Quantity implementation in GoLang is also quite different with two subtypes (based on int64Amount and infDecAmount) instead of two String fields in Java. The GoLang implementation is also aware of it's format type (DecimalExponent, BinarySI or DecimalSI), an information that is heavily used for the canonical format conversion.

In GoLang the canonical conversion is invoked in MarshalJSON() and String(), but I couldn't find any string -> string test cases, just something like {"1.2e3", decQuantity(12, 2, DecimalExponent)},. In the end I came up with this parameterized unit test, based on this manual deployment in a real cluster (to be sure how the converted values actually looks like).

Examples for complex conversions:

• The exponential notation with uppercase E is partially converted to lowercase e. For example:
  • 1E-8 -> 1e-3 (converted to minimum value)
  • 1E-2 -> 10e-3 (converted to other exponent)
  • 1E-0 -> 1E-0 (no conversion)
  • 1E0 -> 1E0 (no conversion)
• When using binary units we have multiple conversion outcomes. For example:
  • 0.0000001Ki -> 1m (converted to minimum value)
  • 0.128Ki -> 131072m (converted to SI unit)
  • 1.5Ki -> 1536 (converted to decimal)
  • 256Ki -> 256Ki (no conversion)
  • 1024Ki -> 1Mi (converted to binary unit)

Examples for TODOs:

• // TODO: If BinarySI formatting is requested but would cause rounding, upgrade to one of the other formats.

In total we have multiple conversion output formats (decimal, exponential, SI units, binary units), we have a minimum value, there are rules for precision and rounding and we have special handling for zero values.

Of course it would be possible to implement the same logic as we currently have in Kubernetes. But I think this will cause problems down the line. My biggest concern is if the implementation will ever be changed in K8s, e.g. if the TODOs are worked on. If an existing conversion would ever change, we would need to know the K8s version to anticipate the correct canonical format in the fabric8 client. This would be a nightmare to keep in sync.

In the end we could have the same problem again. We try to anticipate the canonical format, but the deployed K8s version has a different implementation. And then we have an infinite update loop again.

I would rather like to explore if we can make the SSABasedGenericKubernetesResourceMatcher smarter and e.g. enhance this existing method to align the actual instance with desired, in case the Quantity instances define the same amount.

This approach should be much simpler, doesn't need any invocation of canonical format conversion in the desired() method, should be much more performant (comparison uses Quantity.getNumericalAmount() under the hood vs. much more arithmetic calculations for the canonical format conversion) and resilient to future changes on canonical format implementation in K8s.

[metacosm]

Modifying the sanitizeState method seems reasonable to me.

[murillio4]

I didn't realize my mention of this issue would be tracked here 😅

Anyway, I've implemented a temporary fix in my operator. My approach modifies the SSABasedGenericKubernetesResourceMatcher to normalize Quantity values in both the actualMap and desiredMap, replacing them with numericalAmount.stripTrailingZeros(). This resulted in quite a bit of code, and may not be ideal for a long-term solution.

Since all methods in SSABasedGenericKubernetesResourceMatcher except match are private, I had to clone the entire class to make changes. Would it be possible to convert these methods (or some of them) to protected? This would allow easier extension and customization without needing to replicate all the logic.

[metacosm]

@murillio4 can you create an issue for this, please, mentioning specifically which methods you'd like to see be made accessible?

[Donnerbart]

I finally had some time to continue the work on this and provided a PR with a possible fix.

I've targeted the next branch, but (if accepted) would also like to backport this to main (assuming there are still 4.9.x releases planned before version 5.0.0).

[metacosm]

Fixed in #2565 and #2568.