⚠️ Add MachinePool to e2e framework log collector

test/e2e/common.go

@@ -63,7 +63,7 @@ func dumpSpecResourcesAndCleanup(ctx context.Context, specName string, clusterPr
 	Byf("Dumping logs from the %q workload cluster", cluster.Name)
 
 	// Dump all the logs from the workload cluster before deleting them.
-	clusterProxy.CollectWorkloadClusterLogs(ctx, cluster.Namespace, cluster.Name, filepath.Join(artifactFolder, "clusters", cluster.Name, "machines"))
+	clusterProxy.CollectWorkloadClusterLogs(ctx, cluster.Namespace, cluster.Name, filepath.Join(artifactFolder, "clusters", cluster.Name))
 
 	Byf("Dumping all the Cluster API resources in the %q namespace", namespace.Name)
 

test/framework/cluster_proxy.go

@@ -18,11 +18,13 @@ package framework
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/url"
 	"os"
 	"path"
 	goruntime "runtime"
+	expv1 "sigs.k8s.io/cluster-api/exp/api/v1alpha4"
 	"strings"
 
 	. "github.com/onsi/gomega"
@@ -81,6 +83,7 @@ type ClusterLogCollector interface {
 	// CollectMachineLog collects log from a machine.
 	// TODO: describe output folder struct
 	CollectMachineLog(ctx context.Context, managementClusterClient client.Client, m *clusterv1.Machine, outputPath string) error
+	CollectMachinePoolLog(ctx context.Context, managementClusterClient client.Client, m *expv1.MachinePool, outputPath string) error
 }
 
 // Option is a configuration option supplied to NewClusterProxy.
@@ -226,29 +229,54 @@ func (p *clusterProxy) CollectWorkloadClusterLogs(ctx context.Context, namespace
 
 	for i := range machines.Items {
 		m := &machines.Items[i]
-		err := p.logCollector.CollectMachineLog(ctx, p.GetClient(), m, path.Join(outputPath, m.GetName()))
+		err := p.logCollector.CollectMachineLog(ctx, p.GetClient(), m, path.Join(outputPath, "machines", m.GetName()))
 		if err != nil {
 			// NB. we are treating failures in collecting logs as a non blocking operation (best effort)
 			fmt.Printf("Failed to get logs for machine %s, cluster %s/%s: %v\n", m.GetName(), namespace, name, err)
 		}
 	}
+
+	machinePools, err := getMachinePoolsInCluster(ctx, p.GetClient(), namespace, name)
+	Expect(err).ToNot(HaveOccurred(), "Failed to get machine pools for the %s/%s cluster", namespace, name)
+
+	for i := range machinePools.Items {
+		mp := &machinePools.Items[i]
+		err := p.logCollector.CollectMachinePoolLog(ctx, p.GetClient(), mp, path.Join(outputPath, "machine-pools", mp.GetName()))
+		if err != nil {
+			// NB. we are treating failures in collecting logs as a non blocking operation (best effort)
+			fmt.Printf("Failed to get logs for machine pool %s, cluster %s/%s: %v\n", mp.GetName(), namespace, name, err)
+		}
+	}
 }
 
 func getMachinesInCluster(ctx context.Context, c client.Client, namespace, name string) (*clusterv1.MachineList, error) {
 	if name == "" {
-		return nil, nil
+		return nil, errors.New("cluster name should not be empty")
 	}
 
 	machineList := &clusterv1.MachineList{}
 	labels := map[string]string{clusterv1.ClusterLabelName: name}
-
 	if err := c.List(ctx, machineList, client.InNamespace(namespace), client.MatchingLabels(labels)); err != nil {
 		return nil, err
 	}
 
 	return machineList, nil
 }
 
+func getMachinePoolsInCluster(ctx context.Context, c client.Client, namespace, name string) (*expv1.MachinePoolList, error) {
+	if name == "" {
+		return nil, errors.New("cluster name should not be empty")
+	}
+
+	machinePoolList := &expv1.MachinePoolList{}
+	labels := map[string]string{clusterv1.ClusterLabelName: name}
+	if err := c.List(ctx, machinePoolList, client.InNamespace(namespace), client.MatchingLabels(labels)); err != nil {
+		return nil, err
+	}
+
+	return machinePoolList, nil
+}
+
 func (p *clusterProxy) getKubeconfig(ctx context.Context, namespace string, name string) *api.Config {
 	cl := p.GetClient()
 

test/framework/docker_logcollector.go

@@ -19,9 +19,11 @@ package framework
 import (
 	"context"
 	"fmt"
+	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"os"
 	osExec "os/exec"
 	"path/filepath"
+	expv1 "sigs.k8s.io/cluster-api/exp/api/v1alpha4"
 	"strings"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
@@ -45,6 +47,22 @@ func machineContainerName(cluster, machine string) string {
 
 func (k DockerLogCollector) CollectMachineLog(ctx context.Context, managementClusterClient client.Client, m *clusterv1.Machine, outputPath string) error {
 	containerName := machineContainerName(m.Spec.ClusterName, m.Name)
+	return k.collectLogsFromNode(outputPath, containerName)
+}
+
+func (k DockerLogCollector) CollectMachinePoolLog(ctx context.Context, managementClusterClient client.Client, m *expv1.MachinePool, outputPath string) error {
+	var errs []error
+	for _, instance := range m.Status.NodeRefs {
+		containerName := machineContainerName(m.Spec.ClusterName, instance.Name)
+		if err := k.collectLogsFromNode(filepath.Join(outputPath, instance.Name), containerName); err != nil {
+			// collecting logs is best effort so we proceed to the next instance even if we encounter an error.
+			errs = append(errs, err)
+		}
+	}
+	return kerrors.NewAggregate(errs)
+}
+
+func (k DockerLogCollector) collectLogsFromNode(outputPath string, containerName string) error {
 	execToPathFn := func(outputFileName, command string, args ...string) func() error {
 		return func() error {
 			f, err := fileOnHost(filepath.Join(outputPath, outputFileName))

test/framework/machinepool_helpers.go

@@ -20,7 +20,6 @@ import (
 	"context"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"sigs.k8s.io/cluster-api/test/framework/internal/log"
 	"sigs.k8s.io/cluster-api/util/patch"
 
@@ -92,7 +91,7 @@ type DiscoveryAndWaitForMachinePoolsInput struct {
 	Cluster *clusterv1.Cluster
 }
 
-// DiscoveryAndWaitForMachinePools discovers the MachinePools existing in a cluster and waits for them to be ready (all the machine provisioned).
+// DiscoveryAndWaitForMachinePools discovers the MachinePools existing in a cluster and waits for them to be ready (all the machines provisioned).
 func DiscoveryAndWaitForMachinePools(ctx context.Context, input DiscoveryAndWaitForMachinePoolsInput, intervals ...interface{}) []*clusterv1exp.MachinePool {
 	Expect(ctx).NotTo(BeNil(), "ctx is required for DiscoveryAndWaitForMachinePools")
 	Expect(input.Lister).ToNot(BeNil(), "Invalid argument. input.Lister can't be nil when calling DiscoveryAndWaitForMachinePools")
@@ -131,25 +130,23 @@ func UpgradeMachinePoolAndWait(ctx context.Context, input UpgradeMachinePoolAndW
 	mgmtClient := input.ClusterProxy.GetClient()
 	for i := range input.MachinePools {
 		mp := input.MachinePools[i]
-		log.Logf("Patching the new kubernetes version to Machine Pool %s/%s", mp.Namespace, mp.Name)
+		log.Logf("Patching the new Kubernetes version to Machine Pool %s/%s", mp.Namespace, mp.Name)
 		patchHelper, err := patch.NewHelper(mp, mgmtClient)
 		Expect(err).ToNot(HaveOccurred())
 
+		oldVersion := mp.Spec.Template.Spec.Version
 		mp.Spec.Template.Spec.Version = &input.UpgradeVersion
 		Expect(patchHelper.Patch(ctx, mp)).To(Succeed())
-	}
 
-	for i := range input.MachinePools {
-		mp := input.MachinePools[i]
-		oldVersion := mp.Spec.Template.Spec.Version
 		log.Logf("Waiting for Kubernetes versions of machines in MachinePool %s/%s to be upgraded from %s to %s",
 			mp.Namespace, mp.Name, *oldVersion, input.UpgradeVersion)
 		WaitForMachinePoolInstancesToBeUpgraded(ctx, WaitForMachinePoolInstancesToBeUpgradedInput{
 			Getter:                   mgmtClient,
+			WorkloadClusterGetter:    input.ClusterProxy.GetWorkloadCluster(ctx, input.Cluster.Namespace, input.Cluster.Name).GetClient(),
 			Cluster:                  input.Cluster,
 			MachineCount:             int(*mp.Spec.Replicas),
 			KubernetesUpgradeVersion: input.UpgradeVersion,
-			MachinePool:              *mp,
+			MachinePool:              mp,
 		}, input.WaitForMachinePoolToBeUpgraded...)
 	}
 }
@@ -190,10 +187,11 @@ func ScaleMachinePoolAndWait(ctx context.Context, input ScaleMachinePoolAndWaitI
 // WaitForMachinePoolInstancesToBeUpgradedInput is the input for WaitForMachinePoolInstancesToBeUpgraded.
 type WaitForMachinePoolInstancesToBeUpgradedInput struct {
 	Getter                   Getter
+	WorkloadClusterGetter    Getter
 	Cluster                  *clusterv1.Cluster
 	KubernetesUpgradeVersion string
 	MachineCount             int
-	MachinePool              clusterv1exp.MachinePool
+	MachinePool              *clusterv1exp.MachinePool
 }
 
 // WaitForMachinePoolInstancesToBeUpgraded waits until all instances belonging to a MachinePool are upgraded to the correct kubernetes version.
@@ -207,10 +205,17 @@ func WaitForMachinePoolInstancesToBeUpgraded(ctx context.Context, input WaitForM
 
 	log.Logf("Ensuring all MachinePool Instances have upgraded kubernetes version %s", input.KubernetesUpgradeVersion)
 	Eventually(func() (int, error) {
-		versions := GetMachinePoolInstanceVersions(ctx, GetMachinesPoolInstancesInput{
-			Getter:      input.Getter,
-			Namespace:   input.Cluster.Namespace,
-			MachinePool: input.MachinePool,
+		nn := client.ObjectKey{
+			Namespace: input.MachinePool.Namespace,
+			Name:      input.MachinePool.Name,
+		}
+		if err := input.Getter.Get(ctx, nn, input.MachinePool); err != nil {
+			return 0, err
+		}
+		versions := getMachinePoolInstanceVersions(ctx, GetMachinesPoolInstancesInput{
+			WorkloadClusterGetter: input.WorkloadClusterGetter,
+			Namespace:             input.Cluster.Namespace,
+			MachinePool:           input.MachinePool,
 		})
 
 		matches := 0
@@ -230,41 +235,30 @@ func WaitForMachinePoolInstancesToBeUpgraded(ctx context.Context, input WaitForM
 
 // GetMachinesPoolInstancesInput is the input for GetMachinesPoolInstances.
 type GetMachinesPoolInstancesInput struct {
-	Getter      Getter
-	Namespace   string
-	MachinePool clusterv1exp.MachinePool
+	WorkloadClusterGetter Getter
+	Namespace             string
+	MachinePool           *clusterv1exp.MachinePool
 }
 
-// GetMachinePoolInstanceVersions returns the.
-func GetMachinePoolInstanceVersions(ctx context.Context, input GetMachinesPoolInstancesInput) []string {
-	Expect(ctx).NotTo(BeNil(), "ctx is required for GetMachinePoolInstanceVersions")
-	Expect(input.Namespace).ToNot(BeEmpty(), "Invalid argument. input.Namespace can't be empty when calling GetMachinePoolInstanceVersions")
-	Expect(input.MachinePool).ToNot(BeNil(), "Invalid argument. input.MachineDeployment can't be nil when calling GetMachinePoolInstanceVersions")
-
-	obj := getUnstructuredRef(ctx, input.Getter, &input.MachinePool.Spec.Template.Spec.InfrastructureRef, input.Namespace)
-	instances, found, err := unstructured.NestedSlice(obj.Object, "status", "instances")
-	Expect(err).ToNot(HaveOccurred(), "failed to extract machines from unstructured")
-	if !found {
-		return nil
-	}
+// getMachinePoolInstanceVersions returns the Kubernetes versions of the machine pool instances.
+func getMachinePoolInstanceVersions(ctx context.Context, input GetMachinesPoolInstancesInput) []string {
+	Expect(ctx).NotTo(BeNil(), "ctx is required for getMachinePoolInstanceVersions")
+	Expect(input.WorkloadClusterGetter).ToNot(BeNil(), "Invalid argument. input.WorkloadClusterGetter can't be nil when calling getMachinePoolInstanceVersions")
+	Expect(input.Namespace).ToNot(BeEmpty(), "Invalid argument. input.Namespace can't be empty when calling getMachinePoolInstanceVersions")
+	Expect(input.MachinePool).ToNot(BeNil(), "Invalid argument. input.MachinePool can't be nil when calling getMachinePoolInstanceVersions")
 
+	instances := input.MachinePool.Status.NodeRefs
 	versions := make([]string, len(instances))
 	for i, instance := range instances {
-		version, found, err := unstructured.NestedString(instance.(map[string]interface{}), "version")
-		Expect(err).ToNot(HaveOccurred(), "failed to extract versions from unstructured instance")
-		Expect(found).To(BeTrue(), "unable to find nested version string in unstructured instance")
-		versions[i] = version
+		node := &corev1.Node{}
+		err := input.WorkloadClusterGetter.Get(ctx, client.ObjectKey{Name: instance.Name}, node)
+		if err != nil {
+			versions[i] = "unknown"
+		} else {
+			versions[i] = node.Status.NodeInfo.KubeletVersion
+		}
+		log.Logf("Node %s version is %s", instance.Name, versions[i])
 	}
 
 	return versions
 }
-
-func getUnstructuredRef(ctx context.Context, getter Getter, ref *corev1.ObjectReference, namespace string) *unstructured.Unstructured {
-	obj := new(unstructured.Unstructured)
-	obj.SetAPIVersion(ref.APIVersion)
-	obj.SetKind(ref.Kind)
-	obj.SetName(ref.Name)
-	key := client.ObjectKey{Name: obj.GetName(), Namespace: namespace}
-	Expect(getter.Get(ctx, key, obj)).ToNot(HaveOccurred(), "failed to retrieve %s object %q/%q", obj.GetKind(), key.Namespace, key.Name)
-	return obj
-}