[
https://issues.apache.org/jira/browse/YUNIKORN-2287?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17803133#comment-17803133
]
Craig Condit commented on YUNIKORN-2287:
----------------------------------------
I think the problem is that there’s a disconnect between the YAML
representation and the one in the code itself. YAML doest’t natively support
binary data and so the Kubernetes configmap defines the BinaryData field as
base64-encoded (which makes it ASCII-safe). It then decodes the base64 data and
returns it as a []byte in code (and NOT a string).
The initial implementation of the compressed configuration feature failed to
take this into account and decodes the (already-decoded) data again. Since the
original tests never utilized the K8s framework but simply pass a byte array
this was not caught. Unfortunately, the incorrect implantation shipped publicly
in 1.4.0.
The existing implementation is clearly wrong; however it would still be
possible for a user to utilize the functionality by double-encoding the
BinaryData payload in the configuration file (albeit contrary to the
documentation).
I see 3 possible choices:
1) Leave the existing implementation as-is and document the double-encoding
necessary. This preserves backwards compatibility but forces future users to
jump through additional hoops. This also introduces a 25% storage overhead due
to the inefficiency of base64, reducing the effectiveness of the compression
feature.
2) Fix the problem and add a release note entry noting the original feature
was implemented incorrectly and that in the event that the feature was being
used, configuration needs to be modified. This breaks backwards compatibility,
but gives new users a correct implementation.
3) Fix the issue, document the new behavior and deprecate but don’t remove the
old behavior for 1.5.0. This can be accomplished by first parsing the given
data directly as JSON (without base64 decoding). If this succeeds we are done.
If not, retry with base64-decoded data. If the second pass succeeds, use the
config as given but log a deprecation warning. If both passes fail, throw an
error.
My vote would be step 3. This provides proper behavior moving forward,
preserves compatibility and adds minimal additional code. We can possibly
remove the broken decode step in a future release.
> Decompress function doesn't need to decode base64
> -------------------------------------------------
>
> Key: YUNIKORN-2287
> URL: https://issues.apache.org/jira/browse/YUNIKORN-2287
> Project: Apache YuniKorn
> Issue Type: Bug
> Components: shim - kubernetes
> Reporter: PoAn Yang
> Assignee: PoAn Yang
> Priority: Major
> Labels: e2e, pull-request-available
>
> I'm working on https://issues.apache.org/jira/browse/YUNIKORN-2267. I added
> an example as following, but compression configmap doesn't work.
>
> 1. Use gzip to compress and use base64 to encode the config
> ```
> echo "
> partitions:
> - name: default
> placementrules:
> - name: tag
> value: namespace
> create: true
> queues:
> - name: root
> submitacl: '*'
> queues:
> - name: parent
> submitacl: '*'" | gzip | base64
> ```
> 2. Set the result to `queues.yaml.gz` in binaryData field.
>
> Finally, we can see an error log in scheduler:
> ```
> 2023-12-22T15:32:15.913Z ERROR shim.config conf/schedulerconf.go:458
> failed to decode schedulerConfig entry \{"error": "illegal base64 data
> at input byte 0"}
> github.com/apache/yunikorn-k8shim/pkg/conf.Decompress
> github.com/apache/yunikorn-k8shim/pkg/conf/schedulerconf.go:458
> github.com/apache/yunikorn-k8shim/pkg/conf.FlattenConfigMaps
> github.com/apache/yunikorn-k8shim/pkg/conf/schedulerconf.go:495
> github.com/apache/yunikorn-k8shim/pkg/cache.(*Context).triggerReloadConfig
> github.com/apache/yunikorn-k8shim/pkg/cache/context.go:496
> github.com/apache/yunikorn-k8shim/pkg/cache.(*Context).updateConfigMaps
> github.com/apache/yunikorn-k8shim/pkg/cache/context.go:394
> k8s.io/client-go/tools/cache.ResourceEventHandlerFuncs.OnUpdate
> k8s.io/[email protected]/tools/cache/controller.go:250
> k8s.io/client-go/tools/cache.FilteringResourceEventHandler.OnUpdate
> k8s.io/[email protected]/tools/cache/controller.go:315
> k8s.io/client-go/tools/cache.(*processorListener).run.func1
> k8s.io/[email protected]/tools/cache/shared_informer.go:971
> k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1
> k8s.io/[email protected]/pkg/util/wait/backoff.go:226
> k8s.io/apimachinery/pkg/util/wait.BackoffUntil
> k8s.io/[email protected]/pkg/util/wait/backoff.go:227
> k8s.io/apimachinery/pkg/util/wait.JitterUntil
> k8s.io/[email protected]/pkg/util/wait/backoff.go:204
> k8s.io/apimachinery/pkg/util/wait.Until
> k8s.io/[email protected]/pkg/util/wait/backoff.go:161
> k8s.io/client-go/tools/cache.(*processorListener).run
> k8s.io/[email protected]/tools/cache/shared_informer.go:967
> k8s.io/apimachinery/pkg/util/wait.(*Group).Start.func1
> k8s.io/[email protected]/pkg/util/wait/wait.go:72
> ```
>
> I think the root cause is that the golang object is already decoded, so we
> don't need to decode it again.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
