WINC-740: Configure kube-proxy with WICD #1160

sebsoto · 2022-08-05T01:44:58Z

Adds functionality for WICD to run PowerShell scripts to resolve service variables
Moves network configuration into a PowerShell script, a requirement for it to be done by WICD
Moves kube-proxy configuration into WICD

sebsoto · 2022-08-05T12:07:49Z

/approve cancel

sebsoto · 2022-08-08T12:57:59Z

/retest

sebsoto · 2022-08-10T17:16:26Z

/retest

jrvaldes

Mostly, LGTM. Just minor comments.

Great work @sebsoto

jrvaldes · 2022-08-24T20:20:32Z

pkg/nodeconfig/payload/payload.go

@@ -82,3 +84,39 @@ func NewFileInfo(path string) (*FileInfo, error) {
 		SHA256: fmt.Sprintf("%x", sha256.Sum256(contents)),
 	}, nil
 }
+
+// Validate ensures all required payload files exist
+func Validate() error {


Do you anticipate more validations to be included here? Otherwise, I don't feel the Validate() function is needed just to hold the collection of files. You can call directly checkIfFilesExist.

jrvaldes · 2022-08-24T20:22:45Z

cmd/operator/main_test.go

@@ -1,25 +1 @@
 package main


Do we need to keep the empty file?

saifshaikh48

LGTM aside from the network-config script testing. I am fine with the CNI configuration and kube-proxy starting being separated at a later time.

saifshaikh48 · 2022-09-01T19:39:24Z

cmd/daemon/bootstrap.go

-
-	// Cannot use a cached client as no manager will be started to populate cache
-	directClient, err := controller.NewDirectClient(cfg)
+	sc, err := controller.NewServiceController(context.TODO(), "", controller.Options{Config: cfg})


out of scope of this PR: we should not have to create a service controller object to run bootstrap

That's a good point.

@saifshaikh48 I'm not sure what you mean. I'm not introducing that behavior here. Bootstrap is a method of the service controller struct.
see code in current master:

windows-machine-config-operator/cmd/daemon/bootstrap.go

Line 71 in 82a718c

if err := sc.Bootstrap(desiredVersion); err != nil {

Sorry, I wasn't clear. My comment is a general comment -- ideally we shouldn't have Bootstrap be a method on a controller object. Just something to keep in mind as future work.

Should we create a tech debt story around this?

Sure. https://issues.redhat.com/browse/WINC-886

saifshaikh48

just one comment, LGTM otherwise

saifshaikh48 · 2022-09-08T14:12:03Z

pkg/nodeconfig/payload/payload.go

-	// CNIConfigTemplatePath is the path for CNI config template
-	CNIConfigTemplatePath = payloadDirectory + cniDirectory + "cni-conf-template.json"
+	// NetworkConfigurationScript is the path for generated Network configuration Script
+	NetworkConfigurationScript = payloadDirectory + "/generated/network-conf.ps1"


Should we add this to the dockerfile payload diagram?

#├── generated #│ └── network-conf.ps1

Added the generated directory, as the diagram is describing how the dockerfile is building the payload, not WMCO

alinaryan

Looking good so far. Just curious, why isn’t WICD able to run PS commands like WMCO does? Also, how are you testing?

This commit introduces the Options struct for the controller package. This is a pattern that is seen throughout various kubernetes libraries. All parameters for NewServiceController() that can be made to have a reasonable default value have been moved into this struct which will be passed into the function instead. The purpose of doing this is to provide a cleaner interface for users calling NewServiceController, by reducing the amount of parameters that may not be necessary for the caller, and thus reducing copy pasted repeat code.

Resolves PowerShell variables present in the Windows service configmap when starting Windows services.

sebsoto · 2022-09-12T13:07:54Z

Just curious, why isn’t WICD able to run PS commands like WMCO does?

WICD is designed to enact policy as described by WMCO. Any scripting required to configure a service must be done through the windows-services configmap, as that is what defines what WICD must do to sucessfully configure a node.

Also, how are you testing?

The powershell functionality is being tested through the added WICD unit tests. The kube-proxy changes are being tested through the existing e2e test suite.

mansikulkarni96 · 2022-09-12T14:42:52Z

LGTM

alinaryan · 2022-09-12T16:53:25Z

LGTM

aravindhp

Nice work, @sebsoto

aravindhp · 2022-09-12T16:00:54Z

cmd/daemon/bootstrap.go

-
-	// Cannot use a cached client as no manager will be started to populate cache
-	directClient, err := controller.NewDirectClient(cfg)
+	sc, err := controller.NewServiceController(context.TODO(), "", controller.Options{Config: cfg})


Should we create a tech debt story around this?

pkg/nodeconfig/payload/payload.go

aravindhp · 2022-09-12T18:18:48Z

pkg/windows/windows.go

@@ -547,18 +535,18 @@ func (vm *windows) ConfigureAzureCloudNodeManager(nodeName string) error {
 }

 func (vm *windows) ConfigureKubeProxy(nodeName, hostSubnet string) error {
-	endpointIP, err := vm.createHNSEndpoint()
+	endpointIP, err := vm.Run(NetworkConfScriptPath, true)


Did you create a follow up story to separate this out i.e. introduce allowing WICD to run PowerShell scripts.

openshift-ci · 2022-09-12T18:21:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aravindhp

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [aravindhp]

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

This commit moves the CNI configuration and HNS network setup into a new Powershell script network-conf.ps1. This is required in order to enable the transition of responsibilities from WMCO to WICD. WICD cannot run arbitrary Powershell commands in the same way WMCO does. Instead any required commands must be made part of a script which is ran before starting a specific service. The script `network-conf.ps1` is being generated at runtime. This enables the use of variable replacement for values that will not change while WMCO is running.

This commit moves the responsibility of configuring kube-proxy out of WMCO, and into WICD. WMCO still needs to wait for kube-proxy to be running, as it is currently responsible for uncordoning the node, and applying the version annotation on it, indicating the node has been sucessfully configured.

alinaryan · 2022-09-12T19:52:39Z

/lgtm

openshift-ci-robot · 2022-09-12T21:54:17Z

/retest-required

Remaining retests: 0 against base HEAD 5928b62 and 2 for PR HEAD 95a2c52 in total

openshift-ci-robot · 2022-09-13T01:07:29Z

/retest-required

Remaining retests: 0 against base HEAD 1a1f09b and 1 for PR HEAD 95a2c52 in total

sebsoto · 2022-09-13T12:02:23Z

/retest

sebsoto · 2022-09-13T14:33:46Z

/test vsphere-e2e-operator

sebsoto · 2022-09-13T18:24:37Z

/test gcp-e2e-operator

sebsoto · 2022-09-14T02:48:52Z

/retest-required

openshift-ci · 2022-09-14T06:45:33Z

@sebsoto: 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/test-infra 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 5, 2022

openshift-ci bot requested review from aravindhp and mansikulkarni96 August 5, 2022 01:45

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

sebsoto force-pushed the betterWicdKP branch 2 times, most recently from ad7ddc9 to 5895c78 Compare August 5, 2022 11:26

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

sebsoto force-pushed the betterWicdKP branch 2 times, most recently from dc342d0 to bcc37ca Compare August 5, 2022 14:08

sebsoto changed the title ~~[WIP] Move CNI commands to PS script~~ Move Node network setup to PS script Aug 5, 2022

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 5, 2022

sebsoto force-pushed the betterWicdKP branch from bcc37ca to a8212b5 Compare August 6, 2022 13:09

sebsoto force-pushed the betterWicdKP branch from a8212b5 to 74c9664 Compare August 8, 2022 15:18

sebsoto changed the title ~~Move Node network setup to PS script~~ [WIP] Move Node network setup to PS script Aug 9, 2022

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 9, 2022

sebsoto force-pushed the betterWicdKP branch from 74c9664 to d7406b6 Compare August 17, 2022 18:04

sebsoto changed the title ~~[WIP] Move Node network setup to PS script~~ Move Node network setup to PS script Aug 19, 2022

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, 2022

jrvaldes reviewed Aug 24, 2022

View reviewed changes

sebsoto changed the title ~~Move Node network setup to PS script~~ [WIP] Move Node network setup to PS script Aug 26, 2022

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, 2022

sebsoto force-pushed the betterWicdKP branch 2 times, most recently from f28a609 to a130a45 Compare August 26, 2022 14:00

sebsoto changed the title ~~[WIP] Move Node network setup to PS script~~ WINC-740: Configure kube-proxy with WICD Aug 26, 2022

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 26, 2022

sebsoto force-pushed the betterWicdKP branch from a130a45 to e4ad475 Compare August 26, 2022 17:46

sebsoto changed the title ~~WINC-740: Configure kube-proxy with WICD~~ [WIP] WINC-740: Configure kube-proxy with WICD Aug 26, 2022

saifshaikh48 reviewed Sep 1, 2022

View reviewed changes

sebsoto force-pushed the betterWicdKP branch from 7249208 to 3389087 Compare September 8, 2022 13:51

saifshaikh48 reviewed Sep 8, 2022

View reviewed changes

alinaryan reviewed Sep 8, 2022

View reviewed changes

sebsoto added 3 commits September 8, 2022 19:58

[wicd] Properly return on RunController() error

3eb4691

[wicd] Resolve PowerShell variables

29f5f1f

Resolves PowerShell variables present in the Windows service configmap when starting Windows services.

sebsoto force-pushed the betterWicdKP branch 3 times, most recently from cf47841 to 86ee678 Compare September 12, 2022 13:03

aravindhp approved these changes Sep 12, 2022

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 12, 2022

sebsoto added 2 commits September 12, 2022 15:50

sebsoto force-pushed the betterWicdKP branch from 86ee678 to 95a2c52 Compare September 12, 2022 19:50

openshift-ci bot assigned alinaryan Sep 12, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2022

openshift-merge-robot merged commit 7ae5e7f into openshift:master Sep 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WINC-740: Configure kube-proxy with WICD #1160

WINC-740: Configure kube-proxy with WICD #1160

sebsoto commented Aug 5, 2022 •

edited

sebsoto commented Aug 5, 2022

sebsoto commented Aug 8, 2022

sebsoto commented Aug 10, 2022

jrvaldes left a comment

jrvaldes Aug 24, 2022

jrvaldes Aug 24, 2022

saifshaikh48 left a comment

saifshaikh48 Sep 1, 2022

aravindhp Sep 1, 2022

sebsoto Sep 7, 2022

saifshaikh48 Sep 8, 2022

aravindhp Sep 12, 2022

sebsoto Sep 12, 2022

saifshaikh48 left a comment

saifshaikh48 Sep 8, 2022

sebsoto Sep 12, 2022 •

edited

alinaryan left a comment

sebsoto commented Sep 12, 2022

mansikulkarni96 commented Sep 12, 2022

alinaryan commented Sep 12, 2022

aravindhp left a comment

aravindhp Sep 12, 2022

aravindhp Sep 12, 2022

openshift-ci bot commented Sep 12, 2022

alinaryan commented Sep 12, 2022

openshift-ci-robot commented Sep 12, 2022

openshift-ci-robot commented Sep 13, 2022

sebsoto commented Sep 13, 2022

sebsoto commented Sep 13, 2022

sebsoto commented Sep 13, 2022

sebsoto commented Sep 14, 2022

openshift-ci bot commented Sep 14, 2022

WINC-740: Configure kube-proxy with WICD #1160

WINC-740: Configure kube-proxy with WICD #1160

Conversation

sebsoto commented Aug 5, 2022 • edited

sebsoto commented Aug 5, 2022

sebsoto commented Aug 8, 2022

sebsoto commented Aug 10, 2022

jrvaldes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saifshaikh48 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

saifshaikh48 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebsoto Sep 12, 2022 • edited

Choose a reason for hiding this comment

alinaryan left a comment

Choose a reason for hiding this comment

sebsoto commented Sep 12, 2022

mansikulkarni96 commented Sep 12, 2022

alinaryan commented Sep 12, 2022

aravindhp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 12, 2022

alinaryan commented Sep 12, 2022

openshift-ci-robot commented Sep 12, 2022

openshift-ci-robot commented Sep 13, 2022

sebsoto commented Sep 13, 2022

sebsoto commented Sep 13, 2022

sebsoto commented Sep 13, 2022

sebsoto commented Sep 14, 2022

openshift-ci bot commented Sep 14, 2022

sebsoto commented Aug 5, 2022 •

edited

sebsoto Sep 12, 2022 •

edited