[YUNIKORN-2287] Decompress function doesn't need to decode base64 #752

FrankYang0529 · 2023-12-22T15:38:39Z

What is this PR for?

Fix Decompress function and check the following case can run:

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

Set the result to queues.yaml.gz in binaryData field.

What type of PR is it?

- Bug Fix

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2287

How should this be tested?

Covered by e2e test.

codecov · 2023-12-22T15:48:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ce8ac51) 71.30% compared to head (7a21656) 71.32%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #752      +/-   ##
==========================================
+ Coverage   71.30%   71.32%   +0.02%     
==========================================
  Files          43       43              
  Lines        7600     7592       -8     
==========================================
- Hits         5419     5415       -4     
+ Misses       1979     1976       -3     
+ Partials      202      201       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chia7712 · 2023-12-29T07:19:23Z

@FrankYang0529 I have another idea before reviewing. If this function is never in production, can we just get rid of it to simplify our code base?

@wilfred-s @craigcondit WDYT?

wilfred-s

minor tweaks, see comment in the jira would be nice to document why the decode is removed

wilfred-s · 2024-01-04T04:06:34Z

test/e2e/testdata/compressed_yunikorn-configs.yaml

@@ -0,0 +1,33 @@
+#Licensed to the Apache Software Foundation (ASF) under one


space between # and the wording on all lines

Other test data also need to add space to it. Do we want to fix this together in this PR?

https://github.com/apache/yunikorn-k8shim/blob/master/test/e2e/testdata/sleeppod_template.yaml

wilfred-s · 2024-01-04T04:06:36Z

test/e2e/testdata/compressed_yunikorn-configs.yaml

+metadata:
+  name: yunikorn-configs
+binaryData:
+# echo "


Explain what this commented info is about

chia7712 · 2024-01-04T08:43:38Z

why the decode is removed

@wilfred-s did you feel that decode is a kind of behavior change of k8s? This yunikorn feature is already existent for a period of time. We have to keep compatibility for previous k8s if previous k8s does not do base64 decoding.

If the base64 decoding happens way down in the k8s always, it seems we have not got binarydata work in yunikorn. That is to say, that yunikorn feature is nonexistent actually :)

In short, removing this feature seems to be acceptable. The benefit includes

don't worry for k8s compatibility (if it exists)
simplify code base

chia7712 · 2024-01-04T12:09:34Z

Oh, the PR #675 got merged into 1.4.0 on 2023.09.19

My compatibility concern was redundant as the target k8s is almost equal to current version.

It seems there is no +1 on removing this feature, and maintaining this feature is ok to me.

@FrankYang0529 please address the comments and I will take a look later also.

FrankYang0529 · 2024-01-05T16:29:54Z

see comment in the jira would be nice to document why the decode is removed

I don't have a good reference to show why the configmap in Golang object is already decoded, but I may be able to proof that if object from our utility function GetConfigMapObj is decoded, then object from CoreV1().ConfigMaps(namespace).Get is decoded too. The reason is that both functions use same serializers.

GetConfigMapObj

It uses scheme.Codecs.UniversalDeserializer().Decode.
Codecs is from serializer.NewCodecFactory.
In NewCodecFactory, it gets serializers from newSerializersForScheme.
In newCodecFactory, it collects all serializers as decoders and sets it as runtime.Decoder to universal.
The universal is a runtime.Decoder from recognizer.NewDecoder.
In recognizer.Decoder.Decode, it tries all serializers.

CoreV1().ConfigMaps(namespace).Get

In CoreV1().ConfigMaps(namespace).Get, it calls Do.
In Do, it calls Request.transformResponse.
In transformResponse, it calls r.c.content.Negotiator.Decoder.
In Decoder, it checks media type and content type, and gets serializer from SerializerInfo.
The SerializerInfo is from SerializerInfoForMediaType. The SerializerInfoForMediaType is just a utility function to check which mediaTypes match to contentType, so we have to check where mediaTypes from.
The mediaTypes is from n.serializer.SupportedMediaTypes.
In DecoderToVersion, the serializer is equal to runtime.Decoder.

If we can proof the runtime.Decoder in both functions are same, then they will have same behavior - getting a decoded configmap. We have to check which structure implements SupportedMediaTypes.

In k8shim, we get kubernetes.Clientset from kubernetes.NewForConfig.
In NewForConfig, it calls NewForConfigAndClient.
In NewForConfigAndClient, it calls corev1.NewForConfigAndClient.
In corev1.NewForConfigAndClient.
4-1a. In corev1.NewForConfigAndClient, it calls setConfigDefaults.
4-1b. In setConfigDefaults, it sets NegotiatedSerializer as scheme.Codecs.WithoutConversion. The scheme.Codecs is same as first step in k8s.GetConfigMapObj.
4-1c. In WithoutConversion, it return WithoutConversionCodecFactory{f}. This structure has DecoderToVersion function in step 7 (last list).
4-2a. In corev1.NewForConfigAndClient, it calls rest.RESTClientForConfigAndClient.
4-2b. In rest.RESTClientForConfigAndClient, it sets Negotiator as runtime.NewClientNegotiator. This function uses config.NegotiatedSerializer which is equal to step 4-1b.

Finally, NegotiatedSerializer is WithoutConversionCodecFactory and it also is CodecFactory. The SupportedMediaTypes in CodecFactory returns f.accepts which gets same serializers as f.universal.

craigcondit · 2024-01-05T17:24:52Z

see comment in the jira would be nice to document why the decode is removed

I think there's more than enough comments in the JIRA relating to why this is not needed. Removing an unnecessary line of code from the PR doesn't warrant a comment either. What would it mean to a reader a year from now? (probably nothing at all).

chia7712 · 2024-01-05T17:38:15Z

@FrankYang0529 could you please fix the conflicts?

chia7712

@FrankYang0529 thanks for this fix. please take a look at following comments.

test/e2e/bin_packing/bin_packing_test.go

test/e2e/configmap/configmap_suite_test.go

chia7712 · 2024-01-06T18:34:48Z

@FrankYang0529 please rebase PR as #761 is already merged.

FrankYang0529 · 2024-01-07T13:57:43Z

The e2e failed case is Verify_basic_preemption which is not related to this PR and is filed in another JIRA YUNIKORN-2313.

chia7712

@FrankYang0529 thanks for updating patch. a couple of comments below.

test/e2e/configmap/configmap_suite_test.go

chia7712 · 2024-01-07T17:07:29Z

test/e2e/configmap/configmap_test.go

+		Ω(oldConfigMap).NotTo(BeNil())
+	})
+
+	It("Verify_Compressed_ConfigMap", func() {


It seems to me this e2e should focus on the way that YuniKorn loads the config file. Hence, loading the file is more suitable for this e2e rather than creating a compressed file directly. WDYT?

Do you mean that we just use Verify_Compressed_ConfigMap_From_File and remove Verify_Compressed_ConfigMap?

yep, it seems we should remove Verify_Compressed_ConfigMap and then add a new e2e to load non-compressed config file.

chia7712 · 2024-01-12T08:43:37Z

@FrankYang0529 please fix conflicts and rebase code

chia7712 · 2024-01-12T11:33:24Z

pkg/conf/schedulerconf.go

 	splitKey := strings.Split(key, ".")
 	compressionAlgo := splitKey[len(splitKey)-1]
 	if strings.EqualFold(compressionAlgo, constants.GzipSuffix) {
-		reader := bytes.NewReader(decodedValue)
+		reader := bytes.NewReader(value)
 		gzReader, err := gzip.NewReader(reader)


How about gzReader, err := gzip.NewReader(bytes.NewReader(value))?

Yes, updated it. Thank you.

Signed-off-by: PoAn Yang <[email protected]>

chia7712

LGTM

FrankYang0529 self-assigned this Dec 22, 2023

FrankYang0529 requested a review from chia7712 December 22, 2023 15:39

FrankYang0529 force-pushed the YUNIKORN-2287 branch from f04525e to 65fea0d Compare December 22, 2023 15:42

FrankYang0529 closed this Dec 22, 2023

FrankYang0529 force-pushed the YUNIKORN-2287 branch from 65fea0d to 24c699f Compare December 22, 2023 15:45

FrankYang0529 reopened this Dec 22, 2023

FrankYang0529 force-pushed the YUNIKORN-2287 branch 5 times, most recently from 60037a6 to 04a43c3 Compare December 29, 2023 03:40

wilfred-s requested changes Jan 4, 2024

View reviewed changes

chia7712 mentioned this pull request Jan 4, 2024

[YUNIKORN-1875] Support for Compressed Encoded configMap BinaryData, gzip compression support #675

Closed

10 tasks

FrankYang0529 force-pushed the YUNIKORN-2287 branch from 04a43c3 to 1b11082 Compare January 6, 2024 03:48

chia7712 reviewed Jan 6, 2024

View reviewed changes

test/e2e/bin_packing/bin_packing_test.go Outdated Show resolved Hide resolved

test/e2e/configmap/configmap_suite_test.go Outdated Show resolved Hide resolved

FrankYang0529 force-pushed the YUNIKORN-2287 branch from 1b11082 to 091069d Compare January 6, 2024 12:00

FrankYang0529 force-pushed the YUNIKORN-2287 branch from 091069d to ab813b6 Compare January 7, 2024 03:18

chia7712 reviewed Jan 7, 2024

View reviewed changes

FrankYang0529 force-pushed the YUNIKORN-2287 branch from ab813b6 to 5a11535 Compare January 12, 2024 10:31

chia7712 reviewed Jan 12, 2024

View reviewed changes

[YUNIKORN-2287] Decompress function doesn't need to decode base64

7a21656

Signed-off-by: PoAn Yang <[email protected]>

FrankYang0529 force-pushed the YUNIKORN-2287 branch from 5a11535 to 7a21656 Compare January 12, 2024 16:34

chia7712 approved these changes Jan 14, 2024

View reviewed changes

chia7712 closed this in 57f11dc Jan 14, 2024

FrankYang0529 deleted the YUNIKORN-2287 branch January 15, 2024 02:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[YUNIKORN-2287] Decompress function doesn't need to decode base64 #752

[YUNIKORN-2287] Decompress function doesn't need to decode base64 #752

FrankYang0529 commented Dec 22, 2023

codecov bot commented Dec 22, 2023 •

edited

Loading

chia7712 commented Dec 29, 2023

wilfred-s left a comment

wilfred-s Jan 4, 2024

FrankYang0529 Jan 4, 2024

wilfred-s Jan 4, 2024

chia7712 commented Jan 4, 2024

chia7712 commented Jan 4, 2024

FrankYang0529 commented Jan 5, 2024 •

edited

Loading

craigcondit commented Jan 5, 2024

chia7712 commented Jan 5, 2024

chia7712 left a comment

chia7712 commented Jan 6, 2024

FrankYang0529 commented Jan 7, 2024

chia7712 left a comment

chia7712 Jan 7, 2024

FrankYang0529 Jan 12, 2024

chia7712 Jan 12, 2024

chia7712 commented Jan 12, 2024

chia7712 Jan 12, 2024

FrankYang0529 Jan 12, 2024

chia7712 left a comment

		@@ -0,0 +1,33 @@
		#Licensed to the Apache Software Foundation (ASF) under one

[YUNIKORN-2287] Decompress function doesn't need to decode base64 #752

[YUNIKORN-2287] Decompress function doesn't need to decode base64 #752

Conversation

FrankYang0529 commented Dec 22, 2023

What is this PR for?

What type of PR is it?

What is the Jira issue?

How should this be tested?

codecov bot commented Dec 22, 2023 • edited Loading

Codecov Report

chia7712 commented Dec 29, 2023

wilfred-s left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chia7712 commented Jan 4, 2024

chia7712 commented Jan 4, 2024

FrankYang0529 commented Jan 5, 2024 • edited Loading

GetConfigMapObj

CoreV1().ConfigMaps(namespace).Get

craigcondit commented Jan 5, 2024

chia7712 commented Jan 5, 2024

chia7712 left a comment

Choose a reason for hiding this comment

chia7712 commented Jan 6, 2024

FrankYang0529 commented Jan 7, 2024

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chia7712 commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 22, 2023 •

edited

Loading

FrankYang0529 commented Jan 5, 2024 •

edited

Loading