[wmco] WMC controller #15
Conversation
openshift-ci-robot
added
the
size/XL
openshift-ci-robot
added
the
approved
|
/approve cancel |
openshift-ci-robot
removed
the
approved
There was a problem hiding this comment.
Thanks for the PR, @ravisantoshgudimetla. The structure looks good more or less. Please address my comments.
hack/run-ci-e2e-test.sh
Outdated
| export CLUSTER_ADDR=$(oc cluster-info | head -n1 | sed 's/.*\/\/api.//g'| sed 's/:.*//g') | ||
|
|
||
| # Get operator-sdk binary. | ||
| # TODO: Make this to download the same version we have in godeps |
There was a problem hiding this comment.
This TODO makes no sense.
There was a problem hiding this comment.
Updated it.
pkg/assets/wget-ignore-cert.ps1
Outdated
| @@ -0,0 +1,26 @@ | |||
| # Script that downloads a file from the server to the output location ignoring the server certificate | |||
There was a problem hiding this comment.
Shouldn't this be in an internal assets package?
There was a problem hiding this comment.
I'll change it to internal. When I was working on this, I had this idea of using bindata
| @@ -0,0 +1,139 @@ | |||
| package nodeconfig | |||
There was a problem hiding this comment.
Shouldn't the filename be nodeconfig.go?
There was a problem hiding this comment.
I see both the patterns in the code bases that I worked I will change the file name to nodeconfig just to be in sync with windowsvm.go. For reference
| } | ||
|
|
||
| // Handle the node CSR | ||
| // Note: for the product we want to get the node name from the instance information |
There was a problem hiding this comment.
This should become a story
| if err != nil { | ||
| return errors.Wrapf(err, "unable to create remote directory %v", remoteDir) | ||
| } | ||
| file := "pkg/controller/windowsmachineconfig/powershell/wget-ignore-cert.ps1" |
There was a problem hiding this comment.
I am assuming you will use a well know location like payload eventually and the test setup / Dockerfile will copy it over.
There was a problem hiding this comment.
That is right.
|
|
||
| // runBootstrapper copies the bootstrapper and runs the code on the remote Windows VM | ||
| func (vm *windowsVM) runBootstrapper() error { | ||
| // TODO: This is well known location, we should make sure wmcb exists here |
There was a problem hiding this comment.
I am not for TODOs for stuff like this because it all falls under validation and it is scattered all over the code. Won't it be easier to just do it at least in this instance?
There was a problem hiding this comment.
Right, what I meant was, we can do validation at a single place but until that happens, there should be a TODO.
| if err != nil { | ||
| return errors.Wrap(err, "error running bootstrapper") | ||
| } |
There was a problem hiding this comment.
This is not sufficient. You should also inspect stderr. I think we are doing some checking like this in our WSU / WMCB test cases. This comment applies to all instances where you are not checking the output of stderr
There was a problem hiding this comment.
In the test case scenario, we're running the test binary which gives us strings like FAIL or PANIC etc to validate. So, we cannot have them validated. In case of stderr, as of now, I'm changing the code to check the length of the stderr and if it's greater than 0, I'm returning error.
| // runBootstrapper copies the bootstrapper and runs the code on the remote Windows VM | ||
| func (vm *windowsVM) runBootstrapper() error { | ||
| // TODO: This is well known location, we should make sure wmcb exists here | ||
| if err := vm.CopyFile("/payload/wmcb.exe", remoteDir); err != nil { |
There was a problem hiding this comment.
Please use a constant for the exe. Applies to all places where we are using something from the payload directory.
test/e2e/wmco_test.go
Outdated
|
|
||
| var wmcoTestFramework wmcoFramework | ||
|
|
||
| func TestWMCOE2E(t *testing.T) { |
There was a problem hiding this comment.
I would like this to be split up so that we can test scale up and scale down.
There was a problem hiding this comment.
Cool stuff @ravisantoshgudimetla, left a partial review, will check it out more later
| k8sclientset *kubernetes.Clientset | ||
| } | ||
|
|
||
| func NewNodeConfig(clientset *kubernetes.Clientset) *nodeConfig { |
| // HandleCSRs handles the approval of bootstrap and node CSRs | ||
| func (nc *nodeConfig) HandleCSRs() error { | ||
| // Handle the bootstrap CSR | ||
| err := nc.handleCSR("system:serviceaccount:openshift-machine-config-operator:node-bootstrapper") |
There was a problem hiding this comment.
Move system:serviceaccount:openshift-machine-config-operator:node-bootstrapper to a constant
| return nil | ||
| } | ||
|
|
||
| // findCSR finds the CSR that matches the requestor filter |
There was a problem hiding this comment.
This finds the CSR that contains the requestor filter, not matches. Seems like this could cause issues if we're not looking for an exact match
There was a problem hiding this comment.
Since in our case the machine api won't intervene, this may not be a problem for bootstrap CSRs however we can narrow down the filter to point to node we created. That's part of TODO
| if instance.Spec.AWS == nil && instance.Spec.Azure == nil { | ||
| return reconcile.Result{}, errors.New("both the supported cloud providers are nil") | ||
| } | ||
| // As of now think of AWS implementation. |
There was a problem hiding this comment.
I am thinking of it, what now? :P
Not sure what this comment is saying.
There was a problem hiding this comment.
Deleted.
| if instance.Spec.AWS != nil { | ||
| // We assume the cloud provider secret has been mounted to "~/.awscredentials` path | ||
| r.cloudProvider, err = cloudprovider.CloudProviderFactory(os.Getenv("KUBECONFIG"), | ||
| // We assume the credential path is `/etc/aws/credentials` mounted as a secret. |
There was a problem hiding this comment.
This contradicts the above comment We assume the cloud provider secret has been mounted to "~/.awscredentials path`
There was a problem hiding this comment.
Updated
| // populate the windowsVM map here from configmap as source of truth | ||
| r.windowsVM = make(map[types.WindowsVM]bool) | ||
| } | ||
| if r.k8sclientset == nil { |
There was a problem hiding this comment.
Defensive programming :)
| if r.k8sclientset == nil { | ||
| return reconcile.Result{}, nil | ||
| } | ||
| nodes, err := r.k8sclientset.CoreV1().Nodes().List(metav1.ListOptions{LabelSelector: "node.openshift.io/os_id=Windows"}) |
| // TODO: This can be decomposed further. | ||
| // TIP: Have a single method with operation like deletion, creation as argument | ||
| if desired < current { | ||
| hasDeletionFailed := r.deleteWindowsVMs(current - desired) |
There was a problem hiding this comment.
nit: Could just call this deletionFailed since its a bool
| // Let's not requeue the result for now. We can get the list of errors | ||
| hasCreationFailed := r.createWindowsVMs(desired - current) | ||
| //r.reconcileCredentials() | ||
| return hasCreationFailed |
There was a problem hiding this comment.
nit: Could just call this creationFailed since its a bool
There was a problem hiding this comment.
I removed the variable for now altogether, we can add it when we have error slices later.
| }, | ||
| // TODO: If properly done, we should reconcile for creds here :). | ||
| // TIP: We can fail on VM creation/deletion reconciliation still reach here to make it more readable | ||
| /*if err := r.reconcileCredentials(); err != nil { |
There was a problem hiding this comment.
Removed it, I wanted to give some inputs to @aravindhp so that he can get started on the configmap
There was a problem hiding this comment.
Whew lot of code. Good work @ravisantoshgudimetla PTAL at my comments
| // deleteWindowsVMs returns the instance IDs of the successfully created Windows VMs | ||
| func (r *ReconcileWindowsMachineConfig) deleteWindowsVMs(count int) bool { | ||
| hasVMDeletionFailed := false | ||
| // From the list of Windows VMs choose randomly count number of VMs. As of now sequential |
There was a problem hiding this comment.
Can reword the comment to be more clear:
// Delete a number of VMs from the list of Windows VMs. VMs to be deleted are picked randomly.
// TODO: Right now VMs are picked sequentially, we should pick the VMs randomly
| continue | ||
| } | ||
| // Delete the Windows VM from cloud provider | ||
| log.Info("deleting the Windows VM", instancedID) |
There was a problem hiding this comment.
You provided a variable but no verb here
Try deleting the Windows VM %s, instanceID
|
|
||
| // deleteWindowsVMs returns the instance IDs of the successfully created Windows VMs | ||
| func (r *ReconcileWindowsMachineConfig) deleteWindowsVMs(count int) bool { | ||
| hasVMDeletionFailed := false |
There was a problem hiding this comment.
nit: Since its a bool could just do vmDeletionFailed
| // TODO: Return a slice of errors and unwrap it at the call site. | ||
| func (r *ReconcileWindowsMachineConfig) createWindowsVMs(count int) bool { | ||
| // TODO: hasVMCreationfailed should be removed when we move to collect slice of errors. | ||
| hasVMCreationfailed := false |
There was a problem hiding this comment.
nit: Since its a bool could just do vmCreationFailed
There was a problem hiding this comment.
I changed the logic to return bool depending on slice of errors that we can get for each create operation. So no need for that now.
| hasVMCreationfailed = true | ||
| log.Error(err, "error while creating windows VM", err) | ||
| } | ||
| log.V(5).Info("created the Windows VM", createdVM.GetCredentials().GetInstanceId()) |
There was a problem hiding this comment.
nit: You're changing the way you log when deleting and creating the VMs
log.Info("deleting the Windows VM", instancedID) I think we should stay consistent with this.
Specifically with logging before the action, and using the same command log.Info, as these are actions of equal importance.
There was a problem hiding this comment.
Also need the verb here as well
| nodes, err := kubeclient.CoreV1().Nodes().List(metav1.ListOptions{LabelSelector: "node.openshift.io/os_id=Windows"}) | ||
| if err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| log.Printf("Waiting for availability of %d windows nodes\n", expectedNodeCount) |
There was a problem hiding this comment.
A newline shouldnt be needed for log statements i thought, they should be on their own line automatically?
There was a problem hiding this comment.
I think not with Printf but with Println.
test/e2e/wmco_test.go
Outdated
| log.Println("Created the required number of Windows worker nodes") | ||
| return true, nil | ||
| } | ||
| log.Printf("still waiting for %d number of Windows worker nodes to be created\n", expectedNodeCount) |
There was a problem hiding this comment.
("%d/%d Windows worker nodes created", len(nodes.Items),expectedNodeCount)
|
/test images |
| return nil | ||
| } | ||
|
|
||
| // deleteWindowsVMs returns the instance IDs of the successfully created Windows VMs |
There was a problem hiding this comment.
seems like the comment needs to be updated, the function is returning a bool and it logs the instance Id of successfully deleted VM.
|
/test images |
hack/run-ci-e2e-test.sh
Outdated
| export CLUSTER_ADDR=$(oc cluster-info | head -n1 | sed 's/.*\/\/api.//g'| sed 's/:.*//g') | ||
|
|
||
| # Get operator-sdk binary. | ||
| # TODO: Make this to download the same version we have in godeps |
There was a problem hiding this comment.
Updated it.
pkg/assets/wget-ignore-cert.ps1
Outdated
| @@ -0,0 +1,26 @@ | |||
| # Script that downloads a file from the server to the output location ignoring the server certificate | |||
There was a problem hiding this comment.
I'll change it to internal. When I was working on this, I had this idea of using bindata
| @@ -0,0 +1,139 @@ | |||
| package nodeconfig | |||
There was a problem hiding this comment.
I see both the patterns in the code bases that I worked I will change the file name to nodeconfig just to be in sync with windowsvm.go. For reference
| k8sclientset *kubernetes.Clientset | ||
| } | ||
|
|
||
| func NewNodeConfig(clientset *kubernetes.Clientset) *nodeConfig { |
| if err != nil { | ||
| return errors.Wrap(err, "error running bootstrapper") | ||
| } |
There was a problem hiding this comment.
In the test case scenario, we're running the test binary which gives us strings like FAIL or PANIC etc to validate. So, we cannot have them validated. In case of stderr, as of now, I'm changing the code to check the length of the stderr and if it's greater than 0, I'm returning error.
| // TODO: As of now, getting cluster address from the environment var, remove it. | ||
| // Download the worker ignition to C:\Windows\Tenp\ using the script that ignores the server cert | ||
| ClusterAddress := os.Getenv("CLUSTER_ADDR") | ||
| stdout, stderr, err := vm.Run(wgetIgnoreCertCmd+" -server https://api-int."+ClusterAddress+":22623/config/worker"+" -output "+winTemp+"worker.ign", true) |
| } | ||
| file := "pkg/controller/windowsmachineconfig/powershell/wget-ignore-cert.ps1" | ||
| if err := vm.CopyFile(file, remoteDir); err != nil { | ||
| log.Error(err, "error while copying powershell script") |
There was a problem hiding this comment.
Good catch, I am returning it now.
| // Download the worker ignition to C:\Windows\Tenp\ using the script that ignores the server cert | ||
| ClusterAddress := os.Getenv("CLUSTER_ADDR") | ||
| stdout, stderr, err := vm.Run(wgetIgnoreCertCmd+" -server https://api-int."+ClusterAddress+":22623/config/worker"+" -output "+winTemp+"worker.ign", true) | ||
| log.V(5).Info("stdout associated with the node", "stdout", stdout) |
| ClusterAddress := os.Getenv("CLUSTER_ADDR") | ||
| stdout, stderr, err := vm.Run(wgetIgnoreCertCmd+" -server https://api-int."+ClusterAddress+":22623/config/worker"+" -output "+winTemp+"worker.ign", true) | ||
| log.V(5).Info("stdout associated with the node", "stdout", stdout) | ||
| log.V(5).Info("stderr associated with the node", "stderr", stderr) |
|
/retest |
1 similar comment
|
/retest |
ravisantoshgudimetla
force-pushed
the
add-wmc-controller
branch
4 times, most recently
from
6213fb1 to
7a17966
Compare
|
|
||
| // If any of the Windows VM fails to get deleted consider this as a failure and return false | ||
| if len(errs) > 0 { | ||
| return false |
There was a problem hiding this comment.
Should we log these errors to track how many VMs failed and why?
There was a problem hiding this comment.
Yes please print the errs variable
There was a problem hiding this comment.
I don't want to do it as it's the job of the caller. In future, we'll return this slice altogether for it to be unwrapped by the caller.
There was a problem hiding this comment.
Thanks @ravisantoshgudimetla! Looks mostly good. Looks like test code still isnt finished so I'll wait to review that.
deploy/namespace.yaml
Outdated
| apiVersion: v1 | ||
| kind: Namespace | ||
| metadata: | ||
| name: wmco |
There was a problem hiding this comment.
Why are you using wmco acronym here and windows-machine-config-operator everywhere else in deploy (SA, deployment etc.)
There was a problem hiding this comment.
Good catch, I'll change it. It was something I created for my testing purpose.
| @@ -0,0 +1,18 @@ | |||
| package wellknownlocations | |||
There was a problem hiding this comment.
This is an odd package name
There was a problem hiding this comment.
nit: Maybe change it to paths
There was a problem hiding this comment.
You mean well known paths? If it's a nit, I want to ignore that for now may be we can come back with a better naming later.
hack/run-ci-e2e-test.sh
Outdated
|
|
||
| export CLUSTER_ADDR=$(oc cluster-info | head -n1 | sed 's/.*\/\/api.//g'| sed 's/:.*//g') | ||
|
|
||
| # Copy the cloud credentials and KUBESSH key path so that it can be used by operator |
There was a problem hiding this comment.
so that it can -> so that they can
| package wellknownlocations | ||
|
|
||
| const ( | ||
| // CloudCredentialsPath contains the path to file where cloud credentials are stored. This would've been mounted |
There was a problem hiding this comment.
contains the path to the file
| // PrivateKeyPath contains the path to the private key which is used in decrypting password in case of AWS | ||
| // cloud provider. This would have been mounted as a secret by user | ||
| PrivateKeyPath = "/etc/private-key.pem" | ||
| // WmcbPath contains the path of the Windows Machine Config bootstrapper binary. The container image should already |
There was a problem hiding this comment.
bootstrapper -> Bootstrapper
| log.Info(fmt.Sprintf("deleting the Windows VM: %s", instancedID)) | ||
| if err := r.cloudProvider.DestroyWindowsVM(instancedID); err != nil { | ||
| log.Error(err, "error while deleting windows VM %s", instancedID) | ||
| errs = append(errs, errors.Wrap(err, "One of the VM deletions failed, will reconcile...")) |
There was a problem hiding this comment.
please add which VM failed to delete
There was a problem hiding this comment.
A wrapped error shouldnt end with "..." else you'll get something like "will reconcile...: bad thing happened"
There was a problem hiding this comment.
A wrapped error shouldnt end with "..." else you'll get something like "will reconcile...: bad thing happened"
You're right, copy-paste error.
please add which VM failed to delete
We'll have that information in the cloud provider return error. The error wrapper should add more context.
|
|
||
| // If any of the Windows VM fails to get deleted consider this as a failure and return false | ||
| if len(errs) > 0 { | ||
| return false |
There was a problem hiding this comment.
Yes please print the errs variable
| createdVM, err := r.cloudProvider.CreateWindowsVM() | ||
| if err != nil { | ||
| errs = append(errs, errors.Wrap(err, "error creating windows VM")) | ||
| log.Error(err, "error while creating windows VM") |
There was a problem hiding this comment.
Don't log here, just print errs at the end
There was a problem hiding this comment.
You mean log.Printf? Why?
| if err := nc.Configure(); err != nil { | ||
| // TODO: Unwrap to extract correct error | ||
| errs = append(errs, errors.Wrap(err, "configuring Windows VM failed")) | ||
| log.Error(err, "configuring Windows VM failed", err) |
There was a problem hiding this comment.
Don't log here, just print errs at the end
There was a problem hiding this comment.
The error will have information related to node.
| // Delete the Windows VM from cloud provider | ||
| log.Info("deleting the Windows VM", instancedID) | ||
| log.Info(fmt.Sprintf("deleting the Windows VM: %s", instancedID)) | ||
| if err := r.cloudProvider.DestroyWindowsVM(instancedID); err != nil { | ||
| log.Error(err, "error while deleting windows VM %s", instancedID) |
There was a problem hiding this comment.
missed a ":" here -> log.Error(err, "error while deleting windows VM : %s", instancedID)
| hasVMCreationfailed = true | ||
| log.Error(err, "error while creating windows VM", err) | ||
| errs = append(errs, errors.Wrap(err, "error creating windows VM")) | ||
| log.Error(err, "error while creating windows VM") |
There was a problem hiding this comment.
nit : could remove the word "while" for consistency?
test/e2e/wmco_test.go
Outdated
| @@ -2,6 +2,7 @@ package e2e | |||
|
|
|||
| import ( | |||
| "context" | |||
| "github.com/pkg/errors" | |||
There was a problem hiding this comment.
Separate standard libs and imported
There was a problem hiding this comment.
Good catch, done
|
To see, if openshift/release#7609 has taken effect. /retest |
|
lgtm, thank you @ravisantoshgudimetla for the PR and addressing the comments. |
|
/lgtm |
ravisantoshgudimetla
force-pushed
the
add-wmc-controller
branch
from
3e745e7 to
f2aebba
Compare
|
/retest |
5 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/test images |
2 similar comments
|
/test images |
|
/test images |
|
/test e2e-operator |
|
/lgtm |
ravisantoshgudimetla
force-pushed
the
add-wmc-controller
branch
from
f2aebba to
6d24114
Compare
|
/approve Self approving based on 2 lgtms. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ravisantoshgudimetla 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:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
ravisantoshgudimetla
force-pushed
the
add-wmc-controller
branch
from
6d24114 to
4a232d3
Compare
The WMCO can watch WMC object called instance and react to the changes to it. It spins up and down Windows VMs in existing cloud provider and makes them as Windows worker nodes in the cluster. Also, added a e2e test suite which validates the creation and deletion of the Windows VMs.
ravisantoshgudimetla
force-pushed
the
add-wmc-controller
branch
from
ea32338 to
f46ddec
Compare
|
/lgtm |
…leton [wmcb] code skeleton structure
Initial implementation of WMC controller.
This will fail because some files are present, fixing tests in progress but the code skeleton & structure can reviewed./cc @aravindhp