UPSTREAM: <carry>: openshift: Set instance subnet and load balancers explicitly #35

ingvagabund · 2019-05-31T15:02:17Z

Do not rely on machine.openshift.io/cluster-api-machine-role label to determine
subnet name. Instead, use specific subnet so different worker machine groups
can have different subnet.

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

frobware · 2019-05-31T15:05:28Z

pkg/cloud/azure/actuators/machine/reconciler.go

-		return errors.Errorf("unknown value %s for label `set` on machine %s, skipping machine creation", set, s.scope.Machine.Name)
+
+	if s.scope.MachineConfig.Subnet == "" {
+		return errors.Errorf("unable to create network interface: mising subnet in %q machine provider config", s.scope.Machine.Name)


typo: missing.

awesomenix · 2019-05-31T15:13:53Z

pkg/cloud/azure/actuators/machine/reconciler.go

 		Name:     nicName,
 		VnetName: azure.GenerateVnetName(s.scope.Cluster.Name),
 	}
-	switch set := s.scope.Machine.ObjectMeta.Labels[v1alpha1.MachineRoleLabel]; set {


If the Subnet is empty you could populate the hardcoded ones above just for backward compat.

Something like

if subnet == "" {
} else {
//fallback
}

Eventually the fallback code can be removed

awesomenix · 2019-05-31T15:22:20Z

pkg/cloud/azure/actuators/machine/reconciler.go

-		networkInterfaceSpec.SubnetName = azure.GenerateNodeSubnetName(s.scope.Cluster.Name)
-	case v1alpha1.ControlPlane:
-		networkInterfaceSpec.SubnetName = azure.GenerateControlPlaneSubnetName(s.scope.Cluster.Name)
-		networkInterfaceSpec.PublicLoadBalancerName = azure.GeneratePublicLBName(s.scope.Cluster.Name)


These names are must have, since control plane requires public and internal load balancer for access into the cluster.

its an unfortunate design hope to change it in future. The way it works now is

CreateControlPlaneVM -> CreateNetworkInterface -> AttachInterfacetoloadblancer

Instead hope to work this way

CreateControlPlaneVM -> CreateNetworkInterface
UpdateLoadBalancerRules with NetworkInterface

That way control plane is decoupled from load balancers.

So you still have a use for the control plane code? How does the installer reuses the azure actuator these days? E.g. what resources are create by the azure actuator and which by the installer?

azure actuator only creates vm and sets up configuration of the base infra (example registering network interface with the load balancers)

as for worker nodes it just creates it on the base infra (right now all the names are hardcoded between installer and actuator hence little fragile), aim was to get it decoupled if we had time.

Also azure will never have name conflicts so its ok to hardcode since all the resources are restricted to a certain resource group for each sub/tenantid

ingvagabund · 2019-06-02T08:06:03Z

/hold

enxebre · 2019-06-03T14:18:16Z

pkg/cloud/azure/actuators/machine/reconciler.go

+		if s.scope.MachineConfig.InternalLoadBalancer != "" {
+			networkInterfaceSpec.InternalLoadBalancerName = s.scope.MachineConfig.InternalLoadBalancer
+		}
+	} else {


why do we this fallback? I'd like to remove v1alpha1.MachineRoleLabel dependency

#35 (comment)
#35 (comment)

so @awesomenix if this only for backward compatibility? I don't think we care much at this dev stage, there's no e2e yet. @ingvagabund can we drop all no necessary labels code and putting the counter part PR on installer?

@enxebre Control plane nodes needs to be behind public and internal load balancer, not sure how to determine that without labels. If there is some other method @ingvagabund feel free to change this logic

ah! i see that the master spec would contain the public load balancer and internal load balancer fields. In that case @ingvagabund i support @enxebre comments and drop this as he mentions its more of dev stage.

it would be better if Natrule is dropped as well

awesomenix · 2019-06-03T23:00:56Z

pkg/cloud/azure/actuators/machine/reconciler.go

+			networkInterfaceSpec.SubnetName = azure.GenerateControlPlaneSubnetName(s.scope.Cluster.Name)
+			networkInterfaceSpec.PublicLoadBalancerName = azure.GeneratePublicLBName(s.scope.Cluster.Name)
+			networkInterfaceSpec.InternalLoadBalancerName = azure.GenerateInternalLBName(s.scope.Cluster.Name)
+			networkInterfaceSpec.NatRule = 0


ah! been wanting to get rid of this Natrules as well, since ssh for masters are not enabled by default on openshift. Probably better to provide the natrule as an input

ingvagabund · 2019-06-04T14:04:19Z

@awesomenix added new NatRule field and removed the fallback per #35 (comment) and #35 (comment). PTAL.

ingvagabund · 2019-06-05T07:29:42Z

/hold cancel

awesomenix · 2019-06-05T16:09:41Z

@awesomenix added new NatRule field and removed the fallback per #35 (comment) and #35 (comment). PTAL.

I meant you need to remove this part of the code if NatRule is not set

cluster-api-provider-azure/pkg/cloud/azure/services/networkinterfaces/networkinterfaces.go

Line 117 in ede8126

ID: (*lb.InboundNatRules)[nicSpec.NatRule].ID,

I think openshift doesnt want to enable ssh access by default to masters, in this case, the input Natrule would decide whether to set it on the network interface or not. So making this input optional (hence setting it to nil) would get around the problem

…explicitly Do not rely on machine.openshift.io/cluster-api-machine-role label to determine subnet name and/or load balancer(s) name. Instead, allow to set explicit names for resources so different worker machine groups can have different resources.

ingvagabund · 2019-06-06T11:42:59Z

@awesomenix done, PTAL

awesomenix · 2019-06-06T16:27:42Z

/lgtm

openshift-ci-robot · 2019-06-06T16:27:51Z

@awesomenix: changing LGTM is restricted to assignees, and only openshift/cluster-api-provider-azure repo collaborators may be assigned issues.

Details

In response to this:

/lgtm

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.

openshift-ci-robot · 2019-06-06T16:28:28Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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

The pull request process is described here

Details

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

cluster-api-provider-azure PR [1], allows installer to set the subnets for machines explicitly rather than the implicit names. The new subnet name for control plane VMs is `<cluster-id>-master-subnet`, while the subnet name for compute VMs is `<cluster-id>-worker-subnet` based on the `role` for these MachinePools [1]: openshift/cluster-api-provider-azure#35

Bump to being in changes from openshift/cluster-api-provider-azure#35

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 31, 2019

openshift-ci-robot requested review from frobware and spangenberg May 31, 2019 15:02

frobware reviewed May 31, 2019

View reviewed changes

ingvagabund force-pushed the set-instance-subnet-explicitly branch from b8911b6 to 80ee1bc Compare May 31, 2019 15:09

awesomenix reviewed May 31, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 2, 2019

ingvagabund force-pushed the set-instance-subnet-explicitly branch from 80ee1bc to 32161e7 Compare June 2, 2019 16:36

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 2, 2019

ingvagabund force-pushed the set-instance-subnet-explicitly branch from 32161e7 to 0a79259 Compare June 2, 2019 16:38

ingvagabund changed the title ~~UPSTREAM: <carry>: openshift: Set instance subnet explicitly~~ UPSTREAM: <carry>: openshift: Set instance subnet and load balancers explicitly Jun 2, 2019

enxebre reviewed Jun 3, 2019

View reviewed changes

awesomenix reviewed Jun 3, 2019

View reviewed changes

ingvagabund force-pushed the set-instance-subnet-explicitly branch from 0a79259 to 89dd156 Compare June 4, 2019 13:29

ingvagabund mentioned this pull request Jun 4, 2019

UPSTREAM: <carry>: openshift: Always consider all machines to have node role #31

Closed

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2019

ingvagabund force-pushed the set-instance-subnet-explicitly branch from 89dd156 to 1350002 Compare June 6, 2019 11:33

ingvagabund added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels Jun 6, 2019

openshift-merge-robot merged commit 702ef3a into openshift:master Jun 6, 2019

ingvagabund deleted the set-instance-subnet-explicitly branch June 6, 2019 16:29

abhinavdahiya added a commit to abhinavdahiya/installer that referenced this pull request Jun 7, 2019

vendor: bump cluster-api-provide-azure

cb775e4

Bump to being in changes from openshift/cluster-api-provider-azure#35

abhinavdahiya mentioned this pull request Jun 7, 2019

machines/azure: Set subnets for control plane and compute machines openshift/installer#1833

Merged

UPSTREAM: <carry>: openshift: Set instance subnet and load balancers explicitly #35

UPSTREAM: <carry>: openshift: Set instance subnet and load balancers explicitly #35

Uh oh!

Conversation

ingvagabund commented May 31, 2019

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!

ingvagabund May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ingvagabund commented Jun 2, 2019

Uh oh!

enxebre Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enxebre Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ingvagabund commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ingvagabund commented Jun 5, 2019

Uh oh!

awesomenix commented Jun 5, 2019

Uh oh!

ingvagabund commented Jun 6, 2019

Uh oh!

awesomenix commented Jun 6, 2019

Uh oh!

openshift-ci-robot commented Jun 6, 2019

Uh oh!

openshift-ci-robot commented Jun 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ingvagabund May 31, 2019 •

edited

Loading

enxebre Jun 3, 2019 •

edited

Loading

enxebre Jun 3, 2019 •

edited

Loading

ingvagabund commented Jun 4, 2019 •

edited

Loading