metallb: add metallb controller and speaker builder func #276

gkopels · 2024-03-19T09:29:57Z

This PR adds metallb builder function allowing helm chart fields to be used to populate the controller and speaker config.

ajaggapa · 2024-03-19T11:32:35Z

pkg/metallb/metallb.go

+			Effect:   "NoExecute",
+		},
+	}
+


We can define all 3( Spec, ControllerConfig, ControllerTolerations) together.

builder.Definition.Spec = mlbtypes.MetalLBSpec{ ControllerConfig: &mlbtypes.Config{ PriorityClassName: priorityClassName, RuntimeClassName: runtimeClassName, Annotations: map[string]string{deploymentName: "controller"}, Affinity: &corev1.Affinity{ PodAffinity: &corev1.PodAffinity{ RequiredDuringSchedulingIgnoredDuringExecution: []corev1.PodAffinityTerm{ { LabelSelector: &metaV1.LabelSelector{ MatchLabels: map[string]string{ "component": helmChartControllerTestName, }, }, TopologyKey: "kubernetes.io/hostname", }, }, }, }, }, ControllerTolerations: []corev1.Toleration{ { Key: helmChartKeyExample, Operator: "Exists", Effect: "NoExecute", }, }, }

Good idea. Changed

ajaggapa · 2024-03-19T11:33:10Z

pkg/metallb/metallb.go

+			Effect:   "NoExecute",
+		},
+	}
+


same like above. we can define them together.

ajaggapa

minor suggestions

evgenLevin · 2024-03-19T19:07:33Z

pkg/metallb/metallb.go

@@ -392,3 +393,101 @@ func (builder *Builder) convertToStructured(unsObject *unstructured.Unstructured

 	return metalLb, err
 }
+
+// WithMetallbControllerConfig defines the controller config with a helm chart table.
+func (builder *Builder) WithMetallbControllerConfig(priorityClassName, runtimeClassName, deploymentName,


Please this func to other global With functions.

evgenLevin · 2024-03-19T19:11:56Z

pkg/metallb/metallb.go

+	glog.V(100).Infof("Defining the metallb object with Helm Chart in namespace %s",
+		builder.Definition.Namespace,
+	)
+


Validate all arguments: priorityClassName, runtimeClassName, deploymentName,helmChartControllerTestName, helmChartKeyExample

Looking at CRD, it seems that priorityClassName, runtimeClassName, are optional fields. Just wonder if is needed to enforce them.

They are optional. They can also accept an empty string. I have removed the validation

Not sure if that's a good idea, because an empty field and a field that's not configured are not the same.

Hi I added a verification to make sure its not an empty field

evgenLevin · 2024-03-19T19:16:30Z

pkg/metallb/metallb.go

+func (builder *Builder) WithMetallbSpeakerConfig(speakerPriorityClassName, runtimeClassName, helmChartDemoSpeakerName,
+	helmChartSpeakerTestName, keyName string) *Builder {
+	if valid, err := builder.validate(); !valid {
+		if err != nil {


evgenLevin · 2024-03-19T19:16:40Z

pkg/metallb/metallb.go

+		return builder
+	}
+
+	if !builder.Exists() {


evgenLevin · 2024-03-19T19:16:57Z

pkg/metallb/metallb.go

+	glog.V(100).Infof("Defining the metallb object with Helm Chart in namespace %s",
+		builder.Definition.Namespace,
+	)
+


Validate all arguments: priorityClassName, runtimeClassName, deploymentName,helmChartControllerTestName, helmChartKeyExample

Same for PriorityClassName, RuntimeClassName, those are optional fields.

These are also optional and can receive an empty string. Removing the validations

ajaggapa · 2024-03-20T13:35:44Z

lgtm

evgenLevin · 2024-03-20T19:28:22Z

pkg/metallb/metallb.go

+	}
+
+	if builder.errorMsg != "" {
+		glog.V(100).Infof("Error occurred building metallb Controller Config")


please return builder here

evgenLevin · 2024-03-20T19:28:29Z

pkg/metallb/metallb.go

+	}
+
+	if builder.errorMsg != "" {
+		glog.V(100).Infof("Error occurred building metallb Controller Config")


please return builder here

cdvultur

Small comments on optional fields that are required. Wondering if that is really the intent

cdvultur · 2024-03-25T08:33:24Z

pkg/metallb/metallb.go

+	glog.V(100).Infof("Defining the metallb object with Helm Chart in namespace %s",
+		builder.Definition.Namespace,
+	)
+


Looking at CRD, it seems that priorityClassName, runtimeClassName, are optional fields. Just wonder if is needed to enforce them.

cdvultur · 2024-03-25T08:38:32Z

pkg/metallb/metallb.go

+	glog.V(100).Infof("Defining the metallb object with Helm Chart in namespace %s",
+		builder.Definition.Namespace,
+	)
+


Same for PriorityClassName, RuntimeClassName, those are optional fields.

cdvultur

/lgtm

yprokule · 2024-04-29T12:59:19Z

pkg/metallb/metallb.go

@@ -326,6 +327,195 @@ func (builder *Builder) WithSpeakerNodeSelector(label map[string]string) *Builde
 	return builder
 }

+// WithControllerPriorityClassName adds a priority class name tp the metallb controller config.


should be to ?

Suggested change

// WithControllerPriorityClassName adds a priority class name tp the metallb controller config.

// WithControllerPriorityClassName adds a priority class name to the metallb controller config.

yprokule · 2024-04-29T12:59:38Z

pkg/metallb/metallb.go

+		return builder
+	}
+
+	// if len(priorityClassName) == 0 {


please remove commented code

yprokule · 2024-04-29T13:03:37Z

pkg/metallb/metallb.go

+				PodAffinity: &corev1.PodAffinity{
+					RequiredDuringSchedulingIgnoredDuringExecution: []corev1.PodAffinityTerm{
+						{LabelSelector: &metav1.LabelSelector{
+							MatchLabels: map[string]string{"component": controllerPodAffinityLabel},


could this be more flexible by passing in the map for the MatchLabels and a string parameter for the TopologyKey:

Suggested change

MatchLabels: map[string]string{"component": controllerPodAffinityLabel},

MatchLabels: contollerPodAffinityMap,

gkopels requested a review from kononovn as a code owner March 19, 2024 09:29

gkopels force-pushed the add-metallb-helmchart-config branch from 4e56eb9 to 26a5684 Compare March 19, 2024 10:41

ajaggapa reviewed Mar 19, 2024

View reviewed changes

evgenLevin reviewed Mar 19, 2024

View reviewed changes

gkopels force-pushed the add-metallb-helmchart-config branch 6 times, most recently from be591e3 to 1979312 Compare March 20, 2024 13:09

gkopels changed the title ~~cnf network: add metallb controller and speaker builder func~~ metallb: add metallb controller and speaker builder func Mar 20, 2024

evgenLevin reviewed Mar 20, 2024

View reviewed changes

cdvultur reviewed Mar 25, 2024

View reviewed changes

gkopels force-pushed the add-metallb-helmchart-config branch 5 times, most recently from 5d52665 to 0b9fa5b Compare March 25, 2024 19:21

cdvultur previously approved these changes Mar 26, 2024

View reviewed changes

gkopels dismissed cdvultur’s stale review via 57f1799 March 26, 2024 11:32

gkopels force-pushed the add-metallb-helmchart-config branch 3 times, most recently from 7e94f52 to e0ecc15 Compare March 31, 2024 08:52

gkopels force-pushed the add-metallb-helmchart-config branch 4 times, most recently from de41b86 to fd722cd Compare April 17, 2024 07:51

gkopels force-pushed the add-metallb-helmchart-config branch 5 times, most recently from d6a41b9 to f3976a0 Compare April 18, 2024 05:14

cnf network: add metallb controller and speaker builder func

6616b87

gkopels force-pushed the add-metallb-helmchart-config branch from f3976a0 to 6616b87 Compare April 18, 2024 05:17

yprokule reviewed Apr 29, 2024

View reviewed changes

pkg/metallb/metallb.go

return builder

}

// if len(priorityClassName) == 0 {

Copy link

Collaborator

yprokule Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove commented code

yprokule reviewed Apr 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

metallb: add metallb controller and speaker builder func #276

metallb: add metallb controller and speaker builder func #276

gkopels commented Mar 19, 2024

ajaggapa Mar 19, 2024

gkopels Mar 20, 2024

ajaggapa Mar 19, 2024

gkopels Mar 20, 2024

ajaggapa left a comment

evgenLevin Mar 19, 2024

gkopels Mar 20, 2024

evgenLevin Mar 19, 2024

gkopels Mar 20, 2024

cdvultur Mar 25, 2024

gkopels Mar 25, 2024

evgenLevin Mar 25, 2024

gkopels Mar 26, 2024

evgenLevin Mar 19, 2024

gkopels Mar 20, 2024

evgenLevin Mar 19, 2024

gkopels Mar 20, 2024

evgenLevin Mar 19, 2024

gkopels Mar 20, 2024

cdvultur Mar 25, 2024

gkopels Mar 25, 2024

ajaggapa commented Mar 20, 2024

evgenLevin Mar 20, 2024

gkopels Mar 25, 2024

evgenLevin Mar 20, 2024

gkopels Mar 25, 2024

cdvultur left a comment

cdvultur Mar 25, 2024

cdvultur Mar 25, 2024

cdvultur left a comment

yprokule Apr 29, 2024

yprokule Apr 29, 2024

yprokule Apr 29, 2024

	// WithControllerPriorityClassName adds a priority class name tp the metallb controller config.
	// WithControllerPriorityClassName adds a priority class name to the metallb controller config.

	MatchLabels: map[string]string{"component": controllerPodAffinityLabel},
	MatchLabels: contollerPodAffinityMap,

metallb: add metallb controller and speaker builder func #276

Are you sure you want to change the base?

metallb: add metallb controller and speaker builder func #276

Conversation

gkopels commented Mar 19, 2024

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

ajaggapa 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajaggapa commented Mar 20, 2024

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

cdvultur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdvultur 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