WINC-1222: WMCO applies image mirroring settings to Windows instances #2163

saifshaikh48 · 2024-05-07T20:14:59Z

WINC-1222: WMCO applies image mirroring settings to Windows instances

This PR transfers containerd registry config files generated by WMCO to
each Windows instance based on the cluster's global image mirroring settings,
allowing containerd to pull Windows images from user-defined mirror locations.

The containerd daemon picks up the registry config without requiring restart.

openshift-ci-robot · 2024-05-10T03:51:16Z

@saifshaikh48: This pull request references WINC-1222 which is a valid jira issue.

In response to this:

This PR transfers containerd registry config files generated by WMCO to
each Windows instance based on the cluster's global image mirroring settings,
allowing containerd to pull Windows images from user-defined mirror locations.

The containerd daemon picks up the registry config without requiring restart.

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.

saifshaikh48 · 2024-05-10T03:52:22Z

/test vsphere-disconnected-e2e-operator

for early feedback

saifshaikh48 · 2024-05-10T17:21:14Z

build/Dockerfile

+# Create directory for generated registry config files with open permissions
+RUN mkdir /payload/generated/cri_registry_config
+RUN chmod 0777 /payload/generated/cri_registry_config


Will need to update midstream Dockerfile

saifshaikh48 · 2024-05-10T20:34:37Z

/test aws-e2e-operator
/test azure-e2e-operator
/test vsphere-disconnected-e2e-operator

saifshaikh48 · 2024-05-13T14:20:55Z

/test aws-e2e-operator
/test vsphere-disconnected-e2e-operator
/test gcp-e2e-operator
/test nutanix-e2e-operator

passed on azure, couldn't find anything strange so trying other platforms

saifshaikh48 · 2024-05-13T18:08:33Z

/test aws-e2e-operator
/test vsphere-e2e-operator

azure, gcp, nutanix CI all passed. aws passed locally so seeing what's up in CI
vsphere-disconnected hitting issues since golden images are currently missing from devqe env. other vsphere jobs should not hit this though

sebsoto

Nice work. Please look at my comments.

pkg/registries/registries.go

sebsoto · 2024-05-14T14:10:53Z

pkg/registries/registries.go

+
+// extractHost extracts just the initial host repo from a full image location
+// e.g. mcr.microsoft.com would be extracted from mcr.microsoft.com/oss/kubernetes/pause:3.9
+func extractHost(fullImage string) string {


Host -> Hostname

sebsoto · 2024-05-14T14:13:23Z

build/Dockerfile

@@ -112,6 +112,7 @@ RUN make build-daemon
 #│ ├── csi-proxy.exe
 #├── ecr-credential-provider.exe
 #├── generated/
+#│ └── cri_registry_config/


Why does this need to be created?

ah it doesn't, vestige of the previous solution. I will remove it

test/e2e/mirror_test.go

sebsoto · 2024-05-14T14:20:28Z

test/e2e/mirror_test.go

+ // No point in running the rest of the tests if settings aren't applied
+ return
+ }
+ t.Run("Workload using a mirrored container image", tc.testWorkloadWithMirroredImage)


Please do not make this a separate test.
Rather setup the mirrors as part of the test setup, and we should see the tests on a disconnected job pass as normal.

You can make the mirror setup configurable via a CMD line option,

Not quite sure what you mean here

Should we only test mirroring through the disconnected job?

Do you forsee the mirror_test suite to fail in the disconnected job?

Should we only test mirroring through the disconnected job?

I don't see it necessary to test it elsewhere if the disconnected job is required.

Do you forsee the mirror_test suite to fail in the disconnected job?

I'm not sure what you mean by this.

sebsoto · 2024-05-14T14:29:35Z

pkg/windows/windows.go

@@ -361,11 +369,16 @@ func (vm *windows) EnsureFile(file *payload.FileInfo, remoteDir string) error {
 vm.log.Error(err, "error closing local file", "file", file.Path)
 }
 }()
- vm.log.V(1).Info("copy", "local file", file.Path, "remote dir", remoteDir)
- if err := vm.interact.transfer(f, filepath.Base(file.Path), remoteDir); err != nil {
+ vm.log.V(1).Info("copying", "local file", file.Path, "remote dir", remoteDir)


please revert

sebsoto · 2024-05-14T14:30:00Z

pkg/windows/windows.go

@@ -334,11 +337,16 @@ func (vm *windows) EnsureFileContent(contents []byte, filename string, remoteDir
 // The file already exists with the expected content, do nothing
 return nil
 }
- vm.log.V(1).Info("copy", "file content", filename, "remote dir", remoteDir)
- if err := vm.interact.transfer(bytes.NewReader(contents), filename, remoteDir); err != nil {
+ vm.log.V(1).Info("copying", "file content", filename, "remote dir", remoteDir)


please revert

sebsoto · 2024-05-14T14:31:00Z

pkg/windows/windows.go

+
+ c, err := vm.interact.createSFTPClient()
+ if err != nil {
+ return err


please wrap error for easier debugging

sebsoto · 2024-05-14T14:39:33Z

pkg/windows/connectivity.go

+ dstPath := remoteDir + "\\" + workingPath
+ // Get the directory that will be written into, without trailing slash, and the base file name
+ splitIndex := strings.LastIndexByte(dstPath, '\\')
+ writeDir, filename := dstPath[:splitIndex], dstPath[splitIndex+1:]


Please separate this into two lines for readability

sebsoto · 2024-05-14T14:39:59Z

pkg/windows/connectivity.go

+
+func (c *sshConnectivity) transferFiles(sftpClient *sftp.Client, files map[string][]byte, remoteDir string) error {
+ for workingPath, content := range files {
+ // Load file content into io.Reader


unecessary comment

mansikulkarni96 · 2024-05-14T13:49:22Z

bundle/manifests/windows-machine-config-operator.clusterserviceversion.yaml

@@ -23,7 +23,7 @@ metadata:
 operatorframework.io/suggested-namespace: openshift-windows-machine-config-operator
 operators.openshift.io/valid-subscription: '["Red Hat OpenShift support for Windows
 Containers"]'
- operators.operatorframework.io/builder: operator-sdk-v1.32.0+git
+ operators.operatorframework.io/builder: operator-sdk-v1.32.0


can remove this, if not required

Not required but is there any reason for the +git?

Those are generated when running make bundle, and need to be reverted every time. Make bundle removes the +git.

mansikulkarni96 · 2024-05-14T14:38:38Z

pkg/nodeconfig/nodeconfig.go

@@ -152,6 +153,9 @@ func (nc *nodeConfig) Configure() error {
 if err := nc.createBootstrapFiles(); err != nil {
 return err
 }
+ if err := nc.createRegistryConfigFiles(); err != nil {


we are creating the Containerd directory inside nc.Windows.Boostrap(), why don't we call this function after it is created? If needed, we can also split the bootstrap function into two. Let me know if there is another reason for doing this here.

My thoughts here were that createBootstrapFiles() sets up files needed for kubelet and createRegistryConfigFiles() basically does the same for containerd (and containerd is a pre-req service to kubelet) so the funcs should be called together

we are creating the Containerd directory inside nc.Windows.Boostrap()

This is a good point though, I will explore your suggestion

I thought about this more and decided to keep it where it is (though I can still be swayed)
Additional reasoning was

this way the registry config is created before containerd service actually starts so it doesn't have to do any additional config loading during runtime

createRegistryConfigFiles() creates the required directories if needed so no issue

mansikulkarni96 · 2024-05-14T14:38:50Z

pkg/nodeconfig/nodeconfig.go

@@ -370,6 +374,15 @@ func (nc *nodeConfig) write(pathToData map[string]string) error {
 return nil
 }

+// createBootstrapFiles creates all files on the node required for containerd to mirror images


doc needs update

mansikulkarni96 · 2024-05-14T14:44:51Z

pkg/windows/windows.go

@@ -391,6 +404,26 @@ func (vm *windows) FileExists(path, checksum string) (bool, error) {
 return false, nil
 }

+func (vm *windows) ReplaceDir(files map[string][]byte, remoteDir string) error {


doc'd in the Windows interface

saifshaikh48 · 2024-06-03T16:25:21Z

/retest-required

saifshaikh48 · 2024-06-03T18:00:10Z

/test remaining-required

saifshaikh48 · 2024-06-03T21:59:46Z

/test remaining-required

saifshaikh48 · 2024-06-04T14:31:09Z

/test remaining-required

saifshaikh48 · 2024-06-04T18:10:32Z

/test aws-e2e-operator

mansikulkarni96 · 2024-06-04T18:52:04Z

test/e2e/mirror_test.go

+ badPowershellImageLocation = "test.registry.io/powershell"
+)
+
+// numBaseRegistryConfigFiles represents the number registry config files that exist before this test suite's steps run


mansikulkarni96 · 2024-06-04T18:56:15Z

test/e2e/mirror_test.go

+ require.NoError(t, err, "unable to get node address")
+
+ // retry to give time for registry controller to transfer generated config to all nodes
+ err = wait.PollUntilContextTimeout(context.TODO(), 15*time.Second, 5*time.Minute, true,


isn't the retry time same as retry.Interval?

mansikulkarni96 · 2024-06-04T19:09:37Z

pkg/windows/windows.go

+ if err = vm.interact.transferFiles(sftpClient, files, remoteDir); err != nil {
+ return err
+ }
+ return sftpClient.Close()


shouldn't the client be closed even if we return error in between? This also applies to a few other places in this commit.

This commit transfers containerd mirror registry config files generated by WMCO to each Windows instance based on the cluster's global image mirroring settings. This is done both as part of node configuration and when IDMS/ITMSs change. The containerd daemon picks up the registry config without requiring restart. Edits the way file transfers work for easier reuse of FTP clients.

Includes tests to verify registry config file creation on Windows nodes when cluster mirror registry settings are added, and file deletion when cluster settings are cleared. Mirror setting clearing is tested in the delete suite. This is done to avoid a timing issue in the e2e test suite where we were doing too many drain-causing actions too quickly. Adding/removing registry config to the cluster can cause linux nodes to drain which continuously disrupted the WMCO pod while it was trying to reconcile.

Adds an explicit predicate to react to all events on IDMS/ITMS resources in case the default behavior controller manager watch before changes.

mansikulkarni96 · 2024-06-04T20:12:55Z

/lgtm

jrvaldes · 2024-06-04T20:18:41Z

/lgtm

@saifshaikh48 please fetch and rebase the latest from master before triggering the rest of the tests.

mtnbikenc · 2024-06-04T20:30:39Z

/lgtm

@saifshaikh48 please fetch and rebase the latest from master before triggering the rest of the tests.

@jrvaldes Why is rebasing against master necessary here? Tests are always run against the latest master with the current PR applied?

saifshaikh48 · 2024-06-04T20:51:42Z

/test remaining-required

saifshaikh48 · 2024-06-04T20:57:55Z

/cherry-pick release-4.16

openshift-cherrypick-robot · 2024-06-04T20:57:57Z

@saifshaikh48: once the present PR merges, I will cherry-pick it on top of release-4.16 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.16

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.

jrvaldes · 2024-06-04T21:22:39Z

/lgtm

@saifshaikh48 please fetch and rebase the latest from master before triggering the rest of the tests.

@jrvaldes Why is rebasing against master necessary here? Tests are always run against the latest master with the current PR applied?

No need, just a nice to have.

Lately, I'm noticing the remaining-required re-testing twice if there is a diff from the head of master.

openshift-ci-robot · 2024-06-04T23:54:08Z

/retest-required

Remaining retests: 0 against base HEAD dbb9316 and 2 for PR HEAD 714564b in total

sebsoto · 2024-06-05T01:13:29Z

/test aws-e2e-operator

jrvaldes · 2024-06-05T01:33:40Z

/test aws-e2e-operator

ipi-install failed

saifshaikh48 · 2024-06-05T03:55:53Z

/test aws-e2e-operator

openshift-ci · 2024-06-05T06:01:57Z

@saifshaikh48: all tests passed!

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-cherrypick-robot · 2024-06-05T06:05:11Z

@saifshaikh48: new pull request created: #2199

In response to this:

/cherry-pick release-4.16

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.

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 May 7, 2024

openshift-ci bot requested review from mansikulkarni96 and sebsoto May 7, 2024 20:16

saifshaikh48 force-pushed the mirror-registry-config-containerd branch 6 times, most recently from b16235f to 3f2e966 Compare May 9, 2024 22:24

saifshaikh48 changed the title ~~[WIP] WINC-1222 WMCO applies image mirroring settings to Windows instances~~ [WIP] WINC-1222: WMCO applies image mirroring settings to Windows instances May 10, 2024

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

saifshaikh48 force-pushed the mirror-registry-config-containerd branch 4 times, most recently from a5d30bd to ad79261 Compare May 10, 2024 17:20

saifshaikh48 commented May 10, 2024

View reviewed changes

saifshaikh48 changed the title ~~[WIP] WINC-1222: WMCO applies image mirroring settings to Windows instances~~ WINC-1222: WMCO applies image mirroring settings to Windows instances May 10, 2024

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 May 10, 2024

saifshaikh48 force-pushed the mirror-registry-config-containerd branch 2 times, most recently from 2e77c98 to 5101dc3 Compare May 10, 2024 20:32

saifshaikh48 force-pushed the mirror-registry-config-containerd branch from 5101dc3 to 604d204 Compare May 13, 2024 17:59

saifshaikh48 force-pushed the mirror-registry-config-containerd branch from 604d204 to 617ac10 Compare May 13, 2024 20:39

sebsoto requested changes May 14, 2024

View reviewed changes

mansikulkarni96 suggested changes May 14, 2024

View reviewed changes

saifshaikh48 force-pushed the mirror-registry-config-containerd branch from 617ac10 to 8e74444 Compare May 14, 2024 17:07

saifshaikh48 force-pushed the mirror-registry-config-containerd branch from c914cc9 to 75cf57f Compare June 3, 2024 16:24

saifshaikh48 force-pushed the mirror-registry-config-containerd branch 3 times, most recently from dcd1dd0 to 8fa3636 Compare June 4, 2024 14:29

mansikulkarni96 suggested changes Jun 4, 2024

View reviewed changes

saifshaikh48 added 3 commits June 4, 2024 15:29

[registry_controller] Use explicit predicate

714564b

Adds an explicit predicate to react to all events on IDMS/ITMS resources in case the default behavior controller manager watch before changes.

saifshaikh48 force-pushed the mirror-registry-config-containerd branch from 8fa3636 to 714564b Compare June 4, 2024 19:44

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 4, 2024

openshift-merge-bot bot merged commit 0d0867b into openshift:master Jun 5, 2024
17 checks passed

openshift-cherrypick-robot mentioned this pull request Jun 5, 2024

[release-4.16] WINC-1222: WMCO applies image mirroring settings to Windows instances #2199

Merged

WINC-1222: WMCO applies image mirroring settings to Windows instances #2163

WINC-1222: WMCO applies image mirroring settings to Windows instances #2163

Conversation

saifshaikh48 commented May 7, 2024 • edited

openshift-ci-robot commented May 10, 2024 • edited by openshift-ci bot

saifshaikh48 commented May 10, 2024

Choose a reason for hiding this comment

saifshaikh48 commented May 10, 2024

saifshaikh48 commented May 13, 2024

saifshaikh48 commented May 13, 2024

sebsoto 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

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

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

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

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

Choose a reason for hiding this comment

saifshaikh48 commented Jun 3, 2024

saifshaikh48 commented Jun 3, 2024

saifshaikh48 commented Jun 3, 2024

saifshaikh48 commented Jun 4, 2024

saifshaikh48 commented Jun 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mansikulkarni96 commented Jun 4, 2024

jrvaldes commented Jun 4, 2024

mtnbikenc commented Jun 4, 2024

saifshaikh48 commented Jun 4, 2024

saifshaikh48 commented Jun 4, 2024

openshift-cherrypick-robot commented Jun 4, 2024

jrvaldes commented Jun 4, 2024

openshift-ci-robot commented Jun 4, 2024

sebsoto commented Jun 5, 2024

jrvaldes commented Jun 5, 2024

saifshaikh48 commented Jun 5, 2024

openshift-ci bot commented Jun 5, 2024

openshift-cherrypick-robot commented Jun 5, 2024

saifshaikh48 commented May 7, 2024 •

edited

openshift-ci-robot commented May 10, 2024 •

edited by openshift-ci bot