WINC-1438: Compress payload #3162

sebsoto · 2025-08-13T12:13:26Z

Shrinks the WMCO image from 820MB to 459MB.
No significant change in CI runtime.

sebsoto · 2025-08-13T12:14:54Z

/approve cancel

openshift-ci · 2025-08-13T12:15:18Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign mansikulkarni96 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jrvaldes · 2025-08-13T14:08:12Z

pkg/nodeconfig/payload/payload.go

+// Creates a .tar.gz archive from file data
+func createTarGzFile(data []byte, fileName string, outWriter io.Writer) error {
+	// Chain writers: File -> Gzip -> Tar
+	gzipWriter := gzip.NewWriter(outWriter)


consider using func NewWriterLevel(w io.Writer, level int) (*Writer, error) with BestCompression

https://pkg.go.dev/compress/gzip#BestCompression

actually we can do "Level 1 (Fastest)" for testing and CI,
and "Level 9 (Best Compression)" for final release.

I switched to gzip level 9 in the Containerfiles. There was unfortunately no difference.
I'll stick to using the default compression level within the go code, as that is the general recommendation:
build time=9, run time=default(6),

openshift-ci-robot · 2025-08-19T15:23:20Z

@sebsoto: This pull request references WINC-1438 which is a valid jira issue.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2025-08-19T16:00:22Z

@sebsoto: This pull request references WINC-1438 which is a valid jira issue.

In response to this:

Shrinks the WMCO image from 820MB to 459MB.
No significant change in CI runtime.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

wgahnagl · 2025-08-19T17:22:07Z

/lgtm

jrvaldes

@sebsoto thanks for working on this, looks good overall. Left minor comments with suggestions.

jrvaldes · 2025-08-19T17:21:04Z

Containerfile

 # /payload/
 #├── azure-cloud-node-manager.exe
 #├── cni/
-#│   ├── flannel.exe


Is this file no longer present?

Correct, not sure why this was here

jrvaldes · 2025-08-19T17:21:37Z

build/Dockerfile.ci

 # /payload/
 #├── azure-cloud-node-manager.exe
 #├── cni/
-#│   ├── flannel.exe


there is another instance in

windows-machine-config-operator/build/Dockerfile

Line 101 in ea36c7b

#│ ├── flannel.exe

Removed the file in a separate PR

jrvaldes · 2025-08-19T17:26:24Z

Containerfile

+sha256sum /build/windows-machine-config-operator/pkg/internal/hns.psm1 >> sha256sum && \
+chmod -R 644 /payload && chmod -R +X /payload &&\
+# Create directory for generated files with open permissions, this allows WMCO to write to this directory
+mkdir /payload/generated && \


consider creating and set permission in one shot: mkdir -m 0777 /path/to/directory

jrvaldes · 2025-08-19T17:27:54Z

Containerfile

+chmod -R 644 /payload && chmod -R +X /payload &&\
+# Create directory for generated files with open permissions, this allows WMCO to write to this directory
+mkdir /payload/generated && \
+chmod 0777 /payload/generated


for my own education:

why is Read + Write + Execute needed for group and others?

jrvaldes · 2025-08-19T17:35:48Z

pkg/nodeconfig/payload/payload.go

+// input should be of the format generated by the sha256sum binary: `SHA256SUM  filepath`
+func parseShaFile(fileData []byte) (map[string]string, error) {
+	m := make(map[string]string)
+	for _, line := range strings.Split(string(fileData), "\n") {


I recommend to trim spaces and check if string(fileData) is empty first, is so return error

jrvaldes · 2025-08-19T17:36:41Z

pkg/nodeconfig/payload/payload.go

+func parseShaFile(fileData []byte) (map[string]string, error) {
+	m := make(map[string]string)
+	for _, line := range strings.Split(string(fileData), "\n") {
+		if len(line) == 0 {


trim spaces in line before calculating the length

jrvaldes · 2025-08-19T17:39:07Z

pkg/nodeconfig/payload/payload.go

+		if len(line) == 0 {
+			continue
+		}
+		columnSplit := strings.SplitN(line, "  ", 2)


consider a constant for " ", e.g. sha256sumColumnSeparator

jrvaldes · 2025-08-19T17:40:52Z

pkg/nodeconfig/payload/payload.go

+		}
+		fileName := filepath.Base(columnSplit[1])
+		if _, present := m[fileName]; present {
+			return nil, fmt.Errorf("duplicate sha256sum entry for %s", fileName)


Consider logging the checksum as well, they may be different.

I'm having trouble thinking of a case where this would help us with when debugging.

jrvaldes · 2025-08-19T17:42:01Z

pkg/nodeconfig/payload/payload.go

+		if _, present := m[fileName]; present {
+			return nil, fmt.Errorf("duplicate sha256sum entry for %s", fileName)
+		}
+		m[fileName] = columnSplit[0]


I recommend to check columnSplit[0] is not empty

jrvaldes · 2025-08-19T17:43:28Z

pkg/nodeconfig/payload/payload_test.go

 	assert.Equal(t, string(expectedOut), actual)
 }
+
+func TestParseShaFile(t *testing.T) {


consider including other negative test cases, missing filename, blank lines, etc.

jrvaldes · 2025-08-19T17:47:02Z

@sebsoto, why not get all the directories and files into a single compressed file? So that there is only one SSH connection to transfer the payload and one checksum validation?

sebsoto · 2025-08-20T12:30:42Z

@jrvaldes

@sebsoto, why not get all the directories and files into a single compressed file? So that there is only one SSH connection to transfer the payload and one checksum validation?

@jrvaldes
Doing one big transfer has two major downsides:

If any file in the payload changes, the entire payload needs to be transferred again. This would mainly affect upgrades.
If there is a network interruption during the transfer process, the whole transfer begins again. By breaking up the payload into smaller chunks, the files that have been completely transferred already can be skipped, removing the duplicate work.

jrvaldes · 2025-08-20T13:56:37Z

@jrvaldes

@sebsoto, why not get all the directories and files into a single compressed file? So that there is only one SSH connection to transfer the payload and one checksum validation?

@jrvaldes Doing one big transfer has two major downsides:

If any file in the payload changes, the entire payload needs to be transferred again. This would mainly affect upgrades.

If there is a network interruption during the transfer process, the whole transfer begins again. By breaking up the payload into smaller chunks, the files that have been completely transferred already can be skipped, removing the duplicate work.

Makes sense, thanks for the details.

jrvaldes · 2025-08-20T14:06:15Z

If there is a network interruption during the transfer process, the whole transfer begins again. By breaking up the payload into smaller chunks, the files that have been completely transferred already can be skipped, removing the duplicate work

Following this approach, I recommend grouping related and/or dependent files into a single compressed file to reduce the number of files to transfer. For example:
windows_exporter.tar.gz contains:

windows_exporter.exe, and
windows-exporter-webconfig.yaml

instead of separete gz files for each individual file.

WDYT?

sebsoto · 2025-08-20T16:18:51Z

@jrvaldes
That feels a bit like premature optimization.
I dont think the gains we would get by grouping the files would outweigh the additional complexity to map multiple files with different decompressed SHA256's to an archive.

As we share the same SSH connection for all transfers, the downside of having many small files is that each transfer will cause an exec call to tar to unzip. This will only happen if the file needs to be updated. That will probably result in 10s of milliseconds added, I think its worth it to keep things simple.

jrvaldes · 2025-08-20T16:28:30Z

@jrvaldes That feels a bit like premature optimization. I dont think the gains we would get by grouping the files would outweigh the additional complexity to map multiple files with different decompressed SHA256's to an archive.

As we share the same SSH connection for all transfers, the downside of having many small files is that each transfer will cause an exec call to tar to unzip. This will only happen if the file needs to be updated. That will probably result in 10s of milliseconds added, I think its worth it to keep things simple.

true, thanks for the explanation.

Compresses files at build time with gzip. Reduces size of payload from 550MB to 207MB

Transfers payload files as .tar.gz, and decompresses them on the Windows VM. Reduces the amount of bandwidth required to configure a Windows Node, and reduces the duplicate transfer work caused by network faults triggering a transfer failure.

openshift-ci · 2025-10-09T18:19:20Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2025-10-09T23:11:12Z

@sebsoto: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/nutanix-e2e-operator	`07d266c`	link	true	`/test nutanix-e2e-operator`
ci/prow/platform-none-vsphere-e2e-operator	`07d266c`	link	true	`/test platform-none-vsphere-e2e-operator`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 13, 2025

openshift-ci bot requested review from jrvaldes and mansikulkarni96 August 13, 2025 12:13

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 13, 2025

openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 13, 2025

jrvaldes reviewed Aug 13, 2025

View reviewed changes

sebsoto force-pushed the compressPayload branch 2 times, most recently from 74b0d42 to cd45edb Compare August 14, 2025 10:57

sebsoto changed the title ~~[WIP] Compress payload~~ WINC-1438: Compress payload Aug 19, 2025

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 19, 2025

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 19, 2025

sebsoto force-pushed the compressPayload branch from cd45edb to f5871c8 Compare August 19, 2025 15:53

openshift-ci bot assigned wgahnagl Aug 19, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2025

jrvaldes reviewed Aug 19, 2025

View reviewed changes

sebsoto changed the title ~~WINC-1438: Compress payload~~ [WIP] WINC-1438: Compress payload Aug 26, 2025

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 26, 2025

sebsoto added 2 commits October 9, 2025 14:18

[build] Compress WMCO payload files

886d32d

Compresses files at build time with gzip. Reduces size of payload from 550MB to 207MB

Transfer compressed payload files

07d266c

Transfers payload files as .tar.gz, and decompresses them on the Windows VM. Reduces the amount of bandwidth required to configure a Windows Node, and reduces the duplicate transfer work caused by network faults triggering a transfer failure.

sebsoto force-pushed the compressPayload branch from f5871c8 to 07d266c Compare October 9, 2025 18:19

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 9, 2025

sebsoto changed the title ~~[WIP] WINC-1438: Compress payload~~ WINC-1438: Compress payload Oct 9, 2025

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 9, 2025

WINC-1438: Compress payload #3162

Are you sure you want to change the base?

WINC-1438: Compress payload #3162

Uh oh!

Conversation

sebsoto commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebsoto commented Aug 13, 2025

Uh oh!

openshift-ci bot commented Aug 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Aug 19, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Aug 19, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wgahnagl commented Aug 19, 2025

Uh oh!

jrvaldes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrvaldes commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebsoto commented Aug 20, 2025

Uh oh!

jrvaldes commented Aug 20, 2025

Uh oh!

jrvaldes commented Aug 20, 2025

Uh oh!

sebsoto commented Aug 20, 2025

Uh oh!

jrvaldes commented Aug 20, 2025

Uh oh!

openshift-ci bot commented Oct 9, 2025

Uh oh!

openshift-ci bot commented Oct 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sebsoto commented Aug 13, 2025 •

edited

Loading

openshift-ci-robot commented Aug 19, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 19, 2025 •

edited by openshift-ci bot

Loading

jrvaldes commented Aug 19, 2025 •

edited

Loading