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

🌱 extend test/framework to collect workload cluster nodes #9416

Merged
merged 1 commit into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ maintainers of providers and consumers of our Go API.
### Other
- `clusterctl move` can be blocked temporarily by a provider when an object to be moved is annotated with `clusterctl.cluster.x-k8s.io/block-move`.
- `mdbook releaselink` has been changed to require a `repo` tag when used in markdown files for generating a book with `mdbook`.
- `framework.DumpKubeSystemPodsForCluster` was renamed to `framework.DumpResourcesForCluster` to facilitate the gathering of additional workload cluster resources. Pods in `kube-system` and Nodes are gathered from workload clusters. `kube-system` Pod yaml available in `clusters/*/resources/Pod` and Node yaml is available in `clusters/*/resources/Node`.

### Suggested changes for providers

Expand Down
23 changes: 20 additions & 3 deletions test/e2e/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import (
. "github.com/onsi/ginkgo/v2"
"github.com/onsi/gomega/types"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/klog/v2"
"sigs.k8s.io/controller-runtime/pkg/client"

Expand Down Expand Up @@ -79,13 +81,28 @@ func dumpSpecResourcesAndCleanup(ctx context.Context, specName string, clusterPr
LogPath: filepath.Join(artifactFolder, "clusters", clusterProxy.GetName(), "resources"),
})

// If the cluster still exists, dump kube-system pods of the workload cluster before deleting the cluster.
// If the cluster still exists, dump kube-system pods and nodes of the workload cluster before deleting the cluster.
if err := clusterProxy.GetClient().Get(ctx, client.ObjectKeyFromObject(cluster), &clusterv1.Cluster{}); err == nil {
Byf("Dumping kube-system Pods of Cluster %s", klog.KObj(cluster))
framework.DumpKubeSystemPodsForCluster(ctx, framework.DumpKubeSystemPodsForClusterInput{
Byf("Dumping kube-system Pods and Nodes of Cluster %s", klog.KObj(cluster))
framework.DumpResourcesForCluster(ctx, framework.DumpResourcesForClusterInput{
Lister: clusterProxy.GetWorkloadCluster(ctx, cluster.Namespace, cluster.Name).GetClient(),
Cluster: cluster,
LogPath: filepath.Join(artifactFolder, "clusters", cluster.Name, "resources"),
Resources: []framework.DumpNamespaceAndGVK{
{
GVK: schema.GroupVersionKind{
Version: corev1.SchemeGroupVersion.Version,
Kind: "Pod",
},
Namespace: metav1.NamespaceSystem,
},
{
GVK: schema.GroupVersionKind{
Version: corev1.SchemeGroupVersion.Version,
Kind: "Node",
},
},
},
})
}

Expand Down
68 changes: 37 additions & 31 deletions test/framework/alltypes_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ import (

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
apimeta "k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/klog/v2"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -132,38 +132,44 @@ func DumpAllResources(ctx context.Context, input DumpAllResourcesInput) {
}
}

// DumpKubeSystemPodsForClusterInput is the input for DumpKubeSystemPodsForCluster.
type DumpKubeSystemPodsForClusterInput struct {
Lister Lister
LogPath string
Cluster *clusterv1.Cluster
// DumpNamespaceAndGVK specifies a GVK and namespace to be dumped.
type DumpNamespaceAndGVK struct {
GVK schema.GroupVersionKind
Namespace string
}

// DumpKubeSystemPodsForCluster dumps kube-system Pods to YAML.
func DumpKubeSystemPodsForCluster(ctx context.Context, input DumpKubeSystemPodsForClusterInput) {
Expect(ctx).NotTo(BeNil(), "ctx is required for DumpAllResources")
Expect(input.Lister).NotTo(BeNil(), "input.Lister is required for DumpAllResources")
Expect(input.Cluster).NotTo(BeNil(), "input.Cluster is required for DumpAllResources")

// Note: We intentionally retrieve Pods as Unstructured because we need the Pods as Unstructured for dumpObject.
podList := new(unstructured.UnstructuredList)
podList.SetAPIVersion(corev1.SchemeGroupVersion.String())
podList.SetKind("Pod")
var listErr error
_ = wait.PollUntilContextTimeout(ctx, retryableOperationInterval, retryableOperationTimeout, true, func(ctx context.Context) (bool, error) {
if listErr = input.Lister.List(ctx, podList, client.InNamespace(metav1.NamespaceSystem)); listErr != nil {
return false, nil //nolint:nilerr
}
return true, nil
})
if listErr != nil {
// NB. we are treating failures in collecting kube-system pods as a non-blocking operation (best effort)
fmt.Printf("Failed to list Pods in kube-system for Cluster %s: %v\n", klog.KObj(input.Cluster), listErr)
return
}
// DumpResourcesForClusterInput is the input for DumpResourcesForCluster.
type DumpResourcesForClusterInput struct {
Lister Lister
LogPath string
Cluster *clusterv1.Cluster
Resources []DumpNamespaceAndGVK
}

for i := range podList.Items {
dumpObject(&podList.Items[i], input.LogPath)
// DumpResourcesForCluster dumps specified resources to yaml.
func DumpResourcesForCluster(ctx context.Context, input DumpResourcesForClusterInput) {
Expect(ctx).NotTo(BeNil(), "ctx is required for DumpResourcesForCluster")
Expect(input.Lister).NotTo(BeNil(), "input.Lister is required for DumpResourcesForCluster")
Expect(input.Cluster).NotTo(BeNil(), "input.Cluster is required for DumpResourcesForCluster")

for _, resource := range input.Resources {
resourceList := new(unstructured.UnstructuredList)
resourceList.SetGroupVersionKind(resource.GVK)
var listErr error
_ = wait.PollUntilContextTimeout(ctx, retryableOperationInterval, retryableOperationTimeout, true, func(ctx context.Context) (bool, error) {
if listErr = input.Lister.List(ctx, resourceList, client.InNamespace(resource.Namespace)); listErr != nil {
return false, nil //nolint:nilerr
}
return true, nil
})
if listErr != nil {
// NB. we are treating failures in collecting resources as a non-blocking operation (best effort)
fmt.Printf("Failed to list %s for Cluster %s: %v\n", resource.GVK.Kind, klog.KObj(input.Cluster), listErr)
continue
}
for i := range resourceList.Items {
dumpObject(&resourceList.Items[i], input.LogPath)
}
}
}

Expand All @@ -178,7 +184,7 @@ func dumpObject(resource runtime.Object, logPath string) {
namespace := metaObj.GetNamespace()
name := metaObj.GetName()

resourceFilePath := filepath.Clean(path.Join(logPath, namespace, kind, name+".yaml"))
resourceFilePath := filepath.Clean(path.Join(logPath, kind, namespace, name+".yaml"))
Copy link
Member

@sbueringer sbueringer Sep 19, 2023

Choose a reason for hiding this comment

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

I'm just trying to parse the artifacts folder and this change makes it very had:

After this change:
https://gcsweb.k8s.io/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/9393/pull-cluster-api-e2e-full-main/1702754124908466176/artifacts/clusters/bootstrap/resources/

Before this change:
https://gcsweb.k8s.io/gcs/kubernetes-jenkins/logs/periodic-cluster-api-e2e-release-1-5/1704077878771060736/artifacts/clusters/bootstrap/resources/

Before it was easy to go into a folder for a namespace / test case and go through various resources. Now it's pretty hard because it's always necessary to go up and down two levels. I prefer by far the grouping by namespace and then kind over first kind and then namespace

Can we revert this change?

@killianmuldoon @chrischdi Opinions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine to revert - I think you're right that namespace first is maps better onto how we've actually used this.

Copy link
Member

@sbueringer sbueringer Sep 19, 2023

Choose a reason for hiding this comment

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

PR: #9462 (for reference)

Expect(os.MkdirAll(filepath.Dir(resourceFilePath), 0750)).To(Succeed(), "Failed to create folder %s", filepath.Dir(resourceFilePath))

f, err := os.OpenFile(resourceFilePath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600)
Expand Down
Loading