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

merged 1 commit into from
Nov 6, 2023

Conversation

killianmuldoon
Copy link
Contributor

Improve logging for the self-hosted tests by ensuring Machine and Infrastructure logs are collected before performing the move to the bootstrap cluster.

/area e2e-testing

@k8s-ci-robot k8s-ci-robot added the area/e2e-testing Issues or PRs related to e2e testing label Oct 30, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 30, 2023
@killianmuldoon
Copy link
Contributor Author

/test pull-cluster-api-e2e-full-main

sbueringer
sbueringer reviewed Oct 30, 2023
View reviewed changes
test/framework/cluster_proxy.go Outdated Show resolved Hide resolved
test/e2e/self_hosted.go Outdated Show resolved Hide resolved
test/e2e/self_hosted.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 31, 2023
@killianmuldoon
Improve logging for self-hosted e2e test
2caa534 
Signed-off-by: killianmuldoon <kmuldoon@vmware.com>
killianmuldoon
killianmuldoon commented Nov 1, 2023
View reviewed changes
test/e2e/clusterctl_upgrade.go
@@ -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

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

chrischdi
chrischdi reviewed Nov 2, 2023
View reviewed changes
Copy link
Member

@chrischdi chrischdi left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 6f0d26098024ee5168b35cc3f9db12980c17930c

@chrischdi
Copy link
Member

Super helpful addition which should help to debug self-hosted clusters!

killianmuldoon
killianmuldoon commented Nov 6, 2023
View reviewed changes
Copy link
Contributor Author

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

Going to approve this as it only touches test code and it's useful for debugging current issues with self-hosted clusters.

@killianmuldoon
Copy link
Contributor Author

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: killianmuldoon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2023
@k8s-ci-robot k8s-ci-robot merged commit 88ace20 into kubernetes-sigs:main Nov 6, 2023
20 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.6 milestone Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrischdi chrischdi chrischdi left review comments

@sbueringer sbueringer sbueringer left review comments

@elmiko elmiko Awaiting requested review from elmiko

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

Assignees

@chrischdi chrischdi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/e2e-testing Issues or PRs related to e2e testing cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@killianmuldoon @k8s-ci-robot @chrischdi @sbueringer