EnMasseProject / enmasse

EnMasse - Self-service messaging on Kubernetes and OpenShift
https://enmasseproject.github.io
Apache License 2.0
190 stars 87 forks source link

Invalid PVC names created when using Gluster #1449

Closed ctron closed 6 years ago

ctron commented 6 years ago

When using GlusterFS with auto provisioning, EnMasse might create an invalid PVC name:

From the OpenShift log:

Failed to provision volume with StorageClass "glusterfs-storage": create volume error: failed to create endpoint/service error creating service: Service "glusterfs-dynamic-broker-data-eventdefa-967d3474-8ae1-3a59-a7d1-5dcc90396b1f-0" is invalid: metadata.name: Invalid value: "glusterfs-dynamic-broker-data-eventdefa-967d3474-8ae1-3a59-a7d1-5dcc90396b1f-0": must be no more than 63 characters

lulf commented 6 years ago

In the latest version of the templates, I don't think this should be an issue anymore. I.e. the persistent volume claim template should be broker-data, thus not being dependant on the cluster id anymore.

Make sure the templates you deploy with are updated. I.e. they are no longer committed to templates/install anymore, so you need to run 'make -C templates' in order to generate the latest ones.

lulf commented 6 years ago

@ctron See https://github.com/EnMasseProject/enmasse/issues/1105 for the original issue

ctron commented 6 years ago

I was using EnMasse 0.21, downloaded from the releases page. So I need to run "make" there as well?

I did see that it was shorter for pooled queues. But I was uses sharded queues, and there it was way longer. Too long in fact.

lulf commented 6 years ago

@ctron The fix went in after 0.21 I believe. The fix is for sharded queues, where the value of the persintentvolumeClaim prefix was set to the cluster id, whereas it was totally unnecessary and already handled by the statefulset, so it should be fixed in the next release.

ctron commented 6 years ago

68086a91797bce8ef43df21f1150a3118df36d74 is tagged with 0.21 … so I guess that still is an issue.

lulf commented 6 years ago

@ctron Hmm, you're right!

lulf commented 6 years ago

@ctron Then the only fixes I can think of is to bump the number of "reserved" characters in the cluster id from 10 to ... 31! Then we might as well almost just use the UUID.

ctron commented 6 years ago

The full explanation was in that lengthy e-mail thread which we created in your absence … don't bother to read it .. here is the interesting part, completely out of context :wink:

Yes, they are abbreviated to 10 characters from what I saw, but then they get appended by a name UUID …

    private static int MAX_ADDRESS_ID_LENGTH = 10;
    public static String getShardedClusterId(Address address) {
        return KubeUtil.sanitizeWithUuid(address.getAddress().substring(0, Math.min(MAX_ADDRESS_ID_LENGTH, address.getAddress().length())), UUID.nameUUIDFromBytes(address.getName().getBytes(StandardCharsets.UTF_8)).toString());
    }

Which results in:

"glusterfs-dynamic-" = 18 "broker-data-" = 12 abbreviated name = 10 name UUID = 36 "-" + instance number= 2 (or more)

= 78 (or more)

But even leaving out the abbreviated name wouldn't be enough, as this would only bring it down to 68 characters. The longest part in there (aside from the super-important "glusterfs-dynamic-" prefix) would be the name UUID, which is a good idea, unless you are using a network filesystem which requires you to go with 8.3

ctron commented 6 years ago

@ctron Then the only fixes I can think of is to bump the number of "reserved" characters in the cluster id from 10 to ... 31! Then we might as well almost just use the UUID.

Event that wouldn't be enough

lulf commented 6 years ago

Gosh, such mess :) Ok, we could do:

"glusterfs-dynamic-" = 18 "vol-" = 4 UUID = 36 "-" + instance number = 3 (lets assume max 99 pods in the set).

== 61.

That leaves 3 bytes that we can use however we want!! :)

ctron commented 6 years ago

Yes .. that could work as well :grin: And would be a nice default. Maybe you can add the name of the address as a label to that instance, so that you have a chance to find it again or know what the pods are used for. Or maybe that is already being done, I didn't check.

However I do think that having the ability to influence the cluster ID is a nice addition, which might come in handy should some file system have the idea of 32 or 8.3 :wink:

lulf commented 6 years ago

@ctron I agree, its a nice workaround to have for crisis situations! :)

ctron commented 6 years ago

@ctron I agree, its a nice workaround to have for crisis situations! :)

Exactly … just don't mention it in the docs :wink: