don’t validate timesync during capz E2E

[jackfrancis]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR removes the time sync validation foo, because there is nothing capz-specific about that particular functionality (i.e., nothing in the capz specs has any effect on this Azure VM behavior).

As an aside, this type of in-cluster testing example is a valuable reference to have for potential future use. Thanks @alexeldeib for that work!, which will live on forever in commit history. 🪦

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

• squashed commits
• includes documentation
• adds unit tests

Release note:

NONE

[jackfrancis]

I've opted to keep the getClusterSSHInfo helper func in test/e2e/helpers.go even though it's currently orphaned. I can anticipate that being a thing we might want to do in the future, and keeping that function around may make that effort easier.

[jackfrancis]

/assign @CecileRobertMichon

[CecileRobertMichon]

PR LGTM. As discussed in slack, we're not losing coverage of CAPZ code by removing these tests since nothing in the CAPZ repo itself has anything to do with timesync. If anything, we might want to consider adding this sort of test as part of a https://github.com/kubernetes-sigs/image-builder e2e test suite.

/lgtm
/assign @shysank

[shysank]

Adding slack conversation for more context.

There is an issue specifically about creating a time sync test for capz clusters, and AFAICT there has been no test added in Image builder yet. Should we wait for a test to be added in Image builder, and then remove it? unless this test is taking too much time/resources that we must do it immediately.

Edit: I could be wrong about image builder tests as I haven't looked into deeply, just saw that there were no linked prs in kubernetes-sigs/image-builder#240

[jackfrancis]

@shysank indeed my justification for investigating and removing these tests is observing E2E test flakes

Btw the flakes are not time sync failures per se, but temporary network outages in the ssh tunnel that is used to perform the tests. So my priority was reducing test flakes. Seems like we have two approaches:

1. evaluate the tests that are flaking and, if needed, invest time in making them more resilient
2. if tests are not needed, remove the tests

Obviously #2 is the quickest path forward, but is also the most sensible (IMO) if in fact the tests are not strictly needed in order to validate the foundational cluster-api + capz contracts that govern development velocity.

[shysank]

indeed my justification for investigating and removing these tests is observing E2E test flakes

Btw the flakes are not time sync failures per se, but temporary network outages in the ssh tunnel that is used to perform the tests. So my priority was reducing test flakes. Seems like we have two approaches:

Thanks for the explanation! I think it's fair to remove the tests to avoid false negative signals given that the tests don't verify any capz functionality.

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shysank

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:

• OWNERS [shysank]

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