API to autocreate subnets for external hosts #10621

rajatchopra · 2016-08-25T00:09:59Z

One can now allocate hostsubnets for hosts that are not part of the cluster. This is useful when a host wants to be part of the SDN, but not part of the cluster (e.g. F5)
Trello: https://trello.com/c/fnW1tPCY/155-8-f5-integration-get-lease-from-cluster-cidr

Implementation details:

Allow creation of a hostsubnet resource without the 'subnet' field being present. But only if a specific annotation exists ('hostsubnet.sdn.openshift.io/autocreate').
Let the master also watch hostsubnets, but only bother with the ones that have the annotation. The rest of them are also created by master, but by watching nodes.
When the master learns about a hostsubnet being created with the annotation, it allocates a subnet of the clusterNetwork IPAM, deletes the annotated hostsubnet and recreates a new one with the assigned subnet.

The deletion and recreation have been done so that migration of existing infrastructure can go on smoothly. Even if one were to update master and not the nodes, and add an F5 machine in the middle, no issues should appear.

rajatchopra · 2016-08-25T00:14:49Z

@pravisankar Thanks for the help. Please take a look.
cc @openshift/networking

PS: This is for 3.4 release. So do not merge before the 1.3 has been cut.

liggitt · 2016-08-25T00:15:10Z

pkg/sdn/api/validation/validation.go

@@ -87,7 +87,10 @@ func ValidateHostSubnet(hs *sdnapi.HostSubnet) field.ErrorList {

 	_, _, err := net.ParseCIDR(hs.Subnet)
 	if err != nil {
-		allErrs = append(allErrs, field.Invalid(field.NewPath("subnet"), hs.Subnet, err.Error()))
+		// check if annotation exists, then let the Subnet field be empty
+		if _, ok := hs.Annotations["hostsubnet.sdn.openshift.io/autocreate"]; !ok {


Why not make this an actual field?

This annotation is mainly to address F5 use case. We use actual fields for generic use cases?

When this annotation/field is set, ensure Subnet field is empty?

I am in favour of an actual field. Will be easy to identify such hostsubnets easily post creation too. Thanks.

We use actual fields for generic use cases?

at the point where API-level validation is using the data, it should probably be a field rather than an annotation

Then it makes me against the field idea here. This is not a useful field at runtime, its merely a direction at create time.

pravisankar · 2016-08-25T00:57:40Z

In SubnetStartMaster(), you need to ignore HostSubnets that doesn't have populated Subnet field.

pravisankar · 2016-08-25T01:04:13Z

pkg/sdn/plugin/subnets.go

+			}
+
+			if _, ok := hs.Annotations["hostsubnet.sdn.openshift.io/autocreate"]; ok {
+				err = master.registry.DeleteSubnet(name)


Add a comment on why we are doing delete+create subnet instead of update subnet.

danwinship · 2016-08-31T14:56:56Z

Allow creation of a hostsubnet resource without the 'subnet' field being present. But only if a specific annotation exists ('hostsubnet.sdn.openshift.io/autocreate').

Why not just drop the annotation, and say people with HostSubnet-creating permission can always create HostSubnets-without-Subnet-fields, and the master will always then fill in the missing field?

deletes the annotated hostsubnet and recreates a new one with the assigned subnet.

Why does it do this rather than just update the existing one?

rajatchopra · 2016-08-31T23:38:27Z

Fixed the issues in feedback.
@danwinship We could drop the annotation and be permissive in allowing that hostsubnets be created without subnet being defined. The annotation method just makes it more explicit with respect to the intent. It will prevent the accidental case of subnet gone missing in another case.

Also, added a comment about why a delete+create is done instead of an update. The only reason is to avoid migration issues. The node side watchSubnets skips the actual planting of OVS rules if the hostsubnet was modified but the host name remains the same.

pravisankar · 2016-09-01T20:23:10Z

LGTM, update docs about this feature?

rajatchopra · 2016-09-01T20:26:21Z

Yes, docs need to be updated. But, kind of a half story right now without the F5 use case. The complete F5 native-vxlan thing is coming in another PR, so probably a doc when that is ready. Thanks for the feedback.

liggitt · 2016-09-02T20:25:53Z

pkg/sdn/api/validation/validation.go

@@ -87,7 +88,10 @@ func ValidateHostSubnet(hs *sdnapi.HostSubnet) field.ErrorList {

 	_, _, err := net.ParseCIDR(hs.Subnet)
 	if err != nil {
-		allErrs = append(allErrs, field.Invalid(field.NewPath("subnet"), hs.Subnet, err.Error()))
+		// check if annotation exists, then let the Subnet field be empty
+		if _, ok := hs.Annotations[sdnplugin.AssignHostSubnetAnnotation]; !ok {


this isn't just allowing the subnet field to be empty, it's allowing it to be invalid... that's not what we want is it?

Fixed this. The check is made only on empty field.

rajatchopra · 2016-09-23T00:36:22Z

Refactored with feedback and the restructure from eventQueue to DeltaFifo

knobunc · 2016-09-23T18:40:38Z

[test]

danwinship

LGTM other than one comment nit

danwinship · 2016-09-28T18:26:13Z

pkg/sdn/plugin/subnets.go

@@ -240,6 +241,41 @@ func (node *OsdnNode) initSelfSubnet() error {
 	return nil
 }

+// Only run on the master
+// Watch for all hostsubnet events and if one is found with the right annotation, use the IPAM to dole a real subnet


"use the subnet allocator" not "use the IPAM". "IPAM" refers to assigning IPs to pods, not networks to nodes

pravisankar

LGTM, fix the comment DanWinship pointed out.

knobunc · 2016-09-29T17:45:02Z

[test]

…luster. This is useful when a host wants to be part of the SDN, but not part of the cluster (e.g. F5)

rajatchopra · 2016-10-01T00:07:35Z

re [test]
Flake: #9624

rajatchopra · 2016-10-03T22:14:20Z

re [test]
Flake: #9548 #9490

knobunc · 2016-10-07T19:17:59Z

re [test]

openshift-bot · 2016-10-07T19:22:27Z

Evaluated for origin test up to 6926d71

openshift-bot · 2016-10-07T21:44:54Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9758/)

danwinship · 2016-10-08T13:16:20Z

[merge]

knobunc · 2016-10-12T19:31:16Z

Flake #11315 [merge]

openshift-bot · 2016-10-12T19:32:31Z

Evaluated for origin merge up to 6926d71

openshift-bot · 2016-10-12T20:47:38Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9985/) (Base Commit: a5c2619) (Image: devenv-rhel7_5166)

liggitt reviewed Aug 25, 2016
View reviewed changes

pravisankar reviewed Aug 25, 2016
View reviewed changes

knobunc changed the title ~~API to create autocreate subnets for external hosts~~ API to create autocreate subnets for external hosts [DO NOT MERGE] Aug 25, 2016

rajatchopra force-pushed the f5 branch from 1dcd79f to 5fe71bb Compare August 31, 2016 23:35

rajatchopra changed the title ~~API to create autocreate subnets for external hosts [DO NOT MERGE]~~ API to autocreate subnets for external hosts Sep 1, 2016

liggitt reviewed Sep 2, 2016
View reviewed changes

rajatchopra force-pushed the f5 branch 2 times, most recently from 081564b to 75940fd Compare September 23, 2016 00:34

danwinship approved these changes Sep 28, 2016

View reviewed changes

dcbw added the component/networking label Sep 28, 2016

pravisankar approved these changes Sep 28, 2016

View reviewed changes

rajatchopra force-pushed the f5 branch from 75940fd to 7dd0bd6 Compare September 29, 2016 16:33

One can now allocate hostsubnets for hosts that are not part of the c…

6926d71

…luster. This is useful when a host wants to be part of the SDN, but not part of the cluster (e.g. F5)

rajatchopra force-pushed the f5 branch from 7dd0bd6 to 6926d71 Compare September 30, 2016 21:42

openshift-bot merged commit 2cd1700 into openshift:master Oct 12, 2016

rajatchopra deleted the f5 branch October 31, 2016 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API to autocreate subnets for external hosts #10621

API to autocreate subnets for external hosts #10621

rajatchopra commented Aug 25, 2016 •

edited

Loading

rajatchopra commented Aug 25, 2016

liggitt Aug 25, 2016

pravisankar Aug 25, 2016

pravisankar Aug 25, 2016 •

edited

Loading

rajatchopra Aug 25, 2016

liggitt Aug 26, 2016

rajatchopra Aug 26, 2016

pravisankar commented Aug 25, 2016

pravisankar Aug 25, 2016

danwinship commented Aug 31, 2016

rajatchopra commented Aug 31, 2016

pravisankar commented Sep 1, 2016

rajatchopra commented Sep 1, 2016

liggitt Sep 2, 2016

rajatchopra Sep 23, 2016

rajatchopra commented Sep 23, 2016

knobunc commented Sep 23, 2016

danwinship left a comment

danwinship Sep 28, 2016 •

edited

Loading

pravisankar left a comment

knobunc commented Sep 29, 2016

rajatchopra commented Oct 1, 2016 •

edited

Loading

rajatchopra commented Oct 3, 2016

knobunc commented Oct 7, 2016

openshift-bot commented Oct 7, 2016

openshift-bot commented Oct 7, 2016

danwinship commented Oct 8, 2016

knobunc commented Oct 12, 2016

openshift-bot commented Oct 12, 2016

openshift-bot commented Oct 12, 2016 •

edited

Loading

API to autocreate subnets for external hosts #10621

API to autocreate subnets for external hosts #10621

Conversation

rajatchopra commented Aug 25, 2016 • edited Loading

rajatchopra commented Aug 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pravisankar Aug 25, 2016 • edited Loading

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

pravisankar commented Aug 25, 2016

Choose a reason for hiding this comment

danwinship commented Aug 31, 2016

rajatchopra commented Aug 31, 2016

pravisankar commented Sep 1, 2016

rajatchopra commented Sep 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajatchopra commented Sep 23, 2016

knobunc commented Sep 23, 2016

danwinship left a comment

Choose a reason for hiding this comment

danwinship Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

pravisankar left a comment

Choose a reason for hiding this comment

knobunc commented Sep 29, 2016

rajatchopra commented Oct 1, 2016 • edited Loading

rajatchopra commented Oct 3, 2016

knobunc commented Oct 7, 2016

openshift-bot commented Oct 7, 2016

openshift-bot commented Oct 7, 2016

danwinship commented Oct 8, 2016

knobunc commented Oct 12, 2016

openshift-bot commented Oct 12, 2016

openshift-bot commented Oct 12, 2016 • edited Loading

rajatchopra commented Aug 25, 2016 •

edited

Loading

pravisankar Aug 25, 2016 •

edited

Loading

danwinship Sep 28, 2016 •

edited

Loading

rajatchopra commented Oct 1, 2016 •

edited

Loading

openshift-bot commented Oct 12, 2016 •

edited

Loading