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

🌱 Improve logging for self-hosted e2e test #9637

Merged
merged 1 commit into from
Nov 6, 2023
Merged

🌱 Improve logging for self-hosted e2e test #9637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 2 additions & 8 deletions test/e2e/clusterctl_upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ func ClusterctlUpgradeSpec(ctx context.Context, inputGetter func() ClusterctlUpg
}

// Get a ClusterProxy so we can interact with the workload cluster
managementClusterProxy = input.BootstrapClusterProxy.GetWorkloadCluster(ctx, cluster.Namespace, cluster.Name)
managementClusterProxy = input.BootstrapClusterProxy.GetWorkloadCluster(ctx, cluster.Namespace, cluster.Name, framework.WithMachineLogCollector(input.BootstrapClusterProxy.GetLogCollector()))
Copy link
Contributor Author

@killianmuldoon killianmuldoon Nov 1, 2023 •
edited

Choose a reason for hiding this comment

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

Alternatively we could always add a logCollector to the workload ClusterProxy where the bootstrap ClusterProxy has one. This might be less surprising for users (it was surprising for me 😅)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This won't work if the bootstrap cluster and the workload cluster are from different infrastructure providers. I'm not sure if that's something we really support in our e2e tests though. This logger would also have trouble with e.g. mixed infrastructure.

Should I work on something that's able to pick the logger based on the infrastructure used for the Cluster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chrischdi @sbueringer WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with both variants.

The variant on this PR would help to not create too many (unnecessary?) logs which waste resources. So I lean to keeping this for now.

Copy link
Member

Choose a reason for hiding this comment

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

Current version is fine


// Download the older clusterctl version to be used for setting up the management cluster to be upgraded
log.Logf("Downloading clusterctl binary from %s", initClusterctlBinaryURL)
Expand Down Expand Up @@ -543,13 +543,7 @@ func ClusterctlUpgradeSpec(ctx context.Context, inputGetter func() ClusterctlUpg
AfterEach(func() {
if testNamespace != nil {
// Dump all the logs from the workload cluster before deleting them.
managementClusterProxy.CollectWorkloadClusterLogs(ctx, testNamespace.Name, managementClusterName, filepath.Join(input.ArtifactFolder, "clusters", managementClusterName, "machines"))

framework.DumpAllResources(ctx, framework.DumpAllResourcesInput{
Lister: managementClusterProxy.GetClient(),
Namespace: testNamespace.Name,
LogPath: filepath.Join(input.ArtifactFolder, "clusters", managementClusterResources.Cluster.Name, "resources"),
})
dumpAllResources(ctx, managementClusterProxy, input.ArtifactFolder, testNamespace, managementClusterResources.Cluster)

if !input.SkipCleanup {
switch {
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/self_hosted.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ func SelfHostedSpec(ctx context.Context, inputGetter func() SelfHostedSpecInput)

cluster := clusterResources.Cluster
// Get a ClusterBroker so we can interact with the workload cluster
selfHostedClusterProxy = input.BootstrapClusterProxy.GetWorkloadCluster(ctx, cluster.Namespace, cluster.Name)
selfHostedClusterProxy = input.BootstrapClusterProxy.GetWorkloadCluster(ctx, cluster.Namespace, cluster.Name, framework.WithMachineLogCollector(input.BootstrapClusterProxy.GetLogCollector()))

Byf("Creating a namespace for hosting the %s test spec", specName)
selfHostedNamespace, selfHostedCancelWatches = framework.CreateNamespaceAndWatchEvents(ctx, framework.CreateNamespaceAndWatchEventsInput{
Expand Down
15 changes: 10 additions & 5 deletions test/framework/cluster_proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ type ClusterProxy interface {
Apply(ctx context.Context, resources []byte, args ...string) error

// GetWorkloadCluster returns a proxy to a workload cluster defined in the Kubernetes cluster.
GetWorkloadCluster(ctx context.Context, namespace, name string) ClusterProxy
GetWorkloadCluster(ctx context.Context, namespace, name string, options ...Option) ClusterProxy

// CollectWorkloadClusterLogs collects machines and infrastructure logs from the workload cluster.
CollectWorkloadClusterLogs(ctx context.Context, namespace, name, outputPath string)
Expand Down Expand Up @@ -156,7 +156,7 @@ func NewClusterProxy(name string, kubeconfigPath string, scheme *runtime.Scheme,
}

// newFromAPIConfig returns a clusterProxy given a api.Config and the scheme defining the types hosted in the cluster.
func newFromAPIConfig(name string, config *api.Config, scheme *runtime.Scheme) ClusterProxy {
func newFromAPIConfig(name string, config *api.Config, scheme *runtime.Scheme, options ...Option) ClusterProxy {
// NB. the ClusterProvider is responsible for the cleanup of this file
f, err := os.CreateTemp("", "e2e-kubeconfig")
Expect(err).ToNot(HaveOccurred(), "Failed to create kubeconfig file for the kind cluster %q")
Expand All @@ -165,12 +165,16 @@ func newFromAPIConfig(name string, config *api.Config, scheme *runtime.Scheme) C
err = clientcmd.WriteToFile(*config, kubeconfigPath)
Expect(err).ToNot(HaveOccurred(), "Failed to write kubeconfig for the kind cluster to a file %q")

return &clusterProxy{
proxy := &clusterProxy{
name: name,
kubeconfigPath: kubeconfigPath,
scheme: scheme,
shouldCleanupKubeconfig: true,
}
for _, o := range options {
o(proxy)
}
return proxy
}

// GetName returns the name of the cluster.
Expand Down Expand Up @@ -265,7 +269,7 @@ func (p *clusterProxy) GetLogCollector() ClusterLogCollector {
}

// GetWorkloadCluster returns ClusterProxy for the workload cluster.
func (p *clusterProxy) GetWorkloadCluster(ctx context.Context, namespace, name string) ClusterProxy {
func (p *clusterProxy) GetWorkloadCluster(ctx context.Context, namespace, name string, options ...Option) ClusterProxy {
Expect(ctx).NotTo(BeNil(), "ctx is required for GetWorkloadCluster")
Expect(namespace).NotTo(BeEmpty(), "namespace is required for GetWorkloadCluster")
Expect(name).NotTo(BeEmpty(), "name is required for GetWorkloadCluster")
Expand All @@ -279,12 +283,13 @@ func (p *clusterProxy) GetWorkloadCluster(ctx context.Context, namespace, name s
p.fixConfig(ctx, name, config)
}

return newFromAPIConfig(name, config, p.scheme)
return newFromAPIConfig(name, config, p.scheme, options...)
}

// CollectWorkloadClusterLogs collects machines and infrastructure logs and from the workload cluster.
func (p *clusterProxy) CollectWorkloadClusterLogs(ctx context.Context, namespace, name, outputPath string) {
if p.logCollector == nil {
fmt.Printf("Unable to get logs for workload Cluster %s: log collector is nil.\n", klog.KRef(namespace, name))
return
}

Expand Down
4 changes: 2 additions & 2 deletions test/framework/machine_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ func WaitForControlPlaneMachinesToBeUpgraded(ctx context.Context, input WaitForC
}
}
if len(machines) > upgraded {
return 0, errors.New("old nodes remain")
return 0, errors.New("old Machines remain")
}
return upgraded, nil
}, intervals...).Should(Equal(input.MachineCount), "Timed out waiting for all control-plane machines in Cluster %s to be upgraded to kubernetes version %s", klog.KObj(input.Cluster), input.KubernetesUpgradeVersion)
Expand Down Expand Up @@ -209,7 +209,7 @@ func WaitForMachineDeploymentMachinesToBeUpgraded(ctx context.Context, input Wai
}
}
if len(machines) > upgraded {
return 0, errors.New("old nodes remain")
return 0, errors.New("old Machines remain")
}
return upgraded, nil
}, intervals...).Should(Equal(input.MachineCount), "Timed out waiting for all MachineDeployment %s Machines to be upgraded to kubernetes version %s", klog.KObj(&input.MachineDeployment), input.KubernetesUpgradeVersion)
Expand Down