[wmco] Barebone WMCO initialization by ravisantoshgudimetla · Pull Request #1 · openshift/windows-machine-config-operator

ravisantoshgudimetla · 2020-02-18T21:09:36Z

Followed the steps at https://github.com/operator-framework/operator-sdk/blob/master/doc/user-guide.md to initialize the repo, as of now, following items are added:

A new apiVersion called wmc.openshift.io/v1alpha1
A new kind called WindowsMachineConfig
A deployable CRD, CR, rbac and sa
Controller which watches for WindowsMachineConfig CRs

The goal is to have the operator watch for windowsMachineConfig CR and windowsMachineConfig Configmap in future. As of now, we're just going to watch for the windowsConfig CR(which is singleton)

Following are the commands, that I ran:

    # operator-sdk new windows-machine-config-operator --repo=github.com/example-inc/memcached-operator
    # operator-sdk add api --api-version=wmc.openshift.io/v1alpha1 --kind=WindowsMachineConfig
    # operator-sdk generate k8s
    # operator-sdk generate crds
    # operator-sdk add controller --api-version=wmc.openshift.io/v1alpha1 --kind=WindowsMachineConfig

ravisantoshgudimetla · 2020-02-18T21:11:11Z

/cc @aravindhp @openshift/openshift-team-windows-containers

aravindhp

Thanks for working on this, @ravisantoshgudimetla.

Please add the operator-sdk commands that you ran to initialize the repo. Also shouldn't you add WindowsMachine CR first given you are going to be working on that next?

aravindhp · 2020-02-18T21:28:27Z

+metadata:
+  name: windowsoperatorconfigs.winoperators.config.openshift.io
+spec:
+  group: winoperators.config.openshift.io


Are we sure we can use config.openshift.io for operators that perform day 2 operations? @sdodson @crawford please weigh in here.

@deads2k please weigh in here

This has been resolved over slack. As per @deads2k, config.openshift.io must lifecycle on an OpenShift release cadence. and that the configuration should live with the component.

ravisantoshgudimetla · 2020-02-19T15:50:17Z

Also shouldn't you add WindowsMachine CR first given you are going to be working on that next?

I'll pick it up later when I pick that story. I want to not block you. Also, I wasn't sure, if we have to remove certain fields in the CR, so I did not want to block this PR on that.

aravindhp · 2020-02-19T15:53:54Z

I'll pick it up later when I pick that story. I want to not block you. Also, I wasn't sure, if we have to remove certain fields in the CR, so I did not want to block this PR on that.

Given we are waiting on @deads2k it makes sense to add your CRD first and I can add the WindowsMachineConfig CRD as part of my story. Adding the CRD is not a blocker for me. That way I can start working on it as soon as this merges.

ravisantoshgudimetla · 2020-02-19T16:44:46Z

Please add the operator-sdk commands that you ran to initialize the repo.

I added link to the commands I've run. Do you want me to add the exact commands as well?

aravindhp · 2020-02-19T18:33:49Z

I added link to the commands I've run. Do you want me to add the exact commands as well?

Yes please.

aravindhp

Thanks for the PR, @ravisantoshgudimetla. Mostly LGTM.

aravindhp · 2020-02-20T23:18:10Z

+	 // Replicas represent how many Windows nodes to be added to the
+        // OpenShift cluster


aravindhp · 2020-02-20T23:21:25Z

+        // sshPrivateKey is the name of the secret which contains ssh key
+        // to decrypt the password
+        sshPrivateKey string `json:"sshprivatekey"`
+        // cloudProviderCreds is the name of the secret which contains
+        // credentials of the cloud provider.
+        cloudProviderCredentials string `json:"cloudprovidercredentials,omitempty"`


Please indicate in the variable name that we are using a secret.

It would be better to create a struct for AWS as this is just going to grow. We can add a struct for Azure when we start working on adding Azure support.

sebsoto

Seems cool, thanks @ravisantoshgudimetla

sebsoto · 2020-02-20T23:51:40Z

+        // InstanceType represents the flavor of instance to be used while
+        // creating the virtual machines. Please note that this is common
+        // across all the Windows nodes in the cluster
+        InstanceType string `json:"instancetype,omitempty"`


For the json tags we should use a set naming convention. Kubernetes uses camel case so I suggest we go with that :)

Good catch. I just looked OpenShift api and we moved to that. Thnx.

sebsoto · 2020-02-20T23:52:13Z

+        InstanceType string `json:"instancetype,omitempty"`
+        // sshkeyPair is the sshKeyPair associated with cloudprovider. AWS
+        // asks a keypair to be present for encrypting the Windows VM password
+        sshkeyPair string `json:"sshkeypair"`


sshkeyPair -> sshKeyPair

sebsoto · 2020-02-21T00:02:00Z

+// NOTE: json tags are required.  Any new fields you add must have json tags for the fields to be serialized.
+
+// WindowsMachineConfigSpec defines the desired state of WindowsMachineConfig
+type WindowsMachineConfigSpec struct {


Should Windows build number be added as a field here?
According to the link:

The Windows Server version used by each pod must match that of the node

It seems that there are multiple builds (versions) for Windows Server 2019. Based on that it would seem that we should keep note of the build number so that we would know if a pod could be scheduled on the node or not due to version reasons.

As of now, the focus is on getting the Windows Server 2019 to the cluster(that's why I am not exposing the image to be used as of now). We may not go to the granular level of matching builds in future but we can always increase the surface area of api if we want to.

suhanime

Thank you for the PR @ravisantoshgudimetla. The ground work looks solid.

suhanime · 2020-02-21T16:44:14Z

+	// Get a config to talk to the apiserver
+	cfg, err := config.GetConfig()
+	if err != nil {
+		log.Error(err, "")


Why are the errors swallowed here and in the following sections?

We're not swallowing them, we're not adding context however remember that this PR is just what sdk generates. I'll make code changes in the next PR.

aravindhp · 2020-02-21T18:47:41Z

+	// across all the Windows nodes in the cluster
+	InstanceType string `json:"instanceType,omitempty"`
+	// AWSProvider holds AWS specific cloud provider information.
+	AWSProvider *AWSProvider `json:"AWSProvider"`


From what I understand, this should be awsProvider in the JSON. You can check with the API folks to confirm.

Please remove the provider suffix.

Anything works, in json that we need to edit, the user needs to provide AWSProvider.

aravindhp · 2020-02-21T18:47:52Z

+	// AWSProvider holds AWS specific cloud provider information.
+	AWSProvider *AWSProvider `json:"AWSProvider"`
+	// AzureProvider holds Azure specific cloud provider information
+	AzureProvider *AzureProvider `json:"AzureProvider"`


Rename to Azure and make it azure in the JSON

aravindhp · 2020-02-21T18:50:01Z

+type AWSProvider struct {
+	// SshKeyPair is the sshKeyPair associated with cloudprovider. AWS
+	// asks a keypair to be present for encrypting the Windows VM password
+	SshKeyPair string `json:"sshKeyPair"`


should this be SSHKeyPair?

aravindhp · 2020-02-21T18:50:13Z

+	SshKeyPair string `json:"sshKeyPair"`
+	// SshPrivateKey is the name of the secret which contains ssh key
+	// to decrypt the password
+	SshPrivateKey string `json:"sshPrivateKey"`


Same question here.

No strong preference. I'll change it to SSH.

aravindhp · 2020-02-21T18:50:32Z

+	// to decrypt the password
+	SshPrivateKey string `json:"sshPrivateKey"`
+	// CredentialAccountID is account id associated with AWS provider
+	CredentialAccountID string `json:credentialAccountDd"`


Please change to credentialAccountId in the JSON.

aravindhp

/approve

ravisantoshgudimetla

@aravindhp - I addressed your comments. PTAL

ravisantoshgudimetla · 2020-02-21T19:21:11Z

+	// across all the Windows nodes in the cluster
+	InstanceType string `json:"instanceType,omitempty"`
+	// AWSProvider holds AWS specific cloud provider information.
+	AWSProvider *AWSProvider `json:"AWSProvider"`


Anything works, in json that we need to edit, the user needs to provide AWSProvider.

ravisantoshgudimetla · 2020-02-21T19:21:37Z

+	SshKeyPair string `json:"sshKeyPair"`
+	// SshPrivateKey is the name of the secret which contains ssh key
+	// to decrypt the password
+	SshPrivateKey string `json:"sshPrivateKey"`


No strong preference. I'll change it to SSH.

suhanime · 2020-02-21T19:35:13Z

/lgtm

Followed the steps at https://github.com/operator-framework/operator-sdk/blob/master/doc/user-guide.md to initialize the repo, as of now, following items are added: - A new apiVersion called wmc.openshift.io/v1alpha1 - A new kind called WindowsMachineConfig - A deployable CRD, CR, rbac and sa - Controller which watches for WindowsMachineConfig CRs The goal is to have the operator watch for windowsMachineConfig CR and windowsMachineConfig Configmap in future. As of now, we're just going to watch for the windowsConfig CR(which is singleton) Following are the commands, that I ran: - operator-sdk new windows-machine-config-operator \ --repo=github.com/example-inc/memcached-operator - operator-sdk add api --api-version=wmc.openshift.io/v1alpha1 \ --kind=WindowsMachineConfig - operator-sdk generate k8s - operator-sdk generate crds - operator-sdk add controller --api-version=wmc.openshift.io/v1alpha1 \ --kind=WindowsMachineConfig

aravindhp · 2020-02-21T20:07:04Z

/lgtm

Create OWNERS file

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 18, 2020

openshift-ci-robot requested review from a team and aravindhp February 18, 2020 21:11

aravindhp suggested changes Feb 18, 2020

View reviewed changes

ravisantoshgudimetla force-pushed the barebone-wmco-init branch 2 times, most recently from 8144acb to e4ff810 Compare February 20, 2020 22:18

aravindhp suggested changes Feb 20, 2020

View reviewed changes

sebsoto suggested changes Feb 21, 2020

View reviewed changes

suhanime reviewed Feb 21, 2020

View reviewed changes

ravisantoshgudimetla force-pushed the barebone-wmco-init branch 2 times, most recently from 217b901 to 50a0923 Compare February 21, 2020 17:33

aravindhp suggested changes Feb 21, 2020

View reviewed changes

aravindhp approved these changes Feb 21, 2020

View reviewed changes

ravisantoshgudimetla commented Feb 21, 2020

View reviewed changes

openshift-ci-robot assigned suhanime Feb 21, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 21, 2020

ravisantoshgudimetla force-pushed the barebone-wmco-init branch from ce86de1 to 053f52b Compare February 21, 2020 19:53

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 21, 2020

openshift-ci-robot assigned aravindhp Feb 21, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 21, 2020

aravindhp merged commit d75d5ce into openshift:master Feb 21, 2020

wgahnagl pushed a commit to wgahnagl/windows-machine-config-operator that referenced this pull request Mar 6, 2023

Merge pull request openshift#1 from aravindhp/owners

7140f0b

Create OWNERS file

rrasouli mentioned this pull request Aug 18, 2026

Revert WINC-1972 Batch 7 - CI infrastructure overload (16 test failures) #4487

Closed

dominikholler mentioned this pull request Sep 9, 2026

WINC-2118: Apply cluster TLS profile to windows-exporter webconfig #4583

Draft

		// Replicas represent how many Windows nodes to be added to the
		// OpenShift cluster

Uh oh!

Conversation

ravisantoshgudimetla commented Feb 18, 2020 • edited by aravindhp Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ravisantoshgudimetla commented Feb 18, 2020

Uh oh!

aravindhp 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!

ravisantoshgudimetla commented Feb 19, 2020

Uh oh!

aravindhp commented Feb 19, 2020

Uh oh!

ravisantoshgudimetla commented Feb 19, 2020

Uh oh!

aravindhp commented Feb 19, 2020

Uh oh!

aravindhp 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!

sebsoto 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!

suhanime 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!

aravindhp left a comment

Choose a reason for hiding this comment

Uh oh!

ravisantoshgudimetla 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!

suhanime commented Feb 21, 2020

ravisantoshgudimetla commented Feb 18, 2020 •

edited by aravindhp

Loading