Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

System Test, RDS Core: metalLB traffic segregation test suite was added #392

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

elenagerman
Copy link
Collaborator

No description provided.

@elenagerman elenagerman force-pushed the metallb-segregation-ver branch from 2c2a2bd to d420990 Compare January 30, 2025 16:52
@elenagerman elenagerman requested a review from yprokule January 30, 2025 17:34
@elenagerman elenagerman force-pushed the metallb-segregation-ver branch 6 times, most recently from f072214 to e230adf Compare February 3, 2025 15:35
@elenagerman
Copy link
Collaborator Author

@yprokule PTAL

@elenagerman elenagerman force-pushed the metallb-segregation-ver branch from c764ac0 to e0a8667 Compare February 4, 2025 15:36
@elenagerman elenagerman requested a review from yprokule February 4, 2025 15:37

glog.V(100).Infof("Setting Replicas count")

replicasCnt := len(scheduleOnHosts)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather expose replica count as a parameter to cover a use case where a pod(s) can ran on multiple nodes but different amount of replicas is required (for e.g. a pod could run on node-x, node-y, node-z, but app requires only 2 instances).
Anyway that's only a suggestion so leaving it up to U

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm sorry, but I'm not sure it's a good idea. different nodes/pods count is not exactly our case.
Also, the question is, why provide more nodes than you need pods?
In our case, for both tcpdump and traceroute scenarios, we provide precisely the nodes list where we need to deploy the support tools pods; I mean, we need to monitor a specific node => we need a pod on this node.
I would leave it like this right now if you don't mind.
If we have any future use cases with different requirements, we can modify it. But without a specific proposal for different nodes and pod counts, it's hard to understand how it would be better to change it.

Skip("FRR One Load Balance app not found deployed")
}

glog.V(rdscoreparams.RDSCoreLogLevel).Infof("Getting node name for the loadbalancer one application")
Copy link
Collaborator

Choose a reason for hiding this comment

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

where's the code to get the node name? assume smth like:

lbOneNodeName = lbOnePodsList[0].Object.Spec.NodeName

Comment on lines +464 to +465
glog.V(rdscoreparams.RDSCoreLogLevel).Infof("Searching for pods in %q namespace",
RDSCoreConfig.MetalLBLoadBalancerTwoNamespace)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
glog.V(rdscoreparams.RDSCoreLogLevel).Infof("Searching for pods in %q namespace",
RDSCoreConfig.MetalLBLoadBalancerTwoNamespace)
glog.V(rdscoreparams.RDSCoreLogLevel).Infof("Searching for pods in %q namespace with %q label",
RDSCoreConfig.MetalLBLoadBalancerTwoNamespace, lbDeploymentLabel)


glog.V(rdscoreparams.RDSCoreLogLevel).Infof("Getting node name for the loadbalancer two application")

lbTwoNodeName = lbTwoPodsList[0].Object.Spec.NodeName
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto, what happens if 2 pods found? is it safe to ignore all other pods and always use 1st one?

Comment on lines +598 to +599
glog.V(100).Infof(fmt.Sprintf("the IP routes %s were found on the FRR container %s: %s",
lbIP, containerName, result))
Copy link
Collaborator

Choose a reason for hiding this comment

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

the command is show ip(v6) route bgp so not sure why it's stated that route for requested IP address was present in the output of the aforementioned command:

Suggested change
glog.V(100).Infof(fmt.Sprintf("the IP routes %s were found on the FRR container %s: %s",
lbIP, containerName, result))
glog.V(100).Infof(fmt.Sprintf("BGP routes were found in the FRR container %s\n: %s",
containerName, result))

})

if err != nil {
glog.V(100).Infof("Failed to verify IP route bgp on the FRR container %s: %v", containerName, err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
glog.V(100).Infof("Failed to verify IP route bgp on the FRR container %s: %v", containerName, err)
glog.V(100).Infof("Failed to verify BGP routes in the FRR container %s: %v", containerName, err)

if err != nil {
glog.V(100).Infof("Failed to verify IP route bgp on the FRR container %s: %v", containerName, err)

return fmt.Errorf("failed to verify IP route bgp on the FRR container %s: %w", containerName, err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("failed to verify IP route bgp on the FRR container %s: %w", containerName, err)
return fmt.Errorf("failed to verify BGP routes in the FRR container %s: %w", containerName, err)

}

if !strings.Contains(result, lbIP) {
glog.V(100).Infof("No IP route bgp %s on the FRR container %s was found", lbIP, containerName)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
glog.V(100).Infof("No IP route bgp %s on the FRR container %s was found", lbIP, containerName)
glog.V(100).Infof("No BGP route %s in the FRR container %s was found", lbIP, containerName)

if !strings.Contains(result, lbIP) {
glog.V(100).Infof("No IP route bgp %s on the FRR container %s was found", lbIP, containerName)

return fmt.Errorf("no IP route bgp %s on the FRR container %s was found", lbIP, containerName)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("no IP route bgp %s on the FRR container %s was found", lbIP, containerName)
return fmt.Errorf("no BGP route %s in the FRR container %s was found", lbIP, containerName)

time.Second*3,
true,
func(ctx context.Context) (bool, error) {
for i := 0; i < 3; i++ {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the function is already retried so why this extra for loop?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants