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.

Sign up for GitHub

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

Jump to bottom

chore!: change services to be headless #574

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

chore!: change services to be headless #574

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a7f3913
chore: change services to be headless
mojtaba-esk Oct 15, 2024
fb06a44
Merge branch 'main' into mojtaba/514-headless-services
mojtaba-esk Oct 15, 2024
9ce94f0
fix: GetIP & replace some of them with HostName
mojtaba-esk Oct 15, 2024
8f0a51d
fix: revert to get ip in netshaper test
mojtaba-esk Oct 16, 2024
38437cb
fix: revert to get ip in netshaper test
mojtaba-esk Oct 16, 2024
27e3ce6
chore: applied feedback
mojtaba-esk Oct 16, 2024
942e5b0
Merge branch 'main' into mojtaba/514-headless-services
mojtaba-esk Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions e2e/basic/headless_service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
package basic

import (
"context"
"fmt"
"strconv"
"time"
)

const gopingImage = "ghcr.io/celestiaorg/goping:4803195"

func (s *Suite) TestHeadlessService() {
const (
namePrefix = "headless-srv-test"
numOfPingPackets = 100
numOfTests = 10
packetTimeout = 1 * time.Second
gopingPort = 8001
)
ctx := context.Background()

mother, err := s.Knuu.NewInstance(namePrefix + "mother")
s.Require().NoError(err)

err = mother.Build().SetImage(ctx, gopingImage)
s.Require().NoError(err)

s.Require().NoError(mother.Network().AddPortTCP(gopingPort))
s.Require().NoError(mother.Build().Commit(ctx))

err = mother.Build().SetEnvironmentVariable("SERVE_ADDR", fmt.Sprintf("0.0.0.0:%d", gopingPort))
s.Require().NoError(err)
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved

target, err := mother.CloneWithName(namePrefix + "target")
s.Require().NoError(err)

executor, err := mother.CloneWithName(namePrefix + "executor")
s.Require().NoError(err)

// Prepare ping executor & target
s.Require().NoError(target.Execution().Start(ctx))
s.Require().NoError(executor.Execution().Start(ctx))

targetEndpoint := fmt.Sprintf("%s:%d", target.Network().HostName(), gopingPort)
s.T().Logf("targetEndpoint: %v", targetEndpoint)

s.T().Log("Starting ping test. It takes a while.")
for i := 0; i < numOfTests; i++ {
startTime := time.Now()

output, err := executor.Execution().ExecuteCommand(ctx, "goping", "ping", "-q",
"-c", fmt.Sprint(numOfPingPackets),
"-t", packetTimeout.String(),
"-m", "packetloss",
targetEndpoint)
s.Require().NoError(err)

elapsed := time.Since(startTime)
s.T().Logf("i: %d, test took %d seconds, output: `%s`", i, int64(elapsed.Seconds()), output)
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved

gotPacketloss, err := strconv.ParseFloat(output, 64)
s.Require().NoError(err, fmt.Sprintf("error parsing output: `%s`", output))
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved

s.Assert().Zero(gotPacketloss)
}
}
4 changes: 1 addition & 3 deletions e2e/basic/probe_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,11 @@ func (s *Suite) TestProbe() {
}
s.Require().NoError(web.Monitoring().SetLivenessProbe(&livenessProbe))
s.Require().NoError(web.Build().Commit(ctx))
s.Require().NoError(web.Execution().Start(ctx))

// Test logic
webIP, err := web.Network().GetIP(ctx)
s.Require().NoError(err)

s.Require().NoError(web.Execution().Start(ctx))

wgetOutput, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", webIP)
s.Require().NoError(err)

Expand Down
20 changes: 4 additions & 16 deletions e2e/netshaper/netshaper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,6 @@ func (s *Suite) TestNetShaperBandwidth() {

s.Require().NoError(iperfClient.Execution().Start(ctx))

iperfServerIP, err := iperfServer.Network().GetIP(ctx)
s.Require().NoError(err)

// Perform the test
type testCase struct {
name string
Expand Down Expand Up @@ -84,7 +81,7 @@ func (s *Suite) TestNetShaperBandwidth() {
s.T().Log("Starting bandwidth test. It takes a while.")
startTime := time.Now()
output, err := iperfClient.Execution().ExecuteCommand(ctx,
"iperf3", "-c", iperfServerIP,
"iperf3", "-c", iperfServer.Network().HostName(),
"-t", fmt.Sprint(int64(iperfTestDuration.Seconds())),
"-P", fmt.Sprint(iperfParallelClients), "--json")
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Issues Found: Residual Usage of GetIP and Hardcoded IP Addresses

The shell script results indicate that several files still use the GetIP function or contain hardcoded IP addresses. Notably:

  • Production Code:

    • pkg/instance/network.go:
      • Function GetIP is still present and used.

  • Test Files:

    • pkg/sidecars/netshaper/netshaper.go
    • pkg/k8s/service_test.go
    • e2e/system/folder_test.go
    • e2e/system/files_to_volumes_cm_test.go
    • e2e/basic/probe_test.go
    • e2e/basic/headless_service_test.go

These instances may require similar updates to ensure consistency with the transition to headless services. Please review and address these usages accordingly.

🔗 Analysis chain

LGTM: Successful transition to headless service model

The change from IP address to hostname (.Network().HostName()) aligns perfectly with the PR objective of transitioning to headless services. This modification ensures that the test now uses DNS names instead of IP addresses, which is consistent with the removal of the GetIP function as mentioned in the linked issue #514.

To ensure this change is consistent across the codebase, let's verify other occurrences:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining uses of GetIP or direct IP address usage
rg --type go 'GetIP|[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+'

Length of output: 1328

s.Require().NoError(err)
Expand Down Expand Up @@ -168,9 +165,6 @@ func (s *Suite) TestNetShaperPacketloss() {
}
}

targetIP, err := target.Network().GetIP(ctx)
s.Require().NoError(err)

for _, tc := range tt {
tc := tc
s.Run(tc.name, func() {
Expand All @@ -180,7 +174,7 @@ func (s *Suite) TestNetShaperPacketloss() {
s.T().Log("Starting packetloss test. It takes a while.")
startTime := time.Now()

targetAddress := fmt.Sprintf("%s:%d", targetIP, gopingPort)
targetAddress := fmt.Sprintf("%s:%d", target.Network().HostName(), gopingPort)
output, err := executor.Execution().ExecuteCommand(ctx, "goping", "ping", "-q",
"-c", fmt.Sprint(numOfPingPackets),
"-t", packetTimeout.String(),
Expand Down Expand Up @@ -265,9 +259,6 @@ func (s *Suite) TestNetShaperLatency() {
}
}

targetIP, err := target.Network().GetIP(ctx)
s.Require().NoError(err)

for _, tc := range tt {
tc := tc
s.Run(tc.name, func() {
Expand All @@ -279,7 +270,7 @@ func (s *Suite) TestNetShaperLatency() {
s.T().Log("Starting latency test. It takes a while.")
startTime := time.Now()

targetAddress := fmt.Sprintf("%s:%d", targetIP, gopingPort)
targetAddress := fmt.Sprintf("%s:%d", target.Network().HostName(), gopingPort)
output, err := executor.Execution().ExecuteCommand(ctx,
"goping", "ping", "-q",
"-c", fmt.Sprint(numOfPingPackets),
Expand Down Expand Up @@ -359,9 +350,6 @@ func (s *Suite) TestNetShaperJitter() {
}
}

targetIP, err := target.Network().GetIP(ctx)
s.Require().NoError(err)

for _, tc := range tt {
tc := tc
s.Run(tc.name, func() {
Expand All @@ -373,7 +361,7 @@ func (s *Suite) TestNetShaperJitter() {
s.T().Log("Starting jitter test. It takes a while.")
startTime := time.Now()

targetAddress := fmt.Sprintf("%s:%d", targetIP, gopingPort)
targetAddress := fmt.Sprintf("%s:%d", target.Network().HostName(), gopingPort)
output, err := executor.Execution().ExecuteCommand(ctx,
"goping", "ping", "-q",
"-c", fmt.Sprint(numOfPingPackets),
Expand Down
5 changes: 1 addition & 4 deletions e2e/system/env_to_json_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,7 @@ func (s *Suite) TestEnvToJSON() {

// Test logic
for _, i := range instances {
webIP, err := i.Network().GetIP(ctx)
s.Require().NoError(err)

wget, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", webIP)
wget, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", i.Network().HostName())
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved
s.Require().NoError(err)

expectedBytes, err := json.Marshal(envVars)
Expand Down
7 changes: 1 addition & 6 deletions e2e/system/external_file_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,9 @@ func (s *Suite) TestExternalFile() {
s.Require().NoError(err)

s.Require().NoError(server.Build().Commit(ctx))

// Test logic
serverIP, err := server.Network().GetIP(ctx)
s.Require().NoError(err)

s.Require().NoError(server.Execution().Start(ctx))

wget, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", serverIP)
wget, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", server.Network().HostName())
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved
s.Require().NoError(err)

s.Assert().Contains(wget, "Hello World!")
Expand Down
11 changes: 1 addition & 10 deletions e2e/system/file_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,6 @@ func (s *Suite) TestFile() {
s.Require().NoError(err, "Error committing changes")

// Test logic
s.T().Log("Getting server IP")
var serverfileIP string
err = s.RetryOperation(func() error {
var err error
serverfileIP, err = serverfile.Network().GetIP(ctx)
return err
}, maxRetries)
s.Require().NoError(err, "Error getting server IP")

s.T().Log("Starting server")
err = s.RetryOperation(func() error {
return serverfile.Execution().Start(ctx)
Expand All @@ -60,7 +51,7 @@ func (s *Suite) TestFile() {
var wget string
err = s.RetryOperation(func() error {
var err error
wget, err = executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", serverfileIP)
wget, err = executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", serverfile.Network().HostName())
return err
}, maxRetries)
s.Require().NoError(err, "Error executing wget command")
Expand Down
7 changes: 1 addition & 6 deletions e2e/system/file_test_image_cached_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,11 @@ func (s *Suite) TestFileCached() {

for _, i := range instances {
err := s.RetryOperation(func() error {
webIP, err := i.Network().GetIP(ctx)
if err != nil {
return fmt.Errorf("getting IP: %w", err)
}

if err := i.Execution().WaitInstanceIsRunning(ctx); err != nil {
return fmt.Errorf("waiting for instance to run: %w", err)
}

wget, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", webIP)
wget, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", i.Network().HostName())
if err != nil {
return fmt.Errorf("executing wget: %w", err)
}
Expand Down
33 changes: 9 additions & 24 deletions e2e/system/files_to_volumes_cm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,10 @@ func (s *Suite) TestNoVolumesNoFiles() {

target := s.CreateNginxInstance(ctx, namePrefix+"-target")
s.Require().NoError(target.Build().Commit(ctx))
s.Require().NoError(target.Execution().Start(ctx))

// Test logic
s.Require().NoError(target.Execution().StartAsync(ctx))

webIP, err := target.Network().GetIP(ctx)
s.Require().NoError(err)

s.Require().NoError(target.Execution().WaitInstanceIsRunning(ctx))

wget, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", webIP)
wget, err := executor.Execution().
ExecuteCommand(ctx, "wget", "-q", "-O", "-", target.Network().HostName())
s.Require().NoError(err)

s.Assert().Contains(wget, "Welcome to nginx!")
Expand All @@ -59,13 +53,11 @@ func (s *Suite) TestOneVolumeNoFiles() {
s.Require().NoError(target.Build().Commit(ctx))

// Test logic
s.Require().NoError(target.Execution().StartAsync(ctx))
s.Require().NoError(target.Execution().Start(ctx))

webIP, err := target.Network().GetIP(ctx)
s.Require().NoError(err)

s.Require().NoError(target.Execution().WaitInstanceIsRunning(ctx))

wget, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", webIP)
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved
s.Require().NoError(err)

Expand Down Expand Up @@ -114,13 +106,10 @@ func (s *Suite) TestNoVolumesOneFile() {
}

for _, i := range instances {
webIP, err := i.Network().GetIP(ctx)
s.Require().NoError(err)

err = i.Execution().WaitInstanceIsRunning(ctx)
s.Require().NoError(err)

wget, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", webIP)
wget, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", i.Network().HostName())
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved
s.Require().NoError(err)
wget = strings.TrimSpace(wget)

Expand Down Expand Up @@ -167,11 +156,9 @@ func (s *Suite) TestOneVolumeOneFile() {
}

for _, i := range instances {
webIP, err := i.Network().GetIP(ctx)
s.Require().NoError(err)
s.Require().NoError(i.Execution().WaitInstanceIsRunning(ctx))

wget, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", webIP)
wget, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", i.Network().HostName())
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved
s.Require().NoError(err)
wget = strings.TrimSpace(wget)

Expand Down Expand Up @@ -237,17 +224,15 @@ func (s *Suite) TestOneVolumeTwoFiles() {
}

for _, i := range instances {
webIP, err := i.Network().GetIP(ctx)
s.Require().NoError(err)
s.Require().NoError(i.Execution().WaitInstanceIsRunning(ctx))

wgetIndex, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", webIP)
wgetIndex, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", i.Network().HostName())
s.Require().NoError(err)
wgetIndex = strings.TrimSpace(wgetIndex)
s.Assert().Equal("hello from 1", wgetIndex)

webIP2 := webIP + "/index-2.html"
wgetIndex2, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", webIP2)
webHost2 := i.Network().HostName() + "/index-2.html"
wgetIndex2, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", webHost2)
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved
s.Require().NoError(err)
wgetIndex2 = strings.TrimSpace(wgetIndex2)
s.Assert().Equal("hello from 2", wgetIndex2)
Expand Down
3 changes: 1 addition & 2 deletions e2e/system/folder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,12 @@ func (s *Suite) TestFolder() {
require.NoError(s.T(), err)

require.NoError(s.T(), web.Build().Commit(ctx))
s.Require().NoError(web.Execution().Start(ctx))
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved

// Test logic
webIP, err := web.Network().GetIP(ctx)
s.Require().NoError(err)

s.Require().NoError(web.Execution().Start(ctx))

wget, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", webIP)
s.Require().NoError(err)

Expand Down
5 changes: 1 addition & 4 deletions e2e/system/folder_test_image_cached_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,9 @@ func (s *Suite) TestFolderCached() {
}

for _, i := range instances {
webIP, err := i.Network().GetIP(ctx)
s.Require().NoError(err)

s.Require().NoError(i.Execution().WaitInstanceIsRunning(ctx))

wget, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", webIP)
wget, err := executor.Execution().ExecuteCommand(ctx, "wget", "-q", "-O", "-", i.Network().HostName())
s.Require().NoError(err)

s.Assert().Contains(wget, "Hello World!")
Expand Down
2 changes: 2 additions & 0 deletions pkg/instance/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,5 +220,7 @@ var (
ErrAddingHostToProxyNotAllowed = errors.New("AddingHostToProxyNotAllowed", "adding host to proxy is only allowed in state 'Started' and 'Preparing'. Current state is '%s'")
ErrInstanceNameAlreadyExists = errors.New("InstanceNameAlreadyExists", "instance name '%s' already exists")
ErrSettingSidecarName = errors.New("SettingSidecarName", "error setting sidecar name with prefix '%s' for instance '%s'")
ErrGettingServiceEndpointNotAllowed = errors.New("GettingServiceEndpointNotAllowed", "getting service endpoint is only allowed in state 'Started'. Current state is '%s'")
ErrCannotCloneInstance = errors.New("CannotCloneInstance", "cannot clone instance '%s' in state '%s'")
ErrGettingIPNotAllowed = errors.New("GettingIPNotAllowed", "getting IP is allowed in state 'Started'. Current state is '%s'")
)
34 changes: 11 additions & 23 deletions pkg/instance/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,34 +119,22 @@ func (n *network) AddPortUDP(port int) error {
}

// GetIP returns the IP of the instance
// This function can only be called in the states 'Preparing' and 'Started'
// This function can only be called in the states 'Started'
func (n *network) GetIP(ctx context.Context) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

The problem may be that the developer expects this IP to be static. But in fact, the IP changes if the Pod restarts. A restart can happen either by the users' intentions or 'randomly' by Kubernetes.
One idea is to document it properly, but the developer might oversee this. Another option would be to remove GetIP and entirely require the switch to HostName.

Copy link
Member

Choose a reason for hiding this comment

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

We should definitely document the fact that the IP can change.

Copy link
Member Author

Choose a reason for hiding this comment

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

true. a proper documentation needs to be added.

// Check if i.kubernetesService already has the IP
if n.kubernetesService != nil && n.kubernetesService.Spec.ClusterIP != "" {
return n.kubernetesService.Spec.ClusterIP, nil
}
// If not, proceed with the existing logic to deploy the service and get the IP
svc, err := n.instance.K8sClient.GetService(ctx, n.instance.name)
if err != nil || svc == nil {
// Service does not exist, so we need to deploy it
err := n.deployService(ctx, n.portsTCP, n.portsUDP)
if err != nil {
return "", ErrDeployingServiceForInstance.WithParams(n.instance.name).Wrap(err)
}
svc, err = n.instance.K8sClient.GetService(ctx, n.instance.name)
if err != nil {
return "", ErrGettingServiceForInstance.WithParams(n.instance.name).Wrap(err)
}
if !n.instance.IsInState(StateStarted) {
return "", ErrGettingIPNotAllowed.WithParams(n.instance.state.String())
}

ip := svc.Spec.ClusterIP
if ip == "" {
return "", ErrGettingServiceIP.WithParams(n.instance.name)
pod, err := n.instance.K8sClient.GetFirstPodFromReplicaSet(ctx, n.instance.name)
if err != nil {
return "", ErrGettingPodFromReplicaSet.WithParams(n.instance.name).Wrap(err)
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved
}

// Update i.kubernetesService for future reference
n.kubernetesService = svc
return ip, nil
return pod.Status.PodIP, nil
}

func (n *network) HostName() string {
return n.instance.K8sClient.ServiceDNS(n.instance.name)
}
mojtaba-esk marked this conversation as resolved.
Show resolved Hide resolved

// deployService deploys the service for the instance
Expand Down
Loading
Loading